Problem/Motivation

The sniff, Drupal.Array.Array.LongLineDeclaration is not enabled.

Steps to reproduce

Proposed resolution

Enable Drupal.Array.Array.LongLineDeclaration in the Tests/Core directory.

There are no strict standards for how to split a call to a function across multiple lines. The changes try to be the best fit for readability and the current style used in the file or method.

Remaining tasks

Review

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3549630

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

quietone created an issue. See original summary.

quietone opened merge request !13366

quietone’s picture

quietone commented
Status: Active » Needs review

Time for some reviews

quietone’s picture

quietone commented
Title: Fix LongLineDeclaration in Tests/Core » Fix LongLineDeclaration in in several core/tests/Drupal directories
quietone’s picture

quietone commented
Title: Fix LongLineDeclaration in in several core/tests/Drupal directories » Fix LongLineDeclaration in several core/tests/Drupal directories
quietone’s picture

quietone commented
Issue tags: +Coding standards
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Large MR but seems pretty straight forward and change to phpcs.xml should catch any missed.

astonvictor’s picture

astonvictor commented

[ignore]

quietone’s picture

quietone commented

Rebased with a single conflict, in 

core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php

and core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php. I removed the change from this MR and reformatted the new code from HEAD. Since I used the reformat command from PHPStorm, which I use all the time with no problems, I am restoring leaving at RTBC.

quietone’s picture

quietone commented

Oh dear I wasn't working on HEAD and had a work in progress commit there as well. This reverts that mistake.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs review

Chasing HEAD, 2 more files added.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

I checked the diffs of all of the changes. There wasn't anything missing or weird. It looks good to me.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs review

This needed a rebase that didn't go well. Changes that I reverted kept coming back in. So, I started over using the diff file. Then I made some tweaks in TitleResolvedTest. Those are in the last commit which should be checked.

dcam’s picture

dcam commented
Status: Needs review » Needs work

Setting to Needs Work due to a small consistency issue that I noticed in the latest commit. Feedback is in the MR comments.

quietone’s picture

quietone commented
Status: Needs work » Needs review

@dcam, Thanks

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

Feels weird to be thanked for something that seemed kind of nit-picky. But anyway, the latest changes look OK to me now. Back to RTBC.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs review

Rebase and needed changes because FormSubmitter has added an argument. So NR for those changes.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

The latest commit looks good. It's a straightforward change. And tests are passing, so the linting agrees. Back to RTBC.

quietone’s picture

quietone commented

Another rebase. There were a few conflicts but easy as it from the issue that added lots of return types. And a blank line crept in somewhere.

I think it is safe to keep this at RTBC.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Apologies for my pedantry but I find balanced brackets much easier to read, fixed up all the cases I spotted in suggestions.

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community

No worries on my end. I am glad that someone is taking a close look.

I have applied the suggestions and linting has passed, so restoring RTBC.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

This also needed a rebase. Working on that now.

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community

Rebased and linting has passed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 41ad9252958 to 11.x and fc0b1524058 to 11.3.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • longwave committed fc0b1524 on 11.3.x 
    task: #3549630 Fix LongLineDeclaration in several core/tests/Drupal...

longwave closed merge request !13366

  • longwave committed 41ad9252 on 11.x 
    task: #3549630 Fix LongLineDeclaration in several core/tests/Drupal...

Status: Fixed » Closed (fixed)

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