Problem

  • Default configuration files of modules may contain UUIDs from the originating site on which the config was created.
  • These UUIDs of newly created configuration objects are not universally unique, but identical on all sites instead.

Proposed solution

  1. Force-create a new UUID for each configuration entity object that is imported from default configuration during initial installation of an extension.

Notes

  • Given this and other existing diverging behavior of the default config import process:
    1. The original assumption that the default config import vs. the active/staging store synchronization would be the same is no longer really true.
    2. Next to custom business logic like force-creating new UUIDs, the default config also uses a single, non-swappable file format.
    3. It's probably time to decouple of the default config import code from the config syncing code. Which probably allows us to kill the new isSyncing() flag, too.
Files: 
CommentFileSizeAuthor
#6 drupal8.config-default-uuid.6.patch953 bytessun
FAILED: [[SimpleTest]]: [MySQL] 59,925 pass(es), 2 fail(s), and 0 exception(s). View
#2 drupal8.config-default-uuid.2.patch734 bytessun
FAILED: [[SimpleTest]]: [MySQL] 60,041 pass(es), 2 fail(s), and 0 exception(s). View

Comments

beejeebus’s picture

"Default configuration files of modules may contain UUIDs from the originating site on which the config was created."

i don't think that's true, and should be treated as a bug.

"It's probably time to decouple of the default config import code from the config syncing code. Which probably allows us to kill the new isSyncing() flag, too."

what does this look like?

sun’s picture

Status: Active » Needs review
FileSize
734 bytes
FAILED: [[SimpleTest]]: [MySQL] 60,041 pass(es), 2 fail(s), and 0 exception(s). View

Perhaps we can do it as simple as this?


@beejeebus:

Interpreting it as a "bug" is probably correct. But at the same time, it is also poor DX, because we require all developers to manually edit and manipulate the default configuration files of their installation profiles, modules, and themes.

Enforcing this is a no-brainer for the system and just a single line of code. That's a very cheap win to (1) guarantee universally unique UUIDs under all possible circumstances and (2) vastly improve DX.

The entire default config import process still appears to be located in the single procedural config_install_default_config() function.

beejeebus’s picture

Status: Needs review » Needs work

alexpott, yched and i were talking about this in IRC today, and generally came to the conclusion that blanking the UUID seems fine. i'm kinda meh about it, so i guess we can be more forgiving rather than throw an exception.

this patch needs to take in to account the fact that entities can specify a UUID key, so we can't just look for 'uuid' :-\

other than that, all this patch needs is a test and i'd happily RTBC it.

yched’s picture

As discussed in IRC, not sure what's best:
- enforcing manual cleanup of yml shipped in default config to remove UUIDs is a DX burden
- allowing leftover UUIDs and and silently ignoring them is misleading - "Yeah, there's a UUID in there, but don't trust it, it's going to be discarded. Yeah, you can trust the rest".

Fully agreed on clearly separating the "default config import" & "config sync" code paths, Those are really different processes with very different logic and requirements and nasty cases. In all the recent mind bending discussions, I've tried hard to talk about them as distinct processes with distinct names :)

I don't see how isSyncing() can go away, though. It's used in the sync process so that code like MyConfigEntityType::postSave() or hook_entity_xxx_create() can avoid trigering race conditions. See #2069373: Race conditions on import if CUD on ConfigEntity A triggers changes in ConfigEntity B. Not related to default config objects.

sun’s picture

@beejeebus:

entities can specify a UUID key, so we can't just look for 'uuid'

The Configuration Entity system (cf. ConfigEntityBase/ConfigStorageController) hard-codes a UUID property for all configuration entities.

In preparing this patch, I double-checked the existing code, and it appears that we're not consistently relying on $entity->{$this->uuidKey} from the entity info.

I'm actually not sure whether it makes sense to continue to make the uuid property customizable, instead of hard-coding a uuid property name for all configuration entities.

Though that might be OT here and a different discussion, so if necessary, I'm happy to adjust the patch to look up the uuidKey from the entity info instead.

@yched:

allowing leftover UUIDs and and silently ignoring them is misleading

That's a valid concern. However, I guess the best course of action to prevent that would be a new scaffolding facility to "Export this config entity here into default config for my module", which would export the config entity and remove the UUID before writing — would obviously work on writable filesystems only, but perhaps a start to improve DX and prevent the situation from happening.

In any case, I think it makes sense to enforce this behavior within the system, so that there is an actual guarantee for the expected behavior. Especially if it's as simple as this.

sun’s picture

Status: Needs work » Needs review
FileSize
953 bytes
FAILED: [[SimpleTest]]: [MySQL] 59,925 pass(es), 2 fail(s), and 0 exception(s). View

Respecting a custom uuidKey, I assume the code would have to look like this.

The last submitted patch, 2: drupal8.config-default-uuid.2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: drupal8.config-default-uuid.6.patch, failed testing.

beejeebus’s picture

re the uuidKey, i'd be ok with killing that feature also. i'd support doing that in this patch, but i'd want that to be ok with yched before we do it.

yched’s picture

As a personal opinion, I'd approve killing the uuidKey flexibility too...

But that looks like a far reaching change (across all config + content entity types ?), for which @fago & @Berdir would be the ones to ask for buy-in, more than myself.

In short, big derailing potential IMO :-)

Berdir’s picture

Not sure about uuidKey, what we do need to make sure is that it continues to be optional for non-configuration entities, e.g. aggregator feed items only have an externally-provided guid key and not a uuid.

alexpott’s picture

alexpott’s picture

Issue tags: +Needs tests

We should either proceed with this or add a test to confirm that UUIDs are maintained during a config install

xjm’s picture

alexpott’s picture

I think we should proceed with this issue but only enforce an empty UUID when installing modules or themes. Profiles should get a pass because maybe its a distribution like OpenAtrium and they want to be have a direct link between distribution provided config and the active configuration.

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed

Discussed with alexpott and postponed to 8.1.x. This problem is not as severe now as it was when the original issue was filed, thankfully.

https://www.drupal.org/core/d8-allowed-changes

xjm’s picture

Issue tags: +Triaged D8 major
jweowu’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.