Fix Drupal.Commenting.FunctionComment.ParamNameNoMatch

Updated issue summary with review instructions.

No phpcs errors other than unrelated Drupal.Commenting.FunctionComment.ThrowsComment.

Manual review says this...

1. +++ b/core/lib/Drupal/Component/Assertion/Inspector.php
   @@ -205,7 +205,7 @@ public static function assertAllStrictArrays($traversable) {
       */
   -  public static function assertAllHaveKey() {
   +  public static function assertAllHaveKey($traversable) {
        $args = func_get_args();
        $traversable = array_shift($args);
   
   @@ -396,7 +396,7 @@ public static function assertAllRegularExpressionMatch($pattern, $traversable) {
   -  public static function assertAllObjects() {
   +  public static function assertAllObjects($traversable) {
        $args = func_get_args();
        $traversable = array_shift($args);
   

   Pretty sure these are set up that way for a reason. Don't change the method signature.

2. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -155,7 +155,7 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   -   * @param \Drupal\Core\Render\BubbleableMetadata $bubbleable_metadata|null
   +   * @param \Drupal\Core\Render\BubbleableMetadata $bubbleable_metadata
       *   (optional) An object to which static::generate() and the hooks and
       *   functions that it invokes will add their required bubbleable metadata.
   
   +++ b/core/modules/editor/editor.api.php
   @@ -51,7 +51,7 @@ function hook_editor_js_settings_alter(array &$settings) {
   - * @param \Drupal\filter\FilterFormatInterface $original_format|null
   + * @param \Drupal\filter\FilterFormatInterface $original_format
     *   (optional) The original text format configuration entity (when switching
   
   +++ b/core/modules/editor/editor.module
   @@ -261,7 +261,7 @@ function editor_load($format_id) {
   - * @param \Drupal\filter\FilterFormatInterface $original_format|null
   + * @param \Drupal\filter\FilterFormatInterface $original_format
     *   (optional) The original text format (i.e. when switching text formats,
   

   Put the |null after the type.

I have reverted the changes as suggested with a new patch

Thanks!
Chirag

Needs reroll since core/phpcs.xml.dist changed.

1. +++ b/core/includes/form.inc
   @@ -782,8 +782,8 @@ function batch_set($batch_definition) {
   + *   (optional) URL of the batch processing page.
   + *   Should only be used for separate scripts like update.php.
   

   All lines like this should extend out to an 80 character wrap.

2. +++ b/core/modules/views/src/Views.php
   @@ -283,7 +283,7 @@ public static function getDisabledViews() {
       * @param mixed $exclude_view
   -   *   view or current display to exclude
   +   *   View or current display to exclude
       *   either a
       *   - views object (containing $exclude_view->storage->name and $exclude_view->current_display)
       *   - views name as string:  e.g. my_view
   

   Should be...

   View or current display to exclude. Either a:

   All on one line.

Needs a reroll for changed phpcs.xml.dist.

The rest looks pretty reasonable.

These must have been introduced since that patch:

FILE: ...ojects/drupal8/core/lib/Drupal/Component/Assertion/Inspector.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 200 | ERROR | Doc comment for parameter $traversable does not match
     |       | actual variable name <undefined>
     |       | (Drupal.Commenting.FunctionComment.ParamNameNoMatch)
 390 | ERROR | Doc comment for parameter $traversable does not match
     |       | actual variable name <undefined>
     |       | (Drupal.Commenting.FunctionComment.ParamNameNoMatch)
----------------------------------------------------------------------


FILE: ...lmitchum/projects/drupal8/core/lib/Drupal/Core/Utility/Token.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 158 | ERROR | Doc comment for parameter $bubbleable_metadata|null does
     |       | not match actual variable name $bubbleable_metadata
     |       | (Drupal.Commenting.FunctionComment.ParamNameNoMatch)
----------------------------------------------------------------------


FILE: .../paulmitchum/projects/drupal8/core/modules/editor/editor.api.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 54 | ERROR | Doc comment for parameter $original_format|null does not
    |       | match actual variable name $original_format
    |       | (Drupal.Commenting.FunctionComment.ParamNameNoMatch)
----------------------------------------------------------------------


FILE: ...s/paulmitchum/projects/drupal8/core/modules/editor/editor.module
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 264 | ERROR | Doc comment for parameter $original_format|null does not
     |       | match actual variable name $original_format
     |       | (Drupal.Commenting.FunctionComment.ParamNameNoMatch)
----------------------------------------------------------------------

Time: 5 mins, 59.47 secs; Memory: 126.5Mb

There were some other errors in the log, but I deleted them and they're being handled here: #2747073: Fix Drupal CS regressions for Coder 8.2.8

Rerolled patch along with resolved issue as par comment #14

--- a/core/lib/Drupal/Component/Assertion/Inspector.php
+++ b/core/lib/Drupal/Component/Assertion/Inspector.php

@@ -197,11 +197,6 @@ public static function assertAllStrictArrays($traversable) {
    * to the object that uses them, or are collected into one assertion set for
    * the package.
    *
-   * @param mixed $traversable
-   *   Variable to be examined.
-   * @param string ...
-   *   Keys to be searched for.
-   *
    * @return bool
    *   TRUE if $traversable can be traversed and all members have all keys.
    */
@@ -387,11 +382,6 @@ public static function assertAllRegularExpressionMatch($pattern, $traversable) {

@@ -387,11 +382,6 @@ public static function assertAllRegularExpressionMatch($pattern, $traversable) {
    *     $collection', \'\\Foo\', \'\\Bar\\None\'');
    * @endcode
    *
-   * @param mixed $traversable
-   *   Variable to be examined.
-   * @param string ...
-   *   Classes and interfaces to test objects against.
-   *

These docblocks need to stay because the functions use parameters.

It looks like we don't have a standard for this: https://www.drupal.org/coding-standards/docs#param

Plus coder can't adapt to just @param with a type and no variable name.

So we could wait on the whole thing until we have a standard for this, or we could re-scope this issue a little bit and only handle Drupal.Commenting.FunctionComment.ParamCommentNotCapital.

Since the patch is more than 1 year old and Drupal.Commenting.FunctionComment.ParamCommentNotCapital is fixed in #2902726: Fix 'Drupal.Commenting.FunctionComment.ParamCommentNotCapital' coding standard I guess it's best to focus only on Drupal.Commenting.FunctionComment.ParamNameNoMatch in this issue.
I also updated the review instructions.

Here is the patch.

patch no longer applies.
and testbot is reporting a coding standard error to resolve.

I will lurk on this issue, with a hope of moving it to RTBC as soon as possible.

It is tricky every patch of this type causes others patches to need to reroll... I guess the only way to minimise reroll hell is to get each one committed before proceeding to the next.

Re-roll.

Two more fixes required.

I have had a careful comb through of all the changes... in each case the appropriate changes has been made.

Wow we had a lots of little screw-ups ... with a project of this age ... I guess that is to be expected.

test report no failures.

+1 from me

Removed the "Needs reroll" tag.

We are going through a pattern of commit an issue then reroll all similar issues ... repeat

The only solution is to slow the rate of review and wait until the next active issue get committed. :(

Simple re-rolls like that take no time at all.

Thanks ...

There is a scaling problem ... if I review 10 issues in a day .. that is potentially 9 rerolls.

Well 4 reviews in motion is doable ... but still unwelcome.

Anyway.. this looks good.

@martin107 Maybe postpone a few?

The conflicts are all due to the phpcs.yml.dist changes - if we moved those to one patch on one issue, the code style fixes could go in separately without conflicts.

Noticed a couple of things with the patch:

1. +++ b/core/lib/Drupal/Component/Assertion/Inspector.php
   @@ -205,9 +205,9 @@ public static function assertAllStrictArrays($traversable) {
   +  public static function assertAllHaveKey($traversable) {
        $args = func_get_args();
   -    $traversable = array_shift($args);
   +    unset($args[0]);
   

   $args is no longer used, so we could skip both lines?

2. +++ b/core/lib/Drupal/Component/Assertion/Inspector.php
   @@ -396,9 +396,9 @@ public static function assertAllRegularExpressionMatch($pattern, $traversable) {
   -  public static function assertAllObjects() {
   +  public static function assertAllObjects($traversable) {
        $args = func_get_args();
   -    $traversable = array_shift($args);
   +    unset($args[0]);
    
   

   Same here.

3. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
   @@ -423,9 +423,9 @@ protected function getAndSortConfigData($collection) {
       *
   -   * @param string $old_name
   +   * @param string $name1
       *   The old configuration object name.
   -   * @param string $new_name
   +   * @param string $name2
       *   The new configuration object name.
   

   $old_name and $new_name are more descriptive, why not change the argument variables instead of the docs

4. +++ b/core/lib/Drupal/Core/Executable/ExecutablePluginBase.php
   @@ -60,7 +60,7 @@ public function getConfig() {
       *
   -   * @param string $name
   +   * @param string $key
       *   The name of the configuration option to set.
       * @param mixed $value
   

   The description should match the param but now doesn't.

1. $args is used later in the function and is expected to contain more than one element.
2. ditto
3. Changed the variable names.
4. Changed the description.

I noticed a number of other documentation errors in Drupal\Core\Executable\ExecutablePluginBase, but did not consider them relevant to this issue so have opened a follow-up: #2904711: Missing and incorrect parameter, return, and thrown exception data types in Drupal\Core\Executable\ExecutablePluginBase.

I rerun the tests and everything seems to be fine now. The error was not related to the code in this patch.
Modifications look fine. I agree with Jo's notes for 1. and 2..

I took another look at the params' comments that we've modified in this issue and I think 3 more modifications are needed.

Regarding the changes $args issue ... yep the patch is correct.

From 31,

Maybe postpone a few?

I don't to stop other eyes passing over the issues completely .. it is a tricky balance.

My plan is to try and work down the list from top to bottom ....and stick on one 'til done.

I am happy with all the recent changes.... they improve things and I don't want to get hung up on what is out of scope and what is not.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!