Fix Drupal.Commenting.FunctionComment.IncorrectTypeHint and Drupal.Commenting.FunctionComment.InvalidTypeHint

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -892,7 +892,7 @@ function hook_ENTITY_TYPE_storage_load(array $entities) {
+function hook_entity_presave(\Drupal\Core\Entity\EntityInterface $entity) {

I'm pretty sure all of these hook type hints should be in use statements at the top of the file. It might be a different standard for .api.php files, but I doubt it.

The rest looks pretty OK but I didn't try too hard after finding the above.

I'm pretty sure all of these hook type hints should be in use statements at the top of the file. It might be a different standard for .api.php files, but I doubt it.

There is actually a standard on .api.php pages, indeed. I cannot find an official bit in https://www.drupal.org/node/1354#hooks but could read

If you use a namespace on a class anywhere in documentation, always make sure it is a fully-qualified namespace (beginning with a backslash).

like that.

+++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
@@ -59,7 +59,7 @@ public function query($get_count = FALSE) { }
-   * @param view $view
+   * @param ViewExecutable $view

@@ -67,7 +67,7 @@ public function alter(ViewExecutable $view) {  }
-   * @param view $view
+   * @param ViewExecutable $view

@@ -82,7 +82,7 @@ public function build(ViewExecutable $view) { }
-   * @param view $view
+   * @param ViewExecutable $view

@@ -93,7 +93,7 @@ public function execute(ViewExecutable $view) {  }
-   * @param view $view
+   * @param ViewExecutable $view

Let's make that FQCN, so with the namespace

If you use a namespace on a class anywhere in documentation, always make sure it is a fully-qualified namespace (beginning with a backslash).

I guess it's much easier to read in the context of an API code example than looking for the use statement.

I guess it's much easier to read in the context of an API code example than looking for the use statement.

Yes, that is a good point.

Patch in #2 no longer applies.

automerged

Patch rerolled against 8.4.x

I applied the 'fix-2723621-14.patch' patch and it failed in 'core/phpcs.xml.dist' file.

cit008490cpsubuntu:drupal progestag* 8.4.x$ cat core/phpcs.xml.dist.rej

--- core/phpcs.xml.dist
+++ core/phpcs.xml.dist
@@ -44,10 +44,8 @@
   <rule ref="Drupal.Commenting.DocCommentStar"/>
   <rule ref="Drupal.Commenting.FileComment"/>
   <rule ref="Drupal.Commenting.FunctionComment">
-    <exclude name="Drupal.Commenting.FunctionComment.IncorrectTypeHint"/>
     <exclude name="Drupal.Commenting.FunctionComment.InvalidNoReturn"/>
     <exclude name="Drupal.Commenting.FunctionComment.InvalidReturnNotVoid"/>
-    <exclude name="Drupal.Commenting.FunctionComment.InvalidTypeHint"/>
     <exclude name="Drupal.Commenting.FunctionComment.Missing"/>
     <exclude name="Drupal.Commenting.FunctionComment.MissingFile"/>
     <exclude name="Drupal.Commenting.FunctionComment.MissingParamComment"/>

I fixed the problem I found in the 'fix-2723621-14.patch' patch and posted a new one. ( 'fix-2723621-16.patch')

+++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
@@ -59,7 +59,7 @@ public function query($get_count = FALSE) { }
-   * @param view $view
+   * @param ViewExecutable $view

@@ -67,7 +67,7 @@ public function alter(ViewExecutable $view) {  }
-   * @param view $view
+   * @param ViewExecutable $view

@@ -82,7 +82,7 @@ public function build(ViewExecutable $view) { }
-   * @param view $view
+   * @param ViewExecutable $view

@@ -93,7 +93,7 @@ public function execute(ViewExecutable $view) {  }
-   * @param view $view
+   * @param ViewExecutable $view

Is there a reason we don't use a fully qualified class name (FQCN)?

I added the "coding standards" tag.

Here is the new patch. There were some new errors and I also fixed the issues mentioned at #18.

Re-roll.

Re-roll.

All these changes look justifiable to me.

Looking at the lats testbot report ... I can see no last minute errors have crept in.

In short, I think this patch looks great .. and we should get this in..

+++ b/core/includes/install.inc
@@ -457,7 +457,7 @@ function _drupal_rewrite_settings_dump($variable, $variable_name) {
- * @param object $variable
+ * @param \stdClass $variable

'object' is correct.

https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

Does coder disagree?

_drupal_rewrite_settings_dump_one() is defined as:
function _drupal_rewrite_settings_dump_one(\stdClass $variable, $prefix = '', $suffix = '')
and so PHPCS complains about:
@param object $variable

@param \stdClass is used in other places too.

The coding standard says use object and not stdClass: https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

Also needs a re-roll.

If Coder adds stdClass then it's wrong. If there's not a sniff for object vs. stdClass then there should be. In either (or both) of those cases we should file an issue against Coder.

Here is the updated patch and the interdiff.
The interdiff contains removal of phpcs.local.xml which was present at #26 and shouldn't have been part of the patch.

@Mile23 I guess we should also open a follow-up for other places where \stdClass is used as a typehint. Should we fix them here? Gray area between in and out of scope :). What do you think?

I opened #2911404: Drupal.Commenting.FunctionComment.InvalidTypeHint/FunctionComment - \stdClass for the stdClass vs \stdClass issue.

Checked to see if patch applies, and it does, pretty cleanly (no errors). Not enough time to test to see if the phpcs is now passing.

Hopefully this comment encourages another to run the tests.

Friendly post #2902381: Revert phpcs.xml.dist changes from "Ensure that we're using the right ruleset for coding standards checking" reroll.
No credit needed.

Checked the patch "drupal-coding-standards-2723621-35.patch" .
It applies cleanly. I also ran the phpcs test after that and there were no errors.

+++ b/core/includes/install.inc
@@ -467,7 +467,7 @@ function _drupal_rewrite_settings_dump($variable, $variable_name) {
-function _drupal_rewrite_settings_dump_one(\stdClass $variable, $prefix = '', $suffix = '') {
+function _drupal_rewrite_settings_dump_one($variable, $prefix = '', $suffix = '') {

The patch is still mostly fine, but it does the above. We shouldn't remove a valid type hint.

Reverting that file and then sniffing it yields this:

FILE: /Users/paul/pj2/drupal/core/includes/install.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 470 | ERROR | Unknown type hint "\stdClass" found for $variable
     |       | (Drupal.Commenting.FunctionComment.InvalidTypeHint)
----------------------------------------------------------------------

Time: 1.22 secs; Memory: 12Mb

Changing the hint to stdClass (no \) gets a pass from phpcs. I suppose that's because it's in an .inc file which is in the top-level namespace. InvalidTypeHint doesn't seem to care about that namespace \ in other usages, such as within a namespaced class. So InvalidTypeHint doesn't seem to be at 100% #2911404: Drupal.Commenting.FunctionComment.InvalidTypeHint/FunctionComment - \stdClass

If we add back stdClass (instead of \stdClass), then this patch will be ready to go. If the sniff ever gets fixed, and it turns out stdClass is not right, then it will show up and we can fix it then.

Given it a try.

This addresses the issues from #37 and PHPCS is happy. :-)

+++ b/core/modules/menu_ui/src/Tests/MenuTest.php
@@ -730,7 +730,7 @@ public function verifyMenuLink(MenuLinkContent $item, $item_node, MenuLinkConten
-   * @param \Drupal\menu_link_content\MenuLinkContentInterface $item
+   * @param \Drupal\menu_link_content\MenuLinkContent $item

Why wouldn't we use the interface? Does the method signature need to be fixed instead?

#41 I agree with ideally this would type hint the MenuLinkContentInterface. However, if you look at the MenuUiTest.php file, all other methods typehint the MenuLinkContent. Considering this issue is about bring the type hint in line with the actual code, I suggest we go with the smallest change possible and update the documentation to match the code here and leave the change in the actual code to a follow up.

The MenuUiTest was moved in #2723621: Fix Drupal.Commenting.FunctionComment.IncorrectTypeHint and Drupal.Commenting.FunctionComment.InvalidTypeHint, so the patch no longer applies.

Re-rolled.

Patch still applies, but I see this:

$ ../vendor/bin/phpcs -ps .

FILE: ...cts/drupal8/core/tests/Drupal/KernelTests/AssertContentTrait.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 987 | ERROR | Expected type hint "SimpleXmlElement"; found
     |       | "\SimpleXMLElement" for $element
     |       | (Drupal.Commenting.FunctionComment.IncorrectTypeHint)
----------------------------------------------------------------------


FILE: ...drupal8/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 729 | ERROR | Expected type hint "MenuLinkContentInterface"; found
     |       | "MenuLinkContent" for $item
     |       | (Drupal.Commenting.FunctionComment.IncorrectTypeHint)
----------------------------------------------------------------------


FILE: ...edia/tests/src/Functional/FieldFormatter/OEmbedFormatterTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 131 | ERROR | Unknown type hint "array" found for
     |       | $formatter_settings
     |       | (Drupal.Commenting.FunctionComment.InvalidTypeHint)
----------------------------------------------------------------------

Time: 5 mins, 52.9 secs; Memory: 86Mb

Corrected the 3 errors.

+++ b/core/tests/Drupal/KernelTests/AssertContentTrait.php
@@ -984,7 +984,7 @@ protected function assertFieldByXPath($xpath, $value = NULL, $message = '', $gro
-  protected function getSelectedItem(\SimpleXMLElement $element) {
+  protected function getSelectedItem(SimpleXMLElement $element) {

This line is changed the wrong way around. The @param should mention \SimpleXMLElement (mind the capitals). The current method argument in HEAD is correct.

Corrected my incorrect correction!

Thanks @idebr.

DrupalCI reports 4 additional incorrect typehints at the latest build: https://www.drupal.org/pift-ci-job/1017856

These were added recently in #2975754: Add hooks to act on a new revision being created

/var/lib/drupalci/workspace/jenkins-drupal_patches-65188/source/core/lib/Drupal/Core/Entity/entity.api.php
line 930 Expected type hint "EntityInterface"; found "Drupal\Core\Entity\EntityInterface" for $new_revision
930 Expected type hint "EntityInterface"; found "Drupal\Core\Entity\EntityInterface" for $entity
954 Expected type hint "EntityInterface"; found "Drupal\Core\Entity\EntityInterface" for $new_revision
954 Expected type hint "EntityInterface"; found "Drupal\Core\Entity\EntityInterface" for $entity

This calls back to #6.

So I think we need clarity on whether .api pages should have fully-qualified class names in hints or not, and make sure there's a coder rule for it.

Corrected the 4 coding standards errors.

Ahh the slashes. :-)

LGTM, and the CI, so yay.

+++ b/core/includes/install.inc
@@ -465,7 +465,7 @@ function _drupal_rewrite_settings_dump($variable, $variable_name) {
-function _drupal_rewrite_settings_dump_one(\stdClass $variable, $prefix = '', $suffix = '') {
+function _drupal_rewrite_settings_dump_one(stdClass $variable, $prefix = '', $suffix = '') {

This change is really odd. Imo we shouldn't be making it. The whole coding standard of object vs stdClass for any old object makes sense in every cases apart from this one where the thing is a \stdClass. That said maybe the gains of the patch outweigh this one awkwardness and we can address it more properly in the follow-up. Imo coder should be fixed as per @Mile23's comment on #2911404: Drupal.Commenting.FunctionComment.InvalidTypeHint/FunctionComment - \stdClass. Maybe we should postpone on that - dunno.

Okie.

Here we go... the blocker went in ages ago.

Too many DateFormatInterface classes :)

Turning on new coding standards a minor-only change (although we can backport the cleanups without the rule in some cases to preserve cherry-picks). Since this is just docs, it's backportable; however, the backport patch would need to have a version that does not include the phpcs.xml.dist change.

Needs a reroll.

Re-rolled the patch in #62. Thanks!

Updated patch. Please review!

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -8,6 +8,7 @@
+use Drupal\Core\Entity\EntityInterface;

Can we not add this line and keep the long format. This is an example file for other developers.

For some reason the last two patches are getting applied. So I have excluded the changes done on core/lib/Drupal/Core/Entity/entity.api.php as per the suggestion on #68.

Kindly review the patch.

Oops !!
Updated the patch.

Fixed the reroll, addressed three other issues found by running phpcs.

All code changes look good to me.
I have run PHPCS on my local machine and with the patch applied there are no errors.
For me is it RTBC.

I was wondering if we should create a followup to fix:

diff --git a/core/modules/system/src/Plugin/migrate/destination/EntityDateFormat.php b/core/modules/system/src/Plugin/migrate/destination/EntityDateFormat.php
index 42ba1661c3..9c7daaf1ba 100644
--- a/core/modules/system/src/Plugin/migrate/destination/EntityDateFormat.php
+++ b/core/modules/system/src/Plugin/migrate/destination/EntityDateFormat.php
@@ -2,6 +2,7 @@
 
 namespace Drupal\system\Plugin\migrate\destination;
 
+use Drupal\Core\Datetime\DateFormatInterface;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\migrate\Plugin\migrate\destination\EntityConfigBase;
 
@@ -15,10 +16,11 @@ class EntityDateFormat extends EntityConfigBase {
   /**
    * {@inheritdoc}
    *
-   * @param \Drupal\Core\Datetime\DateFormatInterface $entity
+   * @param \Drupal\Core\Entity\EntityInterface $entity
    *   The date entity.
    */
   protected function updateEntityProperty(EntityInterface $entity, array $parents, $value) {
+    assert($entity instanceof DateFormatInterface);
     if ($parents[0] == 'pattern') {
       $entity->setPattern($value);
     }

Would be good to have a follow-up for #72. I also I think we should leave the assert() out of the 9.1.x backport (along with the hunk that enables the drupalcs checks).

+++ b/core/modules/system/src/Plugin/migrate/destination/EntityDateFormat.php
@@ -15,10 +16,11 @@ class EntityDateFormat extends EntityConfigBase {
   /**
    * {@inheritdoc}
    *
-   * @param \Drupal\Core\Datetime\DateFormatInterface $entity
+   * @param \Drupal\Core\Entity\EntityInterface $entity
    *   The date entity.
    */

This should remove the @param doc entirely. Having {inheritdoc} and a single @param override breaks api.drupal.org - see https://api.drupal.org/api/drupal/core%21modules%21system%21src%21Plugin...

This method has three params but only one is documented.

If we want to change one param we have to provide a full docblock.

Removed the @param doc.
We can handle #72 in a follow-up.

Committed 203ad06 and pushed to 9.2.x. Thanks!

The patch doesn't apply to 9.1.x so fixing only in 9.2.x.