Problem/Motivation

During installation a profile can override configuration provided by modules. However this is limited to install time. If a profile wants to provide a different default for a module that is installed after the profile installed it can't. This was a designed limitation of the ConfigInstaller. However, it is apparent that we have use cases for this. Not least with the testing profile in core which wants to disable translation download. To do this it has the file core/profiles/testing/config/optional/locale.settings.yml. This currently does not work, which results in the test run downloading translations unnecessarily.

Another upside of this change is that we become more friendly to distributions as they can override configuration for modules they don't enable but want to provide a specific opinionated default config for in case the user does install that module.

Proposed resolution

Always check the profile module's optional and default configuration folders for overrides regardless of whether we're doing an installation.

Remaining tasks

Review
Commit

User interface changes

None

API changes

None

Data model changes

None

Why this issue should be an RC target

  1. We assume in our testing profile it works like this
  2. I think given the testing ramifications - always downloading translations - it helps make the test infrastructure performant
  3. It empowers distributions

The patch includes a signature change for a protected method.

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott’s picture

Status: Active » Needs review

The last submitted patch, 2: 2584419-2.test-only.patch, failed testing.

Berdir’s picture

So we're still downloading translations for every locale test that's enabling translations in the UI at the moment?

alexpott’s picture

@Berdir yerp

alexpott’s picture

Issue summary: View changes
Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -113,12 +113,12 @@ public function installDefaultConfig($type, $name) {
           // Gets a profile storage to search for overrides if necessary.
    -      $profile_storage = $this->getProfileStorage($name);
    +      $profile_storages = $this->getProfileStorages($name);
    

    Comment is still singular.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -223,19 +223,22 @@ public function installOptionalConfig(StorageInterface $storage = NULL, $depende
        *
        * @return array
        *   An array of configuration data read from the source storage keyed by the
        *   configuration object name.
        */
    -  protected function getConfigToCreate(StorageInterface $storage, $collection, $prefix = '', StorageInterface $profile_storage = NULL) {
    +  protected function getConfigToCreate(StorageInterface $storage, $collection, $prefix = '', array $profile_storages = []) {
    

    Are we allowed to change a protected method or not? :)

  3. +++ b/core/modules/config/src/Tests/ConfigInstallProfileOverrideTest.php
    @@ -92,12 +92,12 @@ function testInstallProfileConfigOverwrite() {
         }
     
         // Install the config_test module and ensure that the override from the
    -    // install profile is not used. Optional configuration can not override
    +    // install profile is used. Optional configuration can override
         // configuration in a modules config/install directory.
         $this->container->get('module_installer')->install(['config_test']);
    

    Ha ;)

    "We are at war with Eastasia. We've always been at war with Eastasia."

alexpott’s picture

Thanks @Berdir

  1. Fixed
  2. I dunno - I think we should have a rule that protected methods are @internal unless explicitly marked with @api
  3. I am at war with Oceania

I think committers should discuss this issue as possible rc target:

  1. We assume in our testing profile it works like this
  2. I think given the testing ramifications - always downloading translations - it helps make the test infrastructure performant
  3. It empowers distributions
xjm’s picture

xjm’s picture

Issue summary: View changes

Adding the RC targeting reasoning to the summary.

xjm’s picture

Issue tags: -rc target triage +rc target

Discussed with alexpott and agreed that this makes sense as an RC target. There is no direct disruption for RC, just expanded functionality, but it would not be advisable to change the behavior after RC.

xjm’s picture

Issue tags: +Triaged D8 major
DuaelFr’s picture

Status: Needs review » Reviewed & tested by the community

Steps to review:

  1. copy core/modules/locale/config/install/locale.settings.yml to core/profiles/minimal/config/optional/locale.settings.yml
  2. edit core/profiles/minimal/config/optional/locale.settings.yml
  3. change translate_english: false to translate_english: true
  4. $ drush si minimal -y
  5. $ drush en -y locale
  6. $ drush cget locale.setting

Expected result:

cache_strings: true
translate_english: true
javascript:
  directory: languages
translation:
  use_source: remote_and_local
  default_filename: '%project-%version.%language.po'
  default_server_pattern: 'http://ftp.drupal.org/files/translations/%core/%project/%project-%version.%language.po'
  overwrite_customized: false
  overwrite_not_customized: true
  update_interval_days: 0
  path: sites/default/files/translations
  import_enabled: true

Before:

$ drush cget locale.settings
[...]
translate_english: false
[...]

After:

$ drush cget locale.settings
[...]
translate_english: true
[...]

Good job :)

The last submitted patch, 2: 2584419-2.test-only.patch, failed testing.

  • catch committed 41e882f on 8.0.x
    Issue #2584419 by alexpott: Profile configuration overrides should be...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

andypost’s picture

Issue tags: +Needs change record

That needs change record because solves a lot of problems for distributions and profiles

  • catch committed 41e882f on 8.1.x
    Issue #2584419 by alexpott: Profile configuration overrides should be...

Status: Fixed » Closed (fixed)

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