Problem/Motivation

The help_topics module has a plugin cache clearer - \Drupal\help_topics\HelpSectionManager::clearCachedDefinitions() that can trigger under certain conditions and throw exceptions.

This is caused because we call the plugin cache clearer after beginning module uninstall but before rebuild the container and the kernel. This is the wrong way around we should rebuild the container first so plugin cache cleaners have a chance to reflect the changes to the available services.

Steps to reproduce

This is happening in https://www.drupal.org/pift-ci-job/2090326 for a MR which adds dependencies to olivero_test - see #3174107: Add additional testing coverage for Olivero. Module dependencies affect the order in which things occur. This change causes help_topics to be uninstalled after search in the ConfigImportAllTest and this results in it breaking.

Proposed resolution

Call plugin cache cleaners in uninstall after the container rebuild.

Remaining tasks

User interface changes

NOne

API changes

None

Data model changes

None

Release notes snippet

N/a

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

alexpott’s picture

Component: help.module » base system
FileSize
1.87 KB
2.38 KB

Hmmm actually there is a way better fix that makes more sense and will fix more modules...

This problem is caused by triggering a plugin cache clear before rebuilding the container after removing the module from core.extension. That's the wrong way around. It's amazing we've got this far with it that way around.

alexpott’s picture

Issue summary: View changes

The last submitted patch, 3: 3218660-3.test-only.patch, failed testing. View results

alexpott’s picture

Here's a test that doesn't involve an experimental module so we continue to have test coverage if that one doesn't make it. The test only patch is the interdiff.

andypost’s picture

Is it ok to mix help topics test into base system fix?

Otherwise looks straightforward

alexpott’s picture

@andypost I think it is. It proves that uninstalling search is broken when help_topics is still installed and it proves that the change in ModuleInstaller fixes it.

The last submitted patch, 7: 3218660-6.new-test-only.patch, failed testing. View results

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Then it looks ready to go

  • larowlan committed b85bafd on 9.2.x
    Issue #3218660 by alexpott: help_topics module can break during module...
  • larowlan committed 257efe5 on 9.3.x
    Issue #3218660 by alexpott: help_topics module can break during module...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

I think that normally we'd want a test for a base-system bug in a base-system module on the outside chance the experimental module was removed from core.

However in the case of help-topics, it is a) unanimously supported b) close to stable c) going to end up as part of help module if it goes anywhere - so I think in that case it is fine to add the test in this namespace.

Committed 257efe5 and pushed to 9.3.x. Thanks

Because this is a major bug and the risk of disruption is low, I feel its fine to backport to 9.2.x, so did so

Status: Fixed » Closed (fixed)

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