Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#35 | interdiff_30-35.txt | 1.96 KB | munish.kumar |
#35 | 3133033-35.patch | 244.99 KB | munish.kumar |
#30 | interdiff-27-30.txt | 3.86 KB | munish.kumar |
#30 | 3133033-30.patch | 244.96 KB | munish.kumar |
#28 | interdiff-26-27.txt | 98.93 KB | munish.kumar |
Comments
Comment #2
daffie CreditAttribution: daffie commentedIn #3132745-7: Fix Drupal.Array.Array.LongLineDeclaration coding standard for instances of the $modules test property said @xjm the following:
Therefor adding the tags: "beta target", "rc target".
Comment #3
xjmWorking on splitting out the OP's changes for this.
Comment #5
xjmHere's the subset of @swatichouhan012's changes from the original issue that involve
drupalCreateUser()
calls, rebased onto 9.1.x.Comment #6
xjmComment #7
xjm8.9.x version of same.
Comment #8
daffie CreditAttribution: daffie commentedThe code changes that where make look good, only you forgot a lot of changes, like in:
- core/modules/node/tests/src/Functional/NodeQueryAlterTest.php;
- core/modules/block/tests/src/Functional/BlockLanguageTest.php;
- core/modules/contextual/tests/src/Functional/ContextualDynamicContextTest.php.
Comment #9
KapilV CreditAttribution: KapilV as a volunteer and at OpenSense Labs commentedI follow patch #8 and updated the new patch.
@daffie thanks.
Comment #10
daffie CreditAttribution: daffie commented@kapilkumar0324: Could you also post an interdiff file. It makes reviewing you patch a lot easier. See: https://www.drupal.org/documentation/git/interdiff
Comment #11
KapilV CreditAttribution: KapilV as a volunteer and at OpenSense Labs commentedNow I have created interdiff.
@daffie thanks.
Comment #12
daffie CreditAttribution: daffie commentedThese changes are not what this issue is about. In this issue only changes the method calls of drupalCreateUser() where it goes over the 80 characters on 1 line limit. These changes should go in an other issue.
Comment #13
xjmYes, let's please go back to the patch from the original reporter in #5. (Note that it's not my patch; rather, it's the relevant subset of changes the OP provided.)
The next step is to look for other
drupalCreateUser()
calls that contain an array that extends over 80 characters. I would recommend using a regex as was done in #3132745-12: Fix Drupal.Array.Array.LongLineDeclaration coding standard for instances of the $modules test property where you search for anything that:drupalCreateUser([
]
indicating they tried to fit an array all on one line.You can start by fixing the ones @daffie found for you in #8.
Find instances like that and fix them exactly as they're been fixed in the patch in #5.
I'm hiding the other files and removing credit for them. You can get credit by helping add the missed calls to the patch. Thanks!
Comment #14
nitesh624Comment #15
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedworking on it.
Comment #16
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedChanges done for remaining files.
Comment #17
daffie CreditAttribution: daffie commented@Hardik_Patel_12: This patch needs to land first in 9.1. For that branch does the current patch not apply. Therefore does the patch need a reroll.I was wrong about this.Comment #18
daffie CreditAttribution: daffie commentedMy mistake. The patch for 9.1 has still un-addressed remarks from comment #8 and #12.
Comment #19
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #20
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedUploaded the Patch for 9.1 branch and addressed the changes from #8 and #12.
Comment #21
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #22
daffie CreditAttribution: daffie commentedThe patch for 9.1 has still un-addressed remarks from comment #8 and #12.
@munish.kamur: Please keep the status to "needs work" until the remarks from comment #8 and #12 are addressed.
Comment #23
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #24
daffie CreditAttribution: daffie commentedThe points from comment #8 and #12 have been addressed.
All changes in the patch look good.
I still found a couple of cases that need to be addressed:
core/modules/layout_builder/tests/src/Functional/LayoutBuilderAccessTest.php
core/modules/node/tests/src/Functional/NodeFormSaveChangedTimeTest.php
core/modules/user/tests/src/Functional/UserRolesAssignmentTest.php
core/profiles/minimal/tests/src/Functional/MinimalTest.php
core/tests/Drupal/FunctionalTests/Entity/DeleteMultipleFormTest.php
core/tests/Drupal/FunctionalTests/Routing/CaseInsensitivePathTest.php
core/tests/Drupal/FunctionalTests/Routing/RouteCachingNonPathLanguageNegotiationTest.php
Comment #25
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedThanks, @daffie for the quick review, That's my mistake I did not select the correct version in the previous patch. On it will fix the remaining cases.
Comment #26
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented#24 has been addressed in this patch.
Comment #27
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed coding standard issues.
Comment #28
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedcorrect interdiff extension
Comment #29
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedThere are still some coding standards issues that need to be fixed. https://www.drupal.org/pift-ci-job/1711237. On it will fixed all these issues in the next patch.
Comment #30
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed all the coding standards issues. Please review.
Comment #31
daffie CreditAttribution: daffie commented@munish.kumar: Adding the comma to the last permission is a good find. I missed it.
I cannot find any more occurrences of
$this->drupalCreateUser()
where the line goes over the 80 character limit.There are no coding standard violations.
All changes look good to me.
For me it is RTBC.
Comment #32
xjmNice work!
We are in code freeze right now for the releases, but I hope to review this later in the week once the code freeze has ended. (I think it's OK for me to commit because I just set the scope and posted part of the original path author's work.)
Comment #33
xjmBummer, this no longer applies. (Sorry, we had a security release shortly after 9.0.0 as I wasn't able to come back to this.) Can we get a reroll for 9.1.x?
Comment #34
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #35
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi xjm, Please review the changes. Patch has been rerolled for the 9.1.x branch.
Comment #36
daffie CreditAttribution: daffie commentedThe reroll looks good to me.
Back to RTBC.
Comment #37
xjmThanks for rerolling so promptly! Taking a look at this now.
Comment #41
xjmI reviewed with
git diff --word-diff-regex=[^[:space:]] --staged HEAD
to confirm the only non-whitespace changes were the added trailing commas as required by our long array formatting standard, and that onlydrupalCreateUser()
calls were changed.Committed to 9.1.x, 9.0.x, and 8.9.x (surprisingly, git was smart enough to figure out how to auto-merge the cherry-picks). Thanks! Now, onward to other parts of #3116859: [meta] Fix Drupal.Array.Array.LongLineDeclaration coding standard.
Comment #42
xjm