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 inconfig_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
Comment | File | Size | Author |
---|---|---|---|
#13 | 2138559.12.patch | 29.81 KB | alexpott |
#13 | 10-12-interdiff.txt | 13.09 KB | alexpott |
#10 | 2138559.10.patch | 28.12 KB | alexpott |
#10 | 6-10-interdiff.txt | 24.1 KB | alexpott |
#7 | 2138559.6.patch | 24.54 KB | alexpott |
Comments
Comment #1
alexpottThe 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.
Comment #2
claudiu.cristeaSeems related?
Comment #3
alexpottThe 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.
Comment #4
tim.plunkettI haven't formed an opinion on the approach yet, but I noticed one thing:
You need
assertIdentical(FALSE,
here, otherwise 0 will pass here.Comment #6
alexpottPatch 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.
Comment #7
alexpottAdding patch that somehow went missing
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedalexpott++
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.
Comment #9
yched CreditAttribution: yched commentedStill wrapping my head around the approach, short review for now.
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 ?
Extra empty lines
$uuidGenerator ? or, other classes seem to call this $uuidService.
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.
Hm - so $config_entity->uuid is not a UUID anymore, but a
[prefix string, that in some cases is a UUID]__[config id] ?
Comment #10
alexpottSlight change of approach in this patch
Comment #11
alexpottUpdate summary to not refer to hash as we are not creating a hash but we are creating a special creation ID.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedhad 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.
Comment #13
alexpottNow 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.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedyay, green, i think this is looking good.
now we need yched and others to have some time to look at it before RTBC.
Comment #15
alexpott#14 well hang on, if we decide that this is the solution then I suggest this issue also needs to:
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.
Comment #16
derheap CreditAttribution: derheap commentedYes, 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:
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.
Comment #17
yched CreditAttribution: yched commentedOne 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 :-/
Comment #18
alexpott#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.
Comment #19
alexpottActually 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
Comment #20
yched CreditAttribution: yched commentedEr, 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".
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedcreationHash is our arbitrary invention.
can't we just add module/theme as part of the 'namespace' for it? then this problem goes away?
Comment #22
amateescu CreditAttribution: amateescu commentedNot really, different versions of a module can provide different default field config files.
Comment #23
yched CreditAttribution: yched commentedFYI: started work on #2143519: Allow FieldInstance yml files to refer to the Field by field name rather than by field_uuid
Comment #24
alexpottDiscussed with yched, heyrocker, xjm, and mtift. Yched came up with the following scenario
Agreed to close this issue in favour of exploring other solutions which will be documented on #2121751: [META] Making configuration synchronisation work
Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedok. it seems adding a module/theme key would solve #24? but also, meh, i'll roll with whatever plan.
Comment #26
yched CreditAttribution: yched commented@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.
Comment #27
xjm