Problem/Motivation

In #3614825: Allow additional modules to be installed alongside the profile modules it was necessary to add a ::rebuildAll() to this test in order to keep it passing, we should figure out why and try to remove it. The most likely explanation is assigning a service to a class property on the test or similar.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3616202

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:

About issue forks

Comments

catch created an issue. See original summary.

mstrelan’s picture

mstrelan commented

I had Claude step through this with xdebug attached via Jetbrains MCP. TL;DR we can at least reduce this down to ::rebuildContainer instead of ::rebuildAll().

Explaining in my own words:

  1. language.translate_english is set in the compiled container (via LocaleServiceProvider::alter())
  2. That value is set in the LanguageConfigFactoryOverride constructor
  3. When we save the config in the test setup it triggers \Drupal\locale\EventSubscriber\ConfigSubscriber::onConfigSave which calls $this->kernel->invalidateContainer(), but that doesn't rebuild the container until the next request.

Apparently it only works in main by luck. The container is rebuilt during locale's installation, but locale's config is written later. LocaleServiceProvider::alter() can't load locale.settings so the parameter sticks with the default value (TRUE).

Another approach that doesn't require a container rebuild is to move the shouldSkipOverrides check from LanguageConfigFactoryOverride::getStorage to LanguageConfigFactoryOverride::loadOverrides. I suspect that will have other impacts though.

mstrelan opened merge request !16666

mstrelan opened merge request !16667

mstrelan’s picture

mstrelan commented
Status: Active » Needs review

MR !16666 demonstrates that we can swap from ::rebuildAll to ::rebuildContainer, I think we should do this directly in #3614825: Allow additional modules to be installed alongside the profile modules.

MR !16667 tries to solve the root of the problem. It is passing, so maybe we can work with that. Not sure if/how it will affect contrib?

catch’s picture

catch
he/him
Primary language English
commented
Component: views.module » configuration system
Issue tags: +Needs subsystem maintainer review

The language module change looks like an overall simplification, but also like it needs config subsystem maintainer review because I also don't really understand what the potential fallout of that might or might not be either.

mstrelan’s picture

mstrelan commented
Title: Remove ::rebuildAll() call in NumericFormatPluralTest » Remove ::rebuildContainer() call in NumericFormatPluralTest