Updated: Comment #N

Problem/Motivation

When checking the UUID matching of the existing and original entity during an update, ConfigEntityBase mistakenly loads itself instead of the original entity.

Proposed resolution

Don't do that

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

CommentFileSizeAuthor
#17 interdiff.txt3.44 KBtim.plunkett
#17 configentity-2226027-16.patch40.13 KBtim.plunkett
#14 configentity-2226027-14.patch39.34 KBtim.plunkett
#14 interdiff.txt2.37 KBtim.plunkett
#12 configentity-2226027-12.patch39.38 KBtim.plunkett
#9 configentity-2226027-9.patch39.56 KBtim.plunkett
#9 interdiff.txt4.02 KBtim.plunkett
#7 interdiff.txt892 bytestim.plunkett
#7 configentity-2226027-7.patch36.75 KBtim.plunkett
#5 interdiff.txt1.36 KBtim.plunkett
#5 configentity-2226027-5.patch33.27 KBtim.plunkett
#2 configentity-2226027-2-PASS.patch31.91 KBtim.plunkett
#2 configentity-2226027-2-FAIL.patch30.95 KBtim.plunkett
config-entity-original.patch980 bytestim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Created #2226037: Change signature of Entity::loadUnchanged() to accept an EntityInterface instead of random ID

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: Unassigned » tim.plunkett
Priority: Normal » Major
Issue summary: View changes
Status: Active » Needs review
FileSize
configentity-2226027-2-FAIL.patch30.95 KB
configentity-2226027-2-PASS.patch31.91 KB

I'm merging #2220451: Expand coverage of config entity renames into this.

The last submitted patch, 2: configentity-2226027-2-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: configentity-2226027-2-PASS.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
configentity-2226027-5.patch33.27 KB
interdiff.txt1.36 KB

Yay more bugs (with fixes and test coverage)!

Status: Needs review » Needs work

The last submitted patch, 5: configentity-2226027-5.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
configentity-2226027-7.patch36.75 KB
interdiff.txt892 bytes

One more!

Status: Needs review » Needs work

The last submitted patch, 7: configentity-2226027-7.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
interdiff.txt4.02 KB
configentity-2226027-9.patch39.56 KB

Ugh, breakpoint is the worst/weirdest.

Status: Needs review » Needs work

The last submitted patch, 9: configentity-2226027-9.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review

9: configentity-2226027-9.patch queued for re-testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
configentity-2226027-12.patch39.38 KB

Reroll after The Great Storage Rename

Status: Needs review » Needs work

The last submitted patch, 12: configentity-2226027-12.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.37 KB
configentity-2226027-14.patch39.34 KB

Heh, even git can't help me with a silly mistake like this :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -336,9 +348,6 @@ public function save(EntityInterface $entity) {
       $return = SAVED_NEW;

@@ -348,6 +357,9 @@ public function save(EntityInterface $entity) {
       $this->invokeHook('insert', $entity);
...
+    // Immediately update the original ID.
+    $entity->setOriginalId($entity->id());

    Does it make sense to have an original ID on entity inserts? 

  2. +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/Breakpoint.php
@@ -106,6 +106,16 @@ class Breakpoint extends ConfigEntityBase implements BreakpointInterface {
+   */
+  public function id() {
+    if (!isset($this->id)) {
+      $this->id = $this->sourceType . '.' . $this->source . '.' . $this->name;
+    }
+    return $this->id;

@@ -114,13 +124,6 @@ public function save() {
-    // Build an id if none is set.
-    // Since a particular name can be used by multiple theme/modules we need
-    // to make a unique id.
-    if (empty($this->id)) {
-      $this->id = $this->sourceType . '.' . $this->source . '.' . $this->name;
-    }
-

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.php
@@ -104,15 +104,22 @@ public function save() {
-    if (empty($this->id)) {
-      $this->id = $this->sourceType . '.' . $this->source . '.' . $this->name;
-    }
...
   /**
    * {@inheritdoc}
    */
+  public function id() {
+    if (!isset($this->id)) {
+      $this->id = $this->sourceType . '.' . $this->source . '.' . $this->name;
+    }
+    return $this->id;
+  }
+
+  /**
+   * {@inheritdoc}
+   */

    I really like this 

  3. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php
@@ -0,0 +1,723 @@
+class ConfigEntityStorageTest extends UnitTestCase {

    MOAAAAAAAAH!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

#15.1, the reason this is needed is if you tried to do this:

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

Without that, the second save doesn't know what the original ID is, but knows that it's not new, and breaks.

So yes, I think it's fine to update the original ID on an insert.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
configentity-2226027-16.patch40.13 KB
interdiff.txt3.44 KB

Added some comments for the actual changes.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Excellent.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Thank you so much to improve the documentation here!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit 979bb62 on 8.x by catch: 
    Issue #2226027 by tim.plunkett: ConfigEntityBase::preSave() tries to...
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » Unassigned

Thanks!

Status: Fixed » Closed (fixed)

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