Extension review · dev-companion · patch set 2

Label lookups cached for the length of a request

A review of change 1482, with an inventory of every lookup that reads a file and one question: does the cache survive a language switch?

Change
1482 · [BUGFIX] Cache label lookups for the length of a request
Patch set
2 · 3f9c2e1a7d0 · 3 files, +64 −9
Issue
#2071 · Bug · "a page with forty labels answers in 130 ms"
Target
main · 2.4
State
mergeableVerified +1no votes, no comments, no chain
Read
2026-09-11 in a worktree of its own, probes on 2026-09-14
Summary

The change fixes a real, measured fault: every label lookup read its catalogue file again, and a page with forty labels spent 124 of its 131 ms on the same three files. An array on the service, alive for one request, answers the second lookup of a key without a read. The idea is right and small.

Recommendation: not ready to merge. The unit suite for the class fails (F1.1), and a lookup after a language switch answers in the language before it (F1.2), which no test can see (F2.2). Key the cache on the language too, adapt the four tests that expect the second read, and bring the two tests in A.1. Nothing else in the change stops it.

Blocks
Two. A red suite, and a language switch that answers in the old language.
Sent back
Two. A miss reads again, and no test can fail on the cache.
Worth a change
Four. In the change; none of them stops it.
Checked
Two. The cache lives one request, and the other lookups stay as they are.

The change

What the change is, where it stands, and how the review read it. Then how the code it touches works, what went wrong, and what the change does about it.

Context

Issue #2071 came in from a reader of the tool reference: a page that resolves forty labels answers in 130 ms, and the trace shows the same file read forty times. The change is small on purpose. It adds an array to the lookup service and reads from it before it reads from the file.

Patch set 2 is a rebase of set 1 with a changelog entry. It stands alone: no chain above it, no votes, no comments, and the pipeline reports green. The pipeline runs the functional suites and not the unit suite for Lookup, which is where F1.1 comes from.

The review read commit 3f9c2e1a7d0 in a worktree of its own, with no rebase, 4 commits behind origin/main. Every suite result and every probe in this document is against that commit and its parent.

How a lookup answers

A tool answers from a source, and the source of typo3_label_lookup is the catalogue files of the installation. The service resolves a key to a file, reads that file for one language, and returns the label. Nothing between the call and the file kept an answer, so every call was a read. The other lookups keep theirs somewhere, and where they keep it decides what a change to one of them can touch.

Two rows of the same four stops: the call, the array on the service, the catalogue reader, and the file. The first lookup passes through all four and reads the file. The second hits the array and goes no further.
The first lookup of a key reads the file; the second stops at the array. Forty labels on a page are three reads, not forty.
The first lookup of a key reads the file; the second stops at the array. Forty labels on a page are three reads, not forty.
Two rows of the same four stops: the call, the array on the service, the catalogue reader, and the file. The first lookup passes through all four and reads the file. The second hits the array and goes no further.
Tool Reads Cached where Per request
typo3_label_lookup LabelLookup::resolve() this change with the change
typo3_icon_lookup IconLookup::resolve() IconRegistry, since 2.1 yes
typo3_schema_lookup SchemaLookup::table() the installation’s own yes
typo3_hint_lookup HintLookup::find() a static, per process reads once

The error

Every call of typo3_label_lookup reads the catalogue again. A page that resolves forty labels reads the same three files forty times, and the read is most of the answer.

The trace from the issue
text 
# typo3_label_lookup · page 12 · 40 labels
read Resources/Private/Language/locallang.xlf (de)   3.1 ms
read Resources/Private/Language/locallang.xlf (de)   3.0 ms
read Resources/Private/Language/locallang.xlf (de)   3.2 ms
…
40 reads · 3 files · 124 ms of a 131 ms answer

What it does

resolve() keeps what it answered in an array on the service, which lives as long as the request. The second lookup of a key returns from there and reads no file.

Classes/Lookup/LabelLookup.php
       public function resolve(string $key, string $language): ?string       {+          if (isset($this->cache[$key])) {+              return $this->cache[$key];+          }           $catalogue = $this->reader->read($this->fileFor($key), $language);-          return $catalogue->get($key);+          return $this->cache[$key] = $catalogue->get($key);       }

Paths traced

Every way through the changed method, followed by hand and then by probe.

Scenario Flow Result
First lookup of a key resolve() → reader->read() → get() → into the array one read
Second lookup, same key, same language the array answers no read
Second lookup, same key, other language the array answers, in the first language wrong answer
Key with no label null goes into the array; isset() is false for it reads again
A second request a new service, an empty array unchanged
typo3_icon_lookup, typo3_schema_lookup not touched unchanged

Findings

By weight, and numbered by it: what stops the change, what goes back to its author, what is worth a change, and what the review opened and found sound. Without the last group the author cannot tell what a review looked at from what it never reached. Every finding in one table first, each row a jump to its entry, and the work they ask for in a second.

Entry Kind Origin
F1.1 The unit suite for the lookup fails: 6 of 41 tests blocks introduced by this change
F1.2 A lookup after a language switch answers in the language before it blocks introduced by this change
F2.1 A missing key reads the catalogue on every call sent back introduced by this change
F2.2 No test can fail on what the change claims sent back introduced by this change
F3.1 The reader is a new instance per call worth a change older than the change
F3.2 The changelog entry names no visible change worth a change introduced by this change
F3.3 The array has no bound worth a change introduced by this change
F3.4 Two trailers in the message worth a change —
F4.1 The cache lives exactly one request checked —
F4.2 The other lookups stay as they are checked —

To do

To do Kind
T1.1 Adapt the four tests that expect the second read, and run the suite before the next patch set. blocks
T1.2 Key the cache on the key and the language, or keep one array per language. blocks
T2.1 Use array_key_exists(), or make the docblock say that a miss reads again. sent back
T2.2 Bring the two tests in A.1, which fail on the parent. sent back
T3.1 Construct the reader once, in the constructor. worth a change
T3.2 Put the number into the entry: 131 ms to 11 ms, forty reads to three. worth a change
T3.4 Drop the second Signed-off-by trailer. worth a change

Blocks submission

F1.1

The unit suite for the lookup fails: 6 of 41 tests

blocks introduced by this change

Every case that resolves one key twice fails. The change does not touch the suite, so nobody ran it, and the pre-merge pipeline goes red on it.

bash 
$ vendor/bin/phpunit tests/Unit/Lookup
Tests: 41, Assertions: 97, Failures: 6.

Four of the six expect the second read the change removes on purpose: adapt those. The other two are F1.2.

T1.1Adapt the four tests that expect the second read, and run the suite before the next patch set.

F1.2

A lookup after a language switch answers in the language before it

blocks introduced by this change

The cache keys on $key alone. The first answer for a key stays, whatever language the next call names. Probe P2 shows it, and the two remaining failures in F1.1 are this. Key on the key and the language, or keep one array per language.

Two calls for the same key, one in German and one in English, both reach the same slot of the array. The German call writes the slot; the English call reads it and answers in German.
Both calls reach one slot. The German call fills it, and the English call answers from it, in German.
Both calls reach one slot. The German call fills it, and the English call answers from it, in German.
Two calls for the same key, one in German and one in English, both reach the same slot of the array. The German call writes the slot; the English call reads it and answers in German.

T1.2Key the cache on the key and the language, or keep one array per language.

Sent back

F2.1

A missing key reads the catalogue on every call

sent back introduced by this change

isset() is false for a cached null, so a key with no label goes to the file each time (probe P3). The docblock says the opposite: that the request keeps a miss. array_key_exists() does what the docblock says. Either the code or the sentence moves.

T2.1Use array_key_exists(), or make the docblock say that a miss reads again.

F2.2

No test can fail on what the change claims

sent back introduced by this change

The coverage table below has the detail. In short: the one test named for the cache counts no reads and passes on the parent too, the language test never resolves one key twice, and a missing key has no test at all. A.1 is two tests that fail on the parent.

T2.2Bring the two tests in A.1, which fail on the parent.

Worth a change

F3.1

The reader is a new instance per call

worth a change older than the change

LabelLookup constructs its reader in resolve(). The service is a singleton, and the reader carries no state, so one reader in the constructor is the same object with one construction fewer per label.

T3.1Construct the reader once, in the constructor.

F3.2

The changelog entry names no visible change

worth a change introduced by this change

An answer that took 131 ms takes 11 ms, and a page with forty labels reads three files instead of forty. The entry says "performance". The number is the sentence a reader of the release page wants.

T3.2Put the number into the entry: 131 ms to 11 ms, forty reads to three.

F3.3

The array has no bound

worth a change introduced by this change

One request resolves at most a few hundred keys, so this costs nothing today. A sentence in the docblock that says so is enough; a bound is not.

F3.4

Two trailers in the message

worth a change

Signed-off-by stands twice. Nothing reads it twice; one goes.

T3.4Drop the second Signed-off-by trailer.

Checked and correct

F4.1

The cache lives exactly one request

checked

The service is request-scoped, and probe P5 shows a second request reads again. Nothing survives into the next answer.

F4.2

The other lookups stay as they are

checked

typo3_icon_lookup and typo3_schema_lookup cache already, in the registry and in the installation; the change reads neither. The hint lookup is a case of its own: the follow-up under Scope.

Remarks at the code

The hunks the findings point at, with each remark under the block at the line it cites. Line numbers are those of the patched file, and each remark goes into the review tool as it stands. A number in brackets names the finding.

Classes/Lookup/LabelLookup.php, from line 41
php 
41    public function resolve(string $key, string $language): ?string42    {43        if (isset($this->cache[$key])) {44            return $this->cache[$key];45        }46        $catalogue = $this->reader->read($this->fileFor($key), $language);47        return $this->cache[$key] = $catalogue->get($key);48    }
43
The key has no language. A lookup after a switch answers in the language before it; probe P2 shows that. Key on both. [F1.2]
46
This read runs again for a key with no label: isset() is false for a cached null. The docblock says the opposite; one of the two has to move. [F2.1]
47
The one path that changed, and no test fails on it yet. A.1 is one. [F2.2]
Classes/Lookup/LabelLookup.php, from line 60
php 
60    private function reader(): CatalogueReader61    {62        return new CatalogueReader($this->files);63    }
62
A new reader per call, in a service the container holds once. The constructor is the place; the reader carries no state of its own. [F3.1]

Evidence

What the review ran, on the change and on its parent, so every finding above has a number behind it.

Probes, parent against change

The probe in A.2, run against the change and against its parent. A probe that turns green proves the fix; one that turns red is a regression the suites did not see.

Probe Parent Change
P1 · resolve() twice, one key, one language fail 2 reads ok 1 read
P2 · resolve() twice, de then en ok answers in en fail answers in de
P3 · a key with no label, twice unchanged 2 reads unchanged 2 reads
P4 · page 12, 40 labels fail 40 reads, 131 ms ok 3 reads, 11 ms
P5 · two requests in one process ok 2 reads ok 2 reads

Suites on the worktree

Commit 3f9c2e1a7d0, every suite from a clean checkout.

Suite Scope Result
unit tests/Unit/Lookup 6 of 41 fail
functional tests/Functional/Tools 88 pass
cgl every PHP file 0 fixable
phpstan level 8, 412 files no errors
e2e — not run

Test coverage

What the tests of the changed class claim, against what each one checks. A test that passes on the parent too proves nothing about the change.

Test Claims Checks
resolveReturnsTheLabel a key resolves one call, one label holds
resolveReadsTheCatalogueOnce the cache works nothing: it counts no reads, and passes on the parent too claims more
resolveHonoursTheLanguage a switch answers in the new language one lookup per language, on two keys misses F1.2
— a missing key no test none

Scope

What the review considered and set aside, what it leaves for a change of its own, and what it ran.

Raised and dropped

  • A cache across requests. Raised because the files change rarely. Dropped: a label edited while the server runs then arrives after a restart, which is the fault the follow-up below names in the hint lookup.
  • The cache in the reader instead of the lookup. Raised because the reader is where the cost is. Dropped: the reader is a new instance per call (F3.1), so a cache there lives one call.
  • A bound on the array. Raised in F3.3 and dropped there: a request resolves a few hundred keys at most.

Follow-ups, not this change

N1 · The hint lookup reads its file once per process, not per request
HintLookup keeps its catalogue in a static, so a hint edited while the server runs arrives after a restart. Right for a build, wrong for a watch. A change of its own.

Surfaces and runs

Worktree
review/1482, no rebase, 4 commits behind origin/main
Read
the three changed files, the class’s tests, the changelog entry, the issue
Suites
unit for Lookup, functional for Tools, cgl, phpstan
Probes
five, on parent and change, in A.2
Not run
e2e, the full unit suite

Appendix

A.1 · The two tests, written for this review and removed again
php 
public function aSecondLookupReadsNoFile(): void
{
    $this->subject->resolve('answer.empty', 'de');
    $this->subject->resolve('answer.empty', 'de');
    self::assertSame(1, $this->reader->reads);
}

public function aLookupAfterALanguageSwitchAnswersInTheNewLanguage(): void
{
    $this->subject->resolve('answer.empty', 'de');
    self::assertSame('Nothing found', $this->subject->resolve('answer.empty', 'en'));
}
A.2 · The probe
php 
$lookup = $container->get(LabelLookup::class);
$reader = $container->get(CatalogueReader::class);

$lookup->resolve('answer.empty', 'de');
$lookup->resolve('answer.empty', 'de');
printf("P1 reads after two lookups: %d\n", $reader->reads);

$en = $lookup->resolve('answer.empty', 'en');
printf("P2 after a switch to en: %s\n", $en);