Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

Adding credit for the patch author from the parent issue.

xjm’s picture

Status: Active » Needs review
FileSize
72.43 KB

Here's the relevant changes from the previous patch, rerolled against 9.1.x HEAD which has also made all the $modules properties protected (again).

xjm’s picture

And the 8.9.x version.

xjm’s picture

FWIW, I also reviewed #4 locally with

git diff --word-diff-regex=[^[:space:]] 9.1.x

to verify that the only non-whitespace changes were the added trailing commas.

xjm’s picture

Issue tags: +beta target, +rc target

This is also one of the sorts of patches we do as beta targets since they go stale quickly, so tagging for that. It's an RC-eligible piece of that work, although the rule itself (once enabled) will need to wait for 9.1.x probably.But we can and should clean up the backport branches first and then add the rule to 9.1 when ready.

longwave’s picture

Status: Needs review » Needs work

I checked both patches with the diff command in #6 and also crafted a regex to look for other similar lines that are longer than 80 characters:

grep -Er 'public static \$modules = .{53,}' core

The 9.0.x/9.1.x patch is fine but the 8.9.x patch needs work as there are 184 lines that match the above regex.

jungle’s picture

Assigned: Unassigned » jungle
jungle’s picture

Assigned: jungle » Unassigned
Status: Needs work » Needs review
FileSize
3.84 KB
61.53 KB
133.34 KB
133.38 KB

Two patches, one for 8.8.x, the other for 8.9.x

xjm’s picture

Hmm are we sure the same changes as in #10's interdiff aren't also needed for the D9 patch? I inspected a couple and the lines seemed too long in D9 as well.

Several of the things adjusted in the interdiff in #10 were actually about exactly 80 lines long. For example, this line in 8.9:

+++ b/core/modules/aggregator/tests/src/Kernel/AggregatorTitleTest.php
@@ -18,7 +18,13 @@ class AggregatorTitleTest extends KernelTestBase {
-  public static $modules = ['file', 'field', 'options', 'aggregator', 'system'];
+  public static $modules = [
+    'file',
+    'field',
+    'options',
+    'aggregator',
+    'system',
+  ];

It's exactly 80 characters long, so technically would not need to be rewrapped. However, following the visbility being changed from public to protected in 9.0.x, the line is several characters over 80 in #4. I'm betting there numerous things that are missing from the author's original change set in #3116859: [meta] Fix Drupal.Array.Array.LongLineDeclaration coding standard because the lines were between 76-80 characters before public was changed to protected.

So, I'd suggest we rewrap those same arrays in both branches if the test still exists in both.

Cheers,
Jess

jungle’s picture

Assigned: Unassigned » jungle
$ grep -Er 'protected static \$modules = .{50,}' core
$ grep -Er 'public static \$modules = .{53,}' core

Slightly, changing the regex in #8 to find more. In fact, we should check both the protected ones and the public ones.

jungle’s picture

A patch made with 9.1.x

Regex public static \$modules = .{53,} found none, BTW.

jungle’s picture

A patch for 8.9.x.

jungle’s picture

A patch for 8.8.x

jungle’s picture

Assigned: jungle » Unassigned
xjm’s picture

Status: Needs review » Needs work

Nice, lots more complete.

From #13:

+++ b/core/modules/aggregator/tests/src/Kernel/AggregatorTitleTest.php
@@ -18,7 +18,13 @@ class AggregatorTitleTest extends KernelTestBase {
+    'system',
+  ];

+++ b/core/modules/block/tests/src/Functional/BlockUiTest.php
@@ -19,7 +19,12 @@ class BlockUiTest extends BrowserTestBase {
+    'condition_test',
+    ];

The indenting of the closing bracket is wrong in the second hunk -- compare to the first hunk where it's properly outdented to the parent level. I think all the newly added hunks need that fix.

longwave’s picture

longwave’s picture

longwave’s picture

These three patches should fix all the misaligned closing brackets.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the patches for 9.1, 9.0, 8.9 and 8.8. Those are the ones from comment #18, #19 and #20.
For the 9.x patch (from comment #18):
- before installing the patch I checked that the command: grep -Er 'protected static \$modules = .{50,}' core returned a lot of results.
- after installing the patch I ran the same command, only there where no results.
- all the code changes in the patch look good.
- all code is correctly indented.
- the patch is for me RTBC.

For the 8.9 patch (from comment #19):
- before installing the patch I checked that the command: grep -Er 'public static \$modules = .{53,}' core returned a lot of results.
- after installing the patch I ran the same command, only there where no results.
- all the code changes in the patch look good.
- all code is correctly indented.
- the patch is for me RTBC.

For the 8.8 patch (from comment #20):
- before installing the patch I checked that the command: grep -Er 'public static \$modules = .{53,}' core returned a lot of results.
- after installing the patch I ran the same command, only there where no results.
- all the code changes in the patch look good.
- all code is correctly indented.
- the patch is for me RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 3f87e0d805 to 9.1.x and d6b6982ae5 to 9.0.x. Thanks!
Committed 815a3b2 and pushed to 8.9.x. Thanks!
Committed 2b16602 and pushed to 8.8.x. Thanks!

Backported all the way because these are test-only changes and keeping everything aligned helps backport bugs.

  • alexpott committed 3f87e0d on 9.1.x
    Issue #3132745 by jungle, longwave, xjm, swatichouhan012, daffie: Fix...

  • alexpott committed d6b6982 on 9.0.x
    Issue #3132745 by jungle, longwave, xjm, swatichouhan012, daffie: Fix...

  • alexpott committed 815a3b2 on 8.9.x
    Issue #3132745 by jungle, longwave, xjm, swatichouhan012, daffie: Fix...

  • alexpott committed 2b16602 on 8.8.x
    Issue #3132745 by jungle, longwave, xjm, swatichouhan012, daffie: Fix...

Status: Fixed » Closed (fixed)

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