Fix Drupal.Array.Array.[ArrayClosingIndentation, ArrayIndentation] coding standard

Good to go.

Return to RTBC, because all tests are marked as passed.

Since this is a big cleanup, let's schedule this one for during the beta/RC phase so that it's less disruptive. We may need to generate a new patch at that time.

Feb. 7 2018 has passed, but I am not sure about the current target version (8.6.x) as xjm has tagged this is as a 'rc target'. This would indicate the target version is 8.5.x, which is currently in beta.

I'm not sure how this got RTBC-ed! Twice! :)

Looking at the tests results there are many CS failures. Actually the phpcs.xml.dist was not modified correctly.
Altering phpcs.xml.dist by:

<rule ref="Drupal.Array.Array.ArrayClosingIndentation" />

we are targeting all sniffs inside Drupal.Array.Array.

We should only target Drupal.Array.Array.ArrayClosingIndentation by using something like:

  <rule ref="Drupal.Array.Array">
    <!-- Sniff for these errors: ArrayClosingIndentation -->
    <exclude name="Drupal.Array.Array.CommaLastItem" />
    <exclude name="Drupal.Array.Array.ArrayIndentation" />
    <exclude name="Drupal.Array.Array.LongLineDeclaration" />
  </rule>

Looking at the patch it seems to fix all issues under Drupal.Array.Array, while it should only fix Drupal.Array.Array.ArrayClosingIndentation.

1. New patch after #2572613: Fix 'Drupal.Array.Array.CommaLastItem' coding standard was committed.
2. Updated phpcs.xml.dist: removed the exclude for Drupal.Array.Array.ArrayClosingIndentation

The patch fixes the problem and it looks very good, I have some problems with some of the changes but they seem to pass phpcs on the bot (and locally).

1. +++ b/core/lib/Drupal/Core/Installer/Exception/AlreadyInstalledException.php
   @@ -26,7 +26,7 @@ public function __construct(TranslationInterface $string_translation) {
   -    ]);
   +]);
   

   phpcs doesn't complain about this, but this doesn't look right.

2. +++ b/core/modules/comment/src/CommentStorage.php
   @@ -182,7 +182,7 @@ public function getNewCommentPageNumber($total_comments, $new_comments, Fieldabl
   -      ])->fetchField();
   +                        ])->fetchField();
   

   This also looks weird, but it is correct.

3. +++ b/core/modules/file/src/FileViewsData.php
   --- a/core/modules/filter/tests/src/Kernel/FilterKernelTest.php
   +++ b/core/modules/filter/tests/src/Kernel/FilterKernelTest.php
   

   Most of the changes here have the same thing, they are correct but it looks weird.

#12 I agree those changes look off. I'll see if there is a more sane indentation that will still pass phpcs.

The weird thing is that phpcs expects the array to indented with 2 more spaces than the line where the array declaration starts.
So looking at core/lib/Drupal/Core/Installer/Exception/AlreadyInstalledException.php both of these are correct:

</ul>', [
..
]);

  </ul>', [
  ..
  ]);

Weird enough. And things get even more weird, as if we go for the first option, for the array items, Drupal.Array.Array.ArrayIndentation conflicts with Drupal.WhiteSpace.ScopeIndent.Incorrect. There's no way to solve both if the html code has no indentation.

I don't think that adding whitespace for the html code is a good idea, so I would extract the array declaration from the function call and put it above. What do you think?

I see there were similar suggestions already above now that I added some comments, adding +1 to having lots of strange cases here that conflict with other sniffs and common sense :)

1. +++ b/core/lib/Drupal/Core/Installer/Exception/AlreadyInstalledException.php
   @@ -26,7 +26,7 @@ public function __construct(TranslationInterface $string_translation) {
          ':base-url' => $GLOBALS['base_url'],
          ':update-url' => $GLOBALS['base_path'] . 'update.php',
   -    ]);
   +]);
        parent::__construct($message, $title);
      }
   

   Yes, definitely a strange one, +1 to move that string out of the array.

2. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
   @@ -304,7 +304,7 @@ public function doMail($module, $key, $to, $langcode, $params = [], $reply = NUL
                '%from' => $message['from'],
                '%to' => $message['to'],
                '%reply' => $message['reply-to'] ? $message['reply-to'] : $this->t('not set'),
   -          ]);
   +            ]);
              $this->messenger()->addError($this->t('Unable to send email. Contact the site administrator if the problem persists.'));
   

   this too, it's then actually on the same level as the lines above. Which are wrong and should be two spaces in.

   While it might get (a lot) bigger, I'm wondering if it would make sense to combine this with correct indentation *inside* an array. But this alone results in some really weird cases where the sniffer produces false or false-looking results? Or do that part first..

3. +++ b/core/modules/comment/src/CommentStorage.php
   @@ -182,7 +182,7 @@ public function getNewCommentPageNumber($total_comments, $new_comments, Fieldabl
            ':entity_type' => $entity->getEntityTypeId(),
            ':thread' => $first_thread,
   -      ])->fetchField();
   +                        ])->fetchField();
        }
   

   also a good one, the multi-line SQL statement above is confusing it. No, I don't think this is correct, the only reason to format the SQL like that is to have it visually close together, could for example extract the query string into a $query variable or so above?

#12.1 This looks a lot better with the added ArrayIndentation sniff

#12.2, #15.3 Moved the query string to a separate variable, so the code looks a bit cleaner indentation-wise

#12.3 Manually added indentation to the assertions that maintains a passing test, but makes the code more readable and passes the phpcs

#14, #15.1 Moved the replacement placeholder values to a separate values, so the code becomes a bit more readable and passes the phpcs

#15.2 Included the fixes and sniff for ArrayIndentation, since they make a lot more sense combined than separately. On the flipside, this makes the patch significantly larger

Also included an interdiff, but it is somewhat large with the new ArrayIndentation sniff

We usually do only one cs-fix per issue, but since these are so related, I can see the argument to do them in one issue. The testbot found 5 remaining cs failures, let's fix those as well.

This should fix the remaining code sniffer violations.

The testbot agrees with this, I went trough the patch again and I agree with most of what was changed here.

This is the only thing I would change, but I think that might be out of scope. Otherwise this looks good to go for me.

+++ b/core/modules/comment/src/Plugin/migrate/source/d6/Comment.php
@@ -21,9 +21,9 @@ class Comment extends DrupalSqlBase {
       ->fields('c', ['cid', 'pid', 'nid', 'uid', 'subject',
-      'comment', 'hostname', 'timestamp', 'status', 'thread', 'name',
-      'mail', 'homepage', 'format',
-    ]);
+        'comment', 'hostname', 'timestamp', 'status', 'thread', 'name',
+        'mail', 'homepage', 'format',
+      ]);

We're changing these lines and they can be reflowed to something like this:

    $query = $this->select('comments', 'c')
      ->fields('c', ['cid', 'pid', 'nid', 'uid', 'subject', 'comment',
        'hostname', 'timestamp', 'status', 'thread', 'name', 'mail', 'homepage',
        'format',
      ]);

#19 Agreed, considering we are touching those lines anyway, we might as well fill the lines with 80 characters.

Setting status back to 'Needs review' after #2973992: Permission issue in Nightwatch step marks all full testruns as unstable

Patch does not apply anymore.

And we should do this against 9.1.x now, right?

Nah, nobody needs it.
Maybe in 3-5 years...

Rebased MR on 9.3.x.

I applied the patch and then ran phpcs. It returned 3 files with errors,

FILE: /var/www/html/core/modules/tour/src/TourViewBuilder.php
FILE: /var/www/html/core/modules/update/tests/src/Functional/UpdateSemverTestBase.php
FILE: /var/www/html/core/modules/user/tests/src/Functional/UserAdminTest.php

Then I reviewed the patch which was fairly easy since it is all formatting changes. Just two questions.

Thanks @quietone for the review, I'll respond/correct (if needed) to the 2 questions in the MR.

I do have a question for you though:

I applied the patch and then ran phpcs. It returned 3 files with errors,

Where does "the patch" come from, is it simply the diff from the MR perhaps?

If that is indeed the case, I don't understand that diff returning 3 PHPCS errors, since the MR itself doesn't seem to return any errors here:
https://dispatcher.drupalci.org/job/drupal_patches/88074/consoleFull (PHPCS output from 00:03:45.970 Checking core/includes/install.core.inc until 00:04:15.468 core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php passed.

Answered Replied to both questions, hoping this will clear things up?

#35. Yes I applied the diff not a patch. I guess that was muscle memory typing 'patch'. The MR does not generate errors because drupalci runs commit-code-check which only runs phpcs on the files changed in the patch. It does not run phpcs on the entire code base.

he MR does not generate errors because drupalci runs commit-code-check which only runs phpcs on the files changed in the patch. It does not run phpcs on the entire code base.

Thus spoketh @quietone in #37

Right, I always forget that.

I also did a manual run and indeed found out that indeed the three mentioned files produced a warning.
Not sure how they passed my manual check, but luckily you caught them.

Fixed them and they are now in the MR.

(Still haven't got time yet to checkout your replies in the MR, hope to get to those later today)

Thanks @quietone for the (as usual) excellent review and explanations so even I can understand :)

Nice work so far! Lots of comments though, there are many cases where something like

  ]
);

is now

]
);

and that can be wrapped back on to one line otherwise it looks wrong to me.

In general I think that pairs of brackets should match their opening line so if we start with

$this->someMethod([

then it should be closed with

]);

(although this is not always doable in some more complicated cases)

Thanks @longwave for the review.

There's one remaining open thread about opening/not opening a follow-up issue left.

Thanks for working on this, this is quite tedious but it will be nice to get all this properly lined up.

I did another pass at the MR and found a few I must have missed, the one that was left outstanding can just be ignored here I think - you are right that it will take a long time to solve via coding standards.

Thanks @longwave for the review.

Rrrrrrrrrandom Test Fail, retesting.

Testbot is failing.

Hi,
Applied PHPCBF in thefiles and created a patch. Please review my work.

@Anwesha Sinha: Could you be so kind and work on the MR. For more info about MR, please see: https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

@anweshasinha: also in this issue we are only trying to fix the array indentation sniffs, your patch changes a large number of files and seems to cover lots of different coding standards sniffs. We also should not be changing e.g. the core/assets directory as this contains third party code that does not belong to Drupal. It also looks like phpcbf is either configured incorrectly or is just doing something wrong, because changes like this do not look correct at all:

-    `<div class="media-embed-error media-embed-error--preview-error">${Drupal.t(
+    ` < div class = "media-embed-error media-embed-error--preview-error" > ${Drupal.t(
       'An error occurred while trying to preview the media. Please save your work and reload this page.',
-    )}</div>`;
+    )} < / div > `;

Ok, I will find out and try to fix it. Thank you

Let's see what TestBot thinks, but I think we should be back in the green after the latest commit.

Thanks for working on this!

I read through the whole MR again, and don't see any remaining issues - except for one change that appears to be out of scope where a double drupalGet() has been removed. Once that has been resolved I think this is ready for RTBC :)

Thanks @longwave and @daffie for their eagle eyes on this.

- Merged latest 9.3.x into MR
- Resolved all threats threads

Back to NR

All known issues have been fixed and all questions answered, this has been nitpicked over by multiple contributors now, to me this is ready to go.

git apply 515.diff --3way                                                                                                                                                       423ms  Mon  1 Nov 10:45:00 2021
error: patch failed: core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldFormatterSettingsTest.php:95
Falling back to three-way merge...
Applied patch to 'core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldFormatterSettingsTest.php' with conflicts.
error: patch failed: core/modules/filter/tests/src/Kernel/FilterKernelTest.php:332
Falling back to three-way merge...
Applied patch to 'core/modules/filter/tests/src/Kernel/FilterKernelTest.php' with conflicts.
error: core/modules/views/tests/src/Unit/EntityViewsDataTest.php: does not exist in index

Merged 9.3.x and fixed two conflicts, back to RTBC.

Needed a further run of phpcbf and then some final manual cleanup, back to NR for someone to review the last commit.

Looks like that are more fixes to make.

• Rebased MR on 9.4.x
• Merged latest commits in MR
• Made TestBot green

I made one commit to this MR but have extensively reviewed the rest of it a number of times, the bot is happy now so I think I can still mark this RTBC.

Also added a release note snippet.

The 9.4.x version does not apply to 10.0.x. We might want two separate MRs here. Setting 10.0.x so that a new MR would be opened against that by default.

This changeset is large and disruptive enough also that it might be worth scheduling it as a beta target. If it ends up getting bounced around a couple more times then that's what I'd recommend. When the 9.4.x beta phase starts depends on when Drupal 10 requirements are completed, but scheduled beta targets would probably end up in the week of May 23.

I rerolled the diff and made a patch.

138 files changed, 714 insertions(+), 733 deletions(-)

Thanks, confirmed that the PHPCS run passes and that we have the change in phpcs.xml.dist.

Changes in the patch file also look good, "git diff --color-words" also only shows the rare cases where we move code around a bit.

Looks good to me!

Can we get a patch for 9.5.x as well? Then we can keep 9.5.x and 10.x aligned. We should land this patch during the beta phase.

Yes, of course. Here is a 9.5.x patch and a diff between that and the 10.0 version.

This also needs a patch for 10.1.x which has an interdiff between 10.0 and 10.1. Needed because of #3215062: Update hook_schema for Y2038.

Tried this, patches need a reroll unfortunately. :(

Lets be a little more specific.

9.5.x

Patch applies. When looking through the code, (and running phpcs again) it seems this PHP file is broken:

core/modules/rdf/tests/src/Functional/StandardProfileTest.php

    $expected_value = [
      'type' => 'literal',
      // There is an extra carriage return in the value when parsing comments as
      // output by Bartik, so it must be added to the expected value.
      'value' => "$text . PHP_EOL;
      'lang' => 'en',
    ];

After running phpcs in core i get this feedback:

FILE: /home/localcopy/drupal-core-9-5-x/web/core/modules/ckeditor5/tests/src/Functional/Update/CKEditor5UpdateImageToolbarItemTest.php
--------------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------------------------------------------------------------
 165 | ERROR | [x] Array indentation error, expected 12 spaces but found 14
 166 | ERROR | [x] Array indentation error, expected 12 spaces but found 14
--------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------------------------------------------


FILE: /home/localcopy/drupal-core-9-5-x/web/core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php
-----------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------------------------
 98 | ERROR | [x] Array indentation error, expected 6 spaces but found 8
 99 | ERROR | [x] Array indentation error, expected 6 spaces but found 8
-----------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------


FILE: /home/localcopy/drupal-core-9-5-x/web/core/modules/hal/tests/src/Functional/taxonomy/TermHalJsonAnonTest.php
------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------
 73 | ERROR | [x] Array indentation error, expected 12 spaces but found 10
------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------


FILE: /home/localcopy/drupal-core-9-5-x/web/core/modules/aggregator/tests/src/Kernel/AggregatorTitleTest.php
------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------
 66 | ERROR | [x] Array closing indentation error, expected 4 spaces but found 6
------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------


FILE: /home/localcopy/drupal-core-9-5-x/web/core/modules/aggregator/src/Plugin/Field/FieldFormatter/AggregatorTitleFormatter.php
--------------------------------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
--------------------------------------------------------------------------------------------------------------------------------
 66 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
 67 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
 68 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
--------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------------------------------------

Time: 14.07 secs; Memory: 12MB

10.0.x

Doesn't apply. Missing files, also when i force through phpstorm, there are conflicts in MediaLibraryTest

vagrant@bjorn-pc:/home/localcopy/drupal-core-10-0-x/web (10.0.x)$ git apply 2937515-63.patch
error: core/modules/ckeditor/tests/src/FunctionalJavascript/MediaLibraryTest.php: No such file or directory
error: core/modules/ckeditor/tests/src/Unit/CKEditorPluginManagerTest.php: No such file or directory
error: core/modules/rdf/tests/src/Functional/StandardProfileTest.php: No such file or directory

10.1.x

Same as for 10.0.x

Thanks for the review.

Here is the 9.5 patch.

And now the patches for D10.

I scanned through the patches and all the changes look correct to me. Some of the interdiffs don't seem to make sense but when I check the related places in the patches they look good. Assuming the test runs agree, this is RTBC.

Committed 803da73 and pushed to 10.1.x. Thanks!
Committed ea27211 and pushed to 10.0.x. Thanks!
Committed d3d2031 and pushed to 9.5.x. Thanks!

#2902636: Fix Drupal.Array.Array.ArrayIndentation coding standard was closed as a duplicate, moving credit to here.