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.

Files: 
CommentFileSizeAuthor
#10 original-id-2218033-10.patch5.38 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,350 pass(es). View

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
5.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,441 pass(es). View

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

sun’s picture

  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

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

FileSize
5.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,452 pass(es). View
702 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

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

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

tim.plunkett’s picture

FileSize
5.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch original-id-2218033-7.patch. Unable to apply patch. See the log in the details link for more information. View
1.7 KB

Doh.

sun’s picture

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

Status: Needs work » Reviewed & tested by the community
FileSize
5.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,350 pass(es). View

Status: Reviewed & tested by the community » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review

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

tim.plunkett’s picture

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

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

"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

Status: Needs work » Needs review

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

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

One more time, with gusto!

webchick’s picture

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.