Fix 'Drupal.WhiteSpace.ScopeIndent' coding standard

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 phpcs.xml.dist is now inclusive so the rule needs adding and also we could split this up into specific errors generated by the sniff to make it a bit more managable.

re-roll, also added Drupal.WhiteSpace.ScopeIndent to phpcs.xml.dist

@andypost can we split this issue up into specific errors defined by Drupal.WhiteSpace.ScopeIndent this way the issue becomes reviewable. See the current phpcs.xml.dist for how to do this - I committed #2707641: Ensure core compliance to Drupal.Commenting.FunctionComment.ParamCommentIndentation (part 2) which does this.

Using the technique described here: #2571965-63: [meta] Fix PHP coding standards in core, stage 1 I came up with these stats:

  12 Drupal.WhiteSpace.ScopeIndent.Incorrect
1161 Drupal.WhiteSpace.ScopeIndent.IncorrectExact

Incorrect isn't very good at auto-fixing. It wants to make changes like this:

diff --git a/core/modules/user/tests/src/Unit/PermissionHandlerTest.php b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
index 6a2b26f..f294e95 100644
--- a/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
+++ b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
@@ -106,11 +106,11 @@ public function testBuildPermissionsYaml() {
     $url = vfsStream::url('modules');
     mkdir($url . '/module_a');
     file_put_contents($url . '/module_a/module_a.permissions.yml',
-"access_module_a: single_description"
+    "access_module_a: single_description"
     );

...which ruins the readability of hard-coded YAML strings in tests.

That leaves us with IncorrectExact, which has a large number of errors, and which also isn't so good at auto-correcting in some circumstances.

There are a number of cases where namespace brackets confuse IncorrectExact on auto-correcting. Those will need to be manually fixed and reviewed carefully.

Here's a patch limited to core/lib/ which should give us some idea of how reviewable this is.

Here's a re-roll and fix to title and scope

      2 Drupal.Commenting.FunctionComment.ThrowsComment
   1043 Drupal.WhiteSpace.ScopeIndent.IncorrectExact

Both ones needs new issues

fixed

had been fixed indents problems

fixed

diff --git a/core/modules/statistics/migration_templates/d6_statistics_settings.yml b/core/modules/statistics/migration_templates/d6_statistics_settings.yml
deleted file mode 100644

Seems a little severe for a coding standards patch...

diff --git a/core/modules/statistics/migration_templates/d6_statistics_settings.yml b/core/modules/statistics/migration_templates/d6_statistics_settings.yml
deleted file mode 100644

unrelated change?

Hi Klausi ,

Can you please explain the unrelated change in this yml file and do we need to revert that in the next patch?

Thanks
~Chirag

@chishah92: Klausi and I were saying that the patch is wrong. It removes YML files which is out of scope for this issue.

Have fixed the YML file deletion in the new patch.

Thanks!
~Chirag

Right, but now you're *adding* a YML file for a coding standards issue.

Please pull the 8.2.x branch again, re-do the work according to the instructions in the issue summary, and make another patch.

Thanks.

This patch now adds new files. Let's ensure to reroll against 8.2.x

I just reran phpcbf to fix the standards this way:

phpcbf --standard=Drupal --sniffs=Drupal.WhiteSpace.ScopeIndent --exclude=WhiteSpace.ScopeIndent.Incorrect .

There are a lot of weird corrections like below one.
Seems like patch was created using wrong way

+++ b/core/lib/Drupal/Component/Assertion/Handle.php
@@ -8,9 +8,9 @@
+    /**
    * Emulates PHP 7 AssertionError as closely as possible.
    *
    * We force this class to exist at the root namespace for PHP 5.
@@ -18,26 +18,26 @@

First line for this block fixed incorrectly.
We no need fix here.

After invetigation on autofixes by phpcbf I see bunch of other wrong fixes:

1. +++ b/core/modules/aggregator/src/FeedAccessControlHandler.php
   @@ -21,11 +21,11 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   -        break;
   

   Wrong indentation fix for break statement

2. +++ b/core/modules/views/src/Tests/Entity/FieldEntityTranslationTest.php
   @@ -81,8 +81,8 @@ public function testTranslationRows() {
    
   -    $this->drupalGet('test_entity_field_renderers/entity_default');
   -    $this->assertRows(
   +$this->drupalGet('test_entity_field_renderers/entity_default');
   +$this->assertRows(
          [
   

   Wrong indentation fix for square brackets array

3. +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
   @@ -192,506 +192,506 @@ function (&$elements, $is_root_call = FALSE) {
   -  public function testRenderTrimmedWithMoreLink() {
   -    $alter = [
   +    public function testRenderTrimmedWithMoreLink() {
   +      $alter = [
          'trim' => TRUE,
          'max_length' => 7,
          'more_link' => TRUE,
   

   Wrong indentation fix for square bracket array elements

After phpcbf it need a lot of manual changes and restores.

So we should fix sniffer in coder first

I propose fix this issue in two steps.
1. Fix first part where we will include small fixes from phpcbf with manual correction.
2. Cover other part with huge fixes when we will found correct solution for that.

In attached files you can find patch for first step.

Remaining scope:
In second step we need rewrite each file from follow list more then 80%

• core/lib/Drupal/Component/Assertion/Handle.php
• core/lib/Drupal/Core/Mail/MailFormatHelper.php
• core/modules/aggregator/src/FeedAccessControlHandler.php
• core/modules/aggregator/tests/src/Unit/Plugin/AggregatorPluginSettingsBaseTest.php
• core/modules/comment/tests/src/Unit/CommentLinkBuilderTest.php
• core/modules/comment/tests/src/Unit/CommentStatisticsUnitTest.php
• core/modules/language/tests/src/Unit/LanguageNegotiationUrlTest.php
• core/modules/migrate/src/MigrateExecutable.php
• core/modules/node/src/NodeTypeAccessControlHandler.php
• core/modules/simpletest/tests/src/Unit/TestInfoParsingTest.php
• core/modules/taxonomy/src/TermAccessControlHandler.php
• core/modules/text/src/Plugin/migrate/cckfield/TextField.php
• core/modules/views/src/Plugin/views/sort/SortPluginBase.php
• core/modules/views/src/Tests/Entity/FieldEntityTranslationTest.php
• core/modules/views/tests/src/Unit/Controller/ViewAjaxControllerTest.php
• core/modules/views/tests/src/Unit/EntityViewsDataTest.php
• core/modules/views/tests/src/Unit/Plugin/Block/ViewsBlockTest.php
• core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
• core/modules/views/tests/src/Unit/Plugin/views/field/EntityOperationsUnitTest.php
• core/modules/views/tests/src/Unit/Routing/ViewPageControllerTest.php
• core/tests/Drupal/KernelTests/AssertConfigTrait.php
• core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php
• core/tests/Drupal/Tests/Core/Asset/CssCollectionRendererUnitTest.php
• core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
• core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php
• core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php
• core/tests/Drupal/Tests/Core/Entity/KeyValueStore/KeyValueEntityStorageTest.php
• core/tests/Drupal/Tests/Core/Form/FormTestBase.php
• core/tests/Drupal/Tests/Core/Plugin/Discovery/HookDiscoveryTest.php
• core/tests/Drupal/Tests/Core/Render/Element/MachineNameTest.php
• core/tests/Drupal/Tests/Core/Session/PermissionsHashGeneratorTest.php
• core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php

Yep, makes sense to file new issue and fix sniffer

The change to core/phpcs.xml.dist is missing? We should enable the sniff there. If phpcbf does not work as desired please file an issue in the Coder issue queue with a snippet to reproduce the problem.

Added changes to phpcs.xml.dist

+++ b/core/phpcs.xml.dist
@@ -89,6 +89,7 @@
+  <rule ref="Drupal.WhiteSpace.ScopeIndent.Incorrect"/>

That is not correct - the rule ref should only have 3 parts. The last "Incorrect" should be removed. You may need to define excludes for this sniff where we filter out something - see the other sniffs in this file where we do that.

If I run phpcs with this patch and the latest Coder version then I get a couple of Drupal.WhiteSpace.ScopeIndent.IncorrectExact fails, is that intentional?

We need to fix coder to correctly implement coding standards - opened #2787555: Drupal.WhiteSpace.ScopeIndent.IncorrectExact so we can use it for core for the exact rule. Drupal.WhiteSpace.ScopeIndent.Incorrect looks like we need to finalise the standards for anonymous functions before we attempt that.

Created #2787567: Refactor \Drupal\Component\Assertion\Handle to not break PSR-0/4 one class per file to handle one of the tricky situations.

Created #2787577: When tests use multiple namespaces they should do so in a coding standards compliant way to handle alot of the test issues with multiple namespaces.

Need to open separate issues for:

• core/modules/simpletest/tests/src/Unit/TestInfoParsingTest.php
• core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php
• core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php
• core/tests/Drupal/Tests/Core/Form/FormTestBase.php

[Edit: The first 3 are now part of #2787577: When tests use multiple namespaces they should do so in a coding standards compliant way]

Looking at coder in order to not have issues with the switch element we need to fix #2572795: Fix coding standard for closures - Drupal.WhiteSpace.ScopeClosingBrace and Generic.Functions.OpeningFunctionBraceKernighanRitchie first.

Created #2787655: Fix \Drupal\Tests\Core\Form\FormTestBase to not have multiple namespaces to address the final test class with namespacing indent issues.

The blockers have landed...

So much win... the fixer is not perfect... but the rule is it identifies places where the indentation is wrong 100% of the time.

1. +++ b/core/lib/Drupal/Core/Mail/MailFormatHelper.php
   @@ -203,7 +203,7 @@ public static function htmlToText($string, $allowed_tags = NULL) {
   -          // Fall-through.
   +            // Fall-through.
   

   This is the one meh - but I don't think it is a blocker... because there is prior art... just need to find it :)

2. +++ b/core/modules/aggregator/src/FeedAccessControlHandler.php
   @@ -21,11 +21,9 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
          case 'view':
            return AccessResult::allowedIfHasPermission($account, 'access news feeds');
   -        break;
    
          default:
            return AccessResult::allowedIfHasPermission($account, 'administer news feeds');
   -        break;
   

   The breaks here are dead code...

3. +++ b/core/modules/system/tests/src/Kernel/Scripts/DbCommandBaseTest.php
   @@ -99,10 +99,10 @@ public function testPrefix() {
   -//    $command_tester->execute([
   -//      '--prefix' => 'notsimpletest',
   -//    ]);
   -//    $this->assertEquals('notsimpletest', $command->getDatabaseConnection($command_tester->getInput())->tablePrefix());
   +    //    $command_tester->execute([
   +    //      '--prefix' => 'notsimpletest',
   +    //    ]);
   +    //    $this->assertEquals('notsimpletest', $command->getDatabaseConnection($command_tester->getInput())->tablePrefix());
   

   Probably should delete this dead code.

Self-review.

I verified the changes with git diff --color-words and they look good! The break statement removals also look good.

I tested with Coder 8.2.8 with the patch applied and there are still 2 errors reported:

FILE: ...ce/drupal-8/core/modules/user/src/Tests/RestRegisterUserTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 136 | ERROR | [x] Line indented incorrectly; expected 8 spaces,
     |       |     found 6
 141 | ERROR | [x] Line indented incorrectly; expected 8 spaces,
     |       |     found 6

There is a weirdly formatted array in there :(
We need to fix that one way or the other because the output of phpcs should be empty with this patch.

@klausi that's really weird my coder is checked out to 8.2.8 and I don't see this. Yes the indentation is wrong but I wonder why?

Turns out we should just enable the entire sniff... since there is only one more file to fix.

still not sure what is happening with #47.
Maybe code sniffer version... mine is PHP_CodeSniffer version 2.5.1 (stable) by Squiz (http://www.squiz.net)

1. +++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
   diff --git a/core/lib/Drupal/Core/Mail/MailFormatHelper.php b/core/lib/Drupal/Core/Mail/MailFormatHelper.php
   index 080b326..e44f889 100644
   
   index 080b326..e44f889 100644
   --- a/core/lib/Drupal/Core/Mail/MailFormatHelper.php
   
   --- a/core/lib/Drupal/Core/Mail/MailFormatHelper.php
   +++ b/core/lib/Drupal/Core/Mail/MailFormatHelper.php
   
   +++ b/core/lib/Drupal/Core/Mail/MailFormatHelper.php
   +++ b/core/lib/Drupal/Core/Mail/MailFormatHelper.php
   @@ -203,7 +203,7 @@ public static function htmlToText($string, $allowed_tags = NULL) {
   
   @@ -203,7 +203,7 @@ public static function htmlToText($string, $allowed_tags = NULL) {
                  $chunk = '';
                }
    
   -          // Fall-through.
   +            // Fall-through.
              case '/li':
              case '/dd':
                array_pop($indent);
   

   That one is a bit weird to be honest. I get where this rule is coming from.

2. +++ b/core/modules/user/src/Plugin/migrate/process/d6/UserUpdate7002.php
   @@ -25,11 +25,11 @@ class UserUpdate7002 extends ProcessPluginBase implements ContainerFactoryPlugin
      /**
   diff --git a/core/modules/user/src/Plugin/views/field/Permissions.php b/core/modules/user/src/Plugin/views/field/Permissions.php
   
   diff --git a/core/modules/user/src/Plugin/views/field/Permissions.php b/core/modules/user/src/Plugin/views/field/Permissions.php
   index 42c78d2..bd5c11d 100644
   
   index 42c78d2..bd5c11d 100644
   --- a/core/modules/user/src/Plugin/views/field/Permissions.php
   
   --- a/core/modules/user/src/Plugin/views/field/Permissions.php
   +++ b/core/modules/user/src/Plugin/views/field/Permissions.php
   
   +++ b/core/modules/user/src/Plugin/views/field/Permissions.php
   +++ b/core/modules/user/src/Plugin/views/field/Permissions.php
   @@ -112,16 +112,4 @@ function render_item($count, $item) {
   
   @@ -112,16 +112,4 @@ function render_item($count, $item) {
        return $item['permission'];
      }
    
   -  /*
   -  protected function documentSelfTokens(&$tokens) {
   -    $tokens['[' . $this->options['id'] . '-role' . ']'] = $this->t('The name of the role.');
   -    $tokens['[' . $this->options['id'] . '-rid' . ']'] = $this->t('The role ID of the role.');
   -  }
   -
   -  protected function addSelfTokens(&$tokens, $item) {
   -    $tokens['[' . $this->options['id'] . '-role' . ']'] = $item['role'];
   -    $tokens['[' . $this->options['id'] . '-rid' . ']'] = $item['rid'];
   -  }
   -  */
   -
    }
   

   Opened a follow up for that: #2788481: Implement documentSelfTokens and addSelfTokens in \Drupal\user\Plugin\views\field\Permissions

3. +++ b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
   @@ -234,21 +235,24 @@ public function testBuildPermissionsYamlCallback() {
   -    file_put_contents($url . '/module_c/module_c.permissions.yml',
   -"permission_callbacks:
   +    file_put_contents($url . '/module_c/module_c.permissions.yml', <<<EOF
   +permission_callbacks:
      - 'Drupal\\user\\Tests\\TestPermissionCallbacks::titleDescriptionRestrictAccess'
   -");
   +EOF
   

   +1 for using more <<

Yeah #50.1 is the only bit of ugliness. I think we document this differently elsewhere... gonna search.

Yep ... looking in \Drupal\views\EntityViewsData::mapSingleFieldViewsData we do

     case 'text_with_summary':
        // Treat these three long text fields the same.
        $field_type = 'text_long';
        // Intentional fall-through here to the default processing!

      default:
        // For most fields, the field type is generic enough to just use
        // the column type to determine the filters etc.
 

To be honest 50.1 is a bit of a pointless comment, it just documents what the code is already doing.

So let's be consistent and consistent in the same switch statement about documenting fall-throughs.

WFM

RTBC + 1. My phpcs output is clean now, looks like my phpcs installation was messed in my last comment - sorry.

Random test fail, back to RTBC.

Filed #2790685: FieldHandlersUpdateTest has random test fails.

This is eligible for commit during rc - unfortunately it is not possible to apply #54 to 8.2. Going to roll a patch for that.

Patches for both branches - 8.3 one is same as #54.

Some random failure in the meantime

I got this seemingly unrelated coding standards fail when committing:

FILE: ...core/tests/Drupal/Tests/Core/Form/FormStateDecoratorBaseTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 274 | ERROR | Parameter type contains illegal character "{"
----------------------------------------------------------------------

But fixed it on commit (to both 8.3.x and 8.2.x).