API page: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

Enter a descriptive title (above) relating to public static function Entity::load, then describe the problem you have found:

This isn't showing all the details from EntityInterface::load. In particular, the param is missing.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

That's a known issue, api.module only supports a standalone @inheritdoc, it breaks when it's extended.

See #1994890: Allow {@inheritdoc} and additional documentation. This violates current standards, but AFAIK, it was done because otherwise PhpStorm doesn't detect ( would shouldn't do stuff to please editors, but it's really, really helpful ;)).

So we'll probably have to wait on an outcome of that issue (which I wouldn't expect any time soon).

joachim’s picture

Title: Entity::load() docs not showing parameter » Entity::load() docs has parameter that is not needed due to inheritdoc
Issue tags: +Novice

I think in this case, the docs are wrong.

Interface:

  /**
   * Loads an entity.
   *
   * @param mixed $id
   *   The id of the entity to load.
   *
   * @return static
   *   The entity object or NULL if there is no entity with the given ID.
   */
  public static function load($id);

Entity:

  /**
   * {@inheritdoc}
   *
   * @return static|null
   *   The entity object or NULL if there is no entity with the given ID.
   */
  public static function load($id) {
    $entity_manager = \Drupal::entityManager();
    return $entity_manager->getStorage($entity_manager->getEntityTypeFromClass(get_called_class()))->load($id);
  }

All the entity adds is '@return static|null', which according to the text of the interface should be in the interface @return anyway.

Berdir’s picture

Now that I think about it, |null actually breaks PhpStorm I think. Yes, that should move to the interface then.

jhodgdon’s picture

For the moment, the choices are:

a) Add the docs to the parent that is being inherited from, if appropriate.

b) Don't use @inheritdoc. Instead, you can make a one-line description, and say something like "See \Full\Name\Of\Parent::methodName() for documentation on parameters".

joachim’s picture

There's nothing in the class docs that doesn't belong in the interface in this particular case, so a) applies here.

marieke_h’s picture

Status: Active » Needs review
FileSize
1.01 KB

Here you go.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

Agreed with #5. Patch looks good!

jhodgdon’s picture

Good, thanks!

alexpott’s picture

This was added to fix IDE typehinting - see #2372323: Static loaders on entity types don't return a properly typed object - I guess this issue can add a comment to indicate why we should leave this alone.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

For #9

alexpott’s picture

alexpott’s picture

In order for IDE's to return produce the correct typehint this needs to be:

   * @return static
   *   The entity object or NULL if there is no entity with the given ID.
er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
538 bytes
498 bytes

Fixed as per #12.

Anonymous’s picture

Status: Needs review » Needs work

Actually your patch removes the docblock that provides the typehinting. It should stay, see #2372323: Static loaders on entity types don't return a properly typed object.

As proposed by @alexpott in #9, this issue should also add a comment as to why it should stay, so we don't remove it in the future. I'm not sure where a good place to add that would be though.

Anonymous’s picture

Any suggestions on how/where we can comment that we should leave that doc alone?

joachim’s picture

> This was added to fix IDE typehinting

That sort of thing belongs in the documentation standards, which are in pages on d.org. Issues relating to changes to these should be filed in https://www.drupal.org/project/issues/drupal_twg.

jhodgdon’s picture

You cannot use @inheritdoc and then have a @return in there. Either use just @inheritdoc or you need a full doc block.

joachim’s picture

Status: Needs work » Reviewed & tested by the community

Ok so if the @return typehint should only be 'static' and can't be 'static|null', then #13 is correct.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

#2372323: Static loaders on entity types don't return a properly typed object includes three additions of the return value with the @inheritdoc in the class rather than the interface, but this patch only alters one of them. If we're going to change it, we should change all three, and make sure the relevant docs are moved to the appropriate interface methods if needed. Thanks!

keopx’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
1.31 KB

I think I made the right changes.

jcnventura’s picture

Working on this in the mentored sprints in Barcelona: https://www.drupal.org/contributor-tasks/review

peterarnold’s picture

Status: Needs review » Reviewed & tested by the community

The patch is working ok. The additional docs where removed. The @inheritdoc was all that it needed.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -70,27 +70,21 @@ public function __construct(array $values, $entity_type) {
       protected function entityManager() {
    

    Um... You can only use inheritdoc if there is documentation on an inherited class or interface.

    I don't think the entityManager() method docs exist anywhere to be inherited?

  2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -70,27 +70,21 @@ public function __construct(array $values, $entity_type) {
    +   * {@inheritdoc}
        */
       protected function languageManager() {
         return \Drupal::languageManager();
    

    Same with languageManager()

  3. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -70,27 +70,21 @@ public function __construct(array $values, $entity_type) {
    +   * {@inheritdoc}
        */
       protected function uuidGenerator() {
    

    Same with uuidGenerator()

keopx’s picture

Status: Needs work » Needs review
FileSize
1.97 KB
2.06 KB

I recovered removed and clear documentation.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Great! This patch is fine. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 2461671-24.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

This patch did not cause that segmentation fault on the test bot.

jcnventura queued 24: 2461671-24.patch for re-testing.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

In #9 @alexpott indicated that this was added to support IDE typehinting. Has anyone tested with e.g. PHPStorm with the latest patch to confirm the typehinting still works in all cases?

jhodgdon’s picture

Status: Needs review » Needs work

Well. You can't actually do @inheritdoc + additional stuff, as things are now. It's either @inheritdoc by itself, or you need to provide full documentation.

I don't get why the IDEs cannot pick up "static" as a type from the interface docs. That seems like a bug in the IDE. And I don't think we should be working around IDE bugs.

So. We either need to:
a) Do this patch. But it also needs to have some updates to the interface docs because they don't match what is being removed from the entity class docs (for instance, the load() function should be static|null and in the interface it is just static). Wait for the IDEs to fix their problem.

b) Work around the IDE bug with a new patch. It needs to instead of having doc blocks like

 * {@inheritdoc}
 * ... more stuff here

have FULL docs for each method that needs additional stuff beyond what the interface has.

I vote for (a). Either way this is Needs Work.

jhodgdon’s picture

For more on the inhertidoc discussion, see #1994890: Allow {@inheritdoc} and additional documentation

But until that is resolved, and it will not be resolved like the current docs are, we can either use @inheritdoc by itself or have complete docs. Not inheritdoc with other stuff. It just plain doesn't work... again see that issue for more discussion.

Berdir’s picture

static on the interface works perfectly fine for me PHPStorm. Just tested it.

What *doesn't* work is the static|null that we have at the moment on the class. This was added when trying to make the docs "more correct" but apparently PHPStorm doesn't understand that.

Being able to have type hinting was one of the main reasons for adding those methods in the first place.

It would be really nice if we'd just do #24 without "fixing" EntityInterface. Yes, we'd have not 100% correct docs, but what we have right now is a pretty big DX loss. And we did start to use static *because* PHPStorm supports it, if I remember correctly :)

jhodgdon’s picture

Um. So you are saying we should just have @inheritdoc on the class and the type hints on the interface, but not do the static|null stuff on the interface, even though it is correct? Can we not file an issue on PHPStorm and get them to fix their IDE, so that we can have correct docs?

Berdir’s picture

Yes, that's what I'm saying. IMHO, having working type hints on ::load() is way more important than having a |null in the @return there :)

jhodgdon’s picture

OK, well in that case we should keep the current patch, except for the Interface part that adds static|null because even though that is correct, PHPStorm can't handle it and I guess we are working around that bug instead of asking them to fix it.

Berdir’s picture

Oh, I'm all for asking them to fix it :) But I guess that will take months to years if it ever will be?

jhodgdon’s picture

I believe they've been fairly receptive to things like this in the past, though I don't know what channel people have used to get things like this fixed.

jhodgdon’s picture

Anyway. If someone can make a new patch that just omits the interface change from this patch, we should be good to go.

keopx’s picture

Status: Needs work » Needs review
FileSize
1.45 KB

Here new patch

keopx’s picture

FileSize
538 bytes

Sorry here interdiff

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -497,9 +492,6 @@ public function getCacheMaxAge() {
    -   *
    -   * @return static|null
    -   *   The entity object or NULL if there is no entity with the given ID.
    

    This does not work in PHPStorm either - what we need is to just remove the |null and then you get proper typehints for $node = Node::load(1); ... for example PHPStorm will find getType() from the NodeInterface...

  2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -508,10 +500,6 @@ public static function load($id) {
    -   *
    -   * @return static[]
    -   *   An array of entity objects indexed by their IDs. Returns an empty array
    -   *   if no matching entities are found.
    
    @@ -520,9 +508,6 @@ public static function loadMultiple(array $ids = NULL) {
    -   *
    -   * @return static
    -   *   The entity object.
    

    Therefore these are probably wrong too..

Tested with PHPStorm 10.0.1

Can I suggest that we also add comments that the typehints are the way they are because of IDE support.

Berdir’s picture

I tested it too and it worked just fine for me, also PHPStorm 10.0.1.

See discussion above, what we're doing right now (mixing @inheritdoc) and additional docs) does not work at all for our api documentation, if we keep it, we need to completely copy it.

Some PHPStorm guy asked me a while back too about feedback, wrote them a mail about this. Lets see if I will get a response.

jhodgdon’s picture

Sigh. In #32, Berdir said it did work in PHPStorm to have the docs on the interface and use static, but that if it says static|null that doesn't work.

In #41 alexpott says (I think) that PHPStorm doesn't work with this.

Which is it?

In any case, having @inheritdoc+ other stuff is not OK. You can either have @inheritdoc by itself OR you can provide complete docs. So if the patch is not OK then we need to put complete docs on the class for each of the functions that return static. And we should put a comment in the code as to why we are duplicating the docs.

Leaving the "inheritdoc+ other stuff" just is NOT OK. Please.

cilefen’s picture

In any case, having @inheritdoc+ other stuff is not OK.

Is that a Drupal-specific thing? I think phpdoc allows it, for example.

alexpott’s picture

For some reason my IDE is picking up the typehint of EntityStorageInterface::load() - if I remove that it works... very very odd.

alexpott’s picture

So intriguingly if I create a file in DRUPAL_ROOT like c.php and use Node::load() it does not work but if I do it in node.module or in core/lib/Drupal.php - it works fine. Not sure what is going on here... according to PHPStorm it is return EntityInterface|null|static in all the different places...

jhodgdon’s picture

RE #44, see issue #1994890: Allow {@inheritdoc} and additional documentation to discuss, not here please. There is a good discussion of other standards there too.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

Re #46: No idea why that doesn't work but nobody will actually do it, so.. if it works in module files and classes, that should be OK? :)

I still think that this is the way to go. It works sometimes > it never works? :)

alexpott’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6808a6a and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed 9d5bf0b on 8.1.x
    Issue #2461671 by keopx, er.pushpinderrana, marieke_h, jhodgdon,...

  • alexpott committed 6808a6a on
    Issue #2461671 by keopx, er.pushpinderrana, marieke_h, jhodgdon,...
Berdir’s picture

Thanks.

There's an open bug report against PHPStorm about the non-working static|null and also static[] (although I thought that worked for me).

https://youtrack.jetbrains.com/issue/WI-19953

Lets all upvote it so we can get it fixed :)

Status: Fixed » Closed (fixed)

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