- Issue created by @znerol
- π¨π¦Canada Charlie ChX Negyesi πCanada
I reviewed the parent issue and I am not sure what is going on, I am unfamiliar with the lazy notation, the dumper sets
$service['lazy'] = TRUE;
but the word lazy does not appear in Drupal\Component\DependencyInjection\Container. Is there a problem with that? - Merge request !12857Issue #3538294: Use service closure for inner kernel in PageCache middleware β (Open) created by znerol
- π¨πSwitzerland znerol
Added a draft MR. This prevents the event dispatcher from being instantiated when a page is delivered from the cache.
- π¨πSwitzerland znerol
Setting to NR for early feedback. This still needs tests for the deprecation in the page cache middleware. I think we should deprecate the
responder
tag attribute as well. This could be done in a follow-up though. - π¨π¦Canada Charlie ChX Negyesi πCanada
Amazing work, thanks.
But if this is necessary then what is a lazy service, wasn't that supposed to solve this? And if it doesn't, should the code setting that up be removed?
- π¨πSwitzerland znerol
I do not know what happened to lazy services since #2473113: All stack middlewares are constructed at the same time even for cached pages β . They are being removed gradually anyway, so didn't bother to look.
- π¨πSwitzerland znerol
And if it doesn't, should the code setting that up be removed?
Yes. In a follow-up.
- π¨πSwitzerland znerol
Went through an epic git bisect only to find that this broke just two months after it was introduced. Everything happened before 8.0 was even released.
Lazy middlewares and responder tag introduced in: #2473113-47: All stack middlewares are constructed at the same time even for cached pages β commit date: 2025-04-30
Lazy middlewares and responder tag broken by: #2408371-83: Proxies of module interfaces don't work β commit date: 2025-06-30
- π¨πSwitzerland znerol
#2408371: Proxies of module interfaces don't work β introduced
ProxyServicesPass
. But that runs beforeStackedKernelPass
. So thelazy
flag addedStackedKernelPass
to middleware definitions on top of the first responder has no effect.I tried to move the
StackedKernelPass
beforeProxyServicesPass
.diff --git a/core/lib/Drupal/Core/CoreServiceProvider.php b/core/lib/Drupal/Core/CoreServiceProvider.php index cd9766c14b5..26cab2964e6 100644 --- a/core/lib/Drupal/Core/CoreServiceProvider.php +++ b/core/lib/Drupal/Core/CoreServiceProvider.php @@ -73,14 +73,14 @@ public function register(ContainerBuilder $container) { $container->addCompilerPass(new SuperUserAccessPolicyPass()); + $container->addCompilerPass(new StackedKernelPass()); + $container->addCompilerPass(new ProxyServicesPass()); $container->addCompilerPass(new BackendCompilerPass()); $container->addCompilerPass(new CorsCompilerPass()); - $container->addCompilerPass(new StackedKernelPass()); - $container->addCompilerPass(new StackedSessionHandlerPass()); $container->addCompilerPass(new MainContentRenderersPass());
But
ProxyServicesPass
doesn't like that:[warning] Missing proxy class 'Symfony\Component\ProxyClass\HttpKernel\HttpKernel' for lazy service 'http_kernel.basic'. Use the following command to generate the proxy class: php core/scripts/generate-proxy-class.php 'Symfony\Component\HttpKernel\HttpKernel' "[namespace_root_path]" ProxyServicesPass.php:63
Besides the
HttpKernel
, the following middleware classes are reported:Drupal\Core\ProxyClass\StackMiddleware\ContentLength
Drupal\Core\ProxyClass\StackMiddleware\KernelPreHandle
Drupal\Core\ProxyClass\StackMiddleware\Session
Drupal\big_pipe\ProxyClass\StackMiddleware\ContentLength
There are for sure more missing in contrib and custom code. Thus, trying to fix the lazyness is not an option I guess.
The Needs Review Queue Bot β tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- π¨π¦Canada Charlie ChX Negyesi πCanada
The CR is not clear to me. Is there a before/after of what should be done?
- π¨πSwitzerland znerol
Performance (Drupal core standard install)
Software:
% uname -v #1 SMP PREEMPT_DYNAMIC Debian 6.1.140-1 (2025-05-22) % php -v PHP 8.3.23 (cli) (built: Jul 3 2025 16:20:45) (NTS) Copyright (c) The PHP Group Zend Engine v4.3.23, Copyright (c) Zend Technologies with Zend OPcache v8.3.23, Copyright (c), by Zend Technologies with Xdebug v3.4.4, Copyright (c) 2002-2025, by Derick Rethans % podman ps CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES 2967cbf5773c docker.io/library/postgres:16 postgres 4 minutes ago Up 4 minutes ago drupal_memory
Webserver:
% export PHP_CLI_SERVER_WORKERS=4 % php -S localhost:8888
Core baseline:
% composer create-project --no-interaction "drupal/recommended-project:^11.x-dev" core-dev # Install core standard profile # Disable css/js aggregation (not compatible with php built-in webbrowser) # Add node/1 # Rebuild container, clear caches, warm cache % ab -c 1 -n 500 http://localhost:8888/node/1 > ./core-dev-baseline.txt
Requests per second: 44.61 [#/sec] (mean) Time per request: 22.415 [ms] (mean) Time per request: 22.415 [ms] (mean, across all concurrent requests)
Core + patch:
% patch -p1 < /tmp/perf.patch % # Rebuild container, clear caches, warm cache % ab -c 1 -n 500 http://localhost:8888/node/1 > ./core-dev-patched.txt
Requests per second: 55.61 [#/sec] (mean) Time per request: 17.984 [ms] (mean) Time per request: 17.984 [ms] (mean, across all concurrent requests)
- π¨πSwitzerland znerol
Drupal CMS baseline:
% composer create-project --no-interaction "drupal/cms:^1.2.x-dev" cms-dev # Enable some recipes. I had to disable case management and project management due to field storage issues.. # Disable css/js aggregation (not compatible with php built-in webbrowser) # Rebuild container, clear caches, warm cache % ab -c 1 -n 500 http://localhost:8888/ > ./cms-dev-baseline.txt
Requests per second: 29.76 [#/sec] (mean) Time per request: 33.597 [ms] (mean) Time per request: 33.597 [ms] (mean, across all concurrent requests)
Drupal CMS + patch:
% patch -p1 < /tmp/perf.patch % # Rebuild container, clear caches, warm cache % ab -c 1 -n 500 http://localhost:8888/ > ./cms-dev-patched.txt
Requests per second: 44.32 [#/sec] (mean) Time per request: 22.563 [ms] (mean) Time per request: 22.563 [ms] (mean, across all concurrent requests)
There are a couple test failures, not sure if intermittent or unrelated.
One thing to think about, and maybe this is a follow up:
Deprecate prepending the decoratedhttp_kernel.basic
service object directly, so that it's always the service closure prepended as an argument to each http middleware instead. That way we eventually don't need to run reflection and get the constructor arguments at all.- π¨πSwitzerland znerol
The failure of
UncaughtExceptionTest::testLoggerException()
is interesting. This test examines the PHP error.log in order to check whether exceptions get logged when the normal logger failed.https://git.drupalcode.org/issue/drupal-3538294/-/pipelines/560646/test_...
Drupal\FunctionalTests\Bootstrap\UncaughtExceptionTest::testLoggerException The error + the error that the logging service is broken has been written to the error log. Failed asserting that actual size 12 matches expected size 10. core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:251
I reproduced it locally and found that
NegotiationMiddleware
appears in the stack trace with the patch, but doesn't on the 11.x branch. The reason is thatMonkeysInTheControlRoom
middleware is registered with the samepriority
. But sincePriorityTaggedServiceTrait::findAndSortTaggedServices()
uses a different method of sorting, the middlewares swapped their location in the stack.Bumped the monkey middleware to an appropriate priority to solve the test fail (
404
). - π¬π§United Kingdom catch
I think the dates in #12 should be 2015 rather than 2026?
It's a shame we don't currently have a way to cover this with performance tests, but it would end up being almost as specific as the new test coverage, and the explicit test coverage added here looks like it should stop this regressing again. Maybe one day - it should be enough of an improvement to show on the dashboard though which is nice.
Would a service locator be an option here instead of the lazy services pass? We've been switching to those in π Replace lazy service proxy generation with service closures Active and similar issues.
- π¨πSwitzerland znerol
I think the dates in #12 should be 2015 rather than 2026?
Thanks, fixed.
Would a service locator be an option here instead of the lazy services pass? We've been switching to those in π Replace lazy service proxy generation with service closures Active : Remove lazy declaration and proxy class for cron and use service closure instead and similar issues.
I haven't looked into service locators. The cited issue is using service closures (same tech as the PR). Lazy services are not part of the fix (the dysfunctional lazy flagging is removed in this PR).
As for the question whether it would be possible to do this without a custom service pass: I do not yet know. The service pass has two responsibilities: Assemble every service tagged with
http_middleware
into a stack. This is done by prepending a reference to the decorated service to the arguments array of the decorator. The second responsibility is to inject all discovered middlewares into theStackedHttpKernel
. This is required to propagate theterminate
call. Maybe the symfony container provides all the necessary tooling to realize that. But even then, it would be necessary to add some kind of BC pass to move old declarations (tagged withhttp_middleware
to their spot in the stack. I'd be interested to explore. Not sure whether this should be done in this issue or in a follow-up.