Fix type hinting for base entity interfaces

Where in https://drupal.org/node/1354#types does it say you can do @return self or @return static or anything similar?

Sorry... "started using it" and "supported by PHPDoc" doesn't mean "it's a standard for the Drupal project".

Please file an issue to get it added to the standards before filing an issue saying "This code is being updated to the standards" when the standards do not exist.

Postponing this issue until the standards issue is decided/adopted.

I do not really think all of those @return static lines are accurate. I bet most of them really should be @return $this. See
https://drupal.org/node/1354#types

This is also wrong:

-   * @param \Drupal\Core\Entity\EntityInterface[] $entities
+   * @param static[] $entities
    *   An array of entities.

We don't want to use static[]. The previous value was probably right.

We discussed it, and while I was confused initially, I agree that static[] is correct.

The class this is called on is derived based on the EntityType annotation class.

For node, this means it's called with Node objects. If you swap the Node class and use MyBetterNode, Node::preDelete() will not be called anymore at all, instead, it will call MyBetterNode, with MyBetterNode objects.

And re equivalent to type hinting on classes, this is in this case correct and we also do it elsewhere, for example factory methods like FieldDefinition::create().

Looking at examples, you can see that Node::preDelete() has $entity->nid-value hardcoded, but that should be ->id() anyway, so not a great example. File::preDelete() is nice, because it calls getFileUri(), which does not exist on EntityInterface, but it does on File extends FileInterface and it is guaranteed that it will always be called with File classes.

That said, I don't think PhpStorm already supports this they way you'd think it would work? $entities is just array for me when I change preDelete() to static[]

14: drupal_2157467_14.patch queued for re-testing.

OK, I'm fine with static[] -- seems clear enough anyway.

So now the patch is looking pretty good and we're down to nitpicks:

a) ConfigEntity

    * @return bool
+   *   Whether the configuration entity is created, updated or deleted through
+   *   the import process.
    */
   public function isSyncing();

What does this mean? I don't understand the documentation here... It's a boolean and I just don't get what TRUE means vs. FALSE from this text. Can this be clarified? Also, if you want to use a list like "cats, dogs, or elephants", we need a comma before the "or".

I guess that's it! Even I couldn't find anything else to complain about. :)

Maybe just change it to "TRUE if the configuration entity is..."

However, while we are at it, I think we should update it to state why the method exists and when to check for it. See #2124535: Prevent secondary configuration creates and deletes from breaking the ConfigImporter. Meaning, if is TRUE, then you must not do changes, because we are in the process of doing a 1:1 config sync and you shouldn't mess with that. For example, Node::postSave() must not create the body field as it does when you create a new node type in the UI.

Berdir: I'm still confused, can you suggest wording?

What about this?

I think further documentation should happen in the referenced issue.

Not sure if I can, but let my elaborate on that example and maybe you can improve it?

You have a development system where you create two new node types, A and B. For both, the automatism in NodeType::postSave() kicks in and creates the "body" field/field instance for you automatically. For A, that's what you want, for B, you don't need the body field, so you remove it.

Now, you export those two node types, push them to production and sync the configuration changes. Now, the automatism in NodeType::postSave() *must* not run. Because for A, you have the necessary config files already waiting in the sync queue which will create the body field ("will be created") and for B, you don't want the body field at all ("has been removed"). This means that the code in that method must check isSyncing() before doing any changes. In this case, it wouldn't be too bad, you'd have have to remove the body field again from B and the configuration sync system is nice enough to now throw up even though a file that initially did not exist now suddenly exists for A.

But, imagine that you would have not just removed the body field from B, but afterwards created a field named body with a different type or another incompatible change. The entity storage controller would throw an exception that a field type change is not allowed. At the same time, we must run post/pre hook/methods, because for example field entities rely on it to make the appropriate storage changes like creating the field tables.

I tried to find a way to document this (because I think it is very very important for anyone who interacts with config entities like this) but avoid writing a huge block of text as I've done above. Apparently, I made it too short, so you're welcome to improve it :)

I don't think I'm quite happy yet with the docs for isSyncing(), which now would read:

+   * Returns if this entity is changed as part of an import process.

This is not clear... Maybe "is being changed"? Because "if this entity is changed" doesn't make sense to me. This is going to return TRUE if you are in the middle of a config import, right? Also the grammar is wrong... How about:

Returns TRUE if this entity is being changed as part of an import process.

+   *
+   * Code that makes persistent changes to new, changed, or deleted
+   * configuration entities, but not imported entities that may already contain
+   * these changes, must check this method.
+   *
+   * An example is \Drupal\node\Entity\NodeType::postSave(), which adds the
+   * default body field to newly created node types. Imported entities already
+   * contain this field from when they were originally created, and should not
+   * get it again.
    *
    * @return bool
+   *   TRUE if the configuration entity is created, updated or deleted through
+   *   the import process.

Again here, I would say "is being created, ..." not just "is created".

I also went back and looked at the rest of the patch. Can we updated this one:

    * @return string|null
-   *   The original ID, if any.
+   *   The original ID or NULL.
    */
   public function getOriginalId();

I think the original text here was actually a bit better ... well neither one is great. The new text doesn't explain when NULL would be returned. Maybe:

The original ID, or NULL if it is not defined.

But I still don't know... Actually when would it be NULL?

Yes, but what does it mean if it is NULL? Under what circumstances would it be NULL rather than something else?

It is NULL for a new config entity that doesn't have an id when creating it. It is pretty weird, because doing an entity_create() with an id will mean that it will have one. So it's only if you do entity_create() without setting the id key and then assigning an id (because you need to do that for config entities). See ConfigEntityBase::__construct().

Yes, is being changed is better.

Also, the new paragraph isn't really correct:
- This not just about changing the config entity at hand, but also any other possible configuration change.
- It's not a should not, it is a must not and it's *really* important. Doing it wrong can result in completely killing production sites, possibly without a way back other than restoring both the original active configuration and the database from a backup. The other issue is a beta blocker for a reason. So IMHO, we either document it properly here or it ignore it and say that the other issue is responsible for documenting it.

"Returns if this entity is being changed as part of an import process."

That is not good English. "Returns if ..." says "This function only returns if ...". The object of the "Returns" verb is missing here. As I suggested in my last review, this should say "Returns TRUE if..." or something like that.

Also as a nitpick, in the @return, you need a comma before "or".

I'll let Berdir review to see if his concerns have been met too.

Ummm...

+   * Code that makes changes to configuration or the entities that are being
+   * changed, but not imported entities that may already contain
    * these changes, must check this method.

This is a bit awkward. What are you trying to say here? Who must check this method and who doesn't need to?

OK, I just had a discussion with Berdir in IRC about this.

Here is my suggestion for the documentation of this method:

(current first line)
Returns whether this entity is being changed as part of an import process.

(next piece, updated)
If you are writing code that responds to a change in this entity (insert, update, delete, presave, etc.), and your code would result in a configuration change (whether related to this configuration entity, another configuration entity, or non-entity configuration) or your code would result in a change to this entity itself, you need to check and see if this entity change is part of an import process, and skip executing your code if that is the case.

For example, \Drupal\node\Entity\NodeType::postSave() adds the default body field to newly created node type configuration entities, which is a configuration change. You would not want this code to run during an import, because imported entities were already given the body field when they were originally created, and the imported configuration includes all of their currently-configured fields. On the other hand, \Drupal\field\Entity\FieldInstance::postSave() makes sure the caches are emptied whenever any field instance configuration is changed; the code does not change any configuration, so it should be executed even when importing. So, the first method should check $entity->isSynching() and skip executing if it returns TRUE, and the second should not.

How about that?

Sounds great to me :) I think creating the storage tables for new fields would be a better example for the "must always run" case, as you could argue that the configuration sync needs to clear all caches anyway.

OK... The place this is called is a bit more complicated... How about this for the second paragraph:

For example, \Drupal\node\Entity\NodeType::postSave() adds the default body field to newly created node type configuration entities, which is a configuration change. You would not want this code to run during an import, because imported entities were already given the body field when they were originally created, and the imported configuration includes all of their currently-configured fields. On the other hand, \Drupal\field\Entity\Field::preSave() and the methods it calls make sure that the storage tables are created or updated for the field configuration entity, which is not a configuration change, and it must be done whether due to an import or not. So, the first method should check $entity->isSynching() and skip executing if it returns TRUE, and the second should not perform this check.

I'm OK with this patch (not surprising). Any other opinions?

Let's do this :)

Quite a block of content, but I think that's needed to explain this pretty complicated stuff.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
   @@ -80,14 +77,41 @@ public function setSyncing($status);
   +   * Code that makes changes to configuration or the entities that are being
   +   * changed, but not imported entities that may already contain
   +   * these changes, must check this method.
   

   This paragraph is confusing and seems superfluous given the detail in the following paragraph.

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
   @@ -117,7 +141,7 @@ public function set($property_name, $value);
   -   * @return array
   
   +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -224,7 +224,7 @@ public function postSave(EntityStorageControllerInterface $storage_controller, $
   -   * @param array $values
   +   * @param mixed[] $values
   

   Is this really helpful?

OK with me. Back to RTBC unless someone wants to object again. :)

Thanks! Looks like the same patch, minus some other lines that were apparently changed in another issue.

Kicking back to alex since he reviewed this before.

patch no longer applies.

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -303,8 +303,8 @@ public function referencedEntities();
-   *   The original ID, if any. Entity types that do not support renames will
-   *   never have an original ID and will return NULL.
+   *   The original ID, or NULL if no ID was set or for entity types that do not
+   * support renames.

wrong indentation.

Doh, missed that, thanks scor! Looks good now (again).

drupal_2157467_49.patch no longer applies.

error: patch failed: core/lib/Drupal/Core/Entity/EntityInterface.php:226
error: core/lib/Drupal/Core/Entity/EntityInterface.php: patch does not apply

Committed b8c062c and pushed to 8.x. Thanks!