Problem

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.

  1. interface Drupal\Component\Plugin\ContextAwarePluginInterface::setContextValue
  2. class Drupal\Core\Access\AccessResultAllowed::allowed
  3. class Drupal\Core\Access\AccessResultForbidden::forbidden
  4. class Drupal\Core\Access\AccessResultNeutral::neutral
  5. interface Drupal\Core\Asset\AttachedAssetsInterface::createFromRenderArray
  6. interface Drupal\Core\Config\StorageComparerInterface::reset
  7. interface Drupal\Core\Config\StorageInterface::createCollection
  8. class Drupal\Core\Config\Config::setSettingsOverride
  9. class Drupal\Core\Config\Config::setModuleOverride
  10. class Drupal\Core\Config\Config::setOverriddenData
  11. class Drupal\Core\Config\Config::resetOverriddenData
  12. class Drupal\Core\Config\Config::delete
  13. class Drupal\Core\Config\ConfigImporter::reset
  14. class Drupal\Core\Config\ConfigImporter::import
  15. class Drupal\Core\Config\PreExistingConfigException::create
  16. interface Drupal\Core\Database\Query\ExtendableInterface::extend
  17. interface Drupal\Core\Database\Query\ConditionInterface::condition
  18. interface Drupal\Core\Database\Query\ConditionInterface::where
  19. interface Drupal\Core\Database\Query\ConditionInterface::isNull
  20. interface Drupal\Core\Database\Query\ConditionInterface::isNotNull
  21. interface Drupal\Core\Database\Query\ConditionInterface::exists
  22. interface Drupal\Core\Database\Query\ConditionInterface::notExists
  23. interface Drupal\Core\Database\Query\ConditionInterface::conditionGroupFactory
  24. interface Drupal\Core\Database\Query\ConditionInterface::andConditionGroup
  25. interface Drupal\Core\Database\Query\ConditionInterface::orConditionGroup
  26. class Drupal\Core\Database\Query\Insert::from
  27. class Drupal\Core\Database\Query\Merge::conditionTable
  28. class Drupal\Core\Database\Query\Merge::updateFields
  29. class Drupal\Core\Database\Query\Merge::expression
  30. class Drupal\Core\Database\Query\Merge::insertFields
  31. class Drupal\Core\Database\Query\Merge::useDefaults
  32. class Drupal\Core\Database\Query\Merge::fields
  33. class Drupal\Core\Database\Query\Update::fields
  34. class Drupal\Core\Database\Query\Update::expression
  35. class Drupal\Core\Database\Query\Select::prepareCountQuery
  36. interface Drupal\Core\Database\Query\SelectInterface::distinct
  37. interface Drupal\Core\Database\Query\SelectInterface::fields
  38. interface Drupal\Core\Database\Query\SelectInterface::orderBy
  39. interface Drupal\Core\Database\Query\SelectInterface::orderRandom
  40. interface Drupal\Core\Database\Query\SelectInterface::range
  41. interface Drupal\Core\Database\Query\SelectInterface::union
  42. interface Drupal\Core\Database\Query\SelectInterface::groupBy
  43. interface Drupal\Core\Database\Query\SelectInterface::countQuery
  44. interface Drupal\Core\Database\Query\AlterableInterface::addMetaData
  45. class Drupal\Core\Entity\Query\Sql\QueryAggregate::addAggregate
  46. class Drupal\Core\Entity\Query\Sql\QueryAggregate::compileAggregate
  47. class Drupal\Core\Entity\Query\Sql\QueryAggregate::addGroupBy
  48. class Drupal\Core\Entity\Query\Sql\QueryAggregate::addSortAggregate
  49. class Drupal\Core\Entity\Query\Sql\Query::prepare
  50. class Drupal\Core\Entity\Query\Sql\Query::compile
  51. class Drupal\Core\Entity\Query\Sql\Query::addSort
  52. class Drupal\Core\Entity\Query\Sql\Query::finish
  53. interface Drupal\Core\Entity\Query\ConditionAggregateInterface::condition
  54. interface Drupal\Core\Entity\Query\QueryAggregateInterface::aggregate
  55. interface Drupal\Core\Entity\Query\QueryAggregateInterface::groupBy
  56. interface Drupal\Core\Entity\Query\QueryAggregateInterface::conditionAggregate
  57. interface Drupal\Core\Entity\Query\QueryAggregateInterface::existsAggregate
  58. interface Drupal\Core\Entity\Query\QueryAggregateInterface::notExistsAggregate
  59. interface Drupal\Core\Entity\Query\QueryAggregateInterface::sortAggregate
  60. interface Drupal\Core\Entity\Query\QueryInterface::condition
  61. interface Drupal\Core\Entity\Query\QueryInterface::exists
  62. interface Drupal\Core\Entity\Query\QueryInterface::notExists
  63. interface Drupal\Core\Entity\Query\QueryInterface::pager
  64. interface Drupal\Core\Entity\Query\QueryInterface::range
  65. interface Drupal\Core\Entity\Query\QueryInterface::sort
  66. interface Drupal\Core\Entity\Query\QueryInterface::count
  67. interface Drupal\Core\Entity\Query\QueryInterface::tableSort
  68. interface Drupal\Core\Entity\Query\QueryInterface::accessCheck
  69. interface Drupal\Core\Entity\EntityConstraintViolationListInterface::getEntityViolations
  70. interface Drupal\Core\Entity\EntityConstraintViolationListInterface::getByFields
  71. class Drupal\Core\Field\Entity\BaseFieldOverride::createFromBaseFieldDefinition
  72. interface Drupal\Core\Field\FieldConfigInterface::getConfig
  73. class Drupal\Core\Form\EnforcedResponse::createFromException
  74. interface Drupal\Core\Archiver\ArchiverInterface::add
  75. interface Drupal\Core\Archiver\ArchiverInterface::remove
  76. interface Drupal\Core\Archiver\ArchiverInterface::extract
  77. class Drupal\Core\Cache\BackendChain::appendBackend
  78. class Drupal\Core\Cache\BackendChain::prependBackend
  79. interface Drupal\Core\ImageToolkit\ImageToolkitInterface::setSource
  80. interface Drupal\Core\Language\LanguageManagerInterface::reset
  81. interface Drupal\Core\Menu\LocalTaskInterface::setActive
  82. interface Drupal\Core\Session\AccountSwitcherInterface::switchTo
  83. interface Drupal\Core\Session\AccountSwitcherInterface::switchBack
  84. class Drupal\Core\StringTranslation\PluralTranslatableMarkup::createFromTranslatedString
  85. interface Drupal\Core\TypedData\TraversableTypedDataInterface::getParent
  86. interface Drupal\Core\TypedData\TraversableTypedDataInterface::getRoot
  87. interface Drupal\Core\TypedData\TypedDataInterface::applyDefaultValue
  88. class Drupal\Core\TypedData\ListDataDefinition::create
  89. class Drupal\Core\Updater\Updater::factory
  90. class Drupal\Core\Url::fromRoute
  91. class Drupal\Core\Url::fromUri
  92. class Drupal\Core\Url::fromEntityUri
  93. class Drupal\Core\Url::fromInternalUri
  94. class Drupal\Core\Url::fromRouteUri
  95. interface Drupal\block_content\BlockContentInterface::setInfo
  96. interface Drupal\block_content\BlockContentInterface::setRevisionLog
  97. interface Drupal\block_content\BlockContentInterface::setTheme
  98. interface Drupal\comment\CommentInterface::getParentComment
  99. interface Drupal\image\ImageStyleInterface::setName
  100. interface Drupal\locale\StringStorageInterface::save
  101. interface Drupal\locale\StringStorageInterface::delete
  102. class Drupal\locale\TranslationString::setCustomized
  103. interface Drupal\node\NodeInterface::setTitle
  104. interface Drupal\node\NodeInterface::setCreatedTime
  105. interface Drupal\node\NodeInterface::setPromoted
  106. interface Drupal\node\NodeInterface::setSticky
  107. interface Drupal\node\NodeInterface::setRevisionCreationTime
  108. interface Drupal\node\NodeInterface::setRevisionAuthorId
  109. interface Drupal\search\Plugin\SearchInterface::setSearch
  110. interface Drupal\shortcut\ShortcutInterface::setTitle
  111. interface Drupal\shortcut\ShortcutInterface::setWeight
  112. interface Drupal\shortcut\ShortcutSetInterface::resetLinkWeights
  113. class Drupal\system\Plugin\ImageToolkit\GDToolkit::setResource
  114. interface Drupal\user\UserInterface::setUsername
  115. interface Drupal\user\UserInterface::setPassword
  116. interface Drupal\user\UserInterface::setEmail
  117. interface Drupal\user\UserInterface::setLastAccessTime
  118. interface Drupal\user\UserInterface::setLastLoginTime
  119. interface Drupal\user\UserInterface::activate
  120. interface Drupal\user\UserInterface::block
  121. interface Drupal\views\Plugin\views\display\DisplayRouterInterface::getRoutedDisplay
CommentFileSizeAuthor
#16 drupal-2940038-16-return-type-hint-this-and-static.patch57.05 KBSweetchuck
#14 drupal-2940038-14-return-type-hint-this-and-static.patch56.98 KBSweetchuck
#13 drupal-2940038-13-return-type-hint-this-and-static.patch35.64 KBSweetchuck
#8 2940038-8--return_this_docs--step3--out_of_scope.patch52.11 KBdrunken monkey
#8 2940038-8--return_this_docs--step3--out_of_scope--interdiff.txt692 bytesdrunken monkey
#8 2940038-8--return_this_docs--step2--useless_descriptions.patch52.79 KBdrunken monkey
#8 2940038-8--return_this_docs--step2--useless_descriptions--interdiff.txt26.41 KBdrunken monkey
#8 2940038-8--return_this_docs--step1--error_correction.patch46.79 KBdrunken monkey
#8 2940038-8--return_this_docs--step1--error_correction--interdiff.txt13.25 KBdrunken monkey
#6 drupal-2940038-6-return-type-hint-this-and-static.patch48.3 KBSweetchuck
#3 2940038-3.patch47.32 KBrajeshwari10
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sweetchuck created an issue. See original summary.

rajeshwari10’s picture

rajeshwari10 CreditAttribution: rajeshwari10 commented
Assigned: Unassigned » rajeshwari10
rajeshwari10’s picture

rajeshwari10 CreditAttribution: rajeshwari10 commented
Assigned: rajeshwari10 » Unassigned
Status: Active » Needs review
FileSize
2940038-3.patch47.32 KB

Hi,

Adding the patch which has the changes as per mentioned. Please review.

Thanks

Sweetchuck’s picture

Sweetchuck
Primary language Hungarian
Location Budapest
CreditAttribution: Sweetchuck at Cheppers commented
Issue tags: +DX (Developer Experience)

Well done :-)
@return static would be better for these:

  • \Drupal\Core\Config\PreExistingConfigException::create

The \Drupal\Core\Entity\Query\Sql\QueryAggregate::finish overrides the PhpDoc without {@inheritdoc}, so probably the @return should be also explicitly defined.

The following methods are return with a new instance instead of the called object.
So the @return static would be better.

  • \Drupal\Core\Entity\EntityConstraintViolationListInterface::getEntityViolations
  • \Drupal\Core\Entity\EntityConstraintViolationListInterface::getByFields
  • \Drupal\Core\Field\Entity\BaseFieldOverride::createFromBaseFieldDefinition
  • \Drupal\Core\Form\EnforcedResponse::createFromException
  • \Drupal\Core\StringTranslation\PluralTranslatableMarkup::createFromTranslatedString
  • \Drupal\Core\TypedData\ListDataDefinition::create
  • \Drupal\Core\Updater\Updater::factory
  • \Drupal\Core\Url::fromRoute
  • \Drupal\Core\Url::fromRouteMatch
  • \Drupal\Core\Url::fromUri
  • \Drupal\Core\Url::fromEntityUri
  • \Drupal\Core\Url::fromInternalUri
  • \Drupal\comment\CommentInterface::getParentComment

\$this vs $this in

  • \Drupal\Core\Language\LanguageManagerInterface::reset
  • \Drupal\Core\Session\AccountSwitcherInterface::switchBack

Missing:

  • \Drupal\shortcut\ShortcutSetInterface::resetLinkWeights
Sweetchuck’s picture

Sweetchuck
Primary language Hungarian
Location Budapest
CreditAttribution: Sweetchuck at Cheppers commented
Status: Needs review » Needs work
Sweetchuck’s picture

Sweetchuck
Primary language Hungarian
Location Budapest
CreditAttribution: Sweetchuck at Cheppers commented
Status: Needs work » Needs review
FileSize
drupal-2940038-6-return-type-hint-this-and-static.patch48.3 KB
Sweetchuck’s picture

Sweetchuck
Primary language Hungarian
Location Budapest
CreditAttribution: Sweetchuck at Cheppers commented
Related issues: +#2941148: Fix Drupal.Commenting.FunctionComment.MissingReturnType
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Component: other » documentation
Related issues: -#2941148: Fix Drupal.Commenting.FunctionComment.MissingReturnType
FileSize
2940038-8--return_this_docs--step1--error_correction--interdiff.txt13.25 KB
2940038-8--return_this_docs--step1--error_correction.patch46.79 KB
2940038-8--return_this_docs--step2--useless_descriptions--interdiff.txt26.41 KB
2940038-8--return_this_docs--step2--useless_descriptions.patch52.79 KB
2940038-8--return_this_docs--step3--out_of_scope--interdiff.txt692 bytes
2940038-8--return_this_docs--step3--out_of_scope.patch52.11 KB

Thanks 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):

  1. Corrected things I saw as errors – $this or static where it wasn't appropriate.
  2. Since the coding standards explicitly allow omitting the description in these cases, I dropped them, too, where they were just pointless placeholders ("The called object.", mostly).
  3. While I completely agree with the change in \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/static return 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.)

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Component: documentation » other

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.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Sweetchuck’s picture

Sweetchuck
Primary language Hungarian
Location Budapest
CreditAttribution: Sweetchuck at Cheppers commented
Issue summary: View changes
Sweetchuck’s picture

Sweetchuck
Primary language Hungarian
Location Budapest
CreditAttribution: Sweetchuck as a volunteer commented
FileSize
drupal-2940038-13-return-type-hint-this-and-static.patch35.64 KB
Sweetchuck’s picture

Sweetchuck
Primary language Hungarian
Location Budapest
CreditAttribution: Sweetchuck as a volunteer commented
FileSize
drupal-2940038-14-return-type-hint-this-and-static.patch56.98 KB
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Reviewed & tested by the community

Looks 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.

Sweetchuck’s picture

Sweetchuck
Primary language Hungarian
Location Budapest
CreditAttribution: Sweetchuck as a volunteer and at Cheppers commented
FileSize
drupal-2940038-16-return-type-hint-this-and-static.patch57.05 KB

reroll

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed a784791 and pushed to 8.8.x. Thanks!

  • larowlan committed a784791 on 8.8.x 
    Issue #2940038 by Sweetchuck, drunken monkey, rajeshwari10, borisson_:...

Status: Fixed » Closed (fixed)

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