Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|---|---|---|
#20 | 3132745-8.8.x-20.patch | 132.88 KB | longwave |
#19 | 3132745-8.9.x-19.patch | 132.93 KB | longwave |
#18 | 3132745-9.1.x-18.patch | 149.73 KB | longwave |
Comments
Comment #3
xjmAdding credit for the patch author from the parent issue.
Comment #4
xjmHere's the relevant changes from the previous patch, rerolled against 9.1.x HEAD which has also made all the
$modules
properties protected (again).Comment #5
xjmAnd the 8.9.x version.
Comment #6
xjmFWIW, I also reviewed #4 locally with
to verify that the only non-whitespace changes were the added trailing commas.
Comment #7
xjmThis 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.
Comment #8
longwaveI 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.
Comment #9
jungleComment #10
jungleTwo patches, one for 8.8.x, the other for 8.9.x
Comment #11
xjmHmm 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:
It's exactly 80 characters long, so technically would not need to be rewrapped. However, following the visbility being changed from
public
toprotected
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 beforepublic
was changed toprotected
.So, I'd suggest we rewrap those same arrays in both branches if the test still exists in both.
Cheers,
Jess
Comment #12
jungleSlightly, changing the regex in #8 to find more. In fact, we should check both the protected ones and the public ones.
Comment #13
jungleA patch made with 9.1.x
Regex
public static \$modules = .{53,}
found none, BTW.Comment #14
jungleA patch for 8.9.x.
Comment #15
jungleA patch for 8.8.x
Comment #16
jungleComment #17
xjmNice, lots more complete.
From #13:
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.
Comment #18
longwaveAddressed #17.
Comment #19
longwaveComment #20
longwaveThese three patches should fix all the misaligned closing brackets.
Comment #21
daffie CreditAttribution: daffie commentedReviewed 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.
Comment #22
alexpottCommitted 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.