Move default configuration into extension/config/install to clarify its purpose

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.

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

Worth discussing as a beta target, I think.

config_install makes sense to me.

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

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.

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.

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.

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

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.

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.

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

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

Maybe :

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

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. (?)

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

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.

Nice suggestions. Changing title.

Rolling a start at a patch.

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

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.

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.

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

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

Retitling, since the /schema subdirectory already exists.

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

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

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.

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.

Yes, it is. Profiles too.

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.

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

Forgot to tag in #45.

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

Rerolled due to #2184387: Remove SchemaStorage landing.

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

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.

+++ 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

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.

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

+++ 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?

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

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

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

@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

Great work!

Rerolling.

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.

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

+++ 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

Fixed

Fixed

Good spot!

This patch is ripe for reroll problems

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