Problem/Motivation

Follow up from #3572171-16: Persist is_syncing across container rebuilds:

Looks the following code path might have an issue:

  • ModuleInstaller::install(), with the $module_list passed to it having count > 1
  • This calls ModuleInstaller::doInstall(), then if the $module_list passed to doInstall() has count > 1, this calls ModuleInstaller::updateKernel([])
  • Which calls $this->kernel->resetContainer(), and resetContainer does not persist the config installer sync status

Implementations of hook_modules_installed are invoked after this, so the sync status is incorrectly FALSE.

Steps to reproduce

  1. Install a module that has a hook_modules_installed() implementation, with some way to track the value of the $is_syncing boolean value passed in.
  2. Export configuration (e.g. drush cex)
  3. Edit core.extension.yml in the config export and add multiple modules so that they will be installed on config import. For testing, it's easiest to find simple modules that don't install any configuration of their own, so that core.extension.yml is the only needed change.
  4. Example of change in core.extension.yml
    module:
  // ...
  new_module_1: 0
  new_module_2: 0
  // ..
  5. Import configuration (e.g. drush cim)
  6. Note that the is_syncing value in the hook_modules_installed() function is FALSE, when it should be TRUE

Also seen in #3584145: Call to member function removeComponent error after updating to 11.3.6.

Proposed resolution

in DrupalKernel::resetContainer(), save the config installer syncing value before the container is rebuilt, then set the saved value on the new container.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

#3584812: ConfigInstaller isSyncing does not persist to hook_modules_installed() when multiple modules are installed fixes a critical regression in configuration import present in 11.3.6 and 11.3.7. If you've not imported configuration during this time there is nothing you need to do. If you have, after updating it is recommended to follow your site's usual method of deploying configuration change, for example, by running drush deploy.

CommentFileSizeAuthor
#7 3584812-7.diff8.65 KBherved

Issue fork drupal-3584812

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

godotislate created an issue. See original summary.

godotislate opened merge request !15456

godotislate’s picture

godotislate
he/him
commented
Priority: Normal » Major
Status: Active » Needs review
Issue tags: +Needs Review Queue Initiative

MR 15456 ready for review.
Test only failure: https://git.drupalcode.org/project/drupal/-/jobs/9366252

herved’s picture

herved commented

Thank you for looking into this @godotislate !
I've closed #3584145: Call to member function removeComponent error after updating to 11.3.6 which was reported in the paragraphs module as duplicate. The patch here works for me and the reporter.

I don't think I'm the best person to review this, but +1 LGTM

smustgrave’s picture

smustgrave commented

Left 1 comment on the MR, rest looks good

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Fair enough, no other objections to the code.

FYI I found this from view filtering for Needs Review Queue Initiative so glad to see others tag it!

herved’s picture

herved commented
StatusFileSize
new 3584812-7.diff8.65 KB

Static MR diff for composer

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Major » Critical

This is a critical bug - this might have broken recipes too as they use this functionality to override config during module install.

apso’s picture

apso commented

After upgrading from 11.3.5 to 11.3.7 due to the new security update, local installation failed.

Field {field} is unknown.

The patched fixed the issue +1 RTBC.

catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

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

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !15456

xjm’s picture

xjm
she/her
Primary language English
commented

Can we get a release note snippet for this?

xjm’s picture

xjm
she/her
Primary language English
commented

Specifically, is there anything sites who ran into this on 11.3.6 or 11.3.7 need to do?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes

Specifically, is there anything sites who ran into this on 11.3.6 or 11.3.7 need to do?

I think the answer is to re-import configuration. But to be honest hopefully they didn't do a deploy because this could very easily end up deleting data on your site - for example if a field was removed. It's really hard to predict all the possibilities here because it will depend on site configuration.

I've had a go at writing a release note.

xjm’s picture

xjm
she/her
Primary language English
commented

Alright, moved stuff around a little and this is what I have in the 11.3.8 draft now:

Drupal 11.3.6 introduced a critical regression for importing configuration that could result in data loss. This release hotfixes that regression.

Sites that attempted to import configuration on Drupal 11.3.6 or 11.3.7 should re-import it using the site's recommended method (for example, drush deploy). Make note of any differences in configuration, and ensure that data associated with that configuration has not been deleted.

Sites that updated to 11.3.6 or 11.3.7 without importing configuration do not need to take any action.

quietone’s picture

quietone commented
Issue tags: +11.3.8 release notes
xjm’s picture

xjm
she/her
Primary language English
commented

Note: This issue was committed with the wrong commit message, but is present in all three branches, per @catch.

catch’s picture

catch
he/him
Primary language English
commented

Commit hashes:

main: 1e00c0b78f99fb159be709be567306debb094624
11.x: e21a447d784a9a5c7cca39985e560bec4c375228
11.3.x: 8546e506feaa6b7e848a36ce3b51f1e1e3fb366a

Status: Fixed » Closed (fixed)

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