Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
As discussed in #3228623: DX: Creating lazy services is too difficult/obscure/bespoke/brittle the DX for our current lazy service proxies is poor. We tried using symfony lazy services in #3396928: Deprecate Drupal ProxyBuilder in favor of Symfony lazy services however that was not possible due to:
Sadly SF uses eval for the proxy classes which at least I can't see working with our serialized container solution
However we can now use service closures to lazy-load services which are supported by our serialized container.
This is the issue to do this for the cron service.
Steps to reproduce
Proposed resolution
Replacy services tagged 'lazy' with service closures.
Prefer using autowiring and the #[AutowireServiceClosure] attribute over the !service_closure yaml command where possible.
https://symfony.com/doc/current/service_container/autowiring.html#genera...
This requires changing the constructor signature and adding a new method to invoke the closure like so:
Before:
public function __construct(
protected CronInterface $cron,
) {}
Service definition:
services:
my_service:
class: Foo\Bar
arguments: ['@cron']
After:
public function __construct(
protected \Closure $cronClosure,
) {}
protected function getCron(): CronInterface {
return ($this->cronClosure)();
}
Service definition:
services:
my_service:
class: Foo\Bar
arguments: [!service_closure '@cron']
Autowiring approach
Before:
public function __construct(
protected CronInterface $cron,
) {}
Service definition:
services:
my_service:
class: Foo\Bar
autowire: true
After:
public function __construct(
#[AutowireServiceClosure('cron')]
protected \Closure $cronClosure,
) {}
protected function getCron(): CronInterface {
return ($this->cronClosure)();
}
Service definition:
services:
my_service:
class: Foo\Bar
autowire: true
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3483996
Show commands
Start 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:
- 3483996-replace-lazy-service
changes, plain diff MR !9969
Comments
Comment #2
kim.pepperComment #3
kim.pepperComment #4
kim.pepperComment #5
kim.pepperComment #6
kim.pepperComment #8
kim.pepperReplaced the
cronservice proxy with a service closure.Comment #9
kim.pepperComment #10
kim.pepperComment #11
kim.pepperThis might be trickier than I imagined.
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "cron", path: "cron -> automated_cron.subscriber".Comment #12
smustgrave commentedSeems this broke a few tests.
Comment #13
kentr commentedThis test passed when rerun:
Comment #14
kentr commentedComment #15
kentr commentedThe remaining failing test was:
I made a change to fix it and commented in GitLab explaining the change.
A couple of other tests failed in the subsequent automatic pipeline, but they passed with manual reruns:
Comment #16
kentr commentedNR for the change that fixed the test.
Comment #17
godotislateI was able to locally reproduce the circular service definition error in #11, but only once (the first time I tried).
I basically did the same the steps from the failed Validatable config build:
drush si standard -yls core/modules | grep -v sdc | xargs vendor/bin/drush pm:install --yesI ignored an error about not being able to install package manager
drush crAnd I saw the same error:
However, running
drush cragain right after succeeded with no errors.I also tried repeating these steps, with and without running XDebug, and the error did not occur again.
Comment #18
ghost of drupal pastYou see this error because the old container is called with the new codebase. The container needs to be rebuilt so the MR needs an empty update function added which will cause the update subsystem to build a new container.
Comment #19
smustgrave commentedFor #18 please
Comment #20
kentr commentedComment #21
kentr commentedI added an empty
hook_post_update_NAME()to theautomated_cronmodule because the new lazy service closure is on that module.The pipeline is green, but I could only reproduce the error once locally anyway in spite of multiple tries with full wipe / reset.
Comment #22
smustgrave commentedWhat's a good way to test this one/
Comment #23
godotislateLooks like there's one request change still pending.
This is optional, but I think it's also a good opportunity to remove the docblock for the
AutomatedCronconstructor, since coding standards don't require docblocks for constructors anymore.Comment #24
kentr commentedI applied the requested change RE inlining as well as other suggestions.
@smustgrave
The only method I know to test it manually is:
automated_cron.drush cset -y automated_cron.settings interval 1cronran without errors as a result of the page load.According to the IS Remaining Tasks, there are other cases to change. Are there any of these that we don't want to include in this issue?
Also noticed that @wim leers said in #3228623: DX: Creating lazy services is too difficult/obscure/bespoke/brittle:
I don't know how to do that, but I'm game to learn.
Comment #25
godotislateLGTM
Comment #26
catchOne comment on the MR - we don't need the post update, can just be removed.
Also a general question - is this the last lazy class in core? And if so should we deprecated/remove generate-proxy-class.php? Doesn't necessarily need to be in this issue but would be good to open a follow-up.
Comment #27
godotislateChecking now, looks like there are still several others. Not sure on the history of this issue for why only cron is being converted. Tagging for an IS update. Either this issue should be a meta, with individual issues addressing each proxy class, or maybe an update on why only cron is being converted.
Comment #28
catchAh yeah there is a list in remaining tasks, I assume cron was picked just to have an example to look at.
These are probably tricky enough that we should do them in multiple issues, we could maybe re-title this one to just cron and open a new meta for the others?
Comment #29
godotislateComment #30
godotislateCloned to #3514491: [meta] Replace lazy service proxy generation with service closures as the meta and retitled here.
Comment #31
godotislateComment #32
godotislateRebased, removed post update hook per #26. Also autowired the service closure per the original idea in the IS and updated the CR.
Comment #34
acbramley commentedThis is great, I've done the following:
1. Gone through the MR and closed all threads after confirming they were resolved or (in the case of the helpful comments) explained what was going on well enough.
2. Reviewed the code
3. Rebased the MR to ensure tests pass against latest HEAD.
4. Manually tested the changes locally with xdebug ensuring the closure works as expected.
This is ready to go!
Comment #35
berdirI quickly tested that this doesn't break ultimate_cron as a module that replaces the cron service class seems to work fine and makes sense looking at the changes, just wanted to make sure. It has its own proxy class that can be removed once it requires 11.2 then.
Comment #38
catchCommitted/pushed to 11.x and cherry-picked to 11.2.x, thanks!
Comment #40
xjmPublished the CR.