The standard install profile provides system.cron.yml config file, with non-default cron settings, but this file is not used by the install system.

What are the steps required to reproduce the bug?

Install Drupal 8 selecting the standard install profile.

What behavior were you expecting?

Drupal 8 is installed and cron is configured as per the settings in core/profiles/standard/config/system.cron.yml.

What happened instead?

Drupal 8 was installed but cron used the configuration from core/modules/system/config/system.cron.yml, ignoring the configuration provided by the installation profile.

Related issues

#1934700: Automated cron runs should only be enabled by default for Standard profile
#606840: Enable internal page cache by default

Files: 
CommentFileSizeAuthor
#55 interdiff.txt3.06 KBjessebeach
#55 drupal-config-overwrite-1986090-55.patch5.4 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,462 pass(es). View
#52 interdiff.txt1021 bytesmarkpavlitski
#52 drupal-config-overwrite-1986090-51.patch5.57 KBmarkpavlitski
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,419 pass(es). View
#41 drupal-config-overwrite-1986090-41.patch5.58 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,371 pass(es). View
#41 interdiff.txt832 bytesjessebeach
#31 drupal-overwrite-config-1986090-31.patch5.57 KBmarkpavlitski
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,325 pass(es). View
#24 drupal-overwrite-config-1986090-24.patch6.52 KBmarkpavlitski
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-overwrite-config-1986090-24.patch. Unable to apply patch. See the log in the details link for more information. View
#17 drupal-overwrite-config-1986090-17.patch13.26 KBmarkpavlitski
PASSED: [[SimpleTest]]: [MySQL] 59,384 pass(es). View
#17 interdiff.txt6.05 KBmarkpavlitski
#12 drupal-overwrite-config-1986090-12.patch8.84 KBmarkpavlitski
PASSED: [[SimpleTest]]: [MySQL] 55,852 pass(es). View
#9 drupal-overwrite-config-1986090-9.patch8.55 KBmarkpavlitski
PASSED: [[SimpleTest]]: [MySQL] 55,523 pass(es). View
#7 drupal-overwrite-config-1986090-7.patch1.85 KBmarkpavlitski
FAILED: [[SimpleTest]]: [MySQL] 55,523 pass(es), 4 fail(s), and 1 exception(s). View
#1 drupal-standard-profile-cron-config-1986090-1.patch1.47 KBmarkpavlitski
PASSED: [[SimpleTest]]: [MySQL] 55,926 pass(es). View

Comments

markpavlitski’s picture

Status: Active » Needs review
FileSize
1.47 KB
PASSED: [[SimpleTest]]: [MySQL] 55,926 pass(es). View

This patch addresses the issue by setting the configuration in standard_install().

Dave Reid’s picture

Priority: Normal » Major

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.

Dave Reid’s picture

Title: Standard install profile fails to configure cron » Profile default configurations cannot overwrite module default configurations on install (system.cron.yml)

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.

Dave Reid’s picture

Title: Profile default configurations cannot overwrite module default configurations on install (system.cron.yml) » Profile config does not overwrite module default config on install (system.cron.yml)
Status: Needs review » Needs work
Issue tags: +Needs tests

Needs work and tests for a proper fix.

swentel’s picture

Issue tags: +Configuration system

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

markpavlitski’s picture

Status: Needs work » Needs review
FileSize
1.85 KB
FAILED: [[SimpleTest]]: [MySQL] 55,523 pass(es), 4 fail(s), and 1 exception(s). View

Currently modules can only provide new configurations; any files in the /config directory which match an existing configuration are skipped during installation.

This patch allows modules (including install profiles) to make changes to existing configurations during their installation process.

This will mean the fix in #1934700: Automated cron runs should only be enabled by default for Standard profile will actually take effect.

Status: Needs review » Needs work

The last submitted patch, drupal-overwrite-config-1986090-7.patch, failed testing.

markpavlitski’s picture

Status: Needs work » Needs review
FileSize
8.55 KB
PASSED: [[SimpleTest]]: [MySQL] 55,523 pass(es). View

The patch in #7 caused issues when enabling and disabling modules.

This patch only allows the install profile to overwrite existing configurations. Since it is always the last module installed, it should allow profile writers to amend any core configuration as necessary.

Any config files in the install profile are merged in to the existing configuration, so only changes are necessary in the yml file. See also #1934758: Allow installation profiles to partially override module default configuration settings.

Tests for the new functionality are included.

tim.plunkett’s picture

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

markpavlitski’s picture

@tim.plunkett Thanks for the heads up. Will reroll once #1890784: Refactor configuration import and sync functions is committed.

markpavlitski’s picture

FileSize
8.84 KB
PASSED: [[SimpleTest]]: [MySQL] 55,852 pass(es). View

First attempt at re-roll using the new config import system.

kerasai’s picture

pplantinga’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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?

markpavlitski’s picture

@catch I suppose we could define $install_profile as a global variable and set it in the installer, but that seems messier if anything?

Edit: We could pass in additional parameter instead, and leave it up to the caller to decide if it's safe/sensible to overwrite other modules' configs.

function config_install_default_config($type, $name, $allow_overwrite = FALSE) {
  ...
}
markpavlitski’s picture

Status: Needs work » Needs review
FileSize
6.05 KB
13.26 KB
PASSED: [[SimpleTest]]: [MySQL] 59,384 pass(es). View

Adjusted the patch as per catch's suggestions, moving the logic into the installer.

beejeebus’s picture

i don't like this patch.

i don't think we should introduce the 'merge' concept. either overwrite the whole file, or use profile install hook code to get the config object and update it in code.

this looks to me like yet another complication of what should be a simple, dumb process. IMO, the code we have is already waaaay over-engineered for the job.

having said that, i'm not going to block this, so feel free to ignore this comment.

markpavlitski’s picture

@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.

Overwriting the whole config could work, but would mean a bit more effort tracking changes in any other modules provided by the install profile. I.e. if ctools is bundled with an install profile and decides to add an extra config line, the install profile would need to be updated to include that config line too otherwise it could break ctools.

See also #1934700: Automated cron runs should only be enabled by default for Standard profile. The fix committed there doesn't actually do anything, but it had been assumed that this merge behaviour was already present.

catch’s picture

@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.

markpavlitski’s picture

True. I just meant that it would be useful for install profiles to be able to use the same configuration mechanism (YAML files) to deliver site config, rather than having to overwrite every config option in code.

Note that this issue started as a bug fix, removing a YAML file in the standard installer and adding the config change in code (in the way that beejeebus suggested in #18), and has morphed into installer config merging.

Perhaps we should just go back to the original fix instead?

beejeebus’s picture

Status: Needs review » Needs work

re #21, i discussed this with alexpott in IRC.

we both support a simple special case for profiles, where profiles can ship files that overwrite configuration from other modules. please continue with a patch along those lines.

markpavlitski’s picture

Assigned: Unassigned » markpavlitski

@beejeebus Thanks for the feedback, I'm working on this approach now.

markpavlitski’s picture

Status: Needs work » Needs review
FileSize
6.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-overwrite-config-1986090-24.patch. Unable to apply patch. See the log in the details link for more information. View

New patch which allows install profiles to ship config files which can replace config from other modules, as per #22.

Includes test coverage.

No interdiff as the approach is quite different.

markpavlitski’s picture

Assigned: markpavlitski » Unassigned
markpavlitski’s picture

Issue summary: View changes

Adding related issues.

swentel’s picture

Issue summary: View changes

Updated issue summary.

markpavlitski’s picture

Kartagis’s picture

I have tested and hereby confirm that it works.

markpavlitski’s picture

Status: Needs review » Needs work

The last submitted patch, 24: drupal-overwrite-config-1986090-24.patch, failed testing.

swentel’s picture

markpavlitski’s picture

Status: Needs work » Needs review
FileSize
5.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,325 pass(es). View

@swentel, it would be great to get eyes on this issue. What can I do to help get this through?

Re-rolled patch attached.

markpavlitski’s picture

Gábor Hojtsy’s picture

#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.

Gábor Hojtsy’s picture

sun’s picture

Status: Needs review » Needs work

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.

markpavlitski’s picture

@sun I think this was the approach taken in the original patch, up to #17.

The issue is that overrides are only allowable for the install profile, so there has to be a special case somewhere (from #22).

This seemed like the cleanest place to add it, but thoughts welcome.

markpavlitski’s picture

Assigned: Unassigned » markpavlitski
markpavlitski’s picture

Assigned: markpavlitski » Unassigned
Status: Needs work » Needs review

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.

Looking at this further, this is exactly what the patch is doing.
$config_to_install = $source_storage->listAll($name . '.');

ExtensionInstallStorage::listAll() uses ExtensionInstallStorage::getAllFolders() to work out the list of config entries, and the extra code added here is what's adding the proper overrides.

We have to check and only apply the matching overrides, because this is called for every module during install, and we only want to override the config which is about to be enabled.

@sun can you please clarify if I'm missing something?

markpavlitski’s picture

Status: Needs review » Needs work

Correcting status

Berdir’s picture

Status: Needs work » Needs review
jessebeach’s picture

FileSize
832 bytes
5.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,371 pass(es). View

Just correcting a small spelling error in a comment.

jessebeach’s picture

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!

Berdir’s picture

Issue tags: -Needs tests

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.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

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 :)

jessebeach’s picture

Assigned: Unassigned » webchick
Issue tags: +Spark

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

sun’s picture

Status: Reviewed & tested by the community » Needs review

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?

sun’s picture

Assigned: webchick » Unassigned
Status: Needs review » Needs work

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.

alexpott’s picture

Assigned: Unassigned » webchick

@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

jessebeach’s picture

Assigned: webchick » Unassigned

Although this test will be moot

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

markpavlitski’s picture

I will re-roll the patch to change one of the other settings instead. Is it worth leaving the test in for now and removing as a follow-up if/when #2140511: Configuration file name collisions silently ignored for default configuration is removed? Or shall I remove it now?

jessebeach’s picture

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

markpavlitski’s picture

Status: Needs work » Needs review
FileSize
5.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,419 pass(es). View
1021 bytes

This patch sets a higher cron warning period instead. The test is still present.

sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Spark

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
5.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,462 pass(es). View
3.06 KB

Here are the changes outlined in #54.

sun’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

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