Problem/Motivation

If a module called foo has a config file called foo_more.bar.yml in its config/install directory and the foo_more.bar config does not have a dependencies array the ConfigInstaller will permit the module to be installed. This has several bad implications:

  • The configuration will not be cleaned up on uninstall
  • The configuration will not met dependency checks during configuration sync essentially making the active configuration undeployable.

Thanks to @OnkelTem whose questions in IRC led to the creation of this issue.

Proposed resolution

Fix ConfigImporter::validateDependencies() to check this.

Remaining tasks

User interface changes

None

API changes

No API changes but some contrib or custom modules might no longer be able to be installed. They will need to change the configuration name and write upgrade paths.

Data model changes

Potentially new config names for contrib or custom.

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Status: Active » Needs review
FileSize
1.32 KB

Let's see if core breaks. It shouldn't.

alexpott’s picture

alexpott’s picture

Title: ConfigInstaller can create configuration files for unmet dependencies » ConfigInstaller can create configuration files which have unmet dependencies
Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
FileSize
155.82 KB

I'm leaning towards making this a critical because it does totally break module install via config synchronisation. To test this:

  1. Copy contact.settings.yml to foo.bar.yml in the same folder.
  2. Install standard
  3. Export config to sync directory (drush cex -y)
  4. Uninstall the contact module and delete the foo.bar config from active (\Drupal::service('config.storage')->delete('foo.bar');)
  5. Try to run a config import and you see

The last submitted patch, 4: 2718733-4-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2718733-4.patch, failed testing.

alexpott’s picture

Whack-mole :)

alexpott’s picture

Ignore #10's patches... some test config got in there :(

The last submitted patch, 11: 2718733-11.test-only.patch, failed testing.

OnkelTem’s picture

Thanks for nailing this down!
And +1 for #7
Sorry for I can't test it right now.

bircher’s picture

Status: Needs review » Reviewed & tested by the community

The patch rightfully doesn't allow installing modules which provide bogus config.

alexpott’s picture

Issue tags: +Needs change record

This issue should have a change record.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
dawehner’s picture

Status: Needs work » Reviewed & tested by the community

I thought we need some more test coverage, but the install cases are properly covered already. RTBC for the change record

  • catch committed 8a1341a on 8.2.x
    Issue #2718733 by alexpott: ConfigInstaller can create configuration...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x, thanks!

Status: Fixed » Closed (fixed)

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