Problem/Motivation

It'd be great to enable this rule for core but it appears to have several bugs that make this tricky. This issue will collect them together. See #2572801: Fix 'Drupal.WhiteSpace.ScopeIndent' coding standard

Problems

  • One big problem is that we have to decide what to do about things like core/modules/comment/tests/src/Unit/CommentLinkBuilderTest.php where the namespace is not indented for readability. 
  •      switch ($operation) {
       case 'view':
         return AccessResult::allowedIfHasPermission($account, 'access news feeds');
-        break;
+      break;

       default:
         return AccessResult::allowedIfHasPermission($account, 'administer news feeds');
-        break;
+      break;
     }
   }

    This change is incorrect.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Looking at part of the problem I think core is the issue - we shouldn't break PSR-0/4 in runtime code and tests are just doing it wrong. See #2572801-41: Fix 'Drupal.WhiteSpace.ScopeIndent' coding standard, #2787577: When tests use multiple namespaces they should do so in a coding standards compliant way and #2787567: Refactor \Drupal\Component\Assertion\Handle to not break PSR-0/4 one class per file

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I think for part two we might need to get Drupal.WhiteSpace.ScopeClosingBrace.BreakIndent working on core.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

The problem with the break statements in this case is that they are unreachable code and should be removed. They can never be executed after the return statement has happened.

I understand why they are there for readability purposes, so yes we should somehow fix this.

Namespaces: we should not use braces for them at all. In PHP any new namespace declaration just switches the namespace, so no braces necessary.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

OK, for the global namespace you probably need the braces, but otherwise we can do this:

diff --git a/core/modules/comment/tests/src/Unit/CommentLinkBuilderTest.php b/core/modules/comment/tests/src/Unit/CommentLinkBuilderTest.php
index 898e0ab..f63108d 100644
--- a/core/modules/comment/tests/src/Unit/CommentLinkBuilderTest.php
+++ b/core/modules/comment/tests/src/Unit/CommentLinkBuilderTest.php
@@ -1,6 +1,6 @@
 <?php
 
-namespace Drupal\Tests\comment\Unit {
+namespace Drupal\Tests\comment\Unit;
 
 use Drupal\comment\CommentLinkBuilder;
 use Drupal\comment\Plugin\Field\FieldType\CommentItemInterface;
@@ -319,8 +319,6 @@ protected function getMockNode($has_field, $comment_status, $form_location, $com
 
 }
 
-}
-
 namespace {
   if (!function_exists('history_read')) {
     function history_read() {
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@klausi great minds :) #2787577: When tests use multiple namespaces they should do so in a coding standards compliant way

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I'm coming round to thinking that we should remove the breaks if they are unreachable code. It just adds noise.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

There is nothing in https://www.drupal.org/coding-standards#controlstruct mandating the use of a break - but the example hints that it might have to be there.

eric_a’s picture

eric_a commented

The break keyword is nice when scanning for code paths. Getting rid of the early returns could be the solution that makes everybody happy - set one variable for each case and return once.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Active » Closed (works as designed)

So we removed the unreachable break statements in core as solution, nothing we need to do in Coder.