Alter hook for site configure form in DemoUmamiHooks uses outdated services

DemoUmamiHooks::installConfigureSubmit() calls setUserPasswords, ...

FWIW, that comes from #3045966: Set author and editor passwords to the admin password in the demo to make demoing easier.

@godotislate:

It looks like the second commit adds a test and reverts the first commit. Is that on purpose? Is the "Test-only changes" CI job broken?

Thank you for finding that!

I reproduced the error using the main branch. I am adding the error message as text to the issue summary: that makes it easier for screen readers and search engines.

If I ignore the error, then visiting various admin pages (/en/admin/modules for one) leads to a WSOD with the error message

Symfony\Component\Routing\Exception\RouteNotFoundException: Route "update.status" does not exist. in Drupal\Core\Routing\RouteProvider->getRouteByName() (line 214 of core/lib/Drupal/Core/Routing/RouteProvider.php).

and a stack trace.

I repeated the test using the feature branch. I did not see the error in the installer. Instead, I was logged in to the site, redirected to the home page.

I visited a few admin pages, cleared caches, and checked recent log messages. There are some known warnings generated when installing Umami, a couple of warnings about trying to start cron when it is already running, and nothing else of note.

Test coverage

I reviewed the changes to the tests. It makes sense, and the test-only CI job fails with the same error message that I get in manual testing.

I have a tiny reservation that we no longer have the original version of the test: we are modifying the test instead of adding a new one. But it is already a Functional test marked #[Group('#slow')]. Also, I assume the other installer tests do not enable the update module, so we still have test coverage of that code path.

TL;DR: LGTM

Code changes

The fix makes sense:

• We need to avoid serializing service objects, rebuilding the container, then deserializing.
• Therefore we have to use static methods for callbacks (like form submit functions).
• Those static functions have to use \Drupal::service(...) instead of an injected service.

To illustrate, here is an important hunk from the change introduced for this issue:

   /**
    * Sets the password of admin to be the password for all users.
    */
-  public function setUserPasswords(#[\SensitiveParameter] $admin_password): voi
d {
-    $user_storage = $this->entityTypeManager->getStorage('user');
+  public static function setUserPasswords(#[\SensitiveParameter] $admin_password): void {
+    $user_storage = \Drupal::entityTypeManager()->getStorage('user');
     // Collect the IDs of all users with roles editor or author.
     $ids = $user_storage->getQuery()
       ->accessCheck(FALSE)

This function is made static, so it has to use \Drupal::entityTypeManager() instead of the injected $this->entityTypeManager.

The breaking change

I used git bisect to find the issue that caused this problem: #3516264: CKEditor 5 loads all plugin translations on AJAX operations. (I am now very efficient at installing Umami through the admin UI.) It includes these changes:

@@ -26,6 +28,8 @@ class Ckeditor5Hooks {
 
   public function __construct(
     protected LanguageMapper $languageMapper,
+    protected ModuleHandlerInterface $moduleHandler,
+    protected LibraryDependencyResolverInterface $libraryDependencyResolver,
   ) {
 
   }
@@ -221,7 +225,6 @@ public function libraryInfoAlter(&$libraries, $extension): void {
     if ($extension === 'filter') {
       $libraries['drupal.filter.admin']['dependencies'][] = 'ckeditor5/internal.drupal.ckeditor5.filter.admin';
     }
-    $moduleHandler = \Drupal::moduleHandler();
     if ($extension === 'ckeditor5') {
       // Add paths to stylesheets specified by a theme's ckeditor5-stylesheets
       // config property.

In other words, the exact opposite of the fix for this issue: replacing a call to \Drupal::moduleHandler() with an injected service.

That is as close as I get to explaining how that issue caused this one. Is it some interaction between the module handler and the entity type manager from the old service container? (shrug)

I cannot help pointing out that there is a Remaining task on that issue:

Look for side-effects of the proposed resolution.

Maybe now we are ready to check that off. ;)

Needs work: more documentation

@godotislate, I have to admit that I do not understand the interaction of dependency injection (DI), serialization, and container rebuilds as well as you do. (I could not have figured out a fix for this issue in five times as long as you took. Maybe ten times.) For years, I have been following the rule that we should use DI and avoid invoking the \Drupal class in OO code. Now, this issue shows me that there are exceptions, but I still do not know what the exceptions are.

We need some guidelines, and we need some documentation.

Side note: just last week, I was doing a code review at work. Oh, no, someone was injecting the whole container instead of individual services. I know that’s the wrong thing to do! But I could not find a documentation page that said so. I looked hard for such a page.

I am setting this issue to NW: not for code changes but for a more complete explanation, something that we can add to the developer documentation.

@godotislate:

Thanks. I think I understand what is going on.

To summarize: if a class requests an object from the service container, the class should be careful not to use that object after a container rebuild.

It feels like we are playing Whack-a-mole and fixing bugs when that rule is broken. For this issue, when DemoUmamiHooks alters SiteConfigureForm::submitForm(), it has to realize that its objects might be used after a rebuild, so it has to pass a static method. I am not sure that this counts as "tightly coupled" code, but it is coupled.

I guess the saving grace is that "having to deal with container rebuilds in runtime is somewhat unusual."

I wish there were a way to handle this at the container level during a rebuild. I expect that the old container "knows" which of its services have been instantiated. If we could update those objects and hand them off to the new container, then we would never have stale service objects (and we could remove all the code for services from DependencySerializationTrait). Something like this, for each instantiated service:

// Get an already-instantiated service.
$foo = $old_container->get('foo');
// Magically get the same result as $new_container->get('foo').
$foo->update($new_container);
// Then register $foo as the object to return from $new_container->get('foo').

Unfortunately, after the first line, $foo = $new_container->get('foo') does not do what we want and $foo = clone new_container->get('foo') does the opposite of what we want.

For this issue, can you add a code comment before the line

$user_storage = \Drupal::entityTypeManager()->getStorage('user');

explaining why we are not using DI? I know: if anyone tries to "fix" that, then the new test will fail. But let’s use "defense in depth" and save some future developer a cycle of code-test-recode.

I am pretty sure that the switch to DI was not required to fix #3516264: CKEditor 5 loads all plugin translations on AJAX operations. Someone was just cleaning up a call to \Drupal::moduleHandler() and using DI.

I already reviewed (Comment #11) and tested (Comments #9 and #10). Since then, @godotislate added my requested change (a code comment) and marked two methods @internal. I do not think I need to re-test.

Thanks for the updates. I am happy with this version.

This looks great. One small inaccuracy: the comment in setUserPasswords() references SiteConfigureForm::submit() but the actual method is SiteConfigureForm::submitForm().

Found another comment similar to 16, once it's green I think it's ready!

RTBC plus 1

Both comments have been addressed and test only fails as expected!

Committed/pushed to main and 11.x, thanks!