Problem/Motivation
Saving an existing field with a different UUID, including by attempting to import even an absolutely identical copy of the field that has a different UUID, puts a Drupal installation in an unrecoverable state.
- Install 8.x standard.
- Emulate a field machine name/UUID conflict by copying
field.field.field_tags.yml
from the active configuration directory to the staging directory. - In the staging directory, change the
uuid
offield.field.field_tags.yml
slightly, e.g. just change the first character. - Visit
admin/config/development/sync
and click import. - Your site is broken. There's no escape. Any and every page load will show:
Error message Drupal\field\FieldException: Attempt to create an instance of unknown field old-config-uuid-here-nskjdfhisuhdfwf in Drupal\field\Plugin\Core\Entity\FieldInstance->__construct() (line 259 of /Applications/MAMP/htdocs/d8git/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.php). The website has encountered an error. Please try again later.
Enjoy: https://www.dropbox.com/s/ogag30dfpqqkaqb/the_most_excellent_and_lamenta...
Note: This video also illustrates a related featurebug that is mostly just a minor WTF once this issue is resolved, which is that an exactly identical fresh installation has different UUIDs for all the same default configuration.
Cause
The reason the site is unrecoverably broken is that the exception is being thrown in a menu item access check:
- Standard and Minimal include a preconfigured instance of the Tools menu.
- Node adds
node/add
to the Tools menu. - The access callback for
node/add
checksnode_access()
for thecreate
op for each type with a fake node object. - If passed a fake node object,
node_access()
callsentity_create()
to make it into a proper Node entity. entity_create()
constructs the node bundle's field instance, which has a UUID that no longer matches the field UUID.
Proposed resolution
The field API should disallow changing the UUID of an existing field on save, since field instance data depends on this UUID.
Remaining tasks
- Consider moving the field UUID check out of
FieldInstance::__construct()
intoFieldInstance::save()
, so that a data integrity issue does not completely break Drupal. - Provide test coverage ensuring the proper exceptions are thrown, both with an API save and functionally via a config import.
Comment | File | Size | Author |
---|---|---|---|
#94 | 1969698-94.patch | 15.69 KB | damiankloip |
#94 | interdiff-1969698-94.txt | 3.17 KB | damiankloip |
#92 | 1969698-92.patch | 15.6 KB | damiankloip |
#92 | interdiff-1969698-92.txt | 1.65 KB | damiankloip |
#88 | 1969698-88.patch | 15.47 KB | damiankloip |
Comments
Comment #1
xjmI'll start by trying to add UUIDs to all the things.
Comment #2
xjmFields and instances aren't provided as default config yet, so that needs to happen too. And I don't know wtf Breakpoint is doing. But this is a start.
Comment #3
xjmI looked over all the files that appear as "changed" when staged from one fresh standard install to another. Among them, the following are not supplied as default config:
Adding UUIDs to the remainder shortly.
Comment #4
xjmHere's all of the rest. Note that I haven't yet looked at config provided by test modules, modules not enabled in standard, or minimal. Also note that
shortcut.set.default.yml
has UUIDs for links that will differ between the two sites, but this is a separate problem since those are content entities.Overall, much better... from 70 to 38. Er.
I think we might want to validate config entities and throw exceptions if they're missing UUIDs. Maybe a sub-issue of the general validation issue.
Comment #4.0
xjmUpdated issue summary.
Comment #5
xjmWhat will break?
Comment #7
xjmA lot, apparently. :D
Comment #8
xjmSo @webchick and @Crell are very concerned about what they consider to be a DX impact from adding UUIDs to default config entities. @webchick suggested removing UUIDs from config entities altogether. If that were the case, we would have to change the way the field API works.
The situation in HEAD is that some of our exported config entities have them, and some don't; and additionally three modules do not ship with their default config entities.
Comment #9
webchickSummary of IRC discussion...
For those who are not as familiar with CMI and don't "grok" the problem from the issue summary / what's in the video...
Basically, what's happening is a module or profile that defines a ConfigEntity defines will create default config such as:
When this gets written out during installation / module enable / whatever, it'll end up in the site's active directory as:
The UUID is unique (obviously), and so if the dev / prod sites are both two separate installations, they will end up with two different UUIDs. I was not aware of this auto-magic UUID generation thing before, so I guess that went in in some patch I forgot about. :P~ I might even have committed it. ;)
The "Synchronize configuration" page on prod therefore gets confused, and thinks every single one of the ConfigEntity files between dev and prod changed, and tries to "helpfullly" replace all of them when you sync. I'm not sure exactly which one of them caused the fatal error/exception that blew up the site in the video, but basically this is Un-Good, because if this happens with a field, it will cause data loss. There was also a concern expressed about what happens if two modules each define a View or something called "myevents"... xjm was saying that ideally, it would inform you you had two and let you decide which one to delete.
So the proposal is to add UUIDs to default config so they can match between environments.
As a module author, for anything less trivial than say a View or something similar with 50,000 entries in it, I would be inclined to just make these default YAML files by hand like I do in other applications where I've used YAML. I mean, we picked YAML as the format in large part because it's a nice human-editable format. Having to make up a UUID and add it to the file is a pretty non-standard thing and annoying thing to have to do. (Apparently, if I called $entity->save() somewhere in my module, this would auto-generate one for me, which is great, but I wouldn't have known that trick had it not been told to me.)
And my worry is that distro authors (who are generally not as sophisticated as module authors) aren't going to understand this concept and are merely going to copy/paste files from core/standard to their own distros, change all of the other values in that file (id, name, etc.) but leave UUID alone because they don't know what it is and it looks scary, and then end up with real problems because the UUIDs match for totally different things, and possibly 3-4 of them. :P
So the question I posed on IRC is whether or not this could just be handled with just machine names, which are already unique, are easy to define by module authors, and will already match between dev and prod, and what the consequences would be if we did that. The end.
Comment #10
xjmRemoving UUIDs from config entities to resolve this issue seems really extreme to me. They're a really valuable feature. Also, the current field API relies on them. We should fix bugs by fixing bugs, not by removing features.
That said, my discussion with webchick lept way ahead of where we are right now, which is still at the "brainstorm possible solutions" phase and hasn't even gotten feedback from any impacted component maintainers other than myself. I didn't mean to involve a core maintainer; I meant to share a funny/scary video of how it's currently possible to royally hose your site through an entirely plausible use of CMI. :)
Comment #11
xjmAlso, if we want to discuss whether and how the config entity system should or shouldn't UUIDs, can we open a meta for that, and keep this issue focused on the bug in HEAD?
Comment #12
xjmAnd, to clarify, this issue does not propose adding UUIDs to default config entities. It proposes doing so consistently. Many default entities in HEAD already have them. Just, notably, not the major players. ;) (Those being Field, Views, and Blocks.)
Edit: Additionally, non-entity config is not affected.
Comment #13
xjmFiled #1969800: Add UUIDs to default configuration.
Comment #14
xjmRetitling to describe the bug. This issue really wasn't meant for public consumption yet.
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedI haven't thought deeply about this, but I suspect that having the module maintainer ship with a UUID is wrong. The UUID that gets written will depend on whatever UUID implementation that maintainer had when he created the configEntity. Furthermore, everyone who uses that module will get the same UUID in his DB. Thats stretches the usual definition of unique.
So my gut is to simply remove UUIDs from the exports in core and make that recommendation for contrib. We could add UUID stripping as an option for drush config-get.
Comment #16
xjmSo, I did some more testing, and the extra spectacular broken of this is exclusively due to fields at the moment (which makes sense, since fields are the only things so far that care what their UUIDs are).
If I install Drupal before fields as config, then most the UUIDs are silently overwritten by staging config from naked install A to naked install B, and there's just a persistent error about shortcut is caused by the content entity UUIDs referring to non-existent content entities.
Sounds like we need to consider this a field bug then. =/
Comment #17
xjm@moshe weitzman, could you comment on #1969800: Add UUIDs to default configuration?
Comment #18
xjmIf we fix the field API, the only WTF will be a zillion pieces of apparently "changed" configuration that differ only by a UID. (Similar to the CMI save-as-strings issue; the user sees a bunch of changes reported that he/she didn't make). It cannot, however, be the low-level Configuration system's responsibility to handle that--that's on ConfigEntity or the entity system.
Comment #19
xjmSo I'm having trouble reproducing this even with a web test, but here's the STR:
field.field.field_tags.yml
andfield.instance.node.article.field_tags.yml
from the active configuration directory to the staging directory.uuid
offield.field.field_tags.yml
slightly, e.g. just change the first digit. Change thefield_uuid
infield.instance.node.article.field_tags.yml
to the same value.admin/config/development/sync
and click import.Comment #19.0
xjmUpdated issue summary.
Comment #19.1
xjmUpdated issue summary.
Comment #19.2
xjmUpdated issue summary.
Comment #19.3
xjmUpdated issue summary.
Comment #19.4
xjmUpdated issue summary.
Comment #19.5
xjmUpdated issue summary.
Comment #19.6
xjmUpdated issue summary.
Comment #19.7
xjmUpdated issue summary.
Comment #20
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 #21
yched CreditAttribution: yched commentedThis is not a Field bug, Field API only reacts to imports as the config system tells him to : "here, this iem is new, this item has been updated, this item has been deleted, deal with it"
The interpretation of the set of imported config into a series of create, update, delete, including reasoning on UUIDs to tell an update from a delete/recreate, is made upstream by CMI.
Field API might be the only ConfigEntity in core today for which a bug in this area has nasty effects, 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 this does not make it a field bug.
Comment #22
yched CreditAttribution: yched commentedAlso, AFAIK, the intended feature of "config imports" has always been targetted for "imports between instances (live, staging, dev) of the same site", but I'm not sure that syncing config between two unrelated setups was ever part of the intended scope ?
Comment #27
xjm@yched, I think it is a field bug -- see attached. This resolves the issue. Working on tests.
All you have to do to completely break your site is try to import a field with a different UUID than an existing field of the same name. This breaks existing instances of that field. We should disallow that.
Comment #28
xjmTypo, obviously. Needs tests. :)
Also, the exception messages should be changed; they currently reflect the language from when I had thme in the constructor, which was not sufficient to prevent them from being imported, and was probably a bad idea anyway.
Comment #28.0
xjmUpdated issue summary.
Comment #29
xjmActually, field already implements a bunch of exception handling for UUIDs, which is why it's possible to completely break your site by simply staging the wrong file. See the updated STR in the summary. :)
Comment #30
xjmWe also might want to consider moving the "bad field UUID" check from
FieldInstance::__construct()
toFieldInstance::save()
.Comment #30.0
xjmUpdated issue summary.
Comment #31
xjmThis could actually happen any time you save a field with a different UUID, not just via a config import.
Comment #31.0
xjmUpdated issue summary.
Comment #31.1
xjmUpdated issue summary.
Comment #31.2
xjmUpdated issue summary.
Comment #31.3
xjmUpdated issue summary.
Comment #31.4
xjmUpdated issue summary.
Comment #31.5
xjmUpdated issue summary.
Comment #31.6
xjmUpdated issue summary.
Comment #32
xjm@yched, can you comment on #1969800: Add UUIDs to default configuration, since fields are the first config entities in core to care what their UUIDs are? That's the real issue to discuss for #21 and #22.
Comment #33
yched CreditAttribution: yched commentedOk, I probably mixed the two issues.
Patch here makes sense, but :
- shouldn't we rather make Field::$uuid protected with an accessor ?
- That's not really specific to Field API ConfigEntities. "UUID is not allowed to change after initial creation" seems like a generic behavior that makes sense for all ConfigEntities. How many other core or contrib modules will figure out the hard way (or worse, not figure out at all) that they need to duplicate similar code in their ConfigEntities ?
Comment #34.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #35
andypostAlso I think it needs to properly implement isNew() or id() method.
Filed #1970110: Should ConfigEntity::isNew() be based on id or uuid ?
Comment #36
tim.plunkettThat would require something like the UUID changes in #1959610: Remove public properties from entity classes
Comment #37
xjmYeah, I debated this with myself when I was debugging this originally. Right now, nothing else in core cares if you change its UUID, and some APIs might want to allow it. I think that the goal should be to fix fields for now--because, broken sites--and then decide whether to move this validation to the ConfigEntity level in another issue, because that's a much bigger task.
Comment #38
yched CreditAttribution: yched commentedSo, right, even if $uuid ends up protected, which would prevent runtime code from tampering with a field's UUID, we'd still need something like that for the case of config imports.
Fixed typo + inexistant var. Let's see how this behaves.
Works for me on principle - but even scoping Field API config entities for now, I'm not sure why similar checks wouldn't be needed for FieldInstance ? And if they are, adding them in both places sounds dangerously close to "they should be somewhere upstream" :-)
Comment #39
swentel CreditAttribution: swentel commentedThis is just babysitting to me and is code that doesn't belong in Field API. I'm inclined to won't fix, but I'll leave it for now.
As Alex Pott put it nicely at DrupalCamp London
We should broaden it to staging config as well :)
Comment #40
xjmThrowing an exception is the opposite of babysitting broken code. Babysitting would be trying to handle the bad UUID. If we make CMI so fragile that putting what looks to be exactly the right file in the staging directory puts Drupal in an unrecoverable state, Drupal 8 will fail. Period.
Comment #41
xjm@swentel, the STR in the summary are just an example. It has nothing to do with "hacking" staged config; that doesn't even make sense. You don't have to change the UUID by hand to trigger this problem--you just have to add any field with the same machine name as one on your site.
Comment #42
xjm#38: field-uuid-mismatch-1969698-38.patch queued for re-testing.
Comment #43
swentel CreditAttribution: swentel commentedSo talked this through with xjm on IRC, I still believe this isn't our problem completely, but I can live with it.
My first reaction was: that's valid for every config entity, but apparently none of the other config entities really bother about uuid's, that makes me scared a little tbh :)
So, fine for RTBC, we need to have a whiteboard at portland ! :)
- edit - actually, still not fine with it ..
Comment #44
xjmOkay, this is fascinating.
I was having a helluva time reproducing the site-annihilating exception in a test. It turns out that what causes the field UUID change to blow up every page of the site is... the tools menu block. Not just having the block module enabled. Not any other block. Just the tools menu. Oh Drupal.
Attached patches illustrate this. I used nodes for debugging because I spent some time fishing for a red herring in the
test_entity
type; I'll revert it to test fields and entities once I'm sure I'm not crazy.Edit: I'll also of course add proper tests for the exception itself; I just wanted to have an integration test as well.
Comment #45
xjmAha. It's because the tools menu includes the
node/add
path, which checksnode_access()
which callsentity_create()
.Good times.
Edit: I updated the summary.
Comment #45.0
xjmUpdated issue summary.
Comment #45.1
xjmUpdated issue summary.
Comment #45.2
xjmUpdated issue summary.
Comment #46
xjmHere's that followup to change it upstream instead:
#1989674: Throw an exception in Entity::save() if the UUID is changed
New patch with decoupled DrupalUnit tests shortly.
Comment #47
xjmHere we go.
This still needs test coverage for the reverse case where the UUID matches but the ID does not. I need to go to sleep, though, so I'll add that tomorrow. :)
Comment #48
xjmThese are clearly copypasta lies.
Comment #49
swentel CreditAttribution: swentel commentedThe thing is, a different UUID is actually a use case for fields, but should be done earlier in the import process, see #1740378: Implement renames in the import cycle.
So, in the end, we need to fix this upstream in config. So sorry, still not ok with this in.
Comment #50
xjmAnd, this is why we write tests.
current()
rather thanreset()
from somewhere else when I wrote the exception.current()
seems to always return false. Edit: I've read http://us3.php.net/current several times and can't sort why. So, whatever I copied that from might also have a bug.Comment #51
alexpottI'm not certain that we should be fixing this specifically for fields... I agree with #49 we need to fix this in import.
Liking the tests though :)
Comment #52
xjmDiscussed in IRC with @swentel, @alexpott, and @damiankloip. "A field maintainer, a config maintainer, and a views maintainer walk into a bar..."
I'm going to reroll this moving the exception and tests into ConfigEntity.
Comment #53
xjmSo, something is wrong with this, but the web test runner is just hanging instead of returning results. Hopefully testbot will do better.
I kept the tests in field for now to ensure the same cases were covered.
Comment #54
xjmOh, that's only half a change. I was in the process of changing this to sprintf().
Comment #56
xjmStill something bugged here.
Comment #57
xjmComment #58
yched CreditAttribution: yched commentedAn issue Field API is going to have if those checks are in ConfigEntityBase::save() is that Field::save() currently does :
1) do its own business-specific sanity checks
2) notify the storage backend (e.g create/update the SQL tables). The storage backend can reject the changes by throwing an exception - save() ends here, the config was not written, this is what we expect.
3) call parent::save() to write to yml
Now if parent::save() raises exceptions, save() dies, but sql storage has already been modified.
Should there be a separate ConfigEntityBase::check() method ?
Comment #59
xjmHrm, that's a tough one. That'd explain why it's fataling out.
Comment #60
yched CreditAttribution: yched commentedWell, apart from this "exception flow" issue :
- The added ConfigEntityBase::save() code never calls parent::save(), nothing won't ever save ;-)
- Since the parent EntityBase::save() just proxies to the storage controller, should those checks be made in the storage controller ?
Comment #61
BerdirYes, we have a different issue to discuss that but for now, Entity::save() is just a proxy and shouldn't contain any code, everything should be within the storage controller.
Comment #62
damiankloip CreditAttribution: damiankloip commentedMaybe a validate() method could work?
Comment #64
xjmYeah, I think adding a validate method and then calling that method in the storage controller might work. OTOH, shouldn't UUIDs be unique regardless of the storage?
Comment #65
damiankloip CreditAttribution: damiankloip commentedok, I think we should not test the whole staging workflow, as it seems we just want to test that a saved configuration entity with a changed uuid blows up and does not save it.
Comment #66
dawehnerI thought we import a config_test entity
Comment #67
damiankloip CreditAttribution: damiankloip commentedComment #69
damiankloip CreditAttribution: damiankloip commentedehmm.
Comment #71
damiankloip CreditAttribution: damiankloip commentedComment #73
tim.plunkettReviewed this, it needed a reroll after the controller injection.
Comment #75
tim.plunkettDrilling down into the factory too early is bad.
Comment #77
damiankloip CreditAttribution: damiankloip commentedI think this should fix things. It weeds out the parts of code that we are just copying/cloning config entities, and obviously saving the same UUID again. This makes this patch and test coverage even more important.
Comment #78
damiankloip CreditAttribution: damiankloip commentedFrick
Comment #80
damiankloip CreditAttribution: damiankloip commented#77: 1969698-77.patch queued for re-testing.
Comment #82
damiankloip CreditAttribution: damiankloip commentedI forgot about this puppy. Let's get it back on track.
Comment #84
damiankloip CreditAttribution: damiankloip commentedI think this should be postponed on #2019651: Add a QueryFactoryInterface for QueryFactory classes. So we can then type the Queryfactory classes properly.
Comment #85
damiankloip CreditAttribution: damiankloip commentedComment #86
damiankloip CreditAttribution: damiankloip commentedI actually don't think that patch I postponed this on will really help us :/ Sorry folks.
Here is a new reroll anyway.
Comment #88
damiankloip CreditAttribution: damiankloip commentedComment #89
andypostWhy this affects only config entities? The check makes sense for content entities as well
Maybe better to implement this check for all entities?
Comment #90
andypostExceptions should use format_string() - consistency++
Comment #91
xjm@andypost, we have #1989674: Throw an exception in Entity::save() if the UUID is changed, but we wanted to fix the major bug with config first and not block that on the EntityNG/ConfigEntity stuff.
Comment #92
damiankloip CreditAttribution: damiankloip commentedI agree with xjm here, let's fix this for configuration entities now. I changed the exceptions to use format_string.
Comment #93
dawehnerI'm wondering whether a different named exception like ConfigDuplicateUUID or ConfigUUIDConflict would be better together with a second one.
What is the reason that this is not part of ConfigEntityBase?
Comment #94
damiankloip CreditAttribution: damiankloip commentedSounds good to me, Let's go with a more descriptive exception name.
Regarding the second point, I already have plans to tackle this over in #2004336: Default UUID key in ConfigEntityType. As currently this is the pattern implemented in all configuration entities. Not just ImageStyle. So until that is solved, ImageStyle should have a uuid property.
Comment #95
dawehnerThank you for the explanation!
Comment #96
alexpottCommitted 356dc8b and pushed to 8.x. Thanks!
Comment #97
xjmYayyyy.
Comment #98
webchickHmmm. So post-this change, how does one properly initialize a dev/prod set up these days?
When I separately install a dev and prod site from copies of the same code base (since you need two differently-named config trees, and this is done on install), and then do my initial "copy all of the files in dev's active to prod's staging" and hit Import all, I get:
...and everything dies.
https://drupal.org/documentation/administer/config needs an update for how to deal with this situation, as well as the fact that we now deploy the entire active directory, not just individual files. That seems sort of change notice-ish, so moving to critical task + active.
Comment #99
xjm@webchick, this is because of #1969800: Add UUIDs to default configuration. When that issue is fixed, the problem you're encountering will be very rare.
The exception seems pretty self-explanatory to me. I guess it doesn't hurt to document it, though.
Comment #100
yched CreditAttribution: yched commentedUUIDS in default config files will only take care of config items that are shipped in, well, default config.
Problem is that "body" field is wired to be created automatically on creation of a node type (see NodeType::postSave() / node_add_body_field()).
core/profiles/standard/config does not contain yml files for body, only for "image" and "tags" fields. The body field & instances are created when importing the node.type.[article|page].yml default config.
Then, body field has different UUIDs on dev and prod if those site instances were installed from scratch separately, and errors like #98 will happen in config import.
Comment #101
xjmRe: #100 -- @yched and @alexpott and I just discussed this in NYC, and we agreed we need to ship the article and page default body fields with standard. Edit: Issue needed.
Comment #102
xjmSee also: #2041887: check that id does not exist before saving a new ConfigEntity (similar, but different)
Comment #103
swentel CreditAttribution: swentel commentedRe: #101 We can just tell in #1969800: Add UUIDs to default configuration to add those config files right ?
Comment #104
swentel CreditAttribution: swentel commentedAlso, wondering if anything is still needed here, I think we can close this one no ?
Comment #105
swentel CreditAttribution: swentel commentedRight, updated the page over at https://drupal.org/documentation/administer/config - not sure if it's good enough.
Comment #106
xjmhttps://drupal.org/node/1823576/revisions/view/2705604/2795385 is the difference on that doc, but I don't know how that addresses the first part of #98?
Comment #107
swentel CreditAttribution: swentel commentedFirst part being the copy and then install ? Isn't that going to fixed by #1969800: Add UUIDs to default configuration ?
Or even before, how to set up ? That doesn't seem to belong here, but in one of those workflow issues no ?
I could also be missing the entire point of course :)
Comment #108
catchComment #109
webchickAre docs not a gate anymore? :)
Comment #110
catchI think swentel already handled the main docs change. The rest seems like #1703168: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs or the linked issues. If this is still critical it needs an updated issue summary - I have no idea what's left here.
Comment #111
webchickFair enough. I'll go add my griping to that issue then.
Comment #112
webchickComment #113.0
(not verified) CreditAttribution: commentedUpdated issue summary.