Problem/Motivation

ConfigStorageController allows a new entity to override an existing entity.

This can cause problems with modules, for example, if two installed modules provide the same default configuration, one of the configuration entities will be silently overwritten. The most immediate effect of making this change, is that various tests have had to be modified to accomodate this desired behavior:
BreakpointThemeTest, ConfigEntityTest, ConfigImportTest, ViewStorageTest, FieldTypeTest

Proposed resolution

ConfigStorageController will not allow saving a "new" entity that overrides an existing entity. A developer would need to test whether or not that ID exists before performing an entity_create()->save(). If they don't an error message should be returned with an entity ID.

Work has been done to make sure that these calls were being correctly tested. A series of patches had failed to pass tests.

Remaining tasks

Patch conflicts with ModuleApiTest::testDependencyResolution() #1776830: Installation and uninstallation of configuration provided by a module that belongs to another module's API

API changes

To override attributes of an entity, a developer should use

$entity = entity_load('foo', 'foo'));
$entity->set('label', 'bar');
$entity->save();

Original report by tim.plunkett

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Tagging.

tim.plunkett’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, conflicting-configurables.patch, failed testing.

xjm’s picture

For configurables, entity_create() wraps ConfigStorageController::create(), which has the comment:

  // Mark this entity as new, so isNew() returns TRUE. This does not check
  // whether a configuration entity with the same ID (if any) already exists.

This comment was introduced in #1798944: Convert config_test entity forms to EntityFormController (strange enough place to begin with). The patch in that issue originally had the following @todo:

+    // @todo If we expect isNew() to return FALSE when given an ID that exists
+    //   already, wrap this in !config($prefix . $id)->getStorage()->exists().
+    //   In that case, do we expect existing values to be merged into $values?

In #37 of that issue @catch asked that a followup be filed for the @todo, but instead the comment was simply removed.

This bug is also causing oddities for testing CRUD hooks for #1871696: Convert block instances to configuration entities to resolve architectural issues.

I think entity_create() should fail if the entity exists already.

tim.plunkett’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
2.53 KB
1.52 KB

Curious about sun's thoughts here.

Status: Needs review » Needs work

The last submitted patch, entity-1887654-4-PASS.patch, failed testing.

sun’s picture

Looks good to me. Can we output the entity ID (without config prefix) in the exception message?

xjm’s picture

This bug also causes configuration entities to be silently overwritten if two installed modules provide the same default configuration. Here's an additional test. I also filed #1888218: Default configuration entities provided by a module should include the module name in the machine name.

I didn't try to fix the exceptions or #7; just noticed this in the process of filing blocks followups.

xjm’s picture

Status: Needs work » Needs review
xjm’s picture

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.phpundefined
@@ -194,4 +194,13 @@ function testUpdated() {
+    module_enable(array('config_test_duplicate'));

Oh, this should also be inside a try/catch.

Status: Needs review » Needs work

The last submitted patch, entity-1887654-8-combined.patch, failed testing.

tim.plunkett’s picture

Assigned: sun » Unassigned
Status: Needs work » Needs review
FileSize
8.46 KB
11.57 KB

BreakpointThemeTest was manually entity_create-ing all of its default config (identical data), which was just a waste.
ConfigEntityTest had a whole section to ensure the exact broken behavior!
ConfigImportTest was missing the try/catch as xjm pointed out in #10
ViewStorageTest was creating a duplicate, but that wasn't needed for the intention of the assertion
FieldTypeTest was using views.view.test_field_type.yml, which was copied instead of moved during #1855228: Move Views core module tests to their respective modules

I also improved the assertion message, just trying to use it to track these 5 down was annoying without it.

damiankloip’s picture

This looks ready to go, except for 1 thing:

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityUnitTest.phpundefined
@@ -40,4 +41,25 @@ public function testStorageControllerMethods() {
+      $this->pass('A conflicting entity was not saved.');

Would this expose the broken behaviour? This would only happen if we get an exception. Otherwise we wouldn't know about it. Maybe this should just set a variable or something instead?

tim.plunkett’s picture

That line actually could be removed, the important part is the two assertions directly afterward.

damiankloip’s picture

Yeah, that's kind of what it looks like. The assertions below will test the actual behavior really. Although, that is a nice one to have anyway, because I have found it never hurts to have extra assertions. One day they come in useful :) up to you.

tim.plunkett’s picture

Okay, talked this over with @damiankloip, this is clearer.

xjm’s picture

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.phpundefined
@@ -137,24 +137,6 @@ function testCRUD() {
-    // Re-create the entity with the same ID and verify updated status.
-    $same_id = entity_create('config_test', array(
-      'id' => $config_test->id(),
-    ));
-    $this->assertIdentical($same_id->isNew(), TRUE);
-    $status = $same_id->save();
-    $this->assertIdentical($status, SAVED_UPDATED);
-
-    // Verify that the entity was overwritten.
-    $same_id = entity_load('config_test', $config_test->id());
-    $this->assertIdentical($same_id->id(), $config_test->id());
-    // Note: Reloading loads from FileStorage, and FileStorage enforces strings.
-    $this->assertIdentical($same_id->label(), '');
-    $this->assertNotEqual($same_id->uuid(), $config_test->uuid());
-
-    // Revert to previous state.
-    $config_test->save();

Wow, I hope I didn't write that. Did we double-check that there's coverage elsewhere in the test for updating the object without using entity_create() incorrectly?

sun’s picture

Closely related: #1889620: config_install_default_config() overwrites existing configuration

Notes on this patch:

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -234,8 +234,14 @@ public function create(array $values) {
+        throw new EntityMalformedException(format_string('The @type entity ID (@id) is not unique.', array('@type' => $this->entityType, '@id' => $entity->id())));

Speaking of "unique"... machine names are not really unique, by design. If we wanted to have real uniqueness, you know... #1609418: Configuration objects are not unique (feel free to call me parrot :P)

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php
@@ -137,24 +137,6 @@ function testCRUD() {
-    // Re-create the entity with the same ID and verify updated status.
-    $same_id = entity_create('config_test', array(
-      'id' => $config_test->id(),
-    ));
-    $this->assertIdentical($same_id->isNew(), TRUE);
-    $status = $same_id->save();
-    $this->assertIdentical($status, SAVED_UPDATED);
-
-    // Verify that the entity was overwritten.
-    $same_id = entity_load('config_test', $config_test->id());
-    $this->assertIdentical($same_id->id(), $config_test->id());
-    // Note: Reloading loads from FileStorage, and FileStorage enforces strings.
-    $this->assertIdentical($same_id->label(), '');
-    $this->assertNotEqual($same_id->uuid(), $config_test->uuid());
-
-    // Revert to previous state.
-    $config_test->save();

IIRC I authored this test code, in early stages of config entities, and I think it was supposed to verify that we're able replace a config entity with a new one.

However, replacing == updating — unless I'm missing something. Calling code should know when to create and when to update an entity.

So I think this removal is fine.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityUnitTest.php
@@ -40,4 +41,27 @@ public function testStorageControllerMethods() {
+  public function testCreatingConflictingEntities() {

Can we rename this to

testUniqueID()

Let's also add a @todo to the method's phpDoc to add tests for renaming + saving an existing config entity to an ID that already exists. (blargh)

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityUnitTest.php
@@ -40,4 +41,27 @@ public function testStorageControllerMethods() {
+    // Create an entity with the same ID, but a different label.
+    $exception = FALSE;
+    try {
+      $entity = entity_create('config_test', array('id' => 'foo', 'label' => 'Bananas'));
+      $entity->save();
+    }
+    catch (EntityMalformedException $e) {
+      $exception = TRUE;
+    }
+    // Creating an entity with the same machine name should not succeed.
+    $this->assertEqual($entity->label(), 'Foo', 'The entity label was not overwritten.');
+    $this->assertEqual($entity->style, 'foo', 'The entity style was not overwritten.');
+    $this->assertTrue($exception, 'An exception was thrown when creating an entity with a duplicate ID.');

1) Can we use the regular exception assertion pattern here?

Copied from elsewhere:

    try {
      $status = $config_test->save();
      $this->pass('EntityMalformedException was not thrown.');
    }
    catch (EntityMalformedException $e) {
      $this->fail('EntityMalformedException was not thrown.');
    }

2) The following assertions are a bit strange... that code wouldn't be reached normally, and if anything, then we'd want to reload the entity to confirm that it is still the same (instead of testing the in-memory object).

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.php
@@ -194,4 +195,17 @@ function testUpdated() {
+  /**
+   * Tests enabling a module with a duplicate configuration entity.
+   */
+  function testDuplicate() {

This test belongs into ConfigInstallTest, not ConfigImportTest.

+++ b/core/modules/config/tests/config_test_duplicate/config/config_test.dynamic.default.yml
@@ -0,0 +1,2 @@
+id: default
+label: Not default

Can we add a YAML comment here to clarify that this config object purposively duplicates config_test.dynamic.default?

+++ b/core/modules/config/tests/config_test_duplicate/config_test_duplicate.info
@@ -0,0 +1,5 @@
+name = Duplicate default configuration
+package = Core

package should be Testing

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs review » Needs work

Thanks for the feedback!

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
5.11 KB
11.99 KB

Rerolled with feedback.

Status: Needs review » Needs work

The last submitted patch, configentity-1887654-20.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
777 bytes
12.75 KB

This is now blocked by #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API.

ModuleApiTest::testDependencyResolution() enables php.module, which ships with a default format. It is then uninstalled, but its config is not cleaned up. When it is reinstalled, module_enable attempts to copy over the format again, which breaks.

This fixes the other fail.

Status: Needs review » Needs work

The last submitted patch, config-1887654-22.patch, failed testing.

dcam’s picture

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

adriancotter’s picture

Assigned: Unassigned » adriancotter

I am going to write an issue summary

adriancotter’s picture

Assigned: adriancotter » Unassigned
Issue tags: -Needs issue summary update

LMK if anything doesn't add up in the issue summary (first one for me).

xjm’s picture

xjm’s picture

Issue summary: View changes

adding more information to the issue summary

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
12.26 KB

Status: Needs review » Needs work

The last submitted patch, config-1887654-28.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
12.18 KB

Reroll

yched’s picture

There are checks / exceptions that could be removed in Field / FieldInstance saveNew() if this is taken care of upstream.

EclipseGc’s picture

basic logic looks pretty sane to me. Hope it passes.

Eclipse

Status: Needs review » Needs work

The last submitted patch, config-1887654-30.patch, failed testing.

chx’s picture

Priority: Major » Critical

This (in user module) tripped today a critical issue so it must be critical. Also, it's a gigantic WTF -- like, "who ate my entity?? WTF is going on here???"

marthinal’s picture

Status: Needs work » Needs review
FileSize
11.45 KB

I made a few changes to the patch. For example at ViewStorageTest.php we are using the same id so, maybe is it possible to use load() method instead?

Status: Needs review » Needs work

The last submitted patch, conf-1887654-35.patch, failed testing.

chx’s picture

Perhaps add a force to entity_create and if it's not being forced which should be the overwhelmingly default case (is there even any sense of it being TRUE??) and try to load and if the load succeeds then tell the caller somehow that we have an entity already. Challenging to get it right. Basically this issue should solve #2108419: user module overwrites role actions breaking config sync cleanly.

tstoeckler’s picture

Well, you could pass TRUE for performance, if you don't want to hit the DB at all, like it is now.

chx convinced me in IRC, that this might actually make a lot of sense. At first I didn't like the idea of entity_create() doing anything other than creating an entity, specifically checking for existing entities. But it's true that the most common use-case is

$entity = entity_create();
$entity->save();

And for this use-case, often times you don't really care whether or not there was an existing entity with the same ID already, you just want to make sure that there is one, afterwards, and that it has the values you specificed.

So ideally, IMO, we would have a wicked EntityFactory which would support stuff like

// What is now entity_create()
$entity = EntityFactory::justCreateAnObjectDontSaveItDontDoAnything($values);
// What is the common use-case of entity_create() currently
EntityFactory::createAnEntityOutOfTheValuesImGivingYouAndSaveItToTheDBUnlessTheresAlreadyAnEntityWithTheSameIdInThatCaseUpdate($values);
// Possibly more...

But until then, I think entity_create($force) makes a lot of sense.

Edit: forgot last sentence.

yched’s picture

Why don't we do the check on save() ? This is where other checks already happen, and is also where the equivalent checks for Field / FieldInstance config entities (that would be deprecated by this patch) are currently made.

tstoeckler’s picture

chx was advocating for that as well.

I could certainly live with it, but (as I said to chx in IRC) I feel like

entity_create('foo', 'bar')->save();
entity_create('foo', 'bar')->save();

should throw an exception, and not magically work. That might be just me, though.

yched’s picture

Sure, I'm just saying we would throw an exception during the second save(), not the second entity_create(). It's *saving* we want to prevent.

chx’s picture

Throwing an exception is not helping import; not at all. It already happens in the following case: on user role creation we create an action (config entity). When we import, the action gets imported first, then the role which tries to recreate the action which exists with an obviously different UUID. Kaboom. This got a band aid by wrapping the entity_create()->save() in an !entity_load() but tstoeckler rightly points out that in contrib this will be a common experience and a giant WTF.

yched’s picture

The right fix for #42 is not this patch, it's "do not trigger config writes during config import" (there's an existing issue for this, and the approach has been agreed during the CMI "hard issues" discussion in Prague - I'm not where I can easoly find the issue #, but it's linked from the gdoc notes for the meeting).
We should still prevent saving a "new" entity when it already exists, but very -1 on seeing it as the fix for #42, and I still think an exception in save() is the way to go. This is already where we check that we dont update an id with a different UUID. Let's not scatter related checks in various random places.

yched’s picture

Issue summary: View changes

Updated issue summary.

blackra’s picture

I hope that we don't need to worry about the race condition of the database changing under the entity between creation and save; the only likely scenarios I can think of would result in a uuid mismatch.

If we do need to take this into account then things become a little more complicated.

alexpott’s picture

Issue summary: View changes
Issue tags: +beta blocker

Tagging

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.36 KB

Here's a new approach, throwing the exception in save(), not in create(). This is no overhead as we don't do it for temporary entities and we already load that file anyway there.

Only updated ConfigEntityTest to test the new behavior, let's see what fails additionally...

Status: Needs review » Needs work

The last submitted patch, 46: conf-1887654-46.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
10.9 KB
8.44 KB

OK, this is looking quite good. Fixing things..

- Remove the custom already-exists check for Field and FieldInstace, updated the tests for the new exception.
- CommentPreviewTest already is already using the standard profile (unfortunately), no need to copy anything from there
- ViewStorageTest uses data from an existing view including id and then changes the id, which acts as mix of a rename and a new entity. Using a new id there from the beginning solves that
- FieldTypeTest failed because there were two views named test_field_type and that test tried to enable both. Renamed the test view.

yched’s picture

Remove the custom already-exists check for Field and FieldInstace, updated the tests for the new exception

<3 @Berdir

damiankloip’s picture

FileSize
10.58 KB
2.09 KB

(Sorry, cant see you on IRC now) I meant that one is a dupe. Something must have gone awry when we did the big port initially or something, or when we moved lots of stuff around.

We can just get rid of the file that's in views/tests/modules/views_test_config/test_views instead, as we don't need that. So then we don't need the rename either. Added plugin_id to the test_field_type view in node as it's missing from there (and was in the one we want to remove) :)

Other than that, I agree that this is looking good.

The last submitted patch, 48: conf-1887654-48.patch, failed testing.

The last submitted patch, 48: conf-1887654-48.patch, failed testing.

Berdir’s picture

50: 1887654-50.patch queued for re-testing.

The last submitted patch, 50: 1887654-50.patch, failed testing.

damiankloip’s picture

FileSize
10.61 KB

Rerolled

dawehner’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -346,6 +348,12 @@ public function save(EntityInterface $entity) {
     $is_new = $config->isNew();
...
+    if ($entity->isNew() && !$config->isNew()) {

Can we just reuse the variable?

damiankloip’s picture

FileSize
10.6 KB
739 bytes

Sure can.

Berdir’s picture

I initially got very confused as to what $is_new actually refers to, the config file or the entity? Given that $config->isNew() is very easy to write, wondering if we shouldn't drop $is_new completely.

damiankloip’s picture

FileSize
10.72 KB
1.05 KB

I am down with that, Let's just remove.

The last submitted patch, 59: 1887654-59.patch, failed testing.

damiankloip’s picture

FileSize
10.97 KB
567 bytes

Whooops. Sorry :)

Damien Tournoud’s picture

Issue summary: View changes

Simplified summary.

Damien Tournoud’s picture

+    if ($entity->isNew() && !$config->isNew()) {
+      throw new EntityStorageException(String::format('@type entity with ID @id already exists.', array('@type' => $this->entityType, '@id' => $id)));
+    }

Should we also test the opposite !$entity->isNew() && $config->isNew()?

yched’s picture

Issue summary: View changes

Updated the summary to clarify that the check is made on save(), not on entity_create()

sun’s picture

Status: Needs review » Reviewed & tested by the community

re: #63: As long as the config object does not exist, it should be writable. I'm not able to see a potential issue in case of the opposite condition (that wouldn't be caught elsewhere already) — if there is one, then I think we should discuss that in a separate issue.

Latest patch looks good to me.

Anonymous’s picture

yep, looks good.

a couple of docs nitpicks:

+    // If the entity is new but a config file for it already exists, prevent
+    // it to be overwritten.
...
+    // Make sure that is not possible to overwrite an existing entity with
+    // a new one.

both of those comments could use some tidying up, but that can be done at commit time i guess, so leaving as RTBC.

catch’s picture

Assigned: Unassigned » alexpott

Giving Alex a chance to look a this before it goes in. I reviewed older patches here but not the latest yet.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 261fb64 and pushed to 8.x. Thanks!

Fixed the comments in commit.

diff --git a/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
index b1052f5..5570aef 100644
--- a/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -347,8 +347,7 @@ public function save(EntityInterface $entity) {
     }
     $config = $this->configFactory->get($prefix . $id);

-    // If the entity is new but a config file for it already exists, prevent
-    // it to be overwritten.
+    // Prevent overwriting an existing configuration file if the entity is new.
     if ($entity->isNew() && !$config->isNew()) {
       throw new EntityStorageException(String::format('@type entity with ID @id already exists.', array('@type' => $this->entityType, '@id' => $id)));
     }
diff --git a/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php
index 955177e..2832c93 100644
--- a/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php
@@ -136,8 +136,8 @@ function testCRUD() {
     $this->assertIdentical($config_test->isNew(), FALSE);
     $this->assertIdentical($config_test->getOriginalId(), $expected['id']);

-    // Make sure that is not possible to overwrite an existing entity with
-    // a new one.
+    // Ensure that creating an entity with the same id as an existing one is not
+    // possible.
     $same_id = entity_create('config_test', array(
       'id' => $config_test->id(),
     ));

Status: Fixed » Closed (fixed)

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