#Coding standards

Hi

I applied the MR !4 and I confirmed that it fixes the PHPCS issues in the module,

➜  disable_web_install git:(main) ✗ curl https://git.drupalcode.org/project/disable_web_install/-/merge_requests/4.diff | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   574    0   574    0     0   1077      0 --:--:-- --:--:-- --:--:--  1099
patching file disable_web_install.module
➜  disable_web_install git:(main) ✗ ..
➜  contrib git:(main) ✗ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml disable_web_install  

FILE: ...local/web/modules/contrib/disable_web_install/disable_web_install.info.yml
--------------------------------------------------------------------------------
FOUND 0 ERRORS 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
--------------------------------------------------------------------------------

Time: 123ms; Memory: 10MB

➜  contrib git:(main) ✗ 

The warning that my terminal reported occurs when a module is installed via composer.

RTBC +1 from me

I have tried to apply the MR 4 in Drpal 10.2.x but MR failed to apply and i got the below error

$ git apply -v 4.diff
4.diff:11: trailing whitespace.
  daterangepicker.min.js, daterangepicker.css" Library.
Checking patch README.md...
Checking patch css/daterangepicker.css...
Checking patch daterange_picker.info.yml...
error: while searching for:
description: 'Provides the ability to select from and to-date.'
package: daterange_picker
core_version_requirement: ^9 || ^10


error: patch failed: daterange_picker.info.yml:3
error: daterange_picker.info.yml: patch does not apply
Checking patch daterange_picker.libraries.yml...
Checking patch daterange_picker.module...
Checking patch src/Plugin/Field/FieldFormatter/DateRangePickerFormatter.php...
Checking patch src/Plugin/Field/FieldType/DateRangePickerType.php...
Checking patch src/Plugin/Field/FieldWidget/DateRangePicker.php...

Thanks!

Perhaps the link to Gitlab under "Resources" can be updated? Currently it points to the old branch https://git.drupalcode.org/project/leaflet_more_maps/-/blob/8.x-1.x/READ...

Instead the link text could be "Documentation" and the link point to https://git.drupalcode.org/project/leaflet_more_maps -- this way, the latest version of the README will always be shown.

Hi, I’ve reviwed MR 36 on Drupal 10.2.x.
The MR applied cleanly.

Few phpcs issues fixed but I still can see the below issues

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

FILE: /Users/apj/projects/doc102/drupal/modules/contrib/config_pages/config_pages.info.yml
-------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS 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
-------------------------------------------------------------------------------------------------------------


FILE: /Users/apj/projects/doc102/drupal/modules/contrib/config_pages/src/Plugin/Condition/ConfigPagesValueAccess.php
----------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------------------------------
 217 | WARNING | Unused variable $cp_type.
 217 | WARNING | Unused variable $data_type.
----------------------------------------------------------------------------------------------------------------------------------

Changing the status back to "Needs work".

Thanks

I propose using the following syntax, and explicitly encouraging (or even requiring) [] over list():

[$country, $state, $city] = $this->getPlaceInfo();
[$country] = $this->getPlaceInfo();
[, , $city] = $this->getPlaceInfo();
['city' => $city] = $this->getPlaceInfoAssociative();

This is wrong as it doesn't call the scheduler settings parent, it also must be done as a merge request now.

Made lots of followups and committed the suggestions made in the MR. Setting back to NR.

(Let's include the .md files.)

apaderno → changed the visibility of the branch 3349806-fix-php_codesniffer-warnings_errors to active.

apaderno → changed the visibility of the branch 3349806-fix-php_codesniffer-warnings_errors to hidden.

apaderno → closed merge request !11

apaderno → changed the visibility of the branch 3349806-fix-phpcs-warnings-errors to hidden.

Hi @apaderno, Should we check and create a new merge request against the 8.x-1.x branch?

Yeah I think it's implicit in the existing coding standard but we should make it explicit.

Please review the patch.

General consensus is not to do this, we have had another month of time to see other opinions. Will close this as won't fix.

As discussed in the coding standard meeting of 22th of may, since there is not more activity we are closing this for now.

Discussed at #3456119: Coding Standards Meeting Tuesday 2024-06-18 2100 UTC → and there was agreement that this is something to pursue. I have updated the IS with the new template.

This was discussed at #3456119: Coding Standards Meeting Tuesday 2024-06-18 2100 UTC → . We are considering adding something for this but have not settled on any details or where this information would go. So far adding something like, "All regexes with more than one group must have a comment' has been suggested. But that does cover how to make a long regex multi-line. Perhaps just adding an example it the best option here.

I have added the new issue summary template.

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

This was discussed briefly at the CS meeting, #3456119: Coding Standards Meeting Tuesday 2024-06-18 2100 UTC → with myself and Björn Brala (bbrala). We only got as far as acknowledging that this is complex and will need further discussion.

This was discussed at the Coding Standards meeting #3456119: Coding Standards Meeting Tuesday 2024-06-18 2100 UTC → by myself, mstrelan, Björn Brala (bbrala) and borisson_.

There was concern about keeping around references to old code. And there was also acknowledgement that we have improved tools for tracking renaming and/or deprecating things so that it seems unnecessary to add '@was'. We concluded that with deprecation notices, change records and static analysis this is not needed. And further, the tools to help here continue to evolve.

Therefor, closing as won't fix.

silvi.addweb → made their first commit to this issue’s fork.

This was discussed at coding standards meetings, #3456119: Coding Standards Meeting Tuesday 2024-06-18 2100 UTC → .

The existing text does cover this case but there was a question if the text should make clear that 'operator' does not mean any combination of operators.

And a sniff will be needed to ensure the agreed format is enforced.

My one comment/concern has been answered. Believe this is good

Thanks for the patch & review. Fixed now.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Resolve Merge conflict and update some comments. Ready for review.

During review please take a closer look at order of @hook, @throws, @annotation in src/Commands/PathautoCommands.php not sure, that this is correct order, welcome for suggestion.

And I don't know how to fix "Missing short description in doc comment" at pathauto.api.php:10. This is last one from Drupal.Commenting in code base for now,

CS Report updated in the IS.

@akshaydalvi212 thanks for your contribution, this issue is only about to fix Drupal.Commenting violations.
I will revert commit b043409c, because it contains changes not related to these cs rules.
And I will resolve merge conflict.

Hi, I have fix below remaining phpcs issue, Please review.

modules/custom/datarange_picker/src/Plugin/Field/FieldFormatter/DateRangePickerFormatter.php
----------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------------------------------------------------------------------
 6 | ERROR | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\Core\Field\FieldItemListInterface.
----------------------------------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 97ms; Memory: 12MB

silvi.addweb → made their first commit to this issue’s fork.

Leaving this issue unassigned, as mentioned in the Issue etiquette →
It says we should avoid assigning tickets to ourselves unless you're a maintainer.

Thanks!

(This happens because the new branch has been created after this issue was created.)

For the 2.x branch is necessary to create a new issue, since in this issue is not possible to create an issue fork for that branch.

silvi.addweb → made their first commit to this issue’s fork.

Thanks a lot @atul_ghate, the patch seems ok. However, I would like the patch to be ported to the new version 2.x. Also, maybe we could rename the README.txt to README.md in the sub-module leaflet_maptiler_token as we did for the main README of the project (see https://www.drupal.org/project/leaflet_maptiler/issues/3339970 📌 Replace README.txt with README.md Fixed ).
For now I will keep this as Needs work.

Hi @bernardopaulino i have resolved conflict please review.

silvi.addweb → made their first commit to this issue’s fork.

silvi.addweb → made their first commit to this issue’s fork.

Jeya sundhar → made their first commit to this issue’s fork.

In a directory containing Drupal 11 files, grep -irn "=&" ./ --include=*.module --include=*.php --include=*.inc --include=*.install --include=*.theme| wc -l returned 93, while grep -irn "=&" ./ --include=*.module --include=*.php --include=*.inc --include=*.install --include=*.theme| wc -l returned 328.

Truly, the first command also count lines like the following ones.

./core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php:1357:          'To maintain the capabilities of this text for
mat, <a target="_blank" href="/admin/help/ckeditor5#migration-settings">the CKEditor 5 migration</a> did the following: Enabled thes
e plugins: (<em class="placeholder">Link, Block quote, Code, List</em>). Added these tags/attributes to the Source Editing Plugin\'s
 <a target="_blank" href="/admin/help/ckeditor5#source-editing">Manually editable HTML tags</a> setting: &lt;cite&gt; &lt;dl&gt; &lt;dt&gt; &lt;dd&gt; &lt;a hreflang&gt; &lt;blockquote cite&gt; &lt;ul type&gt; &lt;ol type=&quot;1 A I&quot;&gt; &lt;h2 id=&quot;jump-*&quot;&gt; &lt;h3 id&gt; &lt;h4 id&gt; &lt;h5 id&gt; &lt;h6 id&gt;. Additional details are available in your logs.',
./core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php:1401:          'As part of migrating to CKEditor 5, it was found that the <em class="placeholder">A CKEditor 4 configured to have span styles</em> text format\'s HTML filters includes plugins that support the following tags, but not some of their attributes. To ensure these attributes remain supported, the following were added to the Source Editing plugin\'s <em>Manually editable HTML tags</em>: &lt;span class=&quot;llama&quot;&gt;. The text format must be saved to make these changes active.',
./core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php:1406:          'To maintain the capabilities of this text format, <a target="_blank" href="/admin/help/ckeditor5#migration-settings">the CKEditor 5 migration</a> did the following:  Added these tags/attributes to the Source Editing Plugin\'s <a target="_blank" href="/admin/help/ckeditor5#source-editing">Manually editable HTML tags</a> setting: &lt;span class=&quot;llama&quot;&gt;. Additional details are available in your logs.',
./core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php:553:          'The following tag is not valid HTML: <em class="placeholder">&lt;blockquote class=&quot;&quot;&gt;</em>.',

silvi.addweb → made their first commit to this issue’s fork.

@atul_ghate thanks for the new merge request. However, there are conflicts with the latest changes in the module. I am sending this back to rework.

@silvi.addweb I've committed fixes, but forgot to mark issue as fixed ;)

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

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

atul_ghate → made their first commit to this issue’s fork.

The patch #2 can no longer be applied. Some of the fixes in this patch have already been committed (see https://git.drupalcode.org/project/leaflet_maptiler/-/commit/5a6c62caf94...). We might need to re-run phpcs to check if there are still errors.

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

Thanks for the help these issues has been resolved in new alpha release, if you still see these errors please create a new issue against 3.x branch.

Yes, vincentlanglet/twig-cs-fixer is a tool with more adoption, more maintained and is more aligned with drupal releases in aspects like phpstan and following symfony releases.

I don't see the failure in the current PR but anyway it needs a rebase.

Hello,

I've tested this problem, and I don't see any issues or warnings without the patch.

xjm → closed merge request !8433

Since 10.3.x is in RC, it is not eligible to have new rules enabled. So, instead of testing a 10.3.x backport, I backported the 10.4.x changes only for parity, with the following commands:

[ayrton:maintainer | Mon 15:24:12] $ git cherry-pick -x 10.4.x
Auto-merging core/modules/responsive_image/responsive_image.module
[10.3.x e64d867c62] Issue #2874067 by kkalashnikov, Nikolay Shapovalov, quietone, b_sharpe, smustgrave, xjm, benjifisher, MerryHamster, dww: Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard
 Date: Mon Jun 17 15:22:41 2024 -0500
 107 files changed, 579 insertions(+), 578 deletions(-)
[ayrton:maintainer | Mon 15:24:20] $ git checkout HEAD^ --  core/phpcs.xml.dist  
[ayrton:maintainer | Mon 15:27:29] $ git commit --amend -m 'Issue #2874067 by kkalashnikov, Nikolay Shapovalov, quietone, b_sharpe, smustgrave, xjm, benjifisher, MerryHamster, dww: Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard'

Thanks everyone!

Thanks @b_sharpe, nice work!

10.4 rebase seems fine.

Fixed up 10.4 backport, passing phpcs now.

Good thing I tested that backport before pushing it; looks like the 10.4.x backport will need some additions. :D

--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 258 | ERROR | Long array syntax must not be used in doc comment code
     |       | annotations
     |       | (Drupal.Commenting.DocCommentLongArraySyntax.DocLongArray)
--------------------------------------------------------------------------------
FILE: ...ds/project/drupal/core/modules/tour/tests/src/Functional/TourTestBasic.php
--------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
--------------------------------------------------------------------------------
 19 | ERROR | Long array syntax must not be used in doc comment code
    |       | annotations
    |       | (Drupal.Commenting.DocCommentLongArraySyntax.DocLongArray)
 20 | ERROR | Long array syntax must not be used in doc comment code
    |       | annotations
    |       | (Drupal.Commenting.DocCommentLongArraySyntax.DocLongArray)
 21 | ERROR | Long array syntax must not be used in doc comment code
    |       | annotations
    |       | (Drupal.Commenting.DocCommentLongArraySyntax.DocLongArray)
 22 | ERROR | Long array syntax must not be used in doc comment code
    |       | annotations
    |       | (Drupal.Commenting.DocCommentLongArraySyntax.DocLongArray)
--------------------------------------------------------------------------------
FILE: ...lds/project/drupal/core/modules/tour/tests/src/Functional/TourTestBase.php
--------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
--------------------------------------------------------------------------------
 23 | ERROR | Long array syntax must not be used in doc comment code
    |       | annotations
    |       | (Drupal.Commenting.DocCommentLongArraySyntax.DocLongArray)
 24 | ERROR | Long array syntax must not be used in doc comment code
    |       | annotations
    |       | (Drupal.Commenting.DocCommentLongArraySyntax.DocLongArray)
 25 | ERROR | Long array syntax must not be used in doc comment code
    |       | annotations
    |       | (Drupal.Commenting.DocCommentLongArraySyntax.DocLongArray)
 26 | ERROR | Long array syntax must not be used in doc comment code
    |       | annotations
    |       | (Drupal.Commenting.DocCommentLongArraySyntax.DocLongArray)
--------------------------------------------------------------------------------
Time: 10.49 secs; Memory: 8MB
PHP CODE SNIFFER REPORT SUMMARY
--------------------------------------------------------------------------------
FILE                                                            ERRORS  WARNINGS
--------------------------------------------------------------------------------
/builds/project/drupal/core/includes/install.inc                1       0
...pal/core/modules/tour/tests/src/Functional/TourTestBase.php  4       0
...al/core/modules/tour/tests/src/Functional/TourTestBasic.php  4       0
--------------------------------------------------------------------------------
A TOTAL OF 9 ERRORS AND 0 WARNINGS WERE FOUND IN 3 FILES
--------------------------------------------------------------------------------

xjm → closed merge request !5441

Committed to 11.x and 11.0.x.

It cherry-picked cleanly to 10.4.x with some auto-merging, but I'm going to open a quick 10.4.x MR here based on that cherry-pick just to ensure there aren't additional instances in D10 that would cause the rule to fail there.

No worries, I prefer to just land the issue in this case. :D

Oops did that before I got the novice email.

Similarly to 📌 Convert code blocks in comments using array() syntax to @code/@endcode Needs review , we're missing an equivalent PHPCS rule to the one from 📌 Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard Needs work for these. We need a followup for that in the appropriate queue.

I'd be fine to treat the rule as either blocker or followup since the scope is really small, but we do need the rule.

📌 Update use of arguments used in comments Active opened, already tagged for title update cuz that one is bad.

So, while these are all totally correct changes, we're missing an equivalent PHPCS rule to the one from 📌 Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard Needs work for these. We need a followup for that in the appropriate queue.

I'd be fine to treat the rule as either blocker or followup since the scope is really small, but we do need the rule.

Followups were already created.

Tagging "Novice" for the task described in #53.

There is a hunk in core/modules/views/src/Plugin/views/field/FieldPluginBase.php that has out-of-scope changes. It could either be a bad merge, or someone correcting the documentation while fixing this bug.

If it's the latter, we should make a followup issue to correct the keys; regardless, the hunk should only include the changes to array().

Going to do a local review of this with git diff --color-words; if someone wants to fix the hunk and file a followup if appropriate, we could get this in still as a beta phase(-ish) cleanup.

Saving issue credits.