We noticed a problem with config_split when exporting a split containing config that is overridden : the override is exported in the YAML file.
This happens because ConfigSplitManager::splitPreview() calls ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval() which then calls ConfigManager::findConfigEntityDependenciesAsEntities() and it returns config containing our override.
Here is how we noticed it:
settings.php like this:$config['user.role.anonymous']['permissions'][9999] = 'my permission'; drush cex.findConfigEntityDependenciesAsEntities() uses loadMultiple() and I think it should use loadMultipleOverrideFree() instead.
If I understand correctly, this method is used to know which config needs to be deleted or changed when deleting config or uninstalling a module. Drupal will not be able to delete or change the overrides defined in code so it does not make sense to return them here.
If findConfigEntityDependenciesAsEntities() is used by some contrib modules, we might need to add a new $loadOverrides = FALSE argument in order to not change the default behavior.
Show commandsStart 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:
changes
Comments
Comment #3
prudloff commentedComment #5
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
As a bug this will need a test case
Comment #6
prudloff commentedComment #7
prudloff commentedWe also encountered this problem while uninstalling a module. Drupal loads the overridden config when trying to remove the permission that depend on this module from roles.
Comment #9
gaëlgFixed by #2910353: Prevent saving config entities when configuration overrides are applied
Comment #10
prudloff commentedReopening because as discussed in #2910353: Prevent saving config entities when configuration overrides are applied it will be easier to fix these problems in separate small patches and keep 2910353 only about triggering an exception when saving overridden config.
Comment #11
prudloff commentedComment #15
berdirThanks for splitting/reopening. I think we should clarify with a core committer whether or not these issues require explicit test coverage. The change is pretty obvious, in the long-term plan in the related issue is to deprecate and eventually disallow using loadMultiple() followed by a save() call. Adding a test for every scenario that we're updating is going to result in a lot of extra tests that we have to write and maintain.
Comment #16
alexpottI think in this case a test would be a good thing because the method is not loading and saving the entities itself. The entities it loads and returns are part of its API.
The fix looks good though and I'd be very happy to see this land. +1 on splitting out the issues too.
Comment #17
prudloff commentedI added a test that fails without the patch.
Comment #18
smustgrave commentedThis one already seemed to get sign off. Test coverage is there https://git.drupalcode.org/issue/drupal-3326900/-/jobs/8925511
Don't see any outstanding threads and have no additional feedback so will mark.
Comment #19
alexpottI think we should document that the configuration entities are loaded override free in \Drupal\Core\Config\ConfigManagerInterface::findConfigEntityDependenciesAsEntities()
Comment #20
prudloff commentedI added a comment on the interface about config being loaded without overrides.
Comment #21
smustgrave commentedFeedback appears to be addressed.
Comment #22
alexpottCommitted and pushed c7a864dce30 to main and dcebf583e6c to 11.x. Thanks!