Needs work
Project:
Drupal core
Version:
main
Component:
configuration entity system
Priority:
Major
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
21 Dec 2013 at 21:48 UTC
Updated:
7 Jun 2024 at 12:22 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
Anonymous (not verified) commented"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?
Comment #2
sunPerhaps 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.Comment #3
Anonymous (not verified) commentedalexpott, 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.
Comment #4
yched commentedAs 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.
Comment #5
sun@beejeebus:
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:
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.
Comment #6
sunRespecting a custom uuidKey, I assume the code would have to look like this.
Comment #9
Anonymous (not verified) commentedre 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.
Comment #10
yched commentedAs 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 :-)
Comment #11
berdirNot 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.
Comment #12
alexpottComment #13
alexpottWe should either proceed with this or add a test to confirm that UUIDs are maintained during a config install
Comment #14
xjm#2201501: Throw an exception if a UUID is defined in default configuration was closed as a duplicate of this issue.
Comment #15
alexpottI 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.
Comment #16
xjmDiscussed 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
Comment #17
xjmComment #18
jweowu commentedComment #34
andypostThere's no reason to postpone it, moreover faced it in #3401876-4: Fix bugs in update path surfaced by config validation