Updated: Comment #N

Problem/Motivation

There is an unwritten assumption that EntityStorageControllerInterface::save() will provide the original entity in $entity->original if the entity is not new (being updated).
This isn't documented that storage controllers should do this, but all of our implementations do it. But yet they do it very differently, because only ConfigEntity has a dedicated method to help with this

ConfigEntityInterface has getOriginalId() and setOriginalId() because it supports renames, but the ability to ask the entity for its original ID is not necessarily Config specific, other entity types can just declare that they have no original ID, and we remove one more thing that calling code (like storage controllers) needs to special case.

Proposed resolution

Move getOriginalId()/setOriginalId() to EntityInterface

Remaining tasks

N/A

User interface changes

N/A

API changes

Any code implementing EntityInterface must now implement getOriginalId() and setOriginalId().
If the entity type does not support renames, then getOriginalId() can return NULL, and setOriginalId() can ignore the parameter.

CommentFileSizeAuthor
#10 original-id-2218033-10.patch5.38 KBtim.plunkett
#7 interdiff.txt1.7 KBtim.plunkett
#7 original-id-2218033-7.patch5.38 KBtim.plunkett
#4 interdiff.txt702 bytestim.plunkett
#4 original-id-2218033-4.patch5.16 KBtim.plunkett
#1 original-id-2218033-1.patch5.14 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
original-id-2218033-1.patch5.14 KB

A helper for $id = $entity->getOriginalId() !== NULL ? $entity->getOriginalId() : $entity->id(); might be nice, not sure.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -422,4 +422,20 @@ public function __sleep() {
+  public function getOriginalId() {
+    // By default, entities do not support the tracking of original IDs.
+    return NULL;
+  }
...
+  public function setOriginalId($id) {
+    // By default, entities do not support the tracking of original IDs.
+    return $this;
+  }

    Hm. That's a bit weird to me.

    What breaks if those two default implementations are removed?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -297,4 +297,22 @@ public function getEntityType();
+   * @return string|null
...
+   * @param string $id

    Hm. Either mixed|null or perhaps we want to be really explicit and make it actually int|string|null...?

    I think I'd prefer the latter, but no strong opinion.

tim.plunkett’s picture

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

1) ContentEntityBase, MenuLink, EntityCacheTest, TestEntity, FieldUITestNoBundle will all need some sort of empty implementation. Why not let it live there?

2) Oh good point. I went back and forth on those types several times, but forgot to update the docblocks

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
original-id-2218033-4.patch5.16 KB
interdiff.txt702 bytes

Chatted with @sun more on IRC, we agreed to leave the implementation on Entity. We can always remove it if we want (maybe after MenuLink is converted).

I also expanded the docblock to explain more about when and why getOriginalId is used.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I liked the (inter)diff in https://privatepaste.com/34c16bba22 — was that a mistake or did you intentionally skip the additional changes? :)

tim.plunkett’s picture

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

Ugh uploaded wrong patch. Will reroll later unless someone beats me to it

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
original-id-2218033-7.patch5.38 KB
interdiff.txt1.7 KB

Doh.

sun’s picture

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

Coolio, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: original-id-2218033-7.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Reviewed & tested by the community
FileSize
original-id-2218033-10.patch5.38 KB

#2217975: ConfigEntityBase::preSave() doesn't properly support renames; ConfigStorageController::save() preforms renames too early moved some code around, git apply --3way was able to resolve it automatically

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: original-id-2218033-10.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

10: original-id-2218033-10.patch queued for re-testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: original-id-2218033-10.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

10: original-id-2218033-10.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 10: original-id-2218033-10.patch, failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

"The test did not complete due to a fatal error." in Drupal\field\Tests\FormTest->testFieldFormDefaultValue() has been coming up a lot today, but has nothing to do with this patch per se. (Test client #958 fwiw) Retesting.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs work » Needs review

10: original-id-2218033-10.patch queued for re-testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

One more time, with gusto!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Gusto gets it done. :)

Committed and pushed to 8.x. Thanks!

  • Commit deb5565 on 8.x by webchick: 
    Issue #2218033 by tim.plunkett: Move getOriginalId()/setOriginalId() to...

Status: Fixed » Closed (fixed)

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