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.

  1. Install 8.x standard.
  2. Emulate a field machine name/UUID conflict by copying field.field.field_tags.yml from the active configuration directory to the staging directory.
  3. In the staging directory, change the uuid of field.field.field_tags.yml slightly, e.g. just change the first character.
  4. Visit admin/config/development/sync and click import.
  5. 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:

  1. Standard and Minimal include a preconfigured instance of the Tools menu.
  2. Node adds node/add to the Tools menu.
  3. The access callback for node/add checks node_access() for the create op for each type with a fake node object.
  4. If passed a fake node object, node_access() calls entity_create() to make it into a proper Node entity.
  5. 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() into FieldInstance::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.
CommentFileSizeAuthor
#94 1969698-94.patch15.69 KBdamiankloip
#94 interdiff-1969698-94.txt3.17 KBdamiankloip
#92 1969698-92.patch15.6 KBdamiankloip
#92 interdiff-1969698-92.txt1.65 KBdamiankloip
#88 1969698-88.patch15.47 KBdamiankloip
#88 interdiff-1969698-88.txt1.29 KBdamiankloip
#86 1969698-86.patch14.18 KBdamiankloip
#82 1969698-82.patch13.68 KBdamiankloip
#82 interdiff-1969698-82.txt4.79 KBdamiankloip
#77 1969698-77.patch9.83 KBdamiankloip
#77 interdiff-1969698-77.txt3.63 KBdamiankloip
#75 config-uuid-1969698-75.patch7.58 KBtim.plunkett
#75 interdiff.txt1.52 KBtim.plunkett
#73 config-uuid-1969698-73.patch7.56 KBtim.plunkett
#73 interdiff.txt4.87 KBtim.plunkett
#71 1969698-71.patch5.61 KBdamiankloip
#71 interdiff-1969698-71.txt4.47 KBdamiankloip
#69 1969698-69.patch4.99 KBdamiankloip
#65 1969698-65.patch5.02 KBdamiankloip
#57 interdiff-sprintf.txt1.54 KBxjm
#56 config-entity-uuid-1969698-56-FAIL.patch5.84 KBxjm
#56 config-entity-uuid-1969698-56-PASS.patch7.47 KBxjm
#53 config-entity-uuid-1969698-52-FAIL.patch5.84 KBxjm
#53 config-entity-uuid-1969698-52-PASS.patch7.65 KBxjm
#50 field-uuid-1969698-49-FAIL.patch5.33 KBxjm
#50 field-uuid-1969698-49-PASS.patch9.65 KBxjm
#50 interdiff-field-uuid.txt7.99 KBxjm
#50 interdiff-docs.txt1.36 KBxjm
#47 field-uuid-1969698-FAIL.patch3.79 KBxjm
#47 field-uuid-1969698-PASS.patch7.39 KBxjm
#47 interdiff-instance.txt2.59 KBxjm
#44 field-uuid-1969698-tests-FAIL.patch1.82 KBxjm
#44 field-uuid-1969698-tests-PASS.patch1.83 KBxjm
#44 interdiff.txt709 bytesxjm
#38 field-uuid-mismatch-1969698-38.patch1.69 KByched
#38 interdiff.txt1.06 KByched
#27 field-1969698-26.patch1.75 KBxjm
#5 uuids-1969698-5.patch13.93 KBxjm
#5 interdiff-uuid-exception.txt718 bytesxjm
#4 uuids-1969698-4.patch13.23 KBxjm
#4 only_38.png25.87 KBxjm
#2 view-and-block-uuids-1969698-2.patch7.08 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Assigned: Unassigned » xjm

I'll start by trying to add UUIDs to all the things.

xjm’s picture

Status: Active » Needs review
FileSize
7.08 KB

Fields 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.

xjm’s picture

I 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:

breakpoint.breakpoint.module.toolbar.narrow
breakpoint.breakpoint.module.toolbar.standard
breakpoint.breakpoint.module.toolbar.wide
breakpoint.breakpoint.theme.bartik.mobile
breakpoint.breakpoint.theme.bartik.narrow
breakpoint.breakpoint.theme.bartik.wide
breakpoint.breakpoint.theme.seven.mobile
breakpoint.breakpoint.theme.seven.wide
breakpoint.breakpoint.theme.stark.mobile
breakpoint.breakpoint.theme.stark.narrow
breakpoint.breakpoint.theme.stark.wide
breakpoint.breakpoint_group.module.toolbar.toolbar
breakpoint.breakpoint_group.theme.bartik.bartik
breakpoint.breakpoint_group.theme.seven.seven
entity.display.comment.comment_node_article.default
entity.display.comment.comment_node_page.default
entity.display.custom_block.basic.default
entity.display.node.article.default
entity.display.node.article.teaser
entity.display.node.page.default
entity.display.node.page.teaser
entity.display.user.user.compact
entity.display.user.user.default
field.field.block_body
field.field.body
field.field.comment_body
field.field.field_image
field.field.field_tags
field.field.user_picture
field.instance.comment.comment_node_article.comment_body
field.instance.comment.comment_node_page.comment_body
field.instance.custom_block.basic.block_body
field.instance.node.article.body
field.instance.node.article.field_image
field.instance.node.article.field_tags
field.instance.node.page.body
field.instance.user.user.user_picture

Adding UUIDs to the remainder shortly.

xjm’s picture

FileSize
25.87 KB
13.23 KB

Here'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.
38 changed files when staging

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

What will break?

Status: Needs review » Needs work

The last submitted patch, uuids-1969698-5.patch, failed testing.

xjm’s picture

A lot, apparently. :D

xjm’s picture

So @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.

webchick’s picture

Summary 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:

 profiles/foo/config/user.role.administrator.yml:
 id: administrator
 label: Administrator
 weight: 2
 langcode: en

When this gets written out during installation / module enable / whatever, it'll end up in the site's active directory as:

sites/default/files/config_XXX/active/user.role.administrator.yml:
 id: administrator
 uuid: b8474e77-6317-4e8b-9922-344eea1358d1 # Some auto-generated UUID.
 label: Administrator
 weight: 2
 langcode: en

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.

xjm’s picture

Removing 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. :)

xjm’s picture

Issue tags: +Configurables

Also, 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?

xjm’s picture

And, 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.

xjm’s picture

xjm’s picture

Title: Configuration entities provided as default config should include UUIDs » UUIDs added to default config entities at installation can badly corrupt staged configuration

Retitling to describe the bug. This issue really wasn't meant for public consumption yet.

moshe weitzman’s picture

I 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.

xjm’s picture

Title: UUIDs added to default config entities at installation can badly corrupt staged configuration » UUIDs added to fields at installation can badly corrupt staged configuration

So, 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. =/

xjm’s picture

@moshe weitzman, could you comment on #1969800: Add UUIDs to default configuration?

xjm’s picture

If 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.

xjm’s picture

Title: UUIDs added to fields at installation can badly corrupt staged configuration » Field UUID changes can badly corrupt configuration
Component: configuration entity system » field system
Issue tags: -VDC, -Blocks-Layouts

So I'm having trouble reproducing this even with a web test, but here's the STR:

  1. Install 8.x standard.
  2. Emulate a field machine name/UUID conflict:
    • Copy field.field.field_tags.yml and field.instance.node.article.field_tags.yml from the active configuration directory to the staging directory.
    • In the staging directory, change the uuid of field.field.field_tags.yml slightly, e.g. just change the first digit. Change the field_uuid in field.instance.node.article.field_tags.yml to the same value.
  3. Visit admin/config/development/sync and click import.
  4. Your site is broken. There's no escape.
xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Note 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.

yched’s picture

Component: field system » configuration system

This 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.

yched’s picture

Also, 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 ?

xjm’s picture

Component: configuration system » field system
Status: Needs work » Needs review
FileSize
1.75 KB

@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.

xjm’s picture

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -345,6 +345,33 @@ public function save() {
+            '@field_id' => $matched_fied['id'],

Typo, 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.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

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.

Actually, 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. :)

xjm’s picture

We also might want to consider moving the "bad field UUID" check from FieldInstance::__construct() to FieldInstance::save().

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Title: Field UUID changes can badly corrupt configuration » Field UUID changes can badly corrupt field data

This could actually happen any time you save a field with a different UUID, not just via a config import.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

@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.

yched’s picture

Ok, 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 ?

Status: Needs review » Needs work

The last submitted patch, field-1969698-26.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Also I think it needs to properly implement isNew() or id() method.
Filed #1970110: Should ConfigEntity::isNew() be based on id or uuid ?

tim.plunkett’s picture

That would require something like the UUID changes in #1959610: Remove public properties from entity classes

xjm’s picture

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 ?

Yeah, 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.

yched’s picture

Status: Needs work » Needs review
FileSize
1.06 KB
1.69 KB

So, 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.

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.

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" :-)

swentel’s picture

This 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

"Don't hack core" is replaced by "Don't hack your active config"

We should broaden it to staging config as well :)

xjm’s picture

This 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.

Throwing 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.

xjm’s picture

@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.

xjm’s picture

swentel’s picture

So talked this through with xjm on IRC, I still believe this isn't our problem completely, but I can live with it.

you just have to add any field with the same machine name as one on your site.

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 ..

xjm’s picture

Okay, 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.

xjm’s picture

Aha. It's because the tools menu includes the node/add path, which checks node_access() which calls entity_create().

Good times.

Edit: I updated the summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Here'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.

xjm’s picture

Here 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. :)

xjm’s picture

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldUUIDConflictTest.phpundefined
@@ -0,0 +1,108 @@
+ * Contains \Drupal\field\Tests\FieldImportChangeTest.
...
+ * Tests updating fields and instances as part of config import.
...
+   * Tests importing an updated field instance.

These are clearly copypasta lies.

swentel’s picture

In the staging directory, change the uuid of field.field.field_tags.yml slightly, e.g. just change the first character.

The 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.

xjm’s picture

And, this is why we write tests.

  1. Moved the entity query for an existing field/instance with the same UUID up; this should happen regardless of whether the field is new or updated.
  2. Discovered the exception from the matched existing UUID wasn't being thrown because I copied current() rather than reset() 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.
alexpott’s picture

I'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 :)

xjm’s picture

Title: Field UUID changes can badly corrupt field data » ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data)

Discussed 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.

xjm’s picture

So, 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.

xjm’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -161,4 +162,45 @@ public function getExportProperties() {
+      throw new ConfigUUIDException(format_string(
+        'Attempt to save a configuration object %s with UUID %s when this UUID is already used for %s',
+        array(
...
+          'Attempt to save a configuration entity %s with UUID %s when this entity already exists with UUID %s',
+          array(
+            $this->id,
+            $this->uuid,
+            $original->uuid,
+          )

Oh, that's only half a change. I was in the process of changing this to sprintf().

Status: Needs review » Needs work

The last submitted patch, config-entity-uuid-1969698-52-PASS.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
7.47 KB
5.84 KB

Still something bugged here.

xjm’s picture

FileSize
1.54 KB
yched’s picture

An 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 ?

xjm’s picture

Hrm, that's a tough one. That'd explain why it's fataling out.

yched’s picture

Well, 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 ?

Berdir’s picture

Yes, 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.

damiankloip’s picture

Maybe a validate() method could work?

Status: Needs review » Needs work

The last submitted patch, config-entity-uuid-1969698-56-PASS.patch, failed testing.

xjm’s picture

Yeah, 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?

damiankloip’s picture

Status: Needs work » Needs review
FileSize
5.02 KB

ok, 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.

dawehner’s picture

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityUUIDConflictTest.phpundefined
@@ -0,0 +1,65 @@
+      $this->fail('Exception thrown when attempting to import a field definition with a UUID that does not match the existing UUID.');
...
+      $this->pass(format_string('Exception thrown when attempting to import a field definition with an ID/UUID combination that does not match existing data: %e.', array('%e' => $e)));

I thought we import a config_test entity

damiankloip’s picture

Assigned: xjm » Unassigned

Status: Needs review » Needs work

The last submitted patch, 1969698-65.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4.99 KB

ehmm.

Status: Needs review » Needs work

The last submitted patch, 1969698-69.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4.47 KB
5.61 KB

Status: Needs review » Needs work

The last submitted patch, 1969698-71.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.87 KB
7.56 KB

Reviewed this, it needed a reroll after the controller injection.

Status: Needs review » Needs work

The last submitted patch, config-uuid-1969698-73.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.52 KB
7.58 KB

Drilling down into the factory too early is bad.

Status: Needs review » Needs work

The last submitted patch, config-uuid-1969698-75.patch, failed testing.

damiankloip’s picture

I 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.

damiankloip’s picture

Status: Needs work » Needs review

Frick

Status: Needs review » Needs work
Issue tags: -Configuration system, -Field API, -VDC, -Blocks-Layouts, -Configurables

The last submitted patch, 1969698-77.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

#77: 1969698-77.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +Field API, +VDC, +Blocks-Layouts, +Configurables

The last submitted patch, 1969698-77.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4.79 KB
13.68 KB

I forgot about this puppy. Let's get it back on track.

Status: Needs review » Needs work

The last submitted patch, 1969698-82.patch, failed testing.

damiankloip’s picture

Status: Needs work » Postponed

I think this should be postponed on #2019651: Add a QueryFactoryInterface for QueryFactory classes. So we can then type the Queryfactory classes properly.

damiankloip’s picture

Status: Postponed » Active
damiankloip’s picture

Status: Active » Needs review
FileSize
14.18 KB

I actually don't think that patch I postponed this on will really help us :/ Sorry folks.

Here is a new reroll anyway.

Status: Needs review » Needs work

The last submitted patch, 1969698-86.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
15.47 KB
andypost’s picture

Why this affects only config entities? The check makes sense for content entities as well

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -160,4 +162,30 @@ public function getExportProperties() {
+    // Ensure this entity's UUID does not exist with a different ID, regardless
+    // of whether it's new or updated.
+    $matching_entities = $storage_controller->getQuery()
+      ->condition('uuid', $this->uuid())
+      ->execute();
+    $matched_entity = reset($matching_entities);
+    if (!empty($matched_entity) && ($matched_entity != $this->id())) {
+      throw new ConfigUUIDException(sprintf('Attempt to save a configuration object %s with UUID %s when this UUID is already used for %s', $this->id(), $this->uuid(), $matched_entity));

Maybe better to implement this check for all entities?

andypost’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -160,4 +162,30 @@ public function getExportProperties() {
+      throw new ConfigUUIDException(sprintf('Attempt to save a configuration object %s with UUID %s when this UUID is already used for %s', $this->id(), $this->uuid(), $matched_entity));
...
+        throw new ConfigUUIDException(sprintf('Attempt to save a configuration entity %s with UUID %s when this entity already exists with UUID %s', $this->id(), $this->uuid(), $original->uuid()));

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityStorageControllerTest.phpundefined
@@ -0,0 +1,64 @@
+      $this->pass(format_string('Exception thrown when attempting to save a configuration entity with a UUID that does not match existing data: %e.', array('%e' => $e)));

Exceptions should use format_string() - consistency++

xjm’s picture

Why this affects only config entities? The check makes sense for content entities as well

@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.

damiankloip’s picture

I agree with xjm here, let's fix this for configuration entities now. I changed the exceptions to use format_string.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigUUIDException.phpundefined
@@ -0,0 +1,15 @@
+class ConfigUUIDException extends ConfigException {

I'm wondering whether a different named exception like ConfigDuplicateUUID or ConfigUUIDConflict would be better together with a second one.

+++ b/core/modules/image/lib/Drupal/image/Plugin/Core/Entity/ImageStyle.phpundefined
@@ -59,6 +59,13 @@ class ImageStyle extends ConfigEntityBase implements ImageStyleInterface {
+   * The UUID for this entity.
+   *
+   * @var string
+   */
+  public $uuid;

What is the reason that this is not part of ConfigEntityBase?

damiankloip’s picture

FileSize
3.17 KB
15.69 KB

Sounds 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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for the explanation!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 356dc8b and pushed to 8.x. Thanks!

xjm’s picture

Yayyyy.

webchick’s picture

Title: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data) » Doc update: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data)
Category: bug » task
Priority: Major » Critical
Status: Fixed » Active

Hmmm. 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:

Drupal\field\FieldException: Attempt to create an instance of unknown field 210d0a61-bc35-4c02-8556-864a572030a4 in Drupal\field\Plugin\Core\Entity\FieldInstance->__construct() (line 244 of /Users/webchick/Sites/demo/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.php).
The website has encountered an error. Please try again later.

...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.

xjm’s picture

@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.

yched’s picture

UUIDS 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.

xjm’s picture

Re: #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.

xjm’s picture

swentel’s picture

Re: #101 We can just tell in #1969800: Add UUIDs to default configuration to add those config files right ?

swentel’s picture

Also, wondering if anything is still needed here, I think we can close this one no ?

swentel’s picture

Right, updated the page over at https://drupal.org/documentation/administer/config - not sure if it's good enough.

xjm’s picture

https://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?

swentel’s picture

First 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 :)

catch’s picture

Priority: Critical » Normal
webchick’s picture

Are docs not a gate anymore? :)

catch’s picture

I 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.

webchick’s picture

Category: task » bug
Priority: Normal » Major
Status: Active » Fixed

Fair enough. I'll go add my griping to that issue then.

webchick’s picture

Title: Doc update: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data) » ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.