Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff-2461671-24-39.txt | 538 bytes | keopx |
#39 | 2461671-39.patch | 1.45 KB | keopx |
#24 | interdiff-2461671-20-24.txt | 2.06 KB | keopx |
#24 | 2461671-24.patch | 1.97 KB | keopx |
#13 | 2461671-13.patch | 498 bytes | er.pushpinderrana |
Comments
Comment #1
BerdirThat'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).
Comment #2
joachim CreditAttribution: joachim commentedI think in this case, the docs are wrong.
Interface:
Entity:
All the entity adds is '@return static|null', which according to the text of the interface should be in the interface @return anyway.
Comment #3
BerdirNow that I think about it, |null actually breaks PhpStorm I think. Yes, that should move to the interface then.
Comment #4
jhodgdonFor 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".
Comment #5
joachim CreditAttribution: joachim commentedThere's nothing in the class docs that doesn't belong in the interface in this particular case, so a) applies here.
Comment #6
marieke_h CreditAttribution: marieke_h at XIO commentedHere you go.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedAgreed with #5. Patch looks good!
Comment #8
jhodgdonGood, thanks!
Comment #9
alexpottThis 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.
Comment #10
alexpottFor #9
Comment #11
alexpottAnd funnily enough #2417339: Fix @return documentation for EntityManagerInterface::loadEntityByConfigTarget() broke #2372323: Static loaders on entity types don't return a properly typed object
Comment #12
alexpottIn order for IDE's to return produce the correct typehint this needs to be:
Comment #13
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedFixed as per #12.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedActually 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.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedAny suggestions on how/where we can comment that we should leave that doc alone?
Comment #16
joachim CreditAttribution: joachim commented> 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.
Comment #17
jhodgdonYou cannot use @inheritdoc and then have a @return in there. Either use just @inheritdoc or you need a full doc block.
Comment #18
joachim CreditAttribution: joachim commentedOk so if the @return typehint should only be 'static' and can't be 'static|null', then #13 is correct.
Comment #19
xjm#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!Comment #20
keopxI think I made the right changes.
Comment #21
jcnventura CreditAttribution: jcnventura at Wunder commentedWorking on this in the mentored sprints in Barcelona: https://www.drupal.org/contributor-tasks/review
Comment #22
peterarnold CreditAttribution: peterarnold commentedThe patch is working ok. The additional docs where removed. The @inheritdoc was all that it needed.
Comment #23
jhodgdonUm... 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?
Same with languageManager()
Same with uuidGenerator()
Comment #24
keopxI recovered removed and clear documentation.
Comment #25
jhodgdonGreat! This patch is fine. Thanks!
Comment #27
jhodgdonThis patch did not cause that segmentation fault on the test bot.
Comment #29
xjmIn #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?
Comment #30
jhodgdonWell. 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
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.
Comment #31
jhodgdonFor 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.
Comment #32
Berdirstatic 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 :)
Comment #33
jhodgdonUm. 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?
Comment #34
BerdirYes, 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 :)
Comment #35
jhodgdonOK, 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.
Comment #36
BerdirOh, I'm all for asking them to fix it :) But I guess that will take months to years if it ever will be?
Comment #37
jhodgdonI 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.
Comment #38
jhodgdonAnyway. If someone can make a new patch that just omits the interface change from this patch, we should be good to go.
Comment #39
keopxHere new patch
Comment #40
keopxSorry here interdiff
Comment #41
alexpottThis 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...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.
Comment #42
BerdirI 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.
Comment #43
jhodgdonSigh. 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.
Comment #44
cilefen CreditAttribution: cilefen commentedIs that a Drupal-specific thing? I think phpdoc allows it, for example.
Comment #45
alexpottFor some reason my IDE is picking up the typehint of EntityStorageInterface::load() - if I remove that it works... very very odd.
Comment #46
alexpottSo 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...
Comment #47
jhodgdonRE #44, see issue #1994890: Allow {@inheritdoc} and additional documentation to discuss, not here please. There is a good discussion of other standards there too.
Comment #48
BerdirRe #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? :)
Comment #49
alexpottComment #50
alexpottCommitted 6808a6a and pushed to 8.0.x and 8.1.x. Thanks!
Comment #53
BerdirThanks.
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 :)