Problem/Motivation

core/tests/Drupal/KernelTests/Core/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php fails on 11.1.x
Discovered in #3524716: [11.1] update gather rules to manage hook preprocess

git bisect reveals commit d9535798624 as the culprit, introduced as backport in #3270499: Add a deprecated module version of ResolvedLibraryDefinitionsFilesMatchTest.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3525090

Command icon 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:

Comments

nicxvan created an issue. See original summary.

donquixote’s picture

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

nicxvan’s picture

Thank you for tracking that down!

donquixote’s picture

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 #3416522: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller, 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);

So in 11.1.x, it does not find 'big_pipe' as a module, so it behaves as if it was a theme.

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.

nicxvan’s picture

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

donquixote’s picture

I tried to cherry-pick the commit from #3416522: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller 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.

donquixote’s picture

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.

nicxvan’s picture

Status: Active » Reviewed & tested by the community

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

donquixote’s picture

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

  • catch committed 63be8a86 on 11.1.x
    Issue #3525090 by donquixote, nicxvan: [11.1]...

  • catch committed 1a37ee3e on 11.x
    Issue #3525090 by donquixote, nicxvan: [11.1]...

  • catch committed b2b77960 on 11.2.x
    Issue #3525090 by donquixote, nicxvan: [11.1]...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.