Problem/Motivation

Running PHPCS in the GitLab CI reports the following issues:

FILE: ...60/web/modules/custom/migrate_tools-3410760/tests/src/Kernel/DrushTest.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 56 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
 57 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
--------------------------------------------------------------------------------
FILE: ...om/migrate_tools-3410760/tests/src/Functional/MigrateExecutionFormTest.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 42 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
 43 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
--------------------------------------------------------------------------------
FILE: ...tests/modules/migrate_tools_test/src/Commands/MigrateToolsTestCommands.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 17 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
--------------------------------------------------------------------------------
FILE: ...ules/custom/migrate_tools-3410760/src/MigrateSharedConfigPluginManager.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 50 | ERROR | [x] Missing function doc comment
    |       |     (Drupal.Commenting.FunctionComment.Missing)
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
FILE: ...760/web/modules/custom/migrate_tools-3410760/src/MigrateIncludeHandler.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 15 | ERROR | [ ] Missing member variable doc comment
    |       |     (Drupal.Commenting.VariableComment.Missing)
 17 | ERROR | [x] Missing function doc comment
    |       |     (Drupal.Commenting.FunctionComment.Missing)
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
FILE: ...3410760/web/modules/custom/migrate_tools-3410760/src/MigrateExecutable.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 85 | ERROR | Missing @var tag in member variable comment
    |       | (Drupal.Commenting.VariableComment.MissingVar)
--------------------------------------------------------------------------------
FILE: ...60/web/modules/custom/migrate_tools-3410760/src/MigrateBatchExecutable.php
--------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
--------------------------------------------------------------------------------
 43 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
 44 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
 45 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
 46 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
--------------------------------------------------------------------------------
FILE: ...60/web/modules/custom/migrate_tools-3410760/src/Routing/RouteProcessor.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 17 | ERROR | [ ] Missing member variable doc comment
    |       |     (Drupal.Commenting.VariableComment.Missing)
 19 | ERROR | [x] Missing function doc comment
    |       |     (Drupal.Commenting.FunctionComment.Missing)
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
FILE: ...s/custom/migrate_tools-3410760/src/EventSubscriber/MigrationImportSync.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 22 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
--------------------------------------------------------------------------------
FILE: ...igrate_tools-3410760/src/EventSubscriber/MigrationDrushCommandProgress.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 19 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
 20 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
--------------------------------------------------------------------------------
FILE: ...s/custom/migrate_tools-3410760/src/Drush/Commands/MigrateToolsCommands.php
--------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
--------------------------------------------------------------------------------
 252 | ERROR | The array declaration extends to column 244 (the limit is 120).
     |       | The array content should be split up over multiple lines
     |       | (Drupal.Arrays.Array.LongLineDeclaration)
 253 | ERROR | The array declaration extends to column 125 (the limit is 120).
     |       | The array content should be split up over multiple lines
     |       | (Drupal.Arrays.Array.LongLineDeclaration)
 751 | ERROR | The array declaration extends to column 186 (the limit is 120).
     |       | The array content should be split up over multiple lines
     |       | (Drupal.Arrays.Array.LongLineDeclaration)
--------------------------------------------------------------------------------
FILE: ...ules/custom/migrate_tools-3410760/src/Discovery/YamlDiscoveryDecorator.php
--------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 24 | ERROR | Parameter $file_cache_key_suffix is not described in comment
    |       | (Drupal.Commenting.FunctionComment.ParamMissingDefinition)
 24 | ERROR | Parameter $key is not described in comment
    |       | (Drupal.Commenting.FunctionComment.ParamMissingDefinition)
 29 | ERROR | Doc comment for parameter $name does not match actual variable
    |       | name $key (Drupal.Commenting.FunctionComment.ParamNameNoMatch)
--------------------------------------------------------------------------------
FILE: ...dules/custom/migrate_tools-3410760/src/Controller/MigrationListBuilder.php
--------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
--------------------------------------------------------------------------------
 29 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
 30 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
 31 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
--------------------------------------------------------------------------------
FILE: ...odules/custom/migrate_tools-3410760/src/Controller/MigrationController.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 26 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
 27 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
--------------------------------------------------------------------------------
FILE: .../modules/custom/migrate_tools-3410760/src/Controller/MessageController.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 25 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
 26 | ERROR | Missing member variable doc comment
    |       | (Drupal.Commenting.VariableComment.Missing)
--------------------------------------------------------------------------------
FILE: ...ools-3410760/web/modules/custom/migrate_tools-3410760/migrate_tools.module
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 4 | ERROR | [x] Doc comment short description must be on the first line
   |       |     (Drupal.Commenting.DocComment.SpacingBeforeShort)
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
Time: 679ms; Memory: 12MB
PHP CODE SNIFFER REPORT SUMMARY
--------------------------------------------------------------------------------
FILE                                                            ERRORS  WARNINGS
--------------------------------------------------------------------------------
...b/modules/custom/migrate_tools-3410760/migrate_tools.module  1       0
...custom/migrate_tools-3410760/src/MigrateBatchExecutable.php  4       0
...ules/custom/migrate_tools-3410760/src/MigrateExecutable.php  1       0
.../custom/migrate_tools-3410760/src/MigrateIncludeHandler.php  2       0
...rate_tools-3410760/src/MigrateSharedConfigPluginManager.php  1       0
.../migrate_tools-3410760/src/Controller/MessageController.php  2       0
...igrate_tools-3410760/src/Controller/MigrationController.php  2       0
...grate_tools-3410760/src/Controller/MigrationListBuilder.php  3       0
...rate_tools-3410760/src/Discovery/YamlDiscoveryDecorator.php  3       0
...e_tools-3410760/src/Drush/Commands/MigrateToolsCommands.php  3       0
...10760/src/EventSubscriber/MigrationDrushCommandProgress.php  2       0
...e_tools-3410760/src/EventSubscriber/MigrationImportSync.php  1       0
...custom/migrate_tools-3410760/src/Routing/RouteProcessor.php  2       0
...igrate_tools_test/src/Commands/MigrateToolsTestCommands.php  1       0
...s-3410760/tests/src/Functional/MigrateExecutionFormTest.php  2       0
...custom/migrate_tools-3410760/tests/src/Kernel/DrushTest.php  2       0
--------------------------------------------------------------------------------
A TOTAL OF 32 ERRORS AND 0 WARNINGS WERE FOUND IN 43 FILES
--------------------------------------------------------------------------------
PHPCBF CAN FIX 4 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
    SOURCE                                                       COUNT
----------------------------------------------------------------------
[ ] Drupal.Commenting.VariableComment.Missing                    21
[ ] Drupal.Arrays.Array.LongLineDeclaration                      3
[x] Drupal.Commenting.FunctionComment.Missing                    3
[ ] Drupal.Commenting.FunctionComment.ParamMissingDefinition     2
[x] Drupal.Commenting.DocComment.SpacingBeforeShort              1
[ ] Drupal.Commenting.FunctionComment.ParamNameNoMatch           1
[ ] Drupal.Commenting.VariableComment.MissingVar                 1
----------------------------------------------------------------------
A TOTAL OF 32 SNIFF VIOLATIONS WERE FOUND IN 7 SOURCES
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SOURCES AUTOMATICALLY (4 VIOLATIONS IN TOTAL)
----------------------------------------------------------------------

Steps to reproduce

Open job log of the following MR:

https://git.drupalcode.org/issue/migrate_tools-3410760/-/jobs/538852

Verify PHPCS failures.

Proposed resolution

Most of the issues are docblock comments. Fill in the missing comments.

Remaining tasks

Make the change.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Issue fork migrate_tools-3411051

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

lostcarpark created an issue. See original summary.

lostcarpark opened merge request !53

lostcarpark’s picture

lostcarpark commented

PHPCS validation now passing with no issues.

Most of the fixes were adding PHP docblocks.

However, in src/Drush/Commands/MigrateToolsCommands.php the issue was long arrays in PHP attributes.

First tried splitting these into multiple lines. This fixed the PHPCS issue, but the PREVIOUS_MAJOR test fails, because attributes not supported by PHP7.4.

Added // phpcs:disable Drupal.Arrays.Array.LongLineDeclaration to ignore. However, the extra comment made PHPCS think the DocBlock was missing, so had to add // phpcs:disable Drupal.Commenting.FunctionComment.Missing, Squiz.WhiteSpace.FunctionSpacing.Before to the function declaration to suppress that warning.

Hopefully, support for D9 will eventually be dropped from the module, and at that point we should tidy this up.

lostcarpark’s picture

lostcarpark commented
Status: Active » Needs review
hetal.solanki’s picture

hetal.solanki
Primary language English
commented
Issue tags: +Coding standards

I have tested MR 53, and i have found some errors below:

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig web/modules/contrib/migrate_tools-3411051/

FILE: /var/www/html/drupal-contribution-d10/web/modules/contrib/migrate_tools-3411051/src/Drush/Commands/MigrateToolsCommands.php
------------------------------------------------------------------------------------------------------------------------------------------
FOUND 4 ERRORS AND 4 WARNINGS AFFECTING 8 LINES
------------------------------------------------------------------------------------------------------------------------------------------
114 | WARNING | Unused variable $group.
481 | WARNING | Unused variable $group_id.
584 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
682 | ERROR | The array declaration extends to column 86 (the limit is 80). The array content should be split up over multiple lines
758 | ERROR | The array declaration extends to column 102 (the limit is 80). The array content should be split up over multiple lines
863 | ERROR | The array declaration extends to column 88 (the limit is 80). The array content should be split up over multiple lines
867 | ERROR | The array declaration extends to column 94 (the limit is 80). The array content should be split up over multiple lines
1072 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
------------------------------------------------------------------------------------------------------------------------------------------

FILE: /var/www/html/drupal-contribution-d10/web/modules/contrib/migrate_tools-3411051/src/MigrateBatchExecutable.php
--------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------
300 | WARNING | Unused variable $migration_id.
--------------------------------------------------------------------------------------------------------------------

FILE: /var/www/html/drupal-contribution-d10/web/modules/contrib/migrate_tools-3411051/src/Form/MigrationExecuteForm.php
--------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------------------------
169 | ERROR | The array declaration extends to column 94 (the limit is 80). The array content should be split up over multiple lines
--------------------------------------------------------------------------------------------------------------------------------------

FILE: /var/www/html/drupal-contribution-d10/web/modules/contrib/migrate_tools-3411051/tests/modules/migrate_shared_config_test/migrate_shared_config_test.info.yml
------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------------------
1 | WARNING | "Description" property is missing in the info.yml file
------------------------------------------------------------------------------------------------------------------------------------------------------------------

vinmayiswamy’s picture

vinmayiswamy commented

I have tested MR !53 in Drupal version 10.2.x, and confirmed it fixes all the errors reported by phpcs. But we still have a few warnings in the /src/Controller/MigrationController.php file.

$ ./vendor/bin/phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml modules/contrib/migrate_tools

FILE: /Users/vins/projects/drupal102x/drupal/modules/contrib/migrate_tools/src/Controller/MigrationController.php
------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
------------------------------------------------------------------------------------------------------------------------------
 12 | WARNING | [x] Unused use statement
 13 | WARNING | [x] Unused use statement
 17 | WARNING | [x] Unused use statement
 19 | WARNING | [x] Unused use statement
------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------------------

Time: 727ms; Memory: 20MB

Thanks!

Chandansha made their first commit to this issue’s fork.

chandansha’s picture

chandansha commented

i have tested MR 53. i FOUND SOME WARNING. i fixed all of warning using PHPCBF COMMAND.
Thanks!!

hetal.solanki’s picture

hetal.solanki
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
vishwenderbaloria’s picture

vishwenderbaloria commented

I tested with MR 53. Found these errors. Please check

FILE: ...ontrib/migrate_tools/src/Drush/Commands/MigrateToolsCommands.php
----------------------------------------------------------------------
FOUND 20 ERRORS AFFECTING 10 LINES
----------------------------------------------------------------------
 104 | ERROR | [x] The first parameter of a multi-line function
     |       |     declaration must be on the line after the opening
     |       |     bracket
 104 | ERROR | [x] Multi-line function declarations must define one
     |       |     parameter per line
 108 | ERROR | [x] Multi-line function declarations must have a
     |       |     trailing comma after the last parameter
 108 | ERROR | [x] The closing parenthesis of a multi-line function
     |       |     declaration must be on a new line
 258 | ERROR | [x] The first parameter of a multi-line function
     |       |     declaration must be on the line after the opening
     |       |     bracket
 258 | ERROR | [x] Multi-line function declarations must define one
     |       |     parameter per line
 263 | ERROR | [x] Multi-line function declarations must have a
     |       |     trailing comma after the last parameter
 263 | ERROR | [x] The closing parenthesis of a multi-line function
     |       |     declaration must be on a new line
 453 | ERROR | [x] The first parameter of a multi-line function
     |       |     declaration must be on the line after the opening
     |       |     bracket
 453 | ERROR | [x] Multi-line function declarations must define one
     |       |     parameter per line
 467 | ERROR | [x] Multi-line function declarations must have a
     |       |     trailing comma after the last parameter
 467 | ERROR | [x] The closing parenthesis of a multi-line function
     |       |     declaration must be on a new line
 551 | ERROR | [x] The first parameter of a multi-line function
     |       |     declaration must be on the line after the opening
     |       |     bracket
 551 | ERROR | [x] Multi-line function declarations must define one
     |       |     parameter per line
 560 | ERROR | [x] Multi-line function declarations must have a
     |       |     trailing comma after the last parameter
 560 | ERROR | [x] The closing parenthesis of a multi-line function
     |       |     declaration must be on a new line
 777 | ERROR | [x] The first parameter of a multi-line function
     |       |     declaration must be on the line after the opening
     |       |     bracket
 777 | ERROR | [x] Multi-line function declarations must define one
     |       |     parameter per line
 781 | ERROR | [x] Multi-line function declarations must have a
     |       |     trailing comma after the last parameter
 781 | ERROR | [x] The closing parenthesis of a multi-line function
     |       |     declaration must be on a new line
----------------------------------------------------------------------
PHPCBF CAN FIX THE 20 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 4.86 secs; Memory: 20MB

heddn made their first commit to this issue’s fork.

jeremy1606 made their first commit to this issue’s fork.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue tags: +PHP 8.4

Thanks! Can you make a new migrate_tools release to get the PHP 8.4 deprecation fix included here out?

jsacksick’s picture

jsacksick commented

+1: would be great to have a new release containing the PHP 8.4 deprecation fix.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

+1 for a new release as well! This is causing our PHPUnit tests to fail on PHP 8.4.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Related issues: +#3411051: Fix PHPCS issues

Cross-linking related issue in migrate_plus: #3508679: PHP 8.4 implicit nullable deprecation

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Related issues: -#3411051: Fix PHPCS issues +#3508679: PHP 8.4 implicit nullable deprecation

Oops linked to this issue by mistake!

mrshowerman’s picture

mrshowerman
Location Munich
commented

This introduced a backwards incompatible change in the MigrateExecutable class constructor.

Now the following

$executable = new MigrateExecutable($migration, new MigrateMessage(), ['limit' => 10]);

fails with this error:

TypeError: Drupal\migrate_tools\MigrateExecutable::__construct(): Argument #3 ($keyValue) must be of type Drupal\Core\KeyValueStore\KeyValueFactoryInterface, array given
mgaskey’s picture

mgaskey commented

As mentioned in #21, these changes included adding parameters to the constructor function. Why would fixing PHPCS issues include such a fundamental change? I added a bug ticket here: https://www.drupal.org/node/3411051

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented
Related issues: +#3536657: New parameters introduced in MigrateExecutable class constructor

Linking #3536657: New parameters introduced in MigrateExecutable class constructor (likely intended link for last comment)