#2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase introduce a few coding standards errors. Lets use phpcbf to fix them on both 8.3.x and 8.4.x.

There was one unused use too...

CommentFileSizeAuthor
#2 2857822-8.4-2.patch19.68 KBalexpott
#2 2857822-8.3-2.patch20.35 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
StatusFileSize
new20.35 KB
new19.68 KB

Here are patches to fix both 8.3.x and 8.4.x and make the phpcs checks pass on both branches.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

I can confirm that the phpcs output is empty again after this patch.

xjm’s picture

Aha, I did note these when I committed the patch, but mistakenly thought they were in HEAD as well. I actually even parsed the messages out of the commit output and saved them in a file to look at today. Lots of (e.g.):

FILE: .../maintainer/core/modules/book/tests/src/Unit/BookManagerTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
105 | ERROR | [x] Expected one space after the comma, 0 found
 ----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY

The diff of course makes it clear how they were introduced by the array conversion. Do we need a small followup to fix phpcbf to handle these cases?

xjm’s picture

I guess the expectation for the fixer might be "run phpcs -> phpcbf -> phpcs -> phpcbf repeatedly until you have no more errors" (which I've had to do with unused use before) so maybe a followup isn't necessary.

xjm’s picture

Title: Fix coding standards issues introduced mostly by 2776975 » Fix coding standards issues introduced mostly by array syntax conversion

Retitling because issue numbers in titles do strange things to the VCS integration, and saving credit.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.4.x and 8.3.x. Thanks!

I accidentally committed the wrong patch to 8.3.x, so I just also pushed a followup commit to remove the additional unused use in that branch.

xjm’s picture

Issue tags: +Needs followup

Tagging for @klausi to evaluate whether there should be a coder followup (see #4 and #5).

almaudoh’s picture

+++ b/core/modules/ckeditor/tests/modules/src/Kernel/CKEditorTest.php
@@ -463,23 +463,23 @@ protected function getDefaultToolbarConfig() {
-        'items' => ['Source',],
+        'items' => ['Source', ],

+++ b/core/modules/field_ui/tests/src/Kernel/EntityDisplayTest.php
@@ -129,7 +129,7 @@ public function testEntityDisplayCRUDSort() {
-    $expected =  [ 0 => 'component_1', 1 => 'component_2', 2 => 'component_3', 'name'];
+    $expected = [ 0 => 'component_1', 1 => 'component_2', 2 => 'component_3', 'name'];

+++ b/core/tests/Drupal/Tests/Component/Gettext/PoHeaderTest.php
@@ -279,7 +279,7 @@ public function providerTestPluralsFormula() {
-        ],],
+        ], ],

These would also need correction...

xjm’s picture

@almaudoh, hm, I'm not getting any PHPCS fails on that file on either branch? Running:

vendor/bin/phpcs core/modules/ckeditor/tests/modules/src/Kernel/CKEditorTest.php --standard="core/phpcs.xml.dist"

Produces no output. If I add an unused use just to confirm the standards are being applied:

vendor/bin/phpcs core/modules/ckeditor/tests/modules/src/Kernel/CKEditorTest.php --standard="core/phpcs.xml.dist"

FILE: .../core/modules/ckeditor/tests/modules/src/Kernel/CKEditorTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 9 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

xjm’s picture

I can confirm the trailing comma is there in the file, but it's not raising an error somehow.

xjm’s picture

Oh! I get it. They were fixed incorrectly, to add trailing spaces rather than removing trailing commas. So something should catch e.g. , ], and fail on that too.

almaudoh’s picture

So something should catch e.g. , ], and fail on that too.

Yeah, the sniff would need to be updated to catch that.

klausi’s picture

Issue tags: -Needs followup

The problem is that Drupal core only enables a subset of the Drupal coding standard in phpcs.xml.dist. That's why the fixer is not effective in some cases where a combination of sniffs is necessary to fix things properly.

So all the weirdness you mention above is properly fixed when you run a global phpcbf --standard=Drupal on the files in question :)
If you still find cases that are not fixed with that please report in the Coder issue queue with examples!

So I think no additional follow-ups are needed here, expanding our phpcs.xml.dist is already covered in #2571965: [meta] Fix PHP coding standards in core, stage 1.

xjm’s picture

@klausi Ah, that's helpful, thanks! I'll add note of that to the committer instructions as well.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.