Fix 'Drupal.ControlStructures.InlineControlStructure' 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.

This does not comply with Drupal's coding standard. The closing brace should be on it's own line and there should be a newline after the opening brace.

+++ b/core/lib/Drupal/Core/FileTransfer/Form/FileTransferAuthorizeForm.php
@@ -69,7 +69,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    if ($authorize_filetransfer_default = $form_state->getValue(array('connection_settings', 'authorize_filetransfer_default'))) {
+    }
     elseif ($authorize_filetransfer_default = $this->config('system.authorize')->get('filetransfer_default'));

The elseif should be fixed as well. I didn't run phpcs locally, so not sure if that would have been caught or not.

In any case we should fix the phpcs.xml.dist file to remove this sniff from the ignore section.

patch outdated

New patch with removed Drupal.ControlStructures.InlineControlStructure sniff from phpcs.xml.dist file.

1. +++ b/core/lib/Drupal/Component/Utility/Unicode.php
   @@ -269,7 +269,8 @@ public static function truncateBytes($string, $len) {
   -    while (--$len >= 0 && ord($string[$len]) >= 0x80 && ord($string[$len]) < 0xC0);
   +    while (--$len >= 0 && ord($string[$len]) >= 0x80 && ord($string[$len]) < 0xC0) {
   +    }
   
   +++ b/core/lib/Drupal/Core/FileTransfer/Form/FileTransferAuthorizeForm.php
   @@ -69,8 +69,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -    if ($authorize_filetransfer_default = $form_state->getValue(array('connection_settings', 'authorize_filetransfer_default')));
   -    elseif ($authorize_filetransfer_default = $this->config('system.authorize')->get('filetransfer_default'));
   +    if ($authorize_filetransfer_default = $form_state->getValue(array('connection_settings', 'authorize_filetransfer_default'))) {
   +    }
   

   looks that needs follow-up to fix wtf

FILE: ...k/dev/drupal/core/modules/simpletest/src/TestServiceProvider.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 39 | ERROR | [x] Inline control structures are not allowed
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

We've got an new one :)

New patch.
Thank you Alexpott
RTBC b/c interdiff really small and clear

1. +++ b/core/lib/Drupal/Component/Utility/Unicode.php
   @@ -269,7 +269,8 @@ public static function truncateBytes($string, $len) {
   -    while (--$len >= 0 && ord($string[$len]) >= 0x80 && ord($string[$len]) < 0xC0);
   +    while (--$len >= 0 && ord($string[$len]) >= 0x80 && ord($string[$len]) < 0xC0) {
   +    }
   
   +++ b/core/modules/simpletest/src/TestServiceProvider.php
   @@ -36,7 +36,7 @@ public function alter(ContainerBuilder $container) {
   -        for ($id = $original_id; $container->hasAlias($id); $id = (string) $container->getAlias($id));
   +        for ($id = $original_id; $container->hasAlias($id); $id = (string) $container->getAlias($id)) {}
   

   I'm not convinced that these changes are an improvement - is there a defined coding standard for this?

2. +++ b/core/lib/Drupal/Core/FileTransfer/Form/FileTransferAuthorizeForm.php
   @@ -69,8 +69,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -    if ($authorize_filetransfer_default = $form_state->getValue(array('connection_settings', 'authorize_filetransfer_default')));
   -    elseif ($authorize_filetransfer_default = $this->config('system.authorize')->get('filetransfer_default'));
   +    if ($authorize_filetransfer_default = $form_state->getValue(array('connection_settings', 'authorize_filetransfer_default'))) {
   +    }
   +    elseif ($authorize_filetransfer_default = $this->config('system.authorize')->get('filetransfer_default')) {
   +    }
        else {
          $authorize_filetransfer_default = key($available_backends);
        }
   

   Perhaps this is better as...

       $authorize_filetransfer_default = $form_state->getValue(array('connection_settings', 'authorize_filetransfer_default')) ?: $this->config('system.authorize')->get('filetransfer_default');
       $authorize_filetransfer_default = $authorize_filetransfer_default ?: key($available_backends);
   

So for point 1...

Always use curly braces even in situations where they are technically optional. Having them increases readability and decreases the likelihood of logic errors being introduced when new lines are added. The opening curly should be on the same line as the opening statement, preceded by one space. The closing curly should be on a line by itself and indented to the same level as the opening statement.

From our coding standards... so...

+++ b/core/modules/simpletest/src/TestServiceProvider.php
@@ -36,7 +36,7 @@ public function alter(ContainerBuilder $container) {
-        for ($id = $original_id; $container->hasAlias($id); $id = (string) $container->getAlias($id));
+        for ($id = $original_id; $container->hasAlias($id); $id = (string) $container->getAlias($id)) {}

The closing } needs to be on a new line.

The closing } needs to be on a new line.

the rule needs tuning or using other sniffers to prevent

PS: reroll

1. +++ b/core/lib/Drupal/Component/Utility/Unicode.php
   @@ -264,7 +264,7 @@ public static function truncateBytes($string, $len) {
   -    while (--$len >= 0 && ord($string[$len]) >= 0x80 && ord($string[$len]) < 0xC0);
   +    while (--$len >= 0 && ord($string[$len]) >= 0x80 && ord($string[$len]) < 0xC0) {}
   

   I'm not sure this is a good change. Maybe we should fix the rule and check what the real standards are.

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -489,8 +489,8 @@ public function makeSequenceName($table, $field) {
   +    if (empty($comments)) {
   +      return ''; }
   
   +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -497,8 +497,8 @@ public function fetchCol($index = 0) {
   +    if (!isset($this->columnNames[$key_index]) || !isset($this->columnNames[$value_index])) {
   +      return array(); }
   

   Let's put the close on a new line.

3. +++ b/core/lib/Drupal/Core/FileTransfer/Form/FileTransferAuthorizeForm.php
   @@ -64,8 +64,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -    if ($authorize_filetransfer_default = $form_state->getValue(array('connection_settings', 'authorize_filetransfer_default')));
   -    elseif ($authorize_filetransfer_default = $this->config('system.authorize')->get('filetransfer_default'));
   +    if ($authorize_filetransfer_default = $form_state->getValue(array('connection_settings', 'authorize_filetransfer_default'))) {}
   +    elseif ($authorize_filetransfer_default = $this->config('system.authorize')->get('filetransfer_default')) {}
   

   This looks super awkward and is fixed in #842620: Update manager can't install modules using FTP due broken FileTransferAuthorizeForm

4. +++ b/core/modules/color/color.module
   @@ -795,9 +795,9 @@ function _color_hsl2rgb($hsl) {
   +  if ($h * 6 < 1) { return $m1 + ($m2 - $m1) * $h * 6; }
   +  if ($h * 2 < 1) { return $m2; }
   +  if ($h * 3 < 2) { return $m1 + ($m2 - $m1) * (0.66666 - $h) * 6; }
   
   @@ -821,9 +821,9 @@ function _color_rgb2hsl($rgb) {
   +    if ($max == $r && $max != $g) { $h += ($g - $b) / $delta; }
   +    if ($max == $g && $max != $b) { $h += (2 + ($b - $r) / $delta); }
   +    if ($max == $b && $max != $r) { $h += (4 + ($r - $g) / $delta); }
   

   Let's format these properly.

5. +++ b/core/modules/simpletest/src/TestServiceProvider.php
   @@ -41,7 +41,7 @@ public static function addRouteProvider(ContainerBuilder $container) {
   -      for ($id = $original_id; $container->hasAlias($id); $id = (string) $container->getAlias($id));
   +      for ($id = $original_id; $container->hasAlias($id); $id = (string) $container->getAlias($id)) {}
   

   As above - I think this change is not a good one.

6. +++ b/core/modules/taxonomy/tests/src/Unit/Menu/TaxonomyLocalTasksTest.php
   @@ -25,7 +25,7 @@ public function testTaxonomyPageLocalTasks($route, $subtask = array()) {
   +    if ($subtask) { $tasks[] = $subtask; }
   
   +++ b/core/modules/user/tests/src/Unit/Menu/UserLocalTasksTest.php
   @@ -69,7 +69,7 @@ public function testUserPageLocalTasks($route, $subtask = array()) {
   +    if ($subtask) { $tasks[] = $subtask; }
   
   +++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeBundleInfoTest.php
   @@ -136,7 +136,7 @@ protected function setUpEntityTypeDefinitions($definitions = []) {
   +        else { throw new PluginNotFoundException($entity_type_id); }
   
   +++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php
   @@ -112,7 +112,7 @@ protected function setUpEntityTypeDefinitions($definitions = []) {
   +        else { throw new PluginNotFoundException($entity_type_id); }
   
   +++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeRepositoryTest.php
   @@ -71,7 +71,7 @@ protected function setUpEntityTypeDefinitions($definitions = []) {
   +        else { throw new PluginNotFoundException($entity_type_id); }
   
   +++ b/core/tests/Drupal/Tests/Core/Field/FieldDefinitionListenerTest.php
   @@ -101,7 +101,7 @@ protected function setUpEntityManager($definitions = array()) {
   +        else { throw new PluginNotFoundException($entity_type_id); }
   

   Let's format these properly.

Done changes as per #21.

Thanks!!

+++ b/core/phpcs.xml.dist
@@ -61,7 +61,7 @@
-  <rule ref="Drupal.ControlStructures.ControlSignature"/>
+  <rule ref="Drupal.ControlStructures.InlineControlStructure"/>

Shouldn't be removing a rule here.

adding rule.

Please find the attached patch.

Please find the updated patch #29.

@pashupathi nath gajawada

Please provide the interdiff.

I have done the changes as per said in #26.

Thanks!!

HI @Rajeshwari Variar,

I have done the changes as suggested by #23 suggested by Alex Pott.

Thanks,

$ git apply 2572699-29_0.patch 
2572699-29_0.patch:48: trailing whitespace.
    return $m2;	
warning: 1 line adds whitespace errors.

Right here:

+++ b/core/modules/color/color.module
@@ -795,10 +795,15 @@ function _color_hsl2rgb($hsl) {
+    return $m2;	

Removed whitespace errors.

Thanks!!

This patch does NOT fixes all instances of the rule:

$ phpcs --standard=core/phpcs.xml.dist core

FILE: ...rs/dawehner/www/d8/core/lib/Drupal/Component/Utility/Unicode.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 267 | ERROR | [x] Inline control structures are not allowed
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: ...ehner/www/d8/core/modules/simpletest/src/TestServiceProvider.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 44 | ERROR | [x] Inline control structures are not allowed
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 2 mins, 9.51 secs; Memory: 129Mb

1. +++ b/core/modules/color/color.module
   @@ -821,11 +826,16 @@ function _color_rgb2hsl($rgb) {
      }
   -
      return array($h, $s, $l);
    }
   

   Nitpick: Unneeded change

2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeBundleInfoTest.php
   @@ -136,7 +136,9 @@ protected function setUpEntityTypeDefinitions($definitions = []) {
   -        else throw new PluginNotFoundException($entity_type_id);
   +        else {
   +          throw new PluginNotFoundException($entity_type_id);
   +        }
   
   +++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php
   @@ -112,7 +112,9 @@ protected function setUpEntityTypeDefinitions($definitions = []) {
   -        else throw new PluginNotFoundException($entity_type_id);
   +        else {
   +          throw new PluginNotFoundException($entity_type_id);
   +        }
   

   Wow, I did not even know that this was valid syntax :)

Re-roll & updates.

There are new phpcs fails was commited

FILE: ...drupal/core/tests/Drupal/Tests/Core/Logger/LoggerChannelTest.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------
 116 | ERROR | [x] Space found before square bracket; expected
     |       |     "$cases[" but found "$cases ["
 123 | ERROR | [x] Space found before square bracket; expected
     |       |     "$cases[" but found "$cases ["
 131 | ERROR | [x] Space found before square bracket; expected
     |       |     "$cases[" but found "$cases ["
 138 | ERROR | [x] Space found before square bracket; expected
     |       |     "$cases[" but found "$cases ["
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...b/drupal/core/modules/node/src/Plugin/migrate/source/d6/Node.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 217 | ERROR | [x] Space found after square bracket; expected
     |       |     "[$field" but found "[ $field"
 217 | ERROR | [x] Space found before square bracket; expected "]]"
     |       |     but found "] ]"
 217 | ERROR | [x] Space found after square bracket; expected
     |       |     "[$field" but found "[ $field"
 217 | ERROR | [x] Space found before square bracket; expected "]]"
     |       |     but found "] ]"
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Please fix them too

We're fixing inline control structures here, not array brackets.

I reviewed the patch in #38 this way:

$ cd path/to/drupal
$ git apply [patch]
$ composer install
$ cd core
$ ../vendor/bin/phpcs -ps

This results in 0 errors, up until the process hangs at 96% which I believe is because of my ancient crappy machine. :-) Update: I just didn't let it run long enough. I wonder what file is doing that.

However, we also see that the testbot tells us that no errors occurred, and since a changed phpcs.xml.dist file will result in a testbot sniff of the whole codebase, we're safe. (Search the console output for 'PHPCS config file modified, sniffing entire project.')

The patch adds:

+++ b/core/phpcs.xml.dist
@@ -65,6 +65,7 @@
+  <rule ref="../vendor/drupal/coder/coder_sniffer/Drupal/Sniffs/ControlStructures/InlineControlStructureSniff.php"/>

So if any errors remained, this would result in error reports within the scope of this issue.

+++ b/core/lib/Drupal/Component/Utility/Unicode.php
@@ -264,7 +264,8 @@ public static function truncateBytes($string, $len) {
     // Scan backwards to beginning of the byte sequence.
-    while (--$len >= 0 && ord($string[$len]) >= 0x80 && ord($string[$len]) < 0xC0);
+    while (--$len >= 0 && ord($string[$len]) >= 0x80 && ord($string[$len]) < 0xC0) {
+    }

+++ b/core/modules/simpletest/src/TestServiceProvider.php
@@ -41,7 +41,8 @@ public static function addRouteProvider(ContainerBuilder $container) {
       // While $container->get() does a recursive resolve, getDefinition() does
       // not, so do it ourselves.
-      for ($id = $original_id; $container->hasAlias($id); $id = (string) $container->getAlias($id));
+      for ($id = $original_id; $container->hasAlias($id); $id = (string) $container->getAlias($id)) {
+      }

These are kind of weird, and are left over from #21.1 and #21.5. We shouldn't rely on side effects to control the loop. I'll say this patch fixes the CS errors within scope, but we might unwrap those loops for readability either here or in a follow-up.

Ah... yes
Rechecked again
Confirm that no errors after patch applying

@Mile23 yeah the long wait at 96% bugged me too - see #2911280: RectangleTest.php takes a very long time to scan for coding standards

Agreed we need a follow-up for #21.1 and #21.5 - I think we should both open that and add a @todo here so marking CNW for that. It's possibly not in scope for the coding standards change to add the @todo, but it's exposed some very unreadable code.

I added #2911497: Make \Drupal\Component\Utility\Unicode::truncateBytes more readable and #2911498: Make TestServiceProvider more readable (cleanup).
Here is the updated patch and the interdiff.

I think the test error relates to this: #2857843: Random fail in Drupal\KernelTests\Core\Entity\ContentEntityChangedTest::testChanged.

no phpcs fails after patch applying
patch contains only sniff related changes
and include @todo comments for #21.1 and #21.5
with links to proper issues.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Looks it was not pushed to 8.4.x

@catch please cherry pick to 8.4.x

I can see 20a43f60d9 commit on 8.4.x, so looks ok, although it doesn't appear here in the issue. But that may be another problem.