Updated: Comment #11

Problem/Motivation

At the moment all configuration entities created with a UUID. This creates issues for programatically created configuration entities during module install because if you enable the same module on two sites then the resulting configuration is different. Issues tagged with default configuration have been explored whether or not it is possible to provide defaults for all configuration created during install time. This approach has several problems:

  • We have to set a special flag during module install that modules have to use to decide whether or not it is appropriate to create additional configuration entities. See https://drupal.org/comment/8160213#comment-8160213
  • Conceptually every site having configuration entities with the same UUIDs is mind boggling.

Proposed resolution

Change UUID to a creation ID that during module, profile and theme install is predictable but uses UUIDs during regular run time.

Remaining tasks

Agree that the approach in the initial patch works.

Then:

  • discuss whether we should rename the UUID key to creation_id
  • fix field instances to not refer to field's UUID (separate issue)
  • agree approach to creation IDs in default config - either force unset in config_install_default_config() or throw an error in config_install_default_config() if present

User interface changes

None

API changes

  • Possible removal of UUID from configuration entities
  • Possible rename of UUID configuration entity key to creation_id
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
21.26 KB

The patch attached changes how the configuration entity uuid is generated to use the CreationHash service instead of the UUID service. Thereby limiting the amount of change to make the approach easy to review.

claudiu.cristea’s picture

Seems related?

alexpott’s picture

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/Breakpoint.php
@@ -119,6 +120,22 @@ class Breakpoint extends ConfigEntityBase implements BreakpointInterface {
+  public static function preCreate(EntityStorageControllerInterface $storage_controller, array &$values) {
+    // Build an id if none is set. Since a particular name can be used by
+    // multiple theme/modules we need to make a unique id.
+    if (empty($values['id'])) {
+      $values['id'] = $values['sourceType'] . '.' . $values['source'] . '.' . $values['name'];
+    }
+
+    // Set the label if none is set.
+    if (empty($values['label'])) {
+      $values['label'] = $values['name'];
+    }
+  }

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.php
@@ -101,6 +102,15 @@ public function __construct(array $values, $entity_type) {
+  public static function preCreate(EntityStorageControllerInterface $storage_controller, array &$values) {
+    if (empty($values['id'])) {
+      $values['id'] = $values['sourceType'] . '.' . $values['source'] . '.' . $values['name'];
+    }
+  }

The changes to the breakpoint module are necessary because config entities were being created without an ID. The ID was being created during the save() method. I think we should not allow this and in fact raise an exception if a configuration entity is created without an ID.

tim.plunkett’s picture

I haven't formed an opinion on the approach yet, but I noticed one thing:

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigInstallWebTest.php
@@ -92,4 +92,45 @@ function testIntegrationModuleReinstallation() {
+    $this->assertFalse(strpos($custom_config_entity_creation_hash, 'ModuleHandler::install'), "Runtime generated configuration creation hash does not contain ModuleHandler::install");

You need assertIdentical(FALSE, here, otherwise 0 will pass here.

Status: Needs review » Needs work

The last submitted patch, 1: 2138559.1.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
8.52 KB

Patch attached fixes the breakpoint tests by changing where the creation hash is generated to after the postCreate() and fix the field related failures by replacing dots with double underscores in the creation hash since the uuid value is being used in field names and dots break MySQL.

alexpott’s picture

FileSize
24.54 KB

Adding patch that somehow went missing

Anonymous’s picture

alexpott++

liking the look of this patch. i'm not sure what we should enforce re. UUIDs in default config, but we should certainly remove all the existing UUIDs in shipped config we have now. just for shits and giggles and stuff.

yched’s picture

Still wrapping my head around the approach, short review for now.

  1. +++ b/core/core.services.yml
    @@ -89,6 +89,11 @@ services:
    +  config.creationhash:
    

    nitpick: I think we tend to use underscores to separate words in service ids (or dots, not sure which applies when...)
    Also, a hash is not really a service. hash_generator ?

  2. +++ b/core/lib/Drupal/Core/Config/CreationHash.php
    @@ -0,0 +1,71 @@
    +use Drupal\Component\Uuid\UuidInterface;
    +
    +
    +/**
    + * Defines a configuration creation hash creator.
    + *
    + *
    

    Extra empty lines

  3. +++ b/core/lib/Drupal/Core/Config/CreationHash.php
    @@ -0,0 +1,71 @@
    +  protected $uuid;
    

    $uuidGenerator ? or, other classes seem to call this $uuidService.

  4. +++ b/core/lib/Drupal/Core/Config/CreationHash.php
    @@ -0,0 +1,71 @@
    +  public function setPrefix($prefix) {
    +    if (empty($this->prefix)) {
    +      $this->prefix = $prefix;
    

    This looks dangerous, it means that after a module_install, the 'ModuleHandler::install' will be used for any config entity created in the rest of the request / php CLI process.

  5. +++ b/core/lib/Drupal/Core/Config/CreationHash.php
    @@ -0,0 +1,71 @@
    +    // We need to replace dots in the ids since the uuid is being used in field
    +    // and table names. Without this \Drupal\field\Tests\FieldImportDeleteTest
    +    // does not pass.
    +    return $this->getPrefix() . '__' . str_replace('.', '__', $id);
    

    Hm - so $config_entity->uuid is not a UUID anymore, but a
    [prefix string, that in some cases is a UUID]__[config id] ?

alexpott’s picture

FileSize
24.1 KB
28.12 KB

Slight change of approach in this patch

  • Renamed CreationHash class to CreationId since we are not creating a hash
  • Fixed all of Yched's feedback - although the patch is still (ab)using the UUID field as I want to get agreement and understand before doing this work
  • The CreationId class now maintains a stack of prefixes which solves Yched's fear about what happens after a ModuleInstall (point 4) and also ensures that the if a profile or module enables a theme in its hook_install then the configuration creation IDs generated are consistent with just enabling that theme through the UI
  • Tried to improve the documentation of the CreationId class to explain what is going on
alexpott’s picture

Title: Replace UUIDs in configuration entities with a creation hash » Replace UUIDs in configuration entities with a creation ID
Issue summary: View changes

Update summary to not refer to hash as we are not creating a hash but we are creating a special creation ID.

Anonymous’s picture

had a chat with alexpott in #drupal-contribute about this patch.

he explained why we need to track nested calls to use a default prefix, because code in hook_install() can enable modules or themes. i suggested we use a default config prefix for the creationId, and just track the depth of calls.

that's the only thing i think needs changing in this patch.

alexpott’s picture

FileSize
13.09 KB
29.81 KB

Now that #1851018: Improve breakpoint configuration implementation. has landed we can test that config entities created during a theme enable are done correctly.

Also this patch refactors the CreationId class to not allow custom prefixes to be set and just uses a class constant. It stores the nesting level since this is important due to the fact that anything can happen in hook_install - we enable other modules, enable themes - so it is possible another call to useStaticPrefix() might be made and we only want to revert to using UUIDs once the nesting level is back to 0.

This patch was also a reroll due to the breakpoints patch.

Anonymous’s picture

yay, green, i think this is looking good.

now we need yched and others to have some time to look at it before RTBC.

alexpott’s picture

#14 well hang on, if we decide that this is the solution then I suggest this issue also needs to:

  • rename the uuid key to creation_id
  • rename the uuid() method to creationId()
  • agree approach to creation IDs in default config - either force unset in config_install_default_config() or throw an error in config_install_default_config() if present

The current patch just does the minimum so that we can review and think about the solution.

Also we should fix it so field instances do use UUIDs to link to the field but do this in a separate issue. Plus I'd like to remove the sucky replacement of dots in the creationId. The following is ugly.

+++ b/core/lib/Drupal/Core/Config/CreationId.php
@@ -0,0 +1,99 @@
+    return $this->getPrefix() . '__' . str_replace('.', '__', $id);
derheap’s picture

Yes, I think UUID should be removed from config and replaced by something like this aproach – autogenerated on install.

Comming from teaching students in using Drupal as a CMS and using it since Drupal 5, I do not see requireing a UUID in a module config works out:

  • UUID are fine for tracking unique bits of information – like nodes. The should generated automatically in the background.
  • Forcing a developer, a designer to touch them directly – generate one, copy, paste it in a config is not a good idea. It removes the human-editing possibilites from yaml. It introduces a new, maybe scarry id – this can be a hurdle. Dealing with config import problems, it took me some times to understand whats going on here, and how UUIDs are working, formated and generated. I have seen them before, but it was a machine thing: they are there for helping the code doing something; I dont have to deal with them.
  • Enfrocing UUIDs in config will end up in copy and pasting existing or badly made up UUIDs like 11111111-1111-1111-1111-111111111111. Both is complety backwards to the desired effect. And it will happen: humans are lazy, me too.

If we need a id for tracking changes – than it should be the same for the same base config of a yaml file.

The following (and real) workflow should be possible: two developers working on the same site install the the module on their dev sites, one pushes an enables the module on the staging server, the other changes the config of the module and pushes them to the staging server. The new config should just work without ID-conflicts.

And one more workflow: I often reuse (or want to reuse) bits form different sites, a view, a path-auto pattern, a content type.
I want to copy the active config of site A to stagging, copy the reused bits form site B to config and import.
That would be a good DX. But therefore the IDs must get out of my way. In the past I often recreated the same view a more then once. I thought CMI should make that easier not harder.
So, please go one with this patch.

yched’s picture

One case that breaks in this approach is:
- module foo ships a field named 'bla'
- module bar ships a totally different field, also named bla'
- install module foo - deploy on prod
- delete the field 'bla', install module bar - deploy on prod
--> the field 'bla' being deployed is considered as being the same that already exists on prod (same creationHash), so is imported as an "update" rather than a "delete/create".

In short, this approach means that "identity" (being "the same configentity" vs "a different one") is based on "unique UUIDs" for stuff created at runtime (UI...), but strictly on name for stuff created at install time. I still feel it's weird :-/

alexpott’s picture

#17 well actually config install only allows a create and not an update so this should not occur. I feel that in this instance we should produce an exception and fail.

alexpott’s picture

Actually had a look at how config_install_default_config() handles default config that already exists in active configuration. Basically it just moves on and does and says nothing. I think this is a mistake but this is for a follow up. The potential bug explained by @yched in #17 does not exist as the the second default config does not get created or imported at all. I think we should create warning and tell the user that the default configuration has not imported - but this is an existing bug and is not this issues concern.

Created an issue for this: #2140511

yched’s picture

Er, no, the bug exists ?
In the steps described in #17, on the second config sync step, the staging config set contains the record for a 'bla' field, which has the same creationID as the 'bla' field in active although they are totally unrelated (e.g different field type). When that field.bla yaml file is processed, exception.

I really think "if it has has been created at install time (by whoever), then it is 'the same' the moment it has the same name" is flawed. I'm mulling over an alternative notion of "creationHash".

Anonymous’s picture

creationHash is our arbitrary invention.

can't we just add module/theme as part of the 'namespace' for it? then this problem goes away?

amateescu’s picture

can't we just add module/theme as part of the 'namespace' for it? then this problem goes away?

Not really, different versions of a module can provide different default field config files.

yched’s picture

alexpott’s picture

Status: Needs review » Closed (won't fix)

Discussed with yched, heyrocker, xjm, and mtift. Yched came up with the following scenario

  1. (On dev) Enable a module that provides a default config entity (eg. a field) called bla
  2. (On live) Deploy this configuration.
  3. (On dev) Disable module and delete the default config entity bla.
  4. (On dev) Enable another module that provides a default config entity (same type) also called bla
  5. (On live) Deploy this configuration - this will then try to update/rename the bla config entity because the creation IDs will match whereas it should do a delete and recreate.

Agreed to close this issue in favour of exploring other solutions which will be documented on #2121751: [META] Making configuration synchronisation work

Anonymous’s picture

ok. it seems adding a module/theme key would solve #24? but also, meh, i'll roll with whatever plan.

yched’s picture

@beejeebus: yes, but then what this prevent is reorganizing who ships which config in your custom site modules (i.e "reorganize / split your features" in D7).
If you do that, new setups cannot sync config with the old setups, because the creation hashes of the config entities will be different.

xjm’s picture

Issue tags: -beta blocker