Config import assumes that 'config_prefix' contains one dot only

To some extent this is related to #1806178-2: Remove code duplication for hook_config_import_*() implementations of config entities which could remove the prefix concept entirely.

The code in question is this:

      foreach ($source_storage->listAll($entity_info['config_prefix']) as $config_name) {
        list(, , $id) = explode('.', $config_name);
        $manifest_data[$id]['name'] = $config_name;
      }

I don't see how "only one dot" is called out here, the explode result is taken from after the second dot, so it assumes it contains exactly TWO dots, not one. Which is *my* problem coming here, since this now fails the regions module patch from #1813910: Add region module to Drupal core (for editable responsive layouts) which operates with 'region' as the config prefix (note no dot), so resulting config files are region.body.yml, region.footer.yml, etc. The region module does not define any other config objects, so it would be overkill to name them region.region.body.yml just to satisfy this code. That seems to be the only workaround for now though, so I am wondering if we have a standard here that my patches do not follow (and where it is documented so I can point people there in the future if they wonder why the repetition in the config keys).

This blocks most dynamic layout patches and the SCOTCH patch for page layout assignment at #1787942: Allow assigning layouts to pages which uses page.front_page.yml, etc. keys.

One refers to the prefix, not the full config object name.

Anyway, you're running exactly into the problem that has been introduced by the config import UI patch.

The code needs to be corrected to take the part after the config prefix as the entity id.

I fixed this in the other config issue about default config installation already, but since it is hindering your issue, it probably makes sense to separate out that fix here.

So something functionally equivalent to this I assume? (In effect remove the first but only the first occurrence of the config prefix regardless of what it is).

mmm, nope, the code needs to retrieve the part after the config prefix (plus dot), but needs to perform an exact match (and must not match and entire suffix). The other issue/patch was:
#1776830-28: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API

+  foreach ($config_entity_types as $entity_type => $entity_info) {
+    $manifest_config = config('manifest.' . $entity_info['config_prefix']);
+    $id_index = count(explode('.', $entity_info['config_prefix']));
+    foreach ($target_storage->listAll($entity_info['config_prefix'] . '.') as $config_name) {
+      $parts = explode('.', $config_name);
+      $manifest_config->set($parts[$id_index] . '.name', $config_name);
+    }
+    $manifest_config->save();
   }

$parts[$id_index] holds the config entity ID.

@sun: I'm not getting this. 'config_prefix' SHOULD appear at the start of the config key since, we are asking with $source_storage->listAll($entity_info['config_prefix']) right? So we are iterating through those items in $config_name. I could also do substr() or preg_replace() matching the string at the start or other tricks. The explode in the patch was supposed to remove the config name only at the start. I feel like I'm not getting something here...

k, let me explain:

+++ b/core/includes/config.inc
@@ -35,7 +35,8 @@ function config_install_default_config($type, $name) {
       foreach ($source_storage->listAll($entity_info['config_prefix']) as $config_name) {
...
+        list(, $id) = explode($entity_info['config_prefix'], $config_name, 2);

1) config_prefix contains the entire prefix that is used for the dynamic config objects.

2) config_prefix can contain 1-n dots/periods. The amount of dots is not enforced anywhere and should not be enforced.

3) config_prefix only denotes a prefix; the suffix that follows can contain more than one part. (or more than one dot)

What we need to extract is the part of the config object name that immediately follows the config_prefix, whereas the config_prefix is of arbitrary length and can contain an arbitrary amount of dots, and not everything after the config_prefix still belongs to the part that is the ID.

In all of these, we need to match "ID":

foo.ID
foo.ID.bar
foo.bar.ID
foo.bar.ID.baz
foo.bar.baz.ID
foo.bar.baz.ID.qux
...

Thus, that's:

# Assuming: foo.bar.baz.ID.qux

# 3 = count(parts in "foo.bar.baz")
$id_index = count(explode('.', $entity_info['config_prefix']));

$parts = explode('.', $config_name);
# $parts[0] = foo
# $parts[1] = bar
# $parts[2] = baz
# $parts[3] = ID
# $parts[4] = qux

$id   = $parts[$id_index];
# ID [==       3]

Marked #1901988: Importing a ConfigEntity with a dot in the machine name fails as duplicate of this issue.

Quoting myself from there:

A central problem space here is that you're making the assumption that everything after the config prefix belongs to the ID.

So far, I never wanted to make that assumption — because it will inherently prevent us from horizontally extending config entity objects; i.e., like this:

mymodule.type.foo               « Only this is the entity config.
mymodule.type.foo.enhancement
mymodule.type.foo.ng
...
mymodule.type.foo.othermodule   « Essentially this.

whereas the config_prefix is of arbitrary length and can contain an arbitrary amount of dots, and not everything after the config_prefix still belongs to the part that is the ID.

What is the use case for multiple dots in a config prefix? Why don't we just standardize on one dot, and enforce that?

Also, what is the use case for something like foo.bar.baz.ID.qux, where this is information after the ID that isn't the ID?

See #1901988: Importing a ConfigEntity with a dot in the machine name fails for the opposite issue.

Okay, I cross-posted with sun marking my issue a duplicate.

sun, if you disagree with the approach of that issue, I invite you to fix the bugs exposed by this patch.

A reasonable fix remains in the other issue.

1) I'm absolutely certain that we do not want to enforce a magic standard for config entity prefixes to contain a single dot only. An entity config object like foo.super.sub.bar should be possible.

2) I also think that a horizontal extensibility should be a possibility for config entity types to decide to be used, if applicable and useful. This essentially circles back into the discussion of how Module A can add settings for a config entity of Module B. Very concrete use-cases are:

node.type.article
node.type.article.entity_display.teaser
node.type.article.comment

It was the block plugins patch that suddenly introduced config entity IDs that may consist of dots — but the architectural validity of that was never formally discussed and agreed on, so let's not make the mistake and interpret that precedent as a rule. :)

Related: #1760358: Provide a way to extract the ID from a config object name

For starters, Can we meet in the middle and allow any length prefix by doing something like this?

Sure. Let's see how #12+#15 fairs.

Though it is $id = substr($config_name, strlen($entity_info['config_prefix'] . '.'));, not $id = substr($config_name, strlen($entity_info['config_prefix']));

Oops, yeah. That's better :) I think I was thinking of the method....

Assigning to sun to review the approach taken here. I think this is good; definitely better than what we have now.

Rerolled due to #1889854: Config import breaks on protected entity properties landing... and added comment to code to explain why Config::clear() can not be used during ConfigStorageController::delete()

+++ b/core/includes/config.incundefined
@@ -35,7 +35,7 @@ function config_install_default_config($type, $name) {
+        $id = substr($config_name, strlen($entity_info['config_prefix'] . '.'));

It seems like its worth making a wrapper function for this, even though its just one line. It would make the code more self-documenting, and it would make future changes easier to address, since it seems likely we aren't done with this topic.

ConfigStorageController already has getConfigPrefix(), so it seems like a good place to add a static method for this.

Working on this.

Here is an extractID method on the ConfigStorageController. Better names for this welcome.

How about idFromConfigName()... Or something like that... I think we need to say what we are extracting from.

That's a better name :) Let's go with that.

Sorry, forgot to change the method name in the test.

And also forgot the comment above in the test..

I hate to bikeshed method names, but I'd like to see it start with a verb like get.

Feel free to change the patch.

s/idFromConfigName/getIDFromConfigName/g

As well as some other clean-ups of the various methods in use replaced by this helper.

Alright. This issue seems to be unnecessarily blocked by my earlier considerations.

I guess it is OK to allow for dotted config entity IDs for now. We can still change our minds later.

However, if we do so, then we definitely need to clarify this somewhere. I'm not sure what the best place is, but quite potentially, the initial class' phpDoc of ConfigStorageController might be most appropriate. In there, we'd need to add a clarification along the lines of this:

Configuration object names of configuration entities are compound of two parts:
- config_prefix: A string denoting the owner (module/extension) of the configuration object, followed by arbitrary other namespace identifiers that are declared by the owning extension; e.g., 'node.type'. The config_prefix does NOT contain a trailing dot. It is typically defined by the entity type's meta-data/annotation.
- ID: A string denoting the actual entity ID within the entity type namespace; e.g., 'article'. Entity IDs may contain dots/periods. The entire remaining string after the config_prefix in a config name forms the entity ID. Additional or custom suffixes are not possible.

Makes sense?

Speaking of, I've seen many core developers running into the pitfall that the config_prefix does not contain a trailing dot. Meanwhile, I think we should do this:
#1914742: Make config_prefix always contain a trailing dot

Aside from that, happy to move forward here. However, some remarks on the latest patch:

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -288,10 +304,14 @@ public function delete(array $entities) {
+      // Remove the entity from the manifest file. Entity id's can contain a dot
+      // so we can not use Config::clear() to remove the entity from the
+      // manifest.
+      $manifest = config('manifest.' . $this->entityInfo['config_prefix']);
+      $manifest_data = $manifest->get();
+      unset($manifest_data[$entity->id()]);

Huh?

Doesn't this directly imply that there is a critical bug in the config manifest architecture?

How can there be any kind of config file that goes through Config and which uses dots in config keys names...? This usage essentially presents a huge conflict with #1701014: Validate config object names

If we do not have a (major) issue for that yet, then we have to create one. It is very important that we are tracking major issues like this, as soon as we discover them, especially at this point in the release cycle.

+++ b/core/modules/config/tests/config_test/config/config_test.dynamic.test.config.yml
@@ -1,4 +1,4 @@
-id: default
+id: test.config

Can we rename this to "dotted.default" ?

"config" is a bit confusing + unnecessary in a config name of a config system test.

Also noteworthy:

By changing and renaming this existing file, we're implicitly removing the explicit config system test coverage for config names without a dot.

Good unit testing practices would dictate that we need to add a separate, dotted default config file, instead of changing the existing, so that there is explicit test coverage for both possible variations.

Can we create a separate 'dotted.default' next to 'default'?

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewTestData.php
@@ -8,6 +8,7 @@
+use Drupal\Core\Config\Entity\ConfigStorageController;

@@ -52,8 +53,8 @@ public static function importTestViews($class, $modules = array()) {
-        foreach ($source_storage->listAll() as $config_name) {
-          list(, , $id) = explode('.', $config_name);
+        foreach ($source_storage->listAll('views.view.') as $config_name) {
+          $id = str_replace('views.view.', '', $config_name);

Huh? :-)

Added that to the docblock with a couple grammar tweaks.

So, yes, manifests incorrectly allow a key that could contain dots. Look at manifest.block.block.yml in your install, for example.
If that's wrong, then it needs a new issue to change manifests.

I renamed the file, but I did not add a second one. I don't see the benefit, and didn't see which tests were important to duplicate.

In converting all of the "list(, , $id) = explode('.', $name);" occurrences, I realized that the importTestViews() method needlessly scans for all files. In theory, the only files in test_views directory are views, but this is cleaner.

See #1915272: Move _breakpoint_delete_breakpoints() into BreakpointGroupStorageController for the follow-up so that all calls to the new getIDFromConfigName() are in storage controllers, where they belong.

Thanks. I will remind you about the loss of explicit test coverage for entity IDs without dots when the first bug report is filed. ;)

#1915800: Keys in manifest files contain dots, but Config does not allow for dots in config keys

I had to look at this a couple of times but this looks fine and the shortcut diff is encouraging. Committed/pushed to 8.x.