Fix 'Drupal.Commenting.HookComment' coding standard

We've changed the way we do phpcs.xml.dist - its inclusive not exclusive.

+++ b/core/modules/locale/locale.module
@@ -219,7 +219,7 @@ function locale_configurable_language_update(ConfigurableLanguageInterface $lang
- * Implements hook_ENTITY_TYPE_delete() for 'configurable_language'.
+ * Implements hook_ENTITY_TYPE_delete().

+++ b/core/modules/node/node.module
@@ -527,7 +527,7 @@ function node_preprocess_html(&$variables) {
- * Implements hook_preprocess_HOOK() for block templates.
+ * Implements hook_preprocess_HOOK().

@@ -1321,7 +1321,7 @@ function node_reindex_node_search($nid) {
- * Implements hook_ENTITY_TYPE_insert() for comment entities.
+ * Implements hook_ENTITY_TYPE_insert().

I'm not sure about these changes. I think we should update the rule to permit this.

AT least #15 is a good novice task.

The change in #15 has been submitted as issue #2821834: Loosen the Drupal.Commenting.HookComment sniff per #2623718, is that ok?

About #15 from core/vendor/drupal/coder/coder_sniffer/Drupal/Sniffs/Commenting/HookCommentSniff.php:92
Format should be "* Implements hook_foo().", "* Implements hook_foo_BAR_ID_bar() for xyz_bar().",, "* Implements hook_foo_BAR_ID_bar() for xyz-bar.html.twig.", "* Implements hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.", or "* Implements hook_foo_BAR_ID_bar() for block templates."

This patch needs rework, here's a patch to check state

Locally

core$ ../vendor/bin/phpcs -sp --sniffs=Drupal.Commenting.HookComment --report=source .
...
PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
Internal.NoCodeFound                                             279
Internal.LineEndings.Mixed                                       54
Drupal.Commenting.HookComment                                    14
Drupal.Commenting.HookComment.HookParamDoc                       4
Drupal.Commenting.HookComment.HookReturnDoc                      1
----------------------------------------------------------------------
A TOTAL OF 352 SNIFF VIOLATIONS WERE FOUND IN 5 SOURCES
----------------------------------------------------------------------

Totally new patch, looks everything fixed

This patch no longer applies, but all the changes in it look really solid. Setting to needs work so someone can reroll this.

Here reroll for 8.6.x

Removing the needs reroll tag, patch applies and after running composer phpcs there are no remaining errors.

+++ b/core/modules/update/update.install
@@ -10,19 +10,18 @@
+ * Describes the status of the site regarding available updates. If
+ * there is no update data, only one record will be returned, indicating that
+ * the status of core can't be determined. If data is available, there will be
+ * two records: one for core, and another for all of contrib (assuming there
+ * are any contributed modules or themes enabled on the site). In addition to
+ * the fields expected by hook_requirements ('value', 'severity', and
+ * optionally 'description'), this array will contain a 'reason' attribute,
+ * which is an integer constant to indicate why the given status is being
+ * returned (UPDATE_NOT_SECURE, UPDATE_NOT_CURRENT, or UPDATE_UNKNOWN). This
+ * is used for generating the appropriate email notification messages during
+ * update_cron(), and might be useful for other modules that invoke
+ * update_requirements() to find out if the site is up to date or not.

This comment needs re-flowing - we can use the full 80 chars here.

We've got some new errors here and they are interesting...

core/modules/workspace/src/EntityAccess.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 8 WARNINGS AFFECTING 8 LINES
----------------------------------------------------------------------
 62 | WARNING | Hook implementations should not duplicate @param
    |         | documentation
    |         | (Drupal.Commenting.HookComment.HookParamDoc)
 64 | WARNING | Hook implementations should not duplicate @param
    |         | documentation
    |         | (Drupal.Commenting.HookComment.HookParamDoc)
 66 | WARNING | Hook implementations should not duplicate @param
    |         | documentation
    |         | (Drupal.Commenting.HookComment.HookParamDoc)
 69 | WARNING | Hook implementations should not duplicate @return
    |         | documentation
    |         | (Drupal.Commenting.HookComment.HookReturnDoc)
 88 | WARNING | Hook implementations should not duplicate @param
    |         | documentation
    |         | (Drupal.Commenting.HookComment.HookParamDoc)
 90 | WARNING | Hook implementations should not duplicate @param
    |         | documentation
    |         | (Drupal.Commenting.HookComment.HookParamDoc)
 92 | WARNING | Hook implementations should not duplicate @param
    |         | documentation
    |         | (Drupal.Commenting.HookComment.HookParamDoc)
 95 | WARNING | Hook implementations should not duplicate @return
    |         | documentation
    |         | (Drupal.Commenting.HookComment.HookReturnDoc)
----------------------------------------------------------------------

This is a class and not a direct hook implementation. I think the rule should be updated to only test procedural code and not classes. Once we have class based hooks they'll probably use an interface and therefore we'll have @inheritdoc.

So we need an issue filed against coder to fix the rule.

Filed an issue in the Coder module: #2971177: Drupal.Commenting.HookComment should only test procedural code and not classes

Pushed a fix for Coder. It will take a couple of weeks until I will make the next release, you can help updating Coder at #3063323: Update drupal/coder to 8.3.6 in the meantime.

The fix for coder has been implemented, currently the failures for this sniff are in the following files, so the issue in #29 is resolved and this can be picked up again:

/core/modules/image/image.install
/core/modules/config/tests/config_install_dependency_test/config_install_dependency_test.module
/core/modules/breakpoint/breakpoint.module
/core/modules/update/update.install
/core/modules/update/update.module
/core/modules/filter/filter.module
/core/modules/language/language.module
/core/modules/content_moderation/tests/modules/content_moderation_test_views/content_moderation_test_views.module
/core/modules/search/tests/modules/search_query_alter/search_query_alter.module
/core/modules/search/tests/modules/search_date_query_alter/search_date_query_alter.module
/core/modules/system/tests/modules/form_test/form_test.module
/core/modules/system/tests/modules/common_test/common_test.module
/core/modules/system/tests/modules/module_test/module_test.module

Re-rolled for 9.2.x.

The patch in #38 is empty

Re-roll patch for 9.2.x.

This is looking close now.

1. +++ b/core/modules/image/image.install
   @@ -33,9 +33,9 @@ function image_uninstall() {
   + * Checks the PHP GD Library.
   

   This isn't strictly true as we allow multiple image libraries - I think we can just remove this line and just say

   /**
    * Implements hook_requirements().
    */
   

2. +++ b/core/phpcs.xml.dist
   @@ -75,6 +75,7 @@
   +  <rule ref="../vendor/drupal/coder/coder_sniffer/Drupal/Sniffs/Commenting/HookCommentSniff.php"/>
   

   This should be

   <rule ref="Drupal.Commenting.HookComment"/>
   

3. +++ b/core/themes/bartik/bartik.theme
   @@ -115,9 +115,10 @@ function bartik_theme_suggestions_form_alter(array &$suggestions, array $variabl
    /**
     * Implements hook_form_alter().
   + *
   + * Adds classes to the search form.
     */
    function bartik_form_alter(&$form, FormStateInterface $form_state, $form_id) {
   -  // Add classes to the search form.
   

   Is this change really in scope here?

@longwave Changes has been done according to #42, Please have a look and advise.

Thanks, those changes all look good. I ran phpcs locally over the full codebase and there is one remaining issue here:

FILE: core/modules/content_moderation/tests/modules/content_moderation_test_views/content_moderation_test_views.module
---------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
---------------------------------------------------------------------------------------------------------------------------------------------------------
 14 | WARNING | Hook implementations should not duplicate @param documentation
 16 | WARNING | Hook implementations should not duplicate @param documentation
---------------------------------------------------------------------------------------------------------------------------------------------------------

In other words the @param lines need to be removed from:

/**
 * Implements hook_views_query_alter().
 *
 * @param \Drupal\views\ViewExecutable $view
 *   The view object about to be processed.
 * @param \Drupal\views\Plugin\views\query\QueryPluginBase $query
 *   The query plugin object for the query.
 *
 * @see \Drupal\Tests\content_moderation\Kernel\ViewsModerationStateSortTest::testSortRevisionBaseTable()
 */

Updated the patch in #43 as suggested in #45. Thanks!

Thanks, this looks ready to commit now.

Committed/pushed to 9.2.x and 9.1.x.

+++ b/core/modules/update/update.module
@@ -390,16 +390,6 @@ function update_fetch_data_finished($success, $results) {
  * @see _update_message_text()
diff --git a/core/phpcs.xml.dist b/core/phpcs.xml.dist

diff --git a/core/phpcs.xml.dist b/core/phpcs.xml.dist
index bf22ac6842..223ae7ed72 100644

index bf22ac6842..223ae7ed72 100644
--- a/core/phpcs.xml.dist

--- a/core/phpcs.xml.dist
+++ b/core/phpcs.xml.dist

+++ b/core/phpcs.xml.dist
+++ b/core/phpcs.xml.dist
@@ -75,6 +75,7 @@

@@ -75,6 +75,7 @@
     <exclude name="Drupal.Commenting.FunctionComment.ParamMissingDefinition"/>
     <exclude name="Drupal.Commenting.FunctionComment.TypeHintMissing"/>
   </rule>
+  <rule ref="Drupal.Commenting.HookComment"/>
   <rule ref="Drupal.Commenting.GenderNeutralComment" />
   <rule ref="Drupal.Commenting.VariableComment">

Removed this hunk for the 9.1.x backport.