Modern IDEs (Netbeans, Phpstorm) can provide intelligent autocomplete functions based on the PhpDoc.
The problem is that if a type hint in the @return defined with the fully qualified name of an interface and that method return with the called object (return $this;) then one can't use all the method from the original object just those methods which are come from the interface.
For example:
The ->condition() method is unknown because the ->->addTag() -
according to the Phpdoc - returns an \Drupal\Core\Database\Query\AlterableInterface instance, and it has no condition() method.
/** @var \Drupal\Core\Entity\ContentEntityStorageInterface $storage */
$storage
->getQuery()
->addTag('xxx')
->condition();
Other example:
/** @var \Drupal\Core\Database\Query\SelectInterface $select */
$select
->condition()
->groupBy();
These are the methods where the @return type hint could be replaced with $this.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | drupal-2940038-16-return-type-hint-this-and-static.patch | 57.05 KB | sweetchuck |
| #8 | 2940038-8--return_this_docs--step3--out_of_scope.patch | 52.11 KB | drunken monkey |
Comments
Comment #2
rajeshwari10 commentedComment #3
rajeshwari10 commentedHi,
Adding the patch which has the changes as per mentioned. Please review.
Thanks
Comment #4
sweetchuckWell done :-)
@return staticwould be better for these:The
\Drupal\Core\Entity\Query\Sql\QueryAggregate::finishoverrides the PhpDoc without{@inheritdoc}, so probably the@returnshould be also explicitly defined.The following methods are return with a new instance instead of the called object.
So the
@return staticwould be better.\$thisvs$thisinMissing:
Comment #5
sweetchuckComment #6
sweetchuckComment #7
sweetchuckComment #8
drunken monkeyThanks a lot for this, would be really great to get this in! The wrong type inference is pretty annoying.
Anyways, while this already a great patch, I did find a few things to improve. Since some of these are disputable, I made my improvements in three steps, so that they can be applied individually, too (at least to some extent):
$thisorstaticwhere it wasn't appropriate.\Drupal\Core\Updater\Updater, I think it's out of scope here and we're better advised to keep this issue narrow, to just the$this/staticreturn types. But as this is most up to opinion, I kept it as a separate, third step.The patches for step 2 and 3 encompass the earlier steps, too, so the step 3 patch includes all the changes I'd propose for this issue.
(Only leaving that last patch for testing – it's only doc changes, so a test is pretty pointless anyways, no need to torture the test bot with three of them.)
(Removing related issue: No need to reference related issues from both sides.)
Comment #9
borisson_There are no valid remaining instances of
"The called object"left in core after this patch is applied.ag "The called object" -Qi core/We should probably find a way to write a phpcs rule for this, but I think this improvement can go in as-is and we can create a phpcs rule later.
Comment #12
sweetchuckComment #13
sweetchuckComment #14
sweetchuckComment #15
catchLooks right to me. I can't really think of a way to do a DrupalCS rule for this one, it's more wrong documentation than code style, so I think it's OK to land the patch then sort out a rule later if we find a way.
Comment #16
sweetchuckreroll
Comment #17
larowlanComment #18
larowlanCommitted a784791 and pushed to 8.8.x. Thanks!