Entity::setOriginalId() does enforceIsNew(FALSE), that is wrong for ConfigEntities

Needs tests and perhaps a comment as to why we are not calling the parent.

I read the IS a couple times but I'm still not 100% grokking this.

If enforceIsNew(FALSE) is problematic for ConfigEntityBase, why not override that method instead?

This makes sense to me.

@alexpott re #15, lucky you. ;)

Advance warning that this is not a review; it is notes from me trying to figure out what the heck is going on. This is the most confounding 2K patch I've dug into in awhile. ;) Feel free to ignore this comment for now and sorry for the noise...

So the docs for setOriginalId() read:

   * @param int|string|null $id
   *   The new ID to set as original ID. If the entity supports renames, setting
   *   NULL will prevent an update from being considered a rename.

(Int or string or NULL, really? And null carries specific meaning? Aiee.)

Then in enforceIsNew():

   * Enforces an entity to be new.
   *
   * Allows migrations to create entities with pre-defined IDs by forcing the
   * entity to be new before saving.
   *
   * @param bool $value
   *   (optional) Whether the entity should be forced to be new. Defaults to
   *   TRUE.

So it references Migrate specifically, on docs for EntityInterface, (not actually, see #17) and passing a null value is equivalent to passing TRUE. Oh goody. ;)

All Entity::enforceIsNew() does is set the protected class property to whatever you passed. Entity::isNew() checks it to do:

    return !empty($this->enforceIsNew) || !$this->id();

(similarly in the overridden User::isNew() which specially allows you to have a user ID of 0 that's not considered new, because Anonymous).

While ConfigEntityBase::isNew() does:

    return !empty($this->enforceIsNew);

The method is also called in:

• setOriginalId() (see below).
• createDuplicate() (always TRUE on the duplicate).
• EntityStorageBase::create() (always TRUE on the new entity).
• EntityStorageBase::save() (always FALSE on the newly saved entity).
• ConfigEntityStorage::createFromStorageRecord() (always TRUE on... whatever the heck that method does. "Allows the configuration entity storage to massage storage values before creating an entity." K.)
• SqlContentEntityStorage::doSave() in the situation when Entity::isNew() is true (which, keep in mind, also checks $enforceIsNew, because it's good to reinforce that your new entities enforced to be new are new. The comment says:
  

        // Ensure the entity is still seen as new after assigning it an id,
        // while storing its data.
  

  Then at the end of the method it sets enforceIsNew(FALSE) because once it's saved it's not new anymore. (Remember this happens again in EntityStorageBase::save(), right after doSave() gets done and caches are reset.)

• The migrate entity destination plugin's protected getEntity() method, set to TRUE when the entity is missing an ID or previous data.
• The same place in the migrate entity revision destination plugin, but setting it to FALSE instead, because I guess there's an assumption that an entity revision has an existing entity already (it loads it the line above).
• ShortcutSet::postSave(), setting it to TRUE when the set is not the default set (contrary to the comment that says it is the default set...), after creating a duplicate of the set and saving it in place of the set. (Keep in mind createDuplicate() already does that.)
• UserStorage::save(), before calling SqlContentEntityStorage::save(), to get the next serial ID when the user is new and enforce that it's new, because something to do with Anonymous having a user ID of 0? Haven't figured this out yet.

The property is also set directly in ContentEntityBase::initializeTranslation() as part of cloning the entity for a new translation, and in EntityDisplayBase::__sleep() and __wakeup() (because it's a protected property we need to care about after serializing).

And then the property gets checked in ConfigEntityBase::isNew() and Entity::isNew()

Then the base implementation in Entity::setOriginalId() is:

    // By default, entities do not support renames and do not have original IDs.
    // If the specified ID is anything except NULL, this should mark this entity
    // as no longer new.
    if ($id !== NULL) {
      $this->enforceIsNew(FALSE);
    }

And... that's it. Huh? I would have expected that it, you know, set the original ID somewhere, like the method docs and name say it does. I guess the idea is that the method is a no-op for entity types that do not support renames, and to support renames, you override it. But what happens when you use Entity::setOriginalId($not_a_null_value) without the method being overridden? It doesn't set enforceIsNew() at all -- neither true nor false -- and doesn't do anything else either. That is at least confusing. If we don't support the concept of an "original ID" without the method being overridden, shouldn't we throw an exception or something?

Keeping in mind Entity::isNew() does:

    return !empty($this->enforceIsNew) || !$this->id();

...which means that if I call setOriginalId() with some value (that is discarded), it will indicate the entity is new if no nonzero ID is set. Conversely, if I call setOriginalId() with a null value... oh wait, the exact same thing happens, because it sets enforceIsNew(FALSE) which is also empty.

Moving on to ConfigEntityBase. ConfigEntityBase::setOriginalId() in HEAD does:

    $this->originalId = $id;

    return parent::setOriginalId($id);

... so the missing setting of an original ID that confused me above, plus setting $enforceIsNew to FALSE, the thing that this patch changes. And ConfigEntityBase::isNew() then does:

    return !empty($this->enforceIsNew);

So a config entity is by default only new when you enforce that it is new. The docblock for it says:

   * EntityInterface::enforceIsNew() is only supported for newly created
   * configuration entities but has no effect after saving, since each
   * configuration entity is unique.

...which makes no sense to me yet.

setOriginalId() gets called in EntityStorageBase::save() (to set the ID of the entity you just saved as the original entity ID, ahhhh) and in file_copy() underneath a @todo referencing #2241865: Do not create a new file entity in order to overwrite an existing entity. Edit: and in ConfigEntityBase::__construct() when the entity ID is not null or an empty string, I guess setting up in case we rename stuff.

Getting more coffee...

So the thing about Migrate is a red herring; enforceIsNew() has existed with that documentation since #1495024: Convert the entity system to PSR-0 and before back to the original entity OO conversion.

Of note:
#2218033: Move getOriginalId()/setOriginalId() to EntityInterface

Also of note: #2241655: EntityStorageInterface::create() should always create a "new" entity

And also #2088241: ConfigEntityInterface::setOriginalID() should return $this.

...And setOriginalId() was added in #1798944: Convert config_test entity forms to EntityFormController, which seems wildly out of scope given the title of that issue, but there it is.

1. So #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) (the original Config Entity patch) introduced the setting of an original ID for config entities only, for renames and such.
2. #1798944: Convert config_test entity forms to EntityFormController introduced the setter for it as (I guess) out-of-scope code cleanup, and added it to the save() method for config entities.
3. #2218033: Move getOriginalId()/setOriginalId() to EntityInterface moved it up to EntityInterface instead of just config entities, with the following reasoning:
   

   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.

   That patch also added the concept of it being a no-op in the base Entity implementation, with config entities overriding it.

4. #2241655: EntityStorageInterface::create() should always create a "new" entity added the behavior of passing an ID enforcing non-newness for both the parent and child implementations, in the same interdiff. There's no indication on that issue that it was an intended change to config entities specifically, other than just the seemingly logical step of updating the existing implementations in the same way.

So I think I convinced myself that this issue's patch is indeed the correct change and not reverting some other, previously intended behavior. However, in #4, @alexpott suggested that we add a comment in ConfigEntityBase::setOriginalId() explaining why we are not calling the parent. I agree. So let's add that (per the reasoning in the summary), and then I think this can go in.

Thanks for your patience, and thanks @amateescu for chatting about the issue in IRC too. :)

@xjm nice work.

Since this is such a minor change setting back to rtbc.

Thanks @alexpott.

Regarding the docs, I guess this is fine for now, but I'm slightly concerned that we'd have to reference this issue in the codebase rather than something actually in the codebase. Usually we only add links to issues as temporary @todos. Reading it, I'd expect an open issue to clean stuff up more, not a fixed one. I think so long as we have the comment we can remove the issue link (as the comment itself can be git blamed at any rate if someone wants the backstory).

Ideally there would be some longer code documentation we could point people to. Which doesn't exist, and isn't in scope here, and I don't even know what it would be, but... something to explain how setOriginalId() and enforceIsNew() and isNew() interact (and maybe to fix them... somehow... I don't exactly know what would be to fix).

Since I don't even know what issue I would file, leaving at RTBC for now.

#25 is a weird testbot blow up - lots of simplexml_import_dom(): Invalid Nodetype to ...

@xjm this is not the only place this is done in the code - see, for example, https://api.drupal.org/api/drupal/core!includes!database.inc/function/db...

And now if someone followed the link they'd find your helpful comments.

Talked with @xjm in IRC - @xjm pointed out that links to issues in docs are not helpful and confusing. Opened #2478811: Document the entity life cycle and the differences between config and content and changed to a @todo

Since it is a docs only change leaving at rtbc.

This includes another patch. Thought for like three git reset --hard I had both applied.

Oh drats... I thought I'd checked for this. alexpott--

Committed and pushed to 8.0.x -- thanks!

Sneaked in a comma on commit:

diff --git a/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
index f554100..0ca35a5 100644
--- a/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -131,7 +131,7 @@ public function getOriginalId() {
    */
   public function setOriginalId($id) {
     // Do not call the parent method since that would mark this entity as no
-    // longer new. Unlike content entities new configuration entities have an
+    // longer new. Unlike content entities, new configuration entities have an
     // ID.
     // @todo https://www.drupal.org/node/2478811 Document the entity life cycle
     //   and the differences between config and content.