Profile config does not overwrite module default config on install (system.cron.yml)

I don't think this is the correct fix, because otherwise what is the point of shipping default config files in the profile?

Marked #1958770: Cron default should never be "Never" as a duplicate of this issue since this had a nicer issue summary already.

I think the issue must be that system ships with a default system.cron.yml file, and so does the standard profile. We have two conflicting default configurations and it doesn't work. This is a major problem for install profiles.

Needs work and tests for a proper fix.

Tagging - afaics, this will need changes deep down in config

All of these functions are about to become methods in #1890784: Refactor configuration import and sync functions, just a heads up.

#12: drupal-overwrite-config-1986090-12.patch queued for re-testing.

I can confirm that this issue exists and that this patch fixes it.

+++ b/core/includes/config.incundefined
@@ -35,12 +35,19 @@ function config_install_default_config($type, $name) {
+    $install_profile = ($type == 'module' && $name == drupal_get_profile());
+    if ($install_profile) {
+      // Allow install profiles to change existing configurations.

The only time we install install profiles is via the installer, couldn't this logic happen there?

@beejeebus I think the main benefit of this method is it allows install profile devs to provide default configs in the same way as everyone else, by just bundling the YAML files, rather than changing config in code.

It's not really the same as everyone else though. Modules don't get to provide overrides of configuration provided by other modules.

Updated issue summary.

I have tested and hereby confirm that it works.

#2140511: Configuration file name collisions silently ignored for default configuration is somewhat related, although the problem there is module/module config collisions in which case the new file will not be copied and should not override the existing file.

Crosspost.

I also discovered this while debugging #2190723: Add a KeyValueStore\FileStorage to replace e.g. ConfigStorage, because the changes to InstallStorage in that issue are causing the profile config file to be discovered and properly installed.

The proposed fix here is wrong though —

1. ExtensionInstallStorage::getAllFolders() simply does not add the installation profile to the folders to scan.
2. It needs to be added last (instead of first), because InstallStorage::getComponentNames() overwrites config names of former directories with config names of later directories.

In other words, the initial/resulting list of $config_to_install in ConfigInstaller::installDefaultConfig() is supposed to contain the full list of configuration file names with proper overrides already, including overrides by the installation profile. A separate scan/list + manual replacement for the profile config only is unnecessary/wrong.

31: drupal-overwrite-config-1986090-31.patch queued for re-testing.

Just correcting a small spelling error in a comment.

In other words, the initial/resulting list of $config_to_install in ConfigInstaller::installDefaultConfig() is supposed to contain the full list of configuration file names with proper overrides already, including overrides by the installation profile.

The initial list of configurations to install is misleading. InstallStorage::listAll() just returns an array of keys. So it's impossible to know what should be overridden at this point. With markpavlitski's patch in #31, it will contain the right filenames (even though their overridden status is not known because the path information is not listed in $config_to_install). InstallStorage::listAll() invokes ExtensionInstallStorage::getAllFolders(), which contains the profile path override.

The real magic happens in ConfigInstaller::installDefaultConfig() with this code:

$new_config = new Config($name, $this->activeStorage, $this->eventDispatcher, $this->typedConfig);
$data = $source_storage->read($name);

Here, FileStorage::read() gets the data for the config file with $data = file_get_contents($this->getFilePath($name));, but getFilePath() is overridden. <code>FileStorage::read() is really invoking InstallStorage::getFilePath(), which then calls ExtensionInstallStorage::getAllFolders(), where we apply the profile override.

So sun's recommendation in #35 is essentially correct and markpavlitski's assertion in #38 that this is what the patch is doing is also correct.

I've reviewed this code up down and sideways and the tests as well. I'd set it to RTBC, but I want to give sun the chance to respond. Hopefully everything is in order and we can get this in to allow further work on some critical beta blocking issues.

Awesome work folks!

I didn't look in depth, so I can't RTBC/confirm that the implementation is correct either, but can verify that this is working as expected on a custom install profile. Removing Needs Tests tag.

Reviewed and tested the code and the solution does indeed look nice and clean. I can not tally @sun's comments in #35 with the patches in #31 and #41 and believe that #35 must have been a review of the earlier approach because afaics the current approach is exactly what @sun is recommending :)

Assigning to webchick to get this on her radar for the next commit spree.

Yes, the new code is what I meant. Some concerns on reviewing the latest patch though:

1. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigInstallWebTest.php
   @@ -96,4 +97,62 @@ function testIntegrationModuleReinstallation() {
   +    // Turn on the test module, which will also attempt to override the
   +    // configuration data, but should be prevented.
   +    \Drupal::moduleHandler()->install(array('config_override_test'));
   ...
   +++ b/core/modules/config/tests/config_override_test/config/system.cron.yml
   

   Isn't that a completely different issue/bug than the scope of this issue?

   Why would we allow any module to override settings (as opposed to config entities) of any other module?

   Shouldn't that ability be limited to installation profiles only?

2. +++ b/core/profiles/testing/config/system.cron.yml
   @@ -0,0 +1,4 @@
   +  autorun: 10800
   

   Enabling the poormanscron feature in the Testing profile by default presents a significant change to the default testing environment for all web tests.

   Instead of enabling autorun, can we change the requirements warning/error thresholds instead, please?

Also, to continue on #46.1)

At the point in time at which config_override_test.module is installed, system.module is installed already, so the active configuration contains the system.cron file imported from system.module's default configuration already.

As a consequence, config_override_test's attempt to provide a system.cron configuration cannot work out, because that config object exists already.

If the ConfigInstaller allows modules to blatantly overwrite your existing, custom configuration (in your active config directory), then that would be a completely different, highly critical security issue, which would have to be addressed in a separate issue.

But I hope that is not the case. In any case, that part of the added test coverage here does not make sense.

@sun
1. Agreed and this is exactly what this is testing :). This patch is adding system.cron.yml to ensure that modules cannot override other modules configuration and it is testing this. Although this test will be moot once #2140511: Configuration file name collisions silently ignored for default configuration so I agree this probably should be removed.
2. Yep makes sense

Although this test will be moot

Noted, I'm fixing up #2140511: Configuration file name collisions silently ignored for default configuration this morning.

I would rather remove the test later (later probably being in a couple days). I'll be responsible for making sure it gets removed.

OK, thanks! @alexpott, please go ahead and commit (doesn't look like you authored any of it) :-)

Change the patch one final review and realised that we should change the test to ensure more clarity about what is going on....

1. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigInstallWebTest.php
   @@ -96,4 +97,62 @@ function testIntegrationModuleReinstallation() {
   +    // The expected active configuration altered by the install profile.
   +    $expected_overriden_data = array(
   

   Lets call this $expected_profile_data since overriden (a) means something very specific to the config system (b) is mispelt :)

2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigInstallWebTest.php
   @@ -96,4 +97,62 @@ function testIntegrationModuleReinstallation() {
   +    // Enter an override-free context to ensure the original data remains.
   +    $old_state = \Drupal::configFactory()->getOverrideState();
   +    \Drupal::configFactory()->setOverrideState(FALSE);
   ...
   +    // Re-enable configuration overrides.
   +    \Drupal::configFactory()->setOverrideState($old_state);
   

   This is not necessary as the configuration override system is not relevant to this change.

3. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigInstallWebTest.php
   @@ -96,4 +97,62 @@ function testIntegrationModuleReinstallation() {
   +    // file directly, otherwise install profile overrides will apply.
   ...
   +    // been overriden by the testing install profile.
   ...
   +    // Turn on the test module, which will also attempt to override the
   

   Lets not use the word override here since this is not about the configuration override system. overwrite is okay... but in fact configuration is never actually overwritten. This change is all where default configuration comes from. It enables installation profiles to provide different default configuration on behalf of modules.

Here are the changes outlined in #54.

Committed 53ef54c and pushed to 8.x. Thanks!