Replace lazy service proxy generation with service closures

Replaced the cron service proxy with a service closure.

This might be trickier than I imagined.
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "cron", path: "cron -> automated_cron.subscriber".

Seems this broke a few tests.

This test passed when rerun:

    1)
    Drupal\Tests\file\Functional\DownloadTest::testPrivateFileTransferWithoutPageCache
    Correctly denied access to a file when file_test sets the header to -1.
    Failed asserting that 200 is identical to 403.
    
    /builds/issue/drupal-3483996/core/modules/file/tests/src/Functional/DownloadTest.php:138
    /builds/issue/drupal-3483996/core/modules/file/tests/src/Functional/DownloadTest.php:76

The remaining failing test was:

1) Drupal\Tests\system\Kernel\System\CronQueueTest::testDelayException
    Failed asserting that 0 matches expected 1001.
    
    /builds/issue/drupal-3483996/core/modules/system/tests/src/Kernel/System/CronQueueTest.php:137

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 on with manual reruns:

There was 1 failure:

1) Drupal\Tests\path\Functional\PathAliasTest::testPathCache
Cache entry was created.
Failed asserting that a boolean is not empty.
/builds/issue/drupal-3483996/core/modules/path/tests/src/Functional/PathAliasTest.php:84
FAILURES!
Tests: 4, Assertions: 113, Failures: 1.

There was 1 error:

1) Drupal\FunctionalJavascriptTests\Tests\JSWebAssertTest::testJsWebAssert
WebDriver\Exception\StaleElementReference: stale element reference: element
is not attached to the page document

NR for the change that fixed the test.

I was able to locally reproduce the circular service definition error in #11 📌 Replace lazy service proxy generation with service closures Active , but only once (the first time I tried).

I basically did the same the steps from the failed Validatable config build:

1. Against latest 11.x, drush si standard -y
2. ls core/modules | grep -v sdc | xargs vendor/bin/drush pm:install --yes
   I ignored an error about not being able to install package manager
3. Switch to MR branch, rebased against latest 11.x
4. drush cr

And I saw the same error:

Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "cron", path: "cron -> automated_cron.subscriber". in /builds/issue/drupal-3483996/core/lib/Drupal/Component/DependencyInjection/Container.php on line 147 #0 /builds/issue/drupal-3483996/core/lib/Drupal/Component/DependencyInjection/Container.php(430): Drupal\Component\DependencyInjection\Container->get('cron', 1)
#1 /builds/issue/drupal-3483996/core/lib/Drupal/Component/DependencyInjection/Container.php(237): Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array)
#2 /builds/issue/drupal-3483996/core/lib/Drupal/Component/DependencyInjection/Container.php(177): Drupal\Component\DependencyInjection\Container->createService(Array, 'automated_cron....')
#3 /builds/issue/drupal-3483996/core/lib/Drupal/Component/DependencyInjection/Container.php(454): Drupal\Component\DependencyInjection\Container->get('automated_cron....', 1)
#4 /builds/issue/drupal-3483996/vendor/symfony/event-dispatcher/EventDispatcher.php(243): Drupal\Component\DependencyInjection\Container->Drupal\Component\DependencyInjection\{closure}()
#5 /builds/issue/drupal-3483996/vendor/symfony/event-dispatcher/EventDispatcher.php(206): Symfony\Component\EventDispatcher\EventDispatcher::Symfony\Component\EventDispatcher\{closure}(Object(Symfony\Component\HttpKernel\Event\TerminateEvent), 'kernel.terminat...', Object(Symfony\Component\EventDispatcher\EventDispatcher))
#6 /builds/issue/drupal-3483996/vendor/symfony/event-dispatcher/EventDispatcher.php(56): Symfony\Component\EventDispatcher\EventDispatcher->callListeners(Array, 'kernel.terminat...', Object(Symfony\Component\HttpKernel\Event\TerminateEvent))
#7 /builds/issue/drupal-3483996/vendor/symfony/http-kernel/HttpKernel.php(114): Symfony\Component\EventDispatcher\EventDispatcher->dispatch(Object(Symfony\Component\HttpKernel\Event\TerminateEvent), 'kernel.terminat...')
#8 /builds/issue/drupal-3483996/core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php(63): Symfony\Component\HttpKernel\HttpKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Render\HtmlResponse))
#9 /builds/issue/drupal-3483996/core/lib/Drupal/Core/DrupalKernel.php(683): Drupal\Core\StackMiddleware\StackedHttpKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Render\HtmlResponse))
#10 /builds/issue/drupal-3483996/vendor/drush/drush/src/Boot/DrupalBoot8.php(312): Drupal\Core\DrupalKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Render\HtmlResponse))
#11 [internal function]: Drush\Boot\DrupalBoot8->terminate()
#12 {main}
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "cron", path: "cron -> automated_cron.subscriber". in Drupal\Component\DependencyInjection\Container->get() (line 147 of /builds/issue/drupal-3483996/core/lib/Drupal/Component/DependencyInjection/Container.php).

However, running drush cr again right after succeeded with no errors.

I also tried repeating these steps, with and without running XDebug, and the error did not occur again.

Yes because the container needs to be rebuilt, the MR needs an empty update function added so there's a forced container rebuild upon drush updb.

For #18 please

I added an empty hook_post_update_NAME() to the automated_cron module 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.

What's a good way to test this one/

Looks 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 AutomatedCron constructor, since coding standards don't require docblocks for constructors anymore.

I applied the requested change RE inlining as well as other suggestions.

@smustgrave

What's a good way to test this one

The only method I know to test it manually is:

1. Enable automated_cron.
2. Set the cron interval to a very low number > 0, such as with drush cset -y automated_cron.settings interval 1
3. After the number of seconds used for the interval, load a page on the site.
4. Confirm that cron ran 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 📌 DX: Creating lazy services is too difficult/obscure/bespoke/brittle Active :

IIRC this was critical for certain aspects of the Drupal bootstrap process to become sufficiently fast in the majority of cases.

So the work that gets done here almost certainly will need to provide profiling data if it changes lazy services, and certainly if it removes them altogether.

I don't know how to do that, but I'm game to learn.

LGTM

One 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.

Also a general question - is this the last lazy class in core?

Checking 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.

Ah 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?

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?

Cloned to 📌 [meta] Replace lazy service proxy generation with service closures Active as the meta and retitled here.

Rebased, removed post update hook per #26. Also autowired the service closure per the original idea in the IS and updated the CR.