Fix coding standard for closures - Drupal.WhiteSpace.ScopeClosingBrace and Generic.Functions.OpeningFunctionBraceKernighanRitchie

As agreed between the mentors at Drupalcon, according to issues to avoid for novices, I am untagging this issue as "Beginner". This issue contains changes across a very wide range of files and might create too many other patches to need to be rerolled at this particular time. This patch has an automated way to be rerolled later so better to implement it after Drupalcon.

Needs reworking our phpcs.xml.dist is now inclusinve so the rule will have to be added rather than removed.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/NativeUpsert.php
@@ -93,7 +93,8 @@ public function __toString() {
-    $insert_fields = array_map(function($f) { return $this->connection->escapeField($f); }, $insert_fields);
+    $insert_fields = array_map(function($f) { return $this->connection->escapeField($f); ¶
+    }, $insert_fields);

Also anonymous functions look a mess.

Inline functions still broken, reroll

We need problem discuss anonymous function coding standards. I don't think there are any. In the interim I think we need to fix the rule to not mess with anonymous functions.

See #1999722: [policy] Define coding standards for anonymous functions (closures)

This is apparently currently postponed on: #2795669: Incorrect autofix of anonymous function body in Drupal.WhiteSpace.ScopeClosingBrace

We got around the overly aggressive fixer in #2676540: Fixer should not delete elaborate @file doc comments for 8.1.x, but IIRC we did that by patching core wherever the fixer would have over-fixed it. While the scope of that rule's implementation was very big, it was also straightforward since it was always at the top of a file. This rule is way more fussy, so I can see postponing it on making the fixer better.

This can be reopened. #2795669: Incorrect autofix of anonymous function body in Drupal.WhiteSpace.ScopeClosingBrace has been closed with "works as designed". @klausi explains how we should tackle it:

You need to run the fixer for closing curly braces AND opening curly braces, then you get the desired autofix (the last code block in the issue summary). The problem is that core only enables part of the Drupal standard, so the fixer can only fix part of the problems.

If you execute the following command then ConfigManager.php will be auto-fixed just fine:

phpcbf --standard=Drupal core/lib/Drupal/Core/Config/ConfigManager.php

If you execute the following command then ConfigManager.php will be auto-fixed just fine:

phpcbf --standard=Drupal core/lib/Drupal/Core/Config/ConfigManager.php

Is it possible to provide a more scoped command to auto-fix? This will fix every standards violation, which is not feasible or in scope to deploy the new closure standards I think.

I've been looking into this. It's not possible to fix this one standard (whitespace around the scope closing brace) on its own without making the code base look a lot worse. If the closing brace is not correctly placed, this usually means that the opening brace is also incorrect.

As commented before this mainly affects closures. I suggest to expand the scope of this issue to fix the coding standards for closures.

There is also a new version of Coder released which adds a bunch of new sniffs. Those need to be fixed or excluded from the ruleset before we can continue with this.

Postponing on #2803779: Update Coder to 8.2.10 and fix errors from improved sniffs.

@xjm asks in comment 17:

Is it possible to provide a more scoped command to auto-fix? This will fix every standards violation, which is not feasible or in scope to deploy the new closure standards I think.

Here is a patch to the ruleset that will autofix the closures. We can't run it yet though before #2803779: Update Coder to 8.2.10 and fix errors from improved sniffs is in.

Updating title to match the new scope. If anyone does not agree with this, please feel free to revert this.

We need a couple of rules to fix the matching opening and closing braces, and just adding the OpeningFunctionBrace sniff will make this autofix closures entirely. I think the additional work to add this one single sniff is quite small in comparison to the benefit of being able to fix all closures at once in core and match the new coding standard.

This is blocked by #2803779: Update Coder to 8.2.10 and fix errors from improved sniffs which is in turn blocked by #2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core. In that last issue Coder is added as a dependency of core. This is probably not something that is acceptable so late in the release cycle for 8.2.x, so this means that it is unfortunately unlikely to get the coding standards for closures fixed in 8.2.x :(

Coder is updated now, so this would be acceptable as an 8.3.x RC target.

Using phpcbf and the changed config in #19, you sometimes get changes like this:

-        array_map(function ($entity) { return $entity->getConfigDependencyName(); }, $affected_dependencies[$dependency_type]),
+        array_map(function ($entity) {
+          return $entity->getConfigDependencyName(); 
+        }, $affected_dependencies[$dependency_type]),

Note that there's a stray space at the end of the return line.

I tried this with 8.4.x (which requires a different version of Coder) for the same result.

Running phpcbf a second time will fix the stray spaces I think :)

I posted a new patch fixing the rejected changes in '2572795-19.patch' one.
I'm keeping 8.3.x branch but it also worked in 8.4.x one.

Squiz.Functions.MultiLineFunctionDeclaration is also fixed in #2901726: Fix 'Squiz.Functions.MultiLineFunctionDeclaration' coding standard. Either we close that one or we don't target Squiz.Functions.MultiLineFunctionDeclaration in this issue. I think we should also mention in the issue title what sniffs we are targeting here.

I updated the title to reflect the scope of the issue. I also updated the summary.

Here is the patch.

+++ b/core/modules/views/tests/src/Kernel/Plugin/StyleGridTest.php
@@ -69,11 +69,16 @@ protected function assertGrid(ViewExecutable $view, $alignment, $columns) {
+      case 5: $width = '20'; ¶
+        break;
+      case 4: $width = '25'; ¶
+        break;
+      case 3: $width = '33.3333'; ¶
+        break;
+      case 2: $width = '50'; ¶
+        break;
+      case 1: $width = '100'; ¶
+        break;

This leaves whitespace at the end of those lines, that looks off.

Good spot, I didn't noticed. Here is the patch and interdiff.

tested, only warnings about empty files :

PHP CODE SNIFFER REPORT SUMMARY
----------------------------------------------------------------------
FILE ERRORS WARNINGS
----------------------------------------------------------------------
...m/tests/fixtures/HtaccessTest/access_test.install 0 1
...ystem/tests/fixtures/HtaccessTest/access_test.inc 0 1
...em/tests/fixtures/HtaccessTest/access_test.module 0 1
...m/tests/fixtures/HtaccessTest/access_test.tpl.php 0 1
...m/tests/fixtures/HtaccessTest/access_test.profile 0 1
...tem/tests/fixtures/HtaccessTest/access_test.theme 0 1
----------------------------------------------------------------------
A TOTAL OF 0 ERRORS AND 6 WARNINGS WERE FOUND IN 6 FILES
----------------------------------------------------------------------

@osab According to your comment this should not have RTBC status. We should not get any errors or warnings. But, are you sure you've tested with the full phpcs.xml.dist?

These commands lead me to believe this patch should be RTBC:

$ git fetch
$ git checkout 8.5.x
$ git pull origin 8.5.x
$ git apply [the patch]
$ composer install
$ cd core/
$ ../vendor/bin/phpcs -ps

The patch looks good, so +1.

1. +++ b/core/lib/Drupal/Component/DependencyInjection/Container.php
   @@ -586,8 +586,6 @@ public function getServiceIds() {
   +  private function __clone() {}
   

   If we do this.

2. +++ b/core/modules/aggregator/tests/src/Unit/Plugin/AggregatorPluginSettingsBaseTest.php
   @@ -109,5 +109,6 @@ public function testSettingsForm() {
    
    if (!function_exists('drupal_set_message')) {
   -  function drupal_set_message() {}
   +  function drupal_set_message() {
   +  }
    }
   

   Why do we do this?

   Do we have explicit coding standards that say empty procedural functions need the newline, or is it the CS rule enforcing it?

{} is allowed only in classes. Not procedural code nor traits. This is what current implementation of Drupal CS does.
In classes, we can also use the style of procedural code. Drupal CS doesn't complain.

I can't find anything related to functions with no body in https://www.drupal.org/docs/develop/standards.

The trait vs class difference is really odd here. As a developer that rule would be impossible to learn. I think before we proceed here we should determine what the standard should be and get the coder rules adjusted accordingly. I've opened #2915420: Empty functions coding standard to discuss and come up with a solution. I think this issue needs to be postponed on that one.

As per my comment at https://www.drupal.org/node/2915420#comment-12296196, both styles are allowed for classes. So, I would adjust the patch to make it consistent and also pass the sniffs.

Re-roll.

Fix consistency.

@mfernea ah okay my bad I misunderstood that classes are allowed the closing brace on the next line too. The current patch looks good and this does not need to be postponed on #2915420: Empty functions coding standard

IMO #43 is good to go. It applies cleanly and raises no CS errors, and given #41 and #44 does not need to be postponed.

Thanks!

Committed and pushed to 8.5.x. Thanks!

We can also backport the cleanups to 8.4.x since they're basically just whitespace changes, but first we need to remove the changes to phpcs.xml.dist as per #2919983: Restore the 8.4.0 phpcs.xml.dist on 8.4.x.

Hm, actually I'm not totally sure what happened because I was sure I checked all of core before committing this, but this does introduce a number of fails:

FILE: ...rupal/Core/Http/Exception/CacheableAccessDeniedHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: .../Drupal/Core/Http/Exception/CacheableBadRequestHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ib/Drupal/Core/Http/Exception/CacheableConflictHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...re/lib/Drupal/Core/Http/Exception/CacheableGoneHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...r/core/lib/Drupal/Core/Http/Exception/CacheableHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...pal/Core/Http/Exception/CacheableLengthRequiredHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...l/Core/Http/Exception/CacheableMethodNotAllowedHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...upal/Core/Http/Exception/CacheableNotAcceptableHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ib/Drupal/Core/Http/Exception/CacheableNotFoundHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...Core/Http/Exception/CacheablePreconditionFailedHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...re/Http/Exception/CacheablePreconditionRequiredHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...Core/Http/Exception/CacheableServiceUnavailableHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...al/Core/Http/Exception/CacheableTooManyRequestsHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...rupal/Core/Http/Exception/CacheableUnauthorizedHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ore/Http/Exception/CacheableUnprocessableEntityHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...re/Http/Exception/CacheableUnsupportedMediaTypeHttpException.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 1 mins, 47.41 secs; Memory: 94Mb

So it looks like there's more to fix before we commit this and I've reverted it for now. Thanks!

Hmm... same for me, and I actually have tried #43 before changing to RTBC so not sure what have happened here. I'll give it a try.

Seems like all of them were introduced in #2920001: Add cacheable HTTP exceptions: Symfony HTTP exceptions + Drupal cacheability metadata committed earlier today.

phpcbf for the rescue 💪

now passes!

That explains it; thanks @zaporylie.

Committed and pushed to 8.5.x (again) and moving back to 8.4.x for the backport without the rule change. Thanks!

  <rule ref="../vendor/drupal/coder/coder_sniffer/Drupal/Sniffs/WhiteSpace/ScopeClosingBraceSniff.php"/>
  <rule ref="Generic.Functions.OpeningFunctionBraceKernighanRitchie">
    <properties>
      <property name="checkClosures" value="true"/>
    </properties>
  </rule>

1. Added rule above to phpcs.xml.dist
2. phpcbf
3. Fixed #31
4. phpcs
5. Removed rule from phpcs.xml.dist

Changes are the same but without rule like #54 said

Thanks @zaporylie. I confirmed that all the changes are within the scope of the rule, reviewed with git diff --staged --color-words to ensure there were no other incidental changes, and ran the test suite against core again just to be sure.

Committed and pushed to 8.4.x. Thanks!