Problem/Motivation

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

Steps to reproduce

Proposed resolution

Fix remaining issue

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

CommentFileSizeAuthor
#10 3549730-nr-bot_l3hepjay.txt15.97 KBneeds-review-queue-bot
#8 3549730-nr-bot_lv5r6ju1.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3549730

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 !13376

quietone’s picture

quietone commented
Issue summary: View changes
Status: Active » Postponed
quietone’s picture

quietone commented
Title: Fix remaining LongLineDeclaration errors in modules » Enable LongLineDeclaration
Issue summary: View changes
Status: Postponed » Needs review

This is a somewhat large change set but it is just formatting changes.

smustgrave’s picture

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

Congrats @quietone on landing this rule.

One of the few times gitlab wasn't great for reviewing but applied locally and spot checked a few and they seem fine to me.

catch made their first commit to this issue’s fork.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3549730-nr-bot_lv5r6ju1.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

quietone’s picture

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

Rebased with no conflicts, Linting has passed so restoring RTBC

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3549730-nr-bot_l3hepjay.txt15.97 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

smustgrave’s picture

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

Bot rebellion

longwave’s picture

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

Added some feedback, there's also a bunch of merge conflicts.

quietone’s picture

quietone commented
Status: Needs work » Needs review
dcam’s picture

dcam commented
Status: Needs review » Needs work

I'm sorry, but this needs another rebase.

quietone’s picture

quietone commented
Status: Needs work » Needs review

@dcam, thanks

smustgrave’s picture

smustgrave commented

Needed another rebase for node.module

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Avoid commit conflicts

The tests are still running for the latest rebase, but I'm going to assume they'll pass.

I did my own review of the changes. I verified that all code has been preserved and nothing was lost, except notably in batch.inc where the array_merge() was removed. There are a few things that I might have done differently, but that's the case with all code, right? They aren't worth mentioning. All feedback has been addressed and the MR comment threads have been closed. This looks good to me.

Because of the far-reaching changes in this MR and the large number of other maintenance issues that are going on right now I've decided to add the "Avoid commit conflicts" tag.

longwave’s picture

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

Doesn't apply to 11.x, do we want to go through the hassle of backporting in order to keep things in sync or not? Not sure which way to go there.

Committed and pushed b36ea26601f to main. 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 b36ea266 on main 
    task: #3549730 Enable LongLineDeclaration

By: quietone
By: smustgrave...

longwave closed merge request !13376

Status: Fixed » Closed (fixed)

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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Closed (fixed) » Needs review

Brought this also up in this slack thread: https://drupal.slack.com/archives/C079NQPQUEN/p1776093790215669

When I tested, I was able to cherry-pick this onto 11.x without conflict. My argument for backporting this would be that this includes several changes to locale include and some .module files, code that we are in the middle of converting, so we will have multiple other issues that will have conflicts on their backports.

that said, we probably need to open a MR to check if there aren't other places that the phpcs rule affects, or only backport the changes without enforcing the rule, as this would also impact contrib modules that rely on the core phpcs configuration. Setting to needs review to make a decision on that, we should IMHO decide to either backport it now or then not, before we start backporting issues like #3577671: Modernize locale file handling which would then conflict again with this.

quietone’s picture

quietone commented

I made an 11.x branch and MR. It isn't showing in the issue summary though. The MR is https://git.drupalcode.org/project/drupal/-/merge_requests/15452

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Thanks, setting to RTBC to make the decision here. I confirmed that cherry-picking the main commit from #3577671: Modernize locale file handling only conflicts on the locale.services.yml file.

catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x to make other backports easier, 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.

  • catch committed d205a05d on 11.x 
    task: #3549730 Enable LongLineDeclaration

By: quietone
By: smustgrave...

catch closed merge request !15452

Status: Fixed » Closed (fixed)

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