Problem/Motivation
Based on a rather frustrating IRC discussion, we apparently aren't all on the same page about the use of UUIDs in config entities. (See #1969698-9: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data).)
Proposed resolution
Discuss. Details to follow.
Remaining tasks
Identify the usecases for UUIDs, the problems with using them, and alternatives.
Related issues
#2077435: Use a yml file to create the forum vocabulary
#2106243: Use yml files to create the forum module's comment and taxonomy term fields
Comment | File | Size | Author |
---|---|---|---|
#58 | 1969800_58.patch | 34.21 KB | chx |
#58 | interdiff.txt | 957 bytes | chx |
#52 | 1969800_51.patch | 32.88 KB | chx |
#48 | 1969800-48.patch | 30.84 KB | tayzlor |
#46 | modified-configs.txt | 2.88 KB | herom |
Comments
Comment #1
xjmOne usecase for UUIDs (of many) is helping a site owner understand when a piece of configuration is something they already have configured that they want to update, versus something that is a "different" entity that they may or may not want to replace their existing data with.
@D34dMan posted these scenarios for our consideration:
Comment #2
xjmMy perspective, and what led me to "discovering" #1969698: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data), is that part of the purpose of CMI is to allow us to create truly robust installation profiles and distributions. IMO, out of the box, an installation profile should initially be exactly the same everywhere I install it.
Additionally, I think that it makes a ton of sense for there to be a canonical UUID that identifies "the
view.view.frontpage
that ships with the node module in 8.0" or "themy_foo
field instance that mymodule provides by default" or "the Magic Pony content type that Super Awesome Distro provides". This was also @heyrocker's suggestion to resolve #1969698: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data) when I described the problem to him last week.Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedshort version: we need UUIDs in default config.
longer version.
during install, i think it makes sense to say "the config system will not override an existing config thingie file". during module install, we will only create new config thingies, we will not overwrite existing ones.
if a module ships thingie.foo.yml, and thingie.foo.yml exists in the active config, we can do one of:
- block the module install, with an appropriate message like 'can't install, because config thingie.foo.yml already exists'
- proceed with the install, but don't change thingie.foo.yml in the active store, and show a message like 'i didn't install thingie.foo.yml, because it already exists'
either way, from the config system's point of view, module install is about creating new thingies only. (and obviously, we don't support delete/rename etc during module install.)
contrast this to import.
during import, updates/deletes/renames of existing config thingies must be supported. at the end of an import, we *must* have thingie.foo.yml updated to the version from the staging dir, end of story.
for the simple update case, unique machine names are all we need. we don't need UUIDs to just update from staging.
so, why do we need UUIDs? because we are assuming that for some sub-systems, there is a big difference between an update and a 'delete-create', and a big difference between a 'rename' and a 'delete-create'. so we need to build a system that allows such sub-systems to know the difference, and do the right thing.
but wait, there's more! :-)
we want to handle module enable/disable events during import. during a module install, we copy stuff from a module's config dir into the active config dir. we absolutely want the UUIDs for that default config to match across environments, right? so, if we don't ship UUIDs in the default config, how do we achieve that?
so yeah, UUIDs in default config. let's do that.
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedOK, so if we ship with UUIDs, then does it matter what format they are in? The UUID generator in core already supports 2 implementations and there can be more. The UUID that gets written to contrib files could very well depend on whatever UUID implementation the maintainer is using.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedi'm not sure i understand the concern in #4.
are UUIDs created by one implementation not compatible with others?
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedI think the issue is that they might not look the same, thats all. Pretty minor.
Comment #7
xjmRestoring tags.
Comment #8
Gábor HojtsyNote that we recently fixed #1964254: Configuration schemas missing langcode and uuid at places, so any removal of uuids should take care of config schemas. Also any additions should check they are also in the schema for the given entity.
Comment #9
yched CreditAttribution: yched commentedUUIDs are crucial to the config import process, they are the only way to tell between "update" and "delete + create new with same name".
Field API might be the only ConfigEntity in core today for which the difference is crucial, mostly because other current systems do not really care between update & delete/create since they are basically just "definitions of some runtime behavior" while fields have associated persistent data to maintain. But core could see other similar cases, and contrib most definitely will.
The current shape of the import process is :
- config system interprets the set of imported config into a series of create, update, delete
- it then calls each configEntity storage controller: "here, this item is new, this item is to be updated, this item is to be deleted, deal with it"
So it's not the task of each subsystem to sort out whether an update is really a delete/create, this has to be handled upstream, and UUIDs are the only way.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedre "So it's not the task of each subsystem to sort out whether an update is really a delete/create, this has to be handled upstream, and UUIDs are the only way." from #9 - yes, i agree completely.
the config system should figure out delete/create vs update.
Comment #11
andypostFiled issue for standard profile fields #1970206: Move fields and instances to standard profile config
Comment #12
larowlanfwiw custom_block module uses custom block uuids in its block plugin ids, instead of using the serial id.
That way the config entities it creates reference the record in {custom_block} table by uuid instead of serial id.
Note also #1757452: Support config entities in entity reference fields
Comment #13
xjmWRT to #12, I think that's actually different because those are UUIDs referencing content entities stored in configuration entities, which is already something we do in a number of other places (like shortcut) without handling it properly, and fail to do in other places (like taxonomy). This issue though is about UUIDs for the configuration entities themselves, and when and whether we provide them in default config.
Comment #14
swentel CreditAttribution: swentel commentedJust adding some things to this issue:
#1892634: Notice and error when trying to import block: another example why UUID's are so important and why I still think the patch #1969698: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data) is not our problem, and not only related to Field API. Note, the issue is relatively old so I'm not sure if it's still the case. It's just that it's so easy to ship something with the same machine name, but different settings and 'break' your site, where 'break' can be a simple string change to complete breakdown.
Another related issue: http://drupal.org/node/1609418#comment-6459520 - pointing directly to the comment. I've been mulling about a 'human friendly solution + uuid in config file names' this evening and came to the same idea as in that comment. IMHO it's not as scary and still readably, and almost a 100% guarantee we can give for D8 to make configuration (whether it's create, update or delete) work perfect :)
- edit - so the comment also points to a direct call in PHP code, I think we can skip that, somehow ..
so yeah, I'm with yched and beejeebus, let's discuss this more in Portland :)
Comment #15
xjmFollowup: #1989674: Throw an exception in Entity::save() if the UUID is changed.
Comment #16
xjmFor sake of full disclosure, here are the concerns @webchick outlined in the other issue:
Comment #17
xjmThanks for linking #1609418: Configuration objects are not unique; somehow I wasn't following that one.
Comment #18
gddJust wanted to poke in here, since I was just doing a site demo and ran into this issue. I am basically 100% in agreement with the above observations that default config must contain a UUID. Webchick's point about machine names is only valid if we remove the ability to edit machine names after creation (that situation is the whole reason we need UUIDs in the first place.) I would actually be OK with this solution to be honest, but it would require a lot more discussion obv.
My situation was similar to the one xjm ran into in the original issue. I installed Drupal, made some changes, then took a fresh install and copied the entire source active config into the dest staging config directory. When I went to synch I had a hundred changes that were nothing but UUIDs, and the import failed on the field issue.
Whatever the solution, if people can't wholesale copy their entire config directory from one site to another without encountering this situation then I kind of think the entire thing is pointless.
Comment #19
gddWe had a long meeting around this at DrupalCon Portland consisting of myself, alexpott, xjm, swentel, yched, effulgentsia, beejeebus and several others. Long story short, everyone came to the agreement that default config needs to have UUIDs. The reasons and implications have been expanded on in great detail above.
So what we need to come out of this is a patch to all default config that adds a UUID, and I believe we also need some code in the upgrade path that detects if the config being upgraded is actually an implementation of default (based on machine name) and adds the existing UUID into it if so. Non-default config with the same machine name as default config get the default config's UUID, I think this is a rare edge case anyways.
Comment #20
xjmThanks @heyrocker!
And it just so happens I have the beginnings of such a patch from #1969698: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data). :)
Comment #21
xjmHere's some.
Comment #23
swentel CreditAttribution: swentel commentedDo we also add them to the default config in test modules ?
Added some more and removed them from the field test files just to be consistent with the rest in case we don't add them to test files.
Also, those failures pass locally for me.
Comment #25
swentel CreditAttribution: swentel commentedOh sweet irony
Comment #26
swentel CreditAttribution: swentel commentedComment #27
aspilicious CreditAttribution: aspilicious commentedI think this issue is ready. Or not?
Comment #28
aspilicious CreditAttribution: aspilicious commentedHoops upgrade path code is missing, see @heyrocker
Comment #29
gddIn Portland we discussed that if any piece of config on the D7 site has a machine name that matches the machine name of a piece of default config in D8, then it should be considered that config and have the D8 default config's UUID added to it rather than having a new one generated. Yes this leaves an edge case where someone could have possibly created a new field with the same machine name as a D8 default field, but I'm honestly not overly concerned.
Comment #30
xjmComment #31
oadaeh CreditAttribution: oadaeh commentedMarked #1892634: Notice and error when trying to import block as a duplicate of this.
Comment #32
tayzlor CreditAttribution: tayzlor commentedUpdated patch which adds in some more uuids that are required.
Comment #33
tayzlor CreditAttribution: tayzlor commentedComment #34
tayzlor CreditAttribution: tayzlor commentedDidnt notice the tags for upgrade path + tests, setting back to needs work.
Comment #35
xjmBecause of #1969698-99: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data), this is at least major, if not critical.
Comment #36
xjm@alexpott and I just discussed this, and we decided that it actually doesn't make sense to add an upgrade path to make the UUIDs the same for D7 data. In that situation, the canonical source for those fields is not the default config directory that ships with Drupal 8.
So, let's add these as they are. I think the next step is then to fail validation on installation if the UUID is not already present in the config entity, to ensure that we don't inadvertently add more default entities without UUIDs in the future.
Comment #37
swentel CreditAttribution: swentel commentedSee https://drupal.org/node/1969698#comment-7640335
We need default config files for article and page body as well.
Comment #38
andypostsuppose same should be done for forum module. body field must be added only for content types created via ui. probably that needs tests
Comment #39
mtiftTagging
Comment #40
mtiftI think the files that need UUIDs contain the text "id:" (including tests and install profiles with config), end in ".yml", and do not currently contain "uuid". Therefore, I will try and find the files that need updating using:
grep -rl "id: " core/ | grep .yml | xargs grep -L uuid
Comment #41
yched CreditAttribution: yched commented#40 might help, but this won't give you all of them
- they don't necessarily contain 'id:' - using 'id' as 'the name if the id property" is only a convention, I don't think all of core respects it
- they might already contain 'uuid' (field.instance.*.yml files have a 'field_uuid' property). You probably want to look for '^uuid'
Comment #42
mtiftThis script is not perfect, and the results required a fair amount of cleanup, but it helped me find more files in need of UUIDs and essentially explains the reasons for the updated patch:
Comment #43
mtiftComment #44
mtiftI found more that need default UUIDs using this method:
This will provide you with a list of files that need UUIDs. You can get a UUID to add to the attached patch by clicking "View Differences."
Also, these (and others, probably) do not provide default config and will need special treatment:
Comment #45
herom CreditAttribution: herom commentedI'm doing this.
Comment #46
herom CreditAttribution: herom commentedunassigning myself.
couldn't figure out how to deal with the ones without default config.
but for anyone who can, here's a list of configs that were modified, after following #44's method.
the first two words is the config prefix (e.g. breakpoint.breakpoint). the rest is the config id.
Comment #47
lnunesbrThe patch #44 is failing (on HEAD revision of 8.x) due some refactoring on issue #2082499.
Actually other files seems to be renamed as well, like menu config files for example.
Comment #48
tayzlor CreditAttribution: tayzlor commentedHere's a re-rolled patch that should apply that also includes a couple more UUIDs that were missing. We still need to think about the ones without default config.
Comment #49
Deciphered CreditAttribution: Deciphered commentedNot trying to derail this issue, but this issue is related #2100043: Dynamically generated default YML use randomized UUIDs.. (Cross posted on recommendation Beejeebus).
Comment #50
chx CreditAttribution: chx commentedThis blocks #2106171: Write tests for simple configuration deployment scenario which is critical.
Comment #52
chx CreditAttribution: chx commentedAdded the and fixed a couple casts while in there.
Comment #53
dawehnerPerfect!!
Comment #54
webchickAn alpha4 without working CMI would be a very sad alpha4.
Comment #56
webchickThat's a bogus fail. Re-testing.
Comment #57
webchick#52: 1969800_51.patch queued for re-testing.
Comment #58
chx CreditAttribution: chx commentedMoar UUIDs needed. Also, no reason this is not RTBC.
Comment #59
mtiftI agree. RTBC.
Comment #60
larowlanRelated:
#2106243: Use yml files to create the forum module's comment and taxonomy term fields
#2077435: Use a yml file to create the forum vocabulary
Comment #60.0
larowlanUpdated issue summary.
Comment #61
Anonymous (not verified) CreditAttribution: Anonymous commentedwould love to see this go in, it's holding up #2106171: Write tests for simple configuration deployment scenario.
Comment #62
webchickSorry, meant to do this before babysitting duty but didn't quite make it. :)
Committed and pushed to 8.x. Thanks!
There was also a small follow-up commit to fix a "rue" to "true".
Comment #63
webchickOh, and this'll need a change notice.
Comment #64
dawehnerDo we have a way to force module developers to put in a UUID into their default entities?
Comment #65
tim.plunkettWe could make config importer refuse to run. We should also add $uuid to ConfigEntityBase
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commented#64 sounds like it should be a separate issue.
Comment #67
chx CreditAttribution: chx commentedThis needs to be rolled back. #2110615: Do not ship with default UUIDs
Comment #67.0
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #68
Gábor HojtsyIn the interest of being able to close this down, I added https://drupal.org/node/2153709 as a change notice. Note the title: "Add default UUIDs to shipped configuration entities (for now)". Continue debating in #2110615: Do not ship with default UUIDs and #2133325: Create a site UUID on install and only allow config sync between sites with the same UUID.