Fix the issues reported by phpcs

The changes are fine. Maintainers should check if it is good to go or not.

The issue summary should always describe what the issue is trying to fix and, in the case, of coding standards issues, show which command has been used, which arguments have been used, and which report that command shown.

Updated the issue summary with required details.

The arguments passed to phpcs do not include the standards to use. If I run phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md ./ on the directory containing a clone of the 7.x-1.x branch, I get a longer report.
It still to see which reports still apply to Drupal 7, for which the minimum PHP version required to run it is lower than PHP 7.

I am using PHP 8.0 and i have done the fix for i18n_redirect.module file particularly. Do i need to fix i18n_redirect.info file phpcs fix also in the same issue?

FILE: /Users/vimalnadar/www/Travelopia/domino-leboat/docroot/profiles/leboat/modules/contrib/i18n/i18n_redirect/i18n_redirect.info
----------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 3 WARNINGS AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------------------------------
1 | WARNING | Remove "project" from the info file, it will be added by drupal.org packaging automatically
1 | WARNING | Remove "datestamp" from the info file, it will be added by drupal.org packaging automatically
1 | WARNING | Remove "version" from the info file, it will be added by drupal.org packaging automatically
1 | ERROR | Duplicate entry for "core" in info file
----------------------------------------------------------------------------------------------------------------------------------

committed patch #2

@joseph.olstad do you need me to reroll the patch or its committed? So we can update the status of this issue?

patch #2 is committed, there appears to be more code sniffer warnings/notices , I look the ones that were smaller and easier to understand.

Fixed some errors reported by phpcs.

Hi,

MR #15 applied successfully.

Admin@DESKTOP-252TO6V MINGW64 ~/Desktop/projects/drupal/web/modules/i18n (7.x-1.x)
$ curl https://www.drupal.org/files/issues/2024-02-27/3355258-15.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  354k  100  354k    0     0  2374k      0 --:--:-- --:--:-- --:--:-- 2394k
patching file README.md
patching file i18n.api.php
patching file i18n.install
patching file i18n.module
patching file i18n.pages.inc
patching file i18n.test
patching file i18n.variable.inc
patching file i18n_block/i18n_block.i18n.inc
patching file i18n_block/i18n_block.inc
patching file i18n_block/i18n_block.info
patching file i18n_block/i18n_block.install
patching file i18n_block/i18n_block.module
patching file i18n_block/i18n_block.test
patching file i18n_field/i18n_field.api.php
patching file i18n_field/i18n_field.i18n.inc
patching file i18n_field/i18n_field.inc
patching file i18n_field/i18n_field.info
patching file i18n_field/i18n_field.install
patching file i18n_field/i18n_field.module
patching file i18n_field/i18n_field.pages.inc
patching file i18n_field/i18n_field.test
patching file i18n_forum/i18n_forum.install
patching file i18n_forum/i18n_forum.module
patching file i18n_forum/i18n_forum.test
patching file i18n_menu/i18n_menu.admin.inc
patching file i18n_menu/i18n_menu.i18n.inc
patching file i18n_menu/i18n_menu.inc
patching file i18n_menu/i18n_menu.install
patching file i18n_menu/i18n_menu.module
patching file i18n_menu/i18n_menu.test
patching file i18n_node/i18n_node.features.inc
patching file i18n_node/i18n_node.i18n.inc
patching file i18n_node/i18n_node.info
patching file i18n_node/i18n_node.install
patching file i18n_node/i18n_node.module
patching file i18n_node/i18n_node.pages.inc
patching file i18n_node/i18n_node.test
patching file i18n_node/i18n_node.variable.inc
patching file i18n_object.inc
patching file i18n_path/README.txt
patching file i18n_path/i18n_path.admin.inc
patching file i18n_path/i18n_path.inc
patching file i18n_path/i18n_path.info
patching file i18n_path/i18n_path.module
patching file i18n_path/i18n_path.test
patching file i18n_select/i18n_select.admin.inc
patching file i18n_select/i18n_select.module
patching file i18n_select/i18n_select.test
patching file i18n_select/i18n_select.variable.inc
patching file i18n_string/i18n_string.admin.inc
patching file i18n_string/i18n_string.api.php
patching file i18n_string/i18n_string.i18n.inc
patching file i18n_string/i18n_string.inc
patching file i18n_string/i18n_string.install
patching file i18n_string/i18n_string.module
patching file i18n_string/i18n_string.pages.inc
patching file i18n_string/i18n_string.test
patching file i18n_string/i18n_string.variable.inc
patching file i18n_sync/README.txt
patching file i18n_sync/i18n_sync.api.php
patching file i18n_sync/i18n_sync.features.inc
patching file i18n_sync/i18n_sync.info
patching file i18n_sync/i18n_sync.install
patching file i18n_sync/i18n_sync.module
patching file i18n_sync/i18n_sync.modules.inc
patching file i18n_sync/i18n_sync.node.inc
patching file i18n_sync/i18n_sync.test
patching file i18n_sync/i18n_sync.variable.inc
patching file i18n_taxonomy/i18n_taxonomy.admin.inc
patching file i18n_taxonomy/i18n_taxonomy.i18n.inc
patching file i18n_taxonomy/i18n_taxonomy.inc
patching file i18n_taxonomy/i18n_taxonomy.install
patching file i18n_taxonomy/i18n_taxonomy.module
patching file i18n_taxonomy/i18n_taxonomy.pages.inc
patching file i18n_taxonomy/i18n_taxonomy.test
patching file i18n_taxonomy/i18n_taxonomy.tokens.inc
patching file i18n_taxonomy/i18n_taxonomy.views.inc
patching file i18n_translation/README.txt
patching file i18n_translation/i18n_translation.admin.inc
patching file i18n_translation/i18n_translation.api.php
patching file i18n_translation/i18n_translation.inc
patching file i18n_translation/i18n_translation.install
patching file i18n_translation/i18n_translation.module
patching file i18n_translation/i18n_translation.pages.inc
patching file i18n_user/i18n_user.install
patching file i18n_user/i18n_user.module
patching file i18n_variable/i18n_variable.class.inc
patching file i18n_variable/i18n_variable.install
patching file i18n_variable/i18n_variable.module
patching file i18n_variable/i18n_variable.test
patching file i18n_variable/i18n_variable.variable.inc
patching file tests/i18n_test.module

Still there are lot of issues remaining.

The gitlab sniffer is more strict.

Someone will have to re-run the phpcbf against the merge request using the Drupal code standards that should be defined in a phpcs.xml

There are warnings/errors I would rather avoid to fix, such as the ones complaining about 18n_object_wrapper::get_title(). It is not possible to fix them in a backward-compatible way, since that would mean keeping that method, which also means PHP_CodeSniffer would complain again about the method/class name because underscores are used.

I am going to check if it is possible to global disable some PHP_CodeSniffer rules using some GitLab CI variables.

Then, the Use null coalesce operator instead of ternary operator rule should be disabled too, as Drupal 7 is though to run also on PHP versions that do not have the null coalesce operator.

I moved the original report to an attached file.

Given the long list of warnings/issues, would not be better to merge this merge request, and open other issues to fix the remaining warnings/errors, for example opening an issue for each sub module?

This merge request adds a phpcs.xml.dist file that limits the warnings/errors to the ones that are worth fixing, so it should be merged to restrict the number of warnings/errors reported for all the modules part of this project.

Strange, the pipeline isn't picking up the recent fixes. I'm going to merge this anyway and see what happens.

Lots of high risk changes being made here, tests are failing.

list removed
?? introduced.

similar changes.

Even if we get to pass test coverage I'm not comfortable with some of these changes being made.

Quite annoyed here, way too much happening all at once. If someone resolves all of the issues that I identified I will review.

The commits I have done are the following.

• 19c9b319
• 1c353b0c
• 4be11bfd
• ada55c68

None of them changes the code to use the null coalescing operator.

Also, the commits I have done do not fix all the warnings/errors. As I have already said, there is too much to fix for a single person in a single issue.

To minimize the warnings/errors reported by PHP_CodeSniffer, it would be necessary to change the documentation comment of inherited methods to only contains {@inheritdoc}. In that way, PHP_CodeSniffer would not complain about missing parameter or return value descriptions so much times.

@apadermo, thanks for making the first round of fixes. It's unfortunate that someone created such a huge mess. It would have been a lot easier to do this submodule by submodule however if you or anyone else is up for some more masochistic fun I'll review the rest.

It could also be they are automatic changes. There are not track of recent commits doing those changes, so they are either previous changes or automatic changes.

I replaced any usage of the null coalescing operator. Let us see what happens now.

I have two nits about MR #7

First one, there's two new code patterns being introduced that will break backwards compatibility with php <= 7.4. Not sure which version exactly but around there.

Then there's some changes that are absolutely useless here:

The null coalescing operator should not be used, except in the case the minimum PHP version required by the module is increased, but this would be a change that is not backward-compatible. I reverted any change that introduced the usage of that operator, but I could have missed some instances.

I did not miss those changes: They are introduced from recent commits in the MR, after I reverted them.

I am keeping this issue assigned to myself, to avoid somebody re-introduces changes we already reverted.

May we agree on which warnings/errors we want to fix? It is not possible to apply Drupal coding standards that are thought for Drupal 8+ to a Drupal 7 module; furthermore, there are changes that comes too late.

For example, a documentation comment that describes the content of a file where one or more classes is not necessary. Do we want to remove them even if they do not contain any @file tag? I would remove them, at least because they do not add any value to the code and often they are not correct.

I keep checking the code for other wrong changes introduced in the past commits. (That's why the issue is still assigned to me.)

Thanks for this @apadermo, a lot of work here.

I am a bit confused with the gitlab pipeline setup we have, how do I run php 5.3, php 5.4, php 5.5, php 5.6, php 7.0, php 7.1 , php 7.2, php 7.3, php 7.4, php 8.0, php 8.1, php 8.2 and php 8.3 pipelines?
Basically I want to run every single pipeline option on this merge request.
?

I checked the GitLab CI documentation on drupal.org, but I did not find any page describing how to test the code in all the possible PHP versions. I found how to set the PHP version to use; maybe it is possible to set a matrix with different PHP versions, but I could not say how.

Reading Scheduling pipelines → , I gather it would be possible to set a different pipeline for each PHP version, which is written in the _TARGET_PHP variable the pipeline reads.