Problem/Motivation

Considering coding standard, following NestedArray::mergeDeep() method should call arguments in one line.
See in Drupal\workflows\Plugin\WorkflowTypeBase and Drupal\Core\Block\BlockBase.

  public function setConfiguration(array $configuration) {
    $this->configuration = NestedArray::mergeDeep(
      $this->defaultConfiguration(),
      $configuration
    );
  }

Proposed resolution

  public function setConfiguration(array $configuration) {
    $this->configuration = NestedArray::mergeDeep($this->defaultConfiguration(), $configuration);
  }
CommentFileSizeAuthor
#2 respect_coding_standard-2852403-2.patch1.77 KBGoZ
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

GoZ created an issue. See original summary.

GoZ’s picture

GoZ CreditAttribution: GoZ at Centarro commented
Status: Active » Needs review
FileSize
respect_coding_standard-2852403-2.patch1.77 KB
GoZ’s picture

GoZ CreditAttribution: GoZ at Centarro commented
Issue tags: +Coding standards
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Yes, both changes look good. Thank you.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Closed (won't fix)

@GoZ can you point to the coding standard which was considered? There's nothing about multi-line function calls in our coding standards https://www.drupal.org/docs/develop/standards/coding-standards#functcall

Also if we're going to implement a coding standard we need agreement for this new standard by creating an issue in https://www.drupal.org/project/coding_standards and then a rule in coder and fixing the entirety of core. I don't think that this is worth it for multi=line function calls.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Yeah, we actually use this multi-line function called pattern all over court for readability. As far as I recall, the only things not allowed to be on multiple lines are Boolean conditions, the one-line summary of functions, and docblock tags that are not parsed by the API module otherwise.

Also see: http://drupal.org/core/scope#coding-standards (basically reiterates #5 in our documented policy).

Thanks!

GoZ’s picture

GoZ CreditAttribution: GoZ at Centarro commented

I thought it was a coding standard after Tim ask me to change it in https://www.drupal.org/node/2851166#comment-11933760.

I also check every NestedArray::mergeDeep() calls in setConfiguration() and they are all in one Line except in this two class.

The purpose of this change was to be more consistent between differents core classes.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

There's nothing about multi-line function calls in our coding standards

Yes, I did look at the coding standard and found the same. But I wasn't sure if the lack of mention meant it was allowed or if the lack of mention was an oversight. And a search only came up with a few special cases of multi-line, so I went with what appeared to be the current practice.

@alexpott and @xjm, thank for the clarification.