ComponentNegotiator::negotiate caches its result using isset() to check for a cache hit. isset() returns FALSE for NULL values, so when doNegotiate() finds no replacement and returns NULL, the common case when using a subtheme, the cached NULL is never read back. doNegotiate() re-runs on every call, iterating over all component definitions each time.
On sites using a subtheme, most components have no replacement, so this affects every find() call. Each doNegotiate() runs array_filter over all component definitions. Benchmarks show ~8× slower negotiation for repeated calls on the same request.
See the regression test added in the MR.
Replace isset() with array_key_exists() so NULL results are properly served from cache.
- Review
- ComponentPluginManager::find() calls createInstance() on every invocation (building a new Component object with its full ComponentMetadata each time). A static cache of resolved instances could be a worthwhile follow-up.
None
None
None
None
None
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 3592946-4.diff | 2.52 KB | herved |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !15941
Comments
Comment #3
herved commentedComment #4
herved commentedMR15941 diff
Comment #5
amitgoyal commentedReviewed MR !15941. The fix replaces isset() with array_key_exists() in ComponentNegotiator::negotiate(), correctly handling cached NULL results so doNegotiate() is not re-run on every call when no replacement is found.
The regression test testNegotiateCachesNullResults adds a fake replacement after the first negotiate() call to prove the cached NULL is served without re-running doNegotiate().
All CI jobs pass. The 'Test-only changes' job failure is expected - it reverts the fix and confirms the regression test catches the bug.
Ran ComponentNegotiatorTest locally (3 tests, 11 assertions, all pass).
LGTM.
Comment #9
longwaveCommitted and pushed 46e88176426 to main and 45da9862260 to 11.x and c8714e5a03c to 11.4.x. Thanks!
Doesn't backport cleanly to 10.6.x, please reopen with a backport MR but it doesn't seem critical to bring back to there now.