EntityInterface's return values are not or incorrectly documented

-   * @return \Drupal\Core\Entity\EntityInterface
-   *   A clone of the current entity with all identifiers unset, so saving
-   *   it inserts a new entity into the storage system.
+   * @return $this

If it's returning a clone, it should not say "@return $this", because it's not returning the object whose method was called.

According to the proposed standard on
#2158497: [policy, no patch] Use "$this" and "static" in documentation for @return types
I think it should be
@return static
right?

- When you are returning the same object, use @return $this.
- When creating a new instance of the same class, use @return static.

The rest looks good to me...

OK. :)

So..

   /**
    * Creates a duplicate of the entity.
    *
-   * @return \Drupal\Core\Entity\EntityInterface
-   *   A clone of the current entity with all identifiers unset, so saving
-   *   it inserts a new entity into the storage system.
+   * @return static
    */
   public function createDuplicate();

Do you think that the description that was there in the @return (which has been removed) was not useful? I think it might be useful to have that information -- it is very specific. So maybe just change the return type to "static" and leave the description there?

I think it's great! Good idea.

So, sorry for missing this earlier, I gave the patch one final review and found this:

+   * @return string
    *   The label of the entity, or NULL if there is no label defined.

Should be string|null, I think, according to the docs?

Everything else looks good. Thanks!

Looks good to me, thanks! At least, the types now match the descriptions. :)

1. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -72,7 +72,7 @@ public function enforceIsNew($value = TRUE);
      /**
       * Returns the type of the entity.
       *
   -   * @return
   +   * @return string
       *   The type of the entity.
       */
   

   This is going to conflict with #2025779: Remove ModuleInfo as it is no longer necessary :(

2. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -80,7 +80,7 @@ public function entityType();
       *   does not make use of different bundles.
   
   @@ -97,7 +97,7 @@ public function label();
      /**
       * Returns the URI elements of the entity.
       *
   -   * @return
   +   * @return array
       *   An array containing the 'path' and 'options' keys used to build the URI
   

   And this with #2167641: EntityInterface::uri() should use route name and not path.

   This is a major and the other one is a critical beta blocker, can we remove those parts as the other issues will change and clean them up?

That second one is tagged "avoid commit conflicts", so at a minimum this patch should not be committed before that issue is resolved.

7: drupal_2180725_7.patch queued for re-testing.

Both referenced issues made it in, so this can now be safely re-rolled and committed I think.

Actually it looks like #2025779: Remove ModuleInfo as it is no longer necessary is not done?

True, but that issue has actually nothing to do with this, I linked the wrong one and didn't notice it.

This is the relevant issue: #2164827: Rename the entityInfo() and entityType() methods on EntityInterface and EntityStorageControllerInterface

Ah, OK. :) Good then, time for a reroll.

1. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -70,7 +70,7 @@ public function isNew();
      /**
   -   * Returns the type of the entity.
   +   * Returns the ID of the type of the entity.
       *
   

   That sounds a bit strange, maybe "the ID of the entity type"?

2. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -118,7 +118,7 @@ public function label();
       *
   -   * @return
   +   * @return mixed[]
       *   An array containing the 'path' and 'options' keys used to build the URI
       *   of the entity, and matching the signature of url().
   

   Never seen that, I think that's just "array"?

3. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -166,7 +166,7 @@ public function hasLinkTemplate($key);
   -   * @return array
   +   * @return string[]
       *   An array of link relationships supported by this entity.
   

   Same here, we afaik only use the [] syntax for classes.

I don't think that's true that "we only use the [] syntax for classes", and I'm pretty sure I've seen it used for other stuff already... but discussing on that other issue.

I also don't think it's true that we don't use contractions in UI text and documentation. Where did you get that idea?

So... when I was reviewing this patch, I only made sure it was accurate and the readability was OK. There are several places where the wording could be made slightly less awkward, but in general I am trying to be less strict about this kind of stuff because we have a lot of documentation that's just awful and really needs help... making all the Drupal docs perfectly beautiful prose is not realistic; making it all accurate and understandable is a more realistic goal. So unless we really want to redo this patch... I think we should just get this in as it is.

Fine with me.

Thanks again - committed to 8.x.