Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daffie created an issue. See original summary.

daffie’s picture

Issue tags: +beta target, +rc target

In #3132745-7: Fix Drupal.Array.Array.LongLineDeclaration coding standard for instances of the $modules test property said @xjm the following:

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.

Therefor adding the tags: "beta target", "rc target".

xjm’s picture

Assigned: Unassigned » xjm

Working on splitting out the OP's changes for this.

xjm’s picture

Status: Active » Needs review
FileSize
91.63 KB

Here's the subset of @swatichouhan012's changes from the original issue that involve drupalCreateUser() calls, rebased onto 9.1.x.

xjm’s picture

Assigned: xjm » Unassigned
xjm’s picture

8.9.x version of same.

daffie’s picture

Status: Needs review » Needs work

The 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.

KapilV’s picture

Status: Needs work » Needs review
FileSize
99.57 KB

I follow patch #8 and updated the new patch.
@daffie thanks.

daffie’s picture

Status: Needs review » Needs work

@kapilkumar0324: Could you also post an interdiff file. It makes reviewing you patch a lot easier. See: https://www.drupal.org/documentation/git/interdiff

KapilV’s picture

Status: Needs work » Needs review
FileSize
8.51 KB

Now I have created interdiff.
@daffie thanks.

daffie’s picture

Status: Needs review » Needs work
+++ b/core/modules/block/tests/src/Functional/BlockLanguageTest.php
@@ -30,10 +30,16 @@ class BlockLanguageTest extends BrowserTestBase {
+  /**
+   *
+   */

@@ -149,9 +155,18 @@ public function testMultipleLanguageTypes() {
-    $this->drupalGet('node', ['query' => ['language' => 'en']]);
+    $this->drupalGet('node', [
+      'query' => [
+        'language' => 'en
+    ',
+      ],
+    ]);
     $this->assertNoText('Powered by Drupal', 'The body of the block does not appear on the page.');
-    $this->drupalGet('node', ['query' => ['language' => 'fr']]);
+    $this->drupalGet('node', [
+      'query' => [
+        'language' => 'fr',
+      ],
+    ]);

@@ -174,9 +189,17 @@ public function testMultipleLanguageTypes() {
-    $this->drupalGet('node', ['query' => ['language' => 'en']]);
+    $this->drupalGet('node', [
+      'query' => [
+        'language' => 'en',
+      ],
+    ]);
     $this->assertNoText('Powered by Drupal', 'The body of the block does not appear on the page.');
-    $this->drupalGet('node', ['query' => ['language' => 'fr']]);
+    $this->drupalGet('node', [
+      'query' => [
+        'language' => 'fr',
+      ],
+    ]);

These 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.

xjm’s picture

Yes, 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:

  • Contains drupalCreateUser([
  • Is over 80 chararacters long
  • Contains a closing ] 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!

nitesh624’s picture

Assigned: Unassigned » nitesh624
Hardik_Patel_12’s picture

Assigned: nitesh624 » Hardik_Patel_12

working on it.

Hardik_Patel_12’s picture

Assigned: Hardik_Patel_12 » Unassigned
Status: Needs work » Needs review
FileSize
187.94 KB
110.84 KB

Changes done for remaining files.

daffie’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

@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.

daffie’s picture

Issue tags: -Needs reroll

My mistake. The patch for 9.1 has still un-addressed remarks from comment #8 and #12.

munish.kumar’s picture

Assigned: Unassigned » munish.kumar
munish.kumar’s picture

Uploaded the Patch for 9.1 branch and addressed the changes from #8 and #12.

munish.kumar’s picture

Assigned: munish.kumar » Unassigned
Status: Needs work » Needs review
daffie’s picture

Status: Needs review » Needs work

The 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.

munish.kumar’s picture

Assigned: Unassigned » munish.kumar
daffie’s picture

The 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

munish.kumar’s picture

Thanks, @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.

munish.kumar’s picture

Version: 8.8.x-dev » 9.1.x-dev
Status: Needs work » Needs review
FileSize
244.75 KB
4.46 KB

#24 has been addressed in this patch.

munish.kumar’s picture

Fixed coding standard issues.

munish.kumar’s picture

FileSize
98.93 KB

correct interdiff extension

munish.kumar’s picture

There 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.

munish.kumar’s picture

Assigned: munish.kumar » Unassigned
FileSize
244.96 KB
3.86 KB

Fixed all the coding standards issues. Please review.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

@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.

xjm’s picture

Nice 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.)

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Bummer, 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?

munish.kumar’s picture

Assigned: Unassigned » munish.kumar
munish.kumar’s picture

Assigned: munish.kumar » Unassigned
Status: Needs work » Needs review
FileSize
244.99 KB
1.96 KB

Hi xjm, Please review the changes. Patch has been rerolled for the 9.1.x branch.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

The reroll looks good to me.
Back to RTBC.

xjm’s picture

Thanks for rerolling so promptly! Taking a look at this now.

  • xjm committed de32f3c on 9.1.x
    Issue #3133033 by munish.kumar, xjm, Hardik_Patel_12, daffie,...

  • xjm committed 50570ec on 9.0.x
    Issue #3133033 by munish.kumar, xjm, Hardik_Patel_12, daffie,...

  • xjm committed f1f335a on 8.9.x
    Issue #3133033 by munish.kumar, xjm, Hardik_Patel_12, daffie,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

I 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 only drupalCreateUser() 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.

xjm’s picture

Version: 9.1.x-dev » 8.9.x-dev

Status: Fixed » Closed (fixed)

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