Problem/Motivation

In IRC today, some people were surprised to learn that editing the default config shipped with themes and modules does not change anything unless you reinstall.

Proposed resolution

Create an install subdirectory within each extension's config directory, and move default configuration to be installed in there. E.g.:

-- mymodule
---- config
------ schema
-------- mymodule.views.schema.yml
------ install
--------- views.view.myview.yml

Remaining tasks

User interface changes

N/A

API changes

Class constants added to Drupal\Core\Config\InstallStorage:

  • CONFIG_INSTALL_DIRECTORY
  • CONFIG_SCHEMA_DIRECTORY
Files: 
CommentFileSizeAuthor
#71 2233787.71.patch99.71 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,928 pass(es). View
#71 66-71-interdiff.txt5.23 KBalexpott
#66 2233787.66.patch105.1 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#66 64-66-interdiff.txt729 bytesalexpott
#64 2233787.64.patch105.09 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,870 pass(es). View
#64 62-64-interdiff.txt2.14 KBalexpott
#62 2233787.62.patch102.96 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,920 pass(es), 2 fail(s), and 0 exception(s). View
#62 58-62-interdiff.txt5.79 KBalexpott
#58 2233787.58.patch95.13 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,631 pass(es), 1 fail(s), and 0 exception(s). View
#50 2233787.50.patch95.62 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,541 pass(es). View
#50 49-50-interdiff.txt5.92 KBalexpott
#49 2233787.49.patch92.88 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,666 pass(es). View
#45 interdiff-31-45.txt1.64 KBxjm
#45 config-2233787-45.patch92.17 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,536 pass(es). View
#36 config-2233787-31-do-not-test.patch3.99 KBxjm
#32 interdiff-29-31.txt920 bytesxjm
#32 config-2233787-31.patch90.52 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,444 pass(es). View
#29 interdiff-25-29.patch940 bytesxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#29 config-2233787-29.patch89.62 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,371 pass(es), 1 fail(s), and 0 exception(s). View
#26 config-2233787-25-do-not-test.patch3.09 KBxjm
#25 interdiff-config-default.txt1.36 KBxjm
#25 config-2233787-25.patch90.54 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,805 pass(es), 5 fail(s), and 0 exception(s). View
#22 config-2233787-22.patch89.95 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Comments

webchick’s picture

Seems like a good idea to me. I did know this is how it works, but with any other file in a module/theme it's "hack hack hack, clear the cache, see what broke" (except for very specific things like hook_install()/hook_uninstall() which are pretty clear) so I can see why people would be confused by this.

sun’s picture

Hm. I don't really like the verbosity of the directory name, but if it need be, I guess I could live with it.

The only concern + request I have is that it should be properly namespaced; i.e.:

Not ./default_config, but ./config_default, please.


In addition, I'm not sure whether "default" is really any more clear to newcomers, compared to now. To be really clear, I think it would have to be named ./config_install

xjm’s picture

Issue tags: +beta target

Worth discussing as a beta target, I think.

mdrummond’s picture

config_install makes sense to me.

xjm’s picture

config_install seems okay to me too. It makes it very clear what its purpose is.

brantwynn’s picture

I would rather see this remain as config but introduce a mechanism to reset configuration from this directory. Similar to how we have a Features revert (in D7), it would be intuitive to be able to revert to this configuration via a button click or drush command.

xjm’s picture

Well, #6 would be a new feature, whereas this is a simple change. So at this point in the release cycle, that should be in contrib IMO.

brantwynn’s picture

I still am not sure if changing the directory name will help - fair to say the 'revert' functionality is not on target for 8.0.0 though.

alexpott’s picture

I'm ++ to this if we clear evidence that this will lead to greater understanding

xjm’s picture

We could even stick READMEs in all the core modules' directories to explain their purpose. That might help too, but not sure if it's overkill.

xjm’s picture

The misconception that you can edit a module's config dir and see changes has come up many times when new developers start trying CMI, so I think that's evidence that there is an issue with understanding the directory's purpose. The question is whether changing the name helps. I think config_install is pretty unambiguous.

xjm’s picture

Mmm. I forgot about the schemata. Maybe they could live in a separate directory?

xjm’s picture

Title: Rename /config directory to /default_config to reduce confusion of editability » Rename /config directory to /default_config or /config_install to reduce confusion of editability
dawehner’s picture

I do agree that config_install really describes the behaviour much better as config as still better than config_default.

xjm’s picture

Maybe :

-- mymodule
---- config
------ schema
-------- mymodule.views.schema.yml
------ install
--------- views.view.myview.yml
sun’s picture

As @brantwynn mentioned, I'm also not sure whether changing the name really improves the DX situation in substantial ways:

The directory name might be more clear, but extension authors are still trying to do something that they naturally expect to work (in some way).

Obviously, changing the config directory names/structure would be a far-reaching API change affecting all core/contrib D8 code that exists today. Compared to that, introducing a "Revert to default config" facility appears to be an API addition that is internal to the configuration system, and which would seemingly resolve the actual DX problem/use-case. (?)

alexpott’s picture

Reverting to default configuration is now much more tricky now we've removed UUIDs from default configuration entities. Managing how that expectation is delivered is going to be a similar challenge.

An possible advantage for the structure outlined in #16 by @xjm is that I think it will look even better when language overrides are in the mix.
Maybe :

-- mymodule
---- config
------ schema
-------- mymodule.views.schema.yml
------ install
--------- views.view.myview.yml
------ language_override
--------- de
-----------views.view.myview.yml
brantwynn’s picture

The outline described in #16 makes a lot of sense to me. It should alleviate some DX woes (config/install says, to me, "this is where my config that gets loaded on install should go"). It will also generally help people to keep their module's config directories more organized. With the additional benefit for language overrides mentioned in #18, this begins to look like a very useful solution.

webchick’s picture

Title: Rename /config directory to /default_config or /config_install to reduce confusion of editability » Split /config directory into /schema, /install, etc. to more clearly outline what gets used when

Nice suggestions. Changing title.

xjm’s picture

Assigned: Unassigned » xjm

Rolling a start at a patch.

xjm’s picture

Status: Active » Needs review
FileSize
89.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Let's see what happens!

Shell script to move the things:

#!/bin/bash                                                                     
CONFIG_DIRS="$( find core -type d -name 'config' )"
for d in $CONFIG_DIRS
do
  mkdir	"$d"/install
  FILES="$( find $d -maxdepth 1 -name "*.yml" -exec basename {} \; )"
  for f in $FILES
  do
    git mv "$d"/"$f" "$d"/install/"$f"
  done
done
xjm’s picture

What happens is that Drupal doesn't install, so let's fix that before we take a bot...

core/modules/entity/config/install/ not found.

xjm’s picture

Status: Needs work » Needs review
FileSize
90.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,805 pass(es), 5 fail(s), and 0 exception(s). View
1.36 KB
xjm’s picture

Status: Needs review » Needs work
FileSize
3.09 KB

So for some reason the patch in #25 prevents almost all the Configuration test group from being listed. It goes from:

Tests to be run:
  - Drupal\config\Tests\ConfigCRUDTest
  - Drupal\config\Tests\ConfigDependencyTest
  - Drupal\config\Tests\ConfigDiffTest
  - Drupal\config\Tests\ConfigEntityListTest
  - Drupal\config\Tests\ConfigEntityStatusTest
  - Drupal\config\Tests\ConfigEntityStatusUITest
  - Drupal\config\Tests\ConfigEntityStorageTest
  - Drupal\config\Tests\ConfigEntityTest
  - Drupal\config\Tests\ConfigEntityUnitTest
  - Drupal\config\Tests\ConfigEventsTest
  - Drupal\config\Tests\ConfigExportImportUITest
  - Drupal\config\Tests\ConfigExportUITest
  - Drupal\config\Tests\ConfigFileContentTest
  - Drupal\config\Tests\ConfigImportAllTest
  - Drupal\config\Tests\ConfigImporterTest
  - Drupal\config\Tests\ConfigImportRecreateTest
  - Drupal\config\Tests\ConfigImportUITest
  - Drupal\config\Tests\ConfigImportUploadTest
  - Drupal\config\Tests\ConfigInstallTest
  - Drupal\config\Tests\ConfigInstallWebTest
  - Drupal\config\Tests\ConfigLanguageOverrideWebTest
  - Drupal\config\Tests\ConfigModuleOverridesTest
  - Drupal\config\Tests\ConfigOtherModuleTest
  - Drupal\config\Tests\ConfigOverridesPriorityTest
  - Drupal\config\Tests\ConfigOverrideTest
  - Drupal\config\Tests\ConfigSchemaTest
  - Drupal\config\Tests\ConfigSingleImportExportTest
  - Drupal\config\Tests\ConfigSnapshotTest
  - Drupal\config\Tests\DefaultConfigTest
  - Drupal\config\Tests\Storage\DatabaseStorageTest
  - Drupal\config\Tests\Storage\FileStorageTest
  - Drupal\system\Tests\Entity\ConfigEntityImportTest
  - Drupal\system\Tests\Entity\ConfigEntityQueryTest

To:

Tests to be run:
  - Drupal\system\Tests\Entity\ConfigEntityImportTest
  - Drupal\system\Tests\Entity\ConfigEntityQueryTest

Haven't sorted why yet, but it's all the tests that belong to config rather than system that don't get run. I checked and the script doesn't move the tests somewhere strange or anything... haven't managed to debug it yet.

Here's the functional changes only, for review and reroll convenience. The script in #22 can be run from the root of the Drupal installation to move all default config to the new paths.

xjm’s picture

LOL. Okay, the script #22 moves the config module's .info.yml and routing files into an install dir.

xjm’s picture

Status: Needs work » Needs review
FileSize
89.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,371 pass(es), 1 fail(s), and 0 exception(s). View
940 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

This should get us to the point that all those nice tests are found and start failing. ;)

The last submitted patch, 29: config-2233787-29.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
90.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,444 pass(es). View
920 bytes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Title: Split /config directory into /schema, /install, etc. to more clearly outline what gets used when » Move default configuration into mymodule/config/install to clarify its purpose

Retitling, since the /schema subdirectory already exists.

Draft change record: https://drupal.org/node/2234799

xjm’s picture

Alright, looks like this is ready for review. :) Here's just the non-renames again.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -95,6 +95,7 @@ public function installDefaultConfig($type, $name) {
         if (is_dir($config_dir)) {
    
    @@ -104,11 +105,14 @@ public function installDefaultConfig($type, $name) {
    +        if (is_dir($config_install_dir)) {
    

    Out of curiosity, when does /config exist without /config/install?

  2. +++ b/core/lib/Drupal/Core/Config/InstallStorage.php
    @@ -169,7 +169,7 @@ public function getComponentNames($type, array $list) {
       protected function getComponentFolder($type, $name) {
    -    return drupal_get_path($type, $name) . '/config';
    +    return drupal_get_path($type, $name) . '/config/install';
    

    Shame this isn't public, then we could use this in those other places and wouldn't need to hunt them down next time.

xjm’s picture

Out of curiosity, when does /config exist without /config/install?

See core/modules/entity/config. Anything that provides schemata (or eventually, language overrides) but no default config of its own.

Shame this isn't public, then we could use this in those other places and wouldn't need to hunt them down next time.

Yeah, I started adding a couple public methods at first, but decided it'd be out of scope and wasn't really sure what.would be clean anyway.

mdrummond’s picture

Title: Move default configuration into mymodule/config/install to clarify its purpose » Move default configuration into mymodule/config/install or my theme/config/install to clarify its purpose

This makes the issue title pretty long, but the original discussion about this was related to themes, so just want to make sure this issue is tackling theme config too.

xjm’s picture

Title: Move default configuration into mymodule/config/install or mytheme/config/install to clarify its purpose » Move default configuration into extension/config/install to clarify its purpose

Yes, it is. Profiles too.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
FileSize
92.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,536 pass(es). View
1.64 KB

Found a couple more things that will silently pass even though they're looking in the wrong directory. Definitely another reason to consider making this API rather than string concatenation all over the place. Thoughts? What's the best way to do that?

The tests should also be reworked to fail if the module installs default config that the test didn't find, so we can catch things like this in the future.

xjm’s picture

Assigned: xjm » alexpott

@alexpott is going to take a look at this next.

xjm’s picture

Issue tags: +Needs tests

Forgot to tag in #45.

xjm’s picture

#2184387: Remove SchemaStorage could make this less ugly and pave the way for happy public API.

alexpott’s picture

FileSize
92.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,666 pass(es). View

Rerolled due to #2184387: Remove SchemaStorage landing.

Due to conflict the only interdiff's possible are a bit misleading.

alexpott’s picture

FileSize
5.92 KB
95.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,541 pass(es). View

How about we just some class constants so that there is only one place where this is hardcoded - well there is config/schema in core.services.yml but I don't see how that can be avoided.

Now we're using the same class constant everywhere assertModuleConfig would fail if default configuration provided by the module did not exist.

sun’s picture

+++ b/core/lib/Drupal/Core/Config/InstallStorage.php
@@ -23,6 +23,16 @@
+  const CONFIG_INSTALL_DIRECTORY = 'config/install';
...
+  const CONFIG_SCHEMA_DIRECTORY = 'config/schema';

I don't think there is a point in having a CONFIG_ prefix for a constant on a class that is part of the Config component... Can we rename those? E.g.:

DIRECTORY_INSTALL
DIRECTORY_SCHEMA

And, perhaps a generally understood abbreviation might even be acceptable in this case?

DIR_INSTALL
DIR_SCHEMA

xjm’s picture

I was thinking about constants too; I think that's perfect.

We already have CONFIG_ACTIVE_DIRECTORY and CONFIG_STAGING_DIRECTORY, so it makes sense to be consistent with that. DIRECTORY_INSTALL doesn't make any sense; adjectives come before nouns in English so that would make INSTALL the noun.

xjm’s picture

+++ b/core/lib/Drupal/Core/Config/InstallStorage.php
@@ -23,6 +23,16 @@
+   * Extension sub-directory containing configuration schema.

Probably should be "containing configuration schemata schemas."

Now we're using the same class constant everywhere assertModuleConfig would fail if default configuration provided by the module did not exist.

But how can we confirm that we haven't missed any other places where we were hardcoding the directory? Is there any way we can expose the test coverage? Or, in particular, nothing failed when I missed changing UpdateModuleHandler, so I wonder if that has any config at all.

Other than that, I'm really liking the look of this.

xjm’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -95,6 +95,7 @@ public function installDefaultConfig($type, $name) {
     $config_dir = drupal_get_path($type, $name) . '/config';
+    $config_install_dir = $config_dir . '/install';
     if (is_dir($config_dir)) {
       if (is_dir($config_dir . '/schema')) {

@@ -104,11 +105,14 @@ public function installDefaultConfig($type, $name) {
+        if (is_dir($config_install_dir)) {

Shouldn't we use the constants in here too?

sun’s picture

The "consistency" is actually one of the problematic parts; the CONFIG_*_DIRECTORY constants (1) do not contain actual pathnames, they are identifiers for two types of config directories that are expected to exist (whereas there can be additional, custom config directories), and (2) the resolved directories are full relative/absolute pathnames, whereas the constants introduced here are subpathnames/subdirectories. In short, they're not the same at all and thus consistency would only lead to confusion.

On the second "linguistic" issue, PHP constants are almost always (1) namespaced with a grouping prefix (to disambiguate from other possibly existing constants) and (2) have the identifier/subject last. For example, PDO, or just simply HEAD.

For class constants, it does not make sense to include the component name in constants, as that would only result in needless repetition (like PDO::PDO_FETCH_ASSOC or Title::TITLE_CHECK_PLAIN).


Note that #2190723: Add a KeyValueStore\FileStorage to replace e.g. ConfigStorage introduces a Config\FileStorageFactory::getExtension() method which yields the default config directory of a given extension (complementing getActive() + getStaging() on the factory).

— However, that cannot be used here (yet), because that patch also changes the architectural design of InstallStorage itself (essentially turning it into a clustered key/value store that operates on multiple storage bins instead of just one).

xjm’s picture

The class is not Config, but various subclasses of FileStorage. And DIRECTORY_INSTALL is not comprehensible. The names in the patch are fine.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Config/ExtensionInstallStorage.php
@@ -32,7 +32,7 @@ class ExtensionInstallStorage extends InstallStorage {
+  public function __construct(StorageInterface $config_storage, $directory = self::CONFIG_INSTALL_DIRECTORY) {

+++ b/core/lib/Drupal/Core/Config/InstallStorage.php
@@ -43,7 +53,7 @@ class InstallStorage extends FileStorage {
+  public function __construct($directory = self::CONFIG_INSTALL_DIRECTORY) {

It would be great to make that static:: just in case someone really overrides it.

alexpott’s picture

FileSize
95.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,631 pass(es), 1 fail(s), and 0 exception(s). View

@dawehner so that leads to Fatal error: "static::" is not allowed in compile-time constants

Rerolled due to stark.settings.yml and seven.settings.yml have been deleted.

diff 2233787.50.patch 2233787.58.patch
1226,1229d1225
< diff --git a/core/themes/seven/config/seven.settings.yml b/core/themes/seven/config/install/seven.settings.yml
< similarity index 100%
< rename from core/themes/seven/config/seven.settings.yml
< rename to core/themes/seven/config/install/seven.settings.yml
1234,1237d1229
< diff --git a/core/themes/stark/config/stark.settings.yml b/core/themes/stark/config/install/stark.settings.yml
< similarity index 100%
< rename from core/themes/stark/config/stark.settings.yml
< rename to core/themes/stark/config/install/stark.settings.yml
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great work!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 58: 2233787.58.patch, failed testing.

xjm’s picture

Assigned: alexpott » xjm

Rerolling.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.79 KB
102.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,920 pass(es), 2 fail(s), and 0 exception(s). View

Rerolled ... new tour in locale and menu module became menu_ui module.

Removed needs tests since config installation and module installation is extensively tested - plus now the ConfigInstaller::installDefaultConfig() also makes use of the class constants.

Status: Needs review » Needs work

The last submitted patch, 62: 2233787.62.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.14 KB
105.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,870 pass(es). View

Fixed the tests... there was a new theme settings test and I did not fix the new locale tour :(

dawehner’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -92,36 +92,36 @@ public function installDefaultConfig($type, $name) {

@@ -92,36 +92,36 @@ public function installDefaultConfig($type, $name) {
+    // If the module provides configuration schema clear the definitions.

I guess we should not talk about modules directly here.

Beside from that +1

alexpott’s picture

FileSize
729 bytes
105.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Fixed

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Fixed

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Config/InstallStorage.php
index 0000000..6781539
--- /dev/null

--- /dev/null
+++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.php

Bad merge :(

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.23 KB
99.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,928 pass(es). View

Good spot!

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! I was wondering why the patch size jumped, that explained it.

alexpott’s picture

Issue tags: +Avoid commit conflicts

This patch is ripe for reroll problems

  • Commit f0bed14 on 8.x by webchick:
    Issue #2233787 by alexpott, xjm | tim.plunkett: Move default...
xjm’s picture

Status: Reviewed & tested by the community » Fixed
xjm’s picture

I've updated all the listed change records and the handbook pages.

Status: Fixed » Closed (fixed)

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