[11.1] ResolvedLibraryDefinitionsFilesMatchTest is failing

git bisect reveals commit d9535798624 as the culprit, introduced as backport in 📌 Add a deprecated module version of ResolvedLibraryDefinitionsFilesMatchTest Active .
This is not released yet, but will be released in 1.1.8.

Thank you for tracking that down!

With some xdebug I find a difference between 11.1.x and 11.x in this test.

In the test, we do the following steps:
- Initialize $this->libraryDiscovery from the container.
- Install additional modules.
- Get a module list from the (new) container.
- For each installed module (and each theme) we get libraries.

This means the module list is coming from the new module handler from the new container.
But the library discovery still has the module handler from the old container.

In 11.x, I suppose since 📌 Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller Active , we are calling ->setModuleList() on the old container, so that now even the old container has the updated module list.
In 11.1.x, that call to ->setModuleList() is not happening.

Then we have this code in LibraryDiscoveryParser:

      if ($this->moduleHandler->moduleExists($extension)) {
        $extension_type = 'module';
      }
      else {
        $extension_type = 'theme';
      }
      $path = $this->extensionPathResolver->getPath($extension_type, $extension);

Some tricks with xdebug

To find out what is going on, I used xdebug and also inserted some debug statements into the code.
Specifically I added some local variables like this in LibraryDiscoveryParser and also in the test itself:

      $module_handler_id = spl_object_id($this->moduleHandler);

And then break points to look into the module list in each module handler instance.

This way we can see where we have the same or a different instance of the module handler.
And we can see which instance has the updated module list or the old module list.

So the fix should be to just get the new module handler from the container right?

I tried to cherry-pick the commit from 📌 Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller Active into 11.1.x.
Now it is still failing, but differently from before.
Now ->setModuleList() is called multiple times, once per "module group".
The old module handler will get 13 of the newly installed modules from the first batch, but not any of the remaining 60 modules.
Now instead of 'big_pipe' it fails on 'block'.

Solution

I think a solution is to not use a property for $this->libraryDiscovery in the test.
Instead, grab the library discovery from the new container after modules are installed.

Perhaps we would also want to do this for 11.x, to not rely on setModuleList() being called.

So the fix should be to just get the new module handler from the container right?

(Sorry, cross post earlier)

We want the library discovery to have the new module handler.
The only way to achieve that is to get the new library discovery after the modules are installed.
(MR asap)

donquixote → changed the visibility of the branch 3525090-11.1-resolvedlibrarydefinitionsfilesmatchtest-is to hidden.

nicxvan → changed the visibility of the branch 3525090-fix-ResolvedLibraryDefinitionsFilesMatchTest-and-3525111-node-revision-overview-cache to hidden.

I think this is ready, the logic makes sense and it seems to be a test artifact.

The failure is another issue like this one which is meeting addressed here: https://www.drupal.org/project/drupal/issues/3525111 🐛 NodeRevisionsAllTest::testRevisions() fails in 11.1.x Active

One question is whether we should do anything for 11.x.
Currently the test is passing in 11.x without the change.

I'm going to go ahead and commit this to both 11.x/11.2.x and 11.1.x to keep things in sync, it feels correct for 11.x too e.g. if we hadn't added the multiple install to 11.2.x, we would have added this for that test to pass, and we wouldn't have then removed it again with multi install because it wouldn't have started failing.

Will keep an eye on the pipelines just to make sure nothing unexpectedly gets even worse.

Thanks for tracking this down!

PS. daily jobs are fixed again so it should be harder for these to hide.

catch → closed merge request !12159

catch → closed merge request !12159