Problem/Motivation

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

Steps to reproduce

Proposed resolution

Enable Drupal.Array.Array.LongLineDeclaration for KernelTests tests.

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
#26 3513561-nr-bot_7nuj3n36.txt91 bytesneeds-review-queue-bot
#23 3513561-nr-bot_1_fgjrra.txt91 bytesneeds-review-queue-bot
#20 3513561-nr-bot_cfhfdmmj.txt91 bytesneeds-review-queue-bot
#13 3513561-nr-bot.txt91 bytesneeds-review-queue-bot
#10 3513561-nr-bot.txt27.43 KBneeds-review-queue-bot
#7 3513561-nr-bot.txt91 bytesneeds-review-queue-bot
#5 3513561-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3513561

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

quietone’s picture

quietone commented
Status: Active » Needs review

Linting passes, so NR.

smustgrave’s picture

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

Pretty straight forward

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3513561-nr-bot.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

There were no conflicts

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3513561-nr-bot.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 » Needs review

A rebase and an additional change.

smustgrave’s picture

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

additional change seems fine.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3513561-nr-bot.txt27.43 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.

quietone’s picture

quietone commented
Status: Needs work » Needs review

Rebase and wew fixes in the last commit to review

smustgrave’s picture

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

Reviewed last commit https://git.drupalcode.org/project/drupal/-/merge_requests/11510/diffs?c... and seem fine

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3513561-nr-bot.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 » Needs review

Rebase and change in EntityDefinitionUpdateMultipleTypesTest.php

smustgrave’s picture

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

Reviewed https://git.drupalcode.org/project/drupal/-/merge_requests/11510/diffs?c... and additional change seems fine.

longwave’s picture

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

While this is largely subjective I feel there's definitely some imbalances going on here.. I feel that starting/ending braces etc should be at the same indentation level at least, and there's some cases that can be rearranged to make them easier to read. I added some sample suggestions but there are other similar cases that should also be considered.

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

oily’s picture

oily commented

Applied @longwave's suggestions. All code structural changes look good imo. Not sure we need to get into a debate on which of the many possible structures is the 'ideal'? This seems to represent a concrete improvement certainly in terms of readability.

oily’s picture

oily commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3513561-nr-bot_cfhfdmmj.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 » Needs review

Rebased, there was a conflict in \Drupal\KernelTests\Core\Entity\EntityQueryTest::testEntityQuery

smustgrave’s picture

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

Seems like a good rebase

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3513561-nr-bot_1_fgjrra.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 » Needs review

Rebased and linting passes. There are new changes that need to be reviewed. These are for the use of "#[TestWith(" and there is no standard yet for that. For readability, it made sense to me to simply add an ignore line to ignore this sniff.

smustgrave’s picture

smustgrave commented

Would assume disabling the phpcs check for those should be fine is there another way to confirm though?

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3513561-nr-bot_7nuj3n36.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 » Needs review

@smustgrave, you can review the Coding Standards to confirm that there are not standards for the formatting of the TestWith attribute.

dcam’s picture

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

I reviewed the last commit that disabled PHPCS around TestWith attributes. That seems like a reasonable approach to me until coding standards are developed. Should a follow-up issue be created for it?

oily’s picture

oily commented

@dcam Re #28 Do you mean the addition to the coding standards as a follow up? That might be a more general issue not necessarily related to this particular issue if it would affect potentially all core tests not just the tests involved in this issue.

quietone’s picture

quietone commented

@dcam and @oily, , there is an existing Coding Standards issue for tests, anything for TestWith would be a child of that. #2057905: [policy, no patch] Discuss the standards for phpunit based tests. In fact the issue summary on that issue need to be brought up to date.

Rebased with a single conflict, in \Drupal\KernelTests\Core\Menu\MenuLinkTreeTest::testCreateLinksInMenu. 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, so I am restoring leaving at RTBC.

longwave’s picture

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

Had another pass at this, found some more where I think it's not quite right.

Sometimes I wish we had a super opinionated formatter like prettier or gofmt.

quietone’s picture

quietone commented
Status: Needs work » Needs review

I responded to all of @longwave's comments. And a rebase.

smustgrave’s picture

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

Rebase seems though not sure how long that will last lol :)

  • longwave committed 30ac7f79 on 11.3.x 
    docs: #3513561 Fix LongLineDeclaration in KernelTests

By: quietone
By:...

  • longwave committed 83e557e4 on 11.x 
    docs: #3513561 Fix LongLineDeclaration in KernelTests

By: quietone
By:...
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

Let's get this in before it needs another reroll :)

Committed and pushed 83e557e40b1 to 11.x and 30ac7f797d6 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 closed merge request !11510

Status: Fixed » Closed (fixed)

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