Closed (fixed)
Project:
Drupal core
Version:
8.3.x-dev
Component:
base system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
4 Mar 2017 at 10:07 UTC
Updated:
18 Mar 2017 at 22:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
alexpottHere are patches to fix both 8.3.x and 8.4.x and make the phpcs checks pass on both branches.
Comment #3
klausiI can confirm that the phpcs output is empty again after this patch.
Comment #4
xjmAha, 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.):
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?
Comment #5
xjmI 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.
Comment #6
xjmRetitling because issue numbers in titles do strange things to the VCS integration, and saving credit.
Comment #7
xjmCommitted 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.
Comment #8
xjmTagging for @klausi to evaluate whether there should be a coder followup (see #4 and #5).
Comment #9
almaudoh commentedThese would also need correction...
Comment #10
xjm@almaudoh, hm, I'm not getting any PHPCS fails on that file on either branch? Running:
Produces no output. If I add an unused use just to confirm the standards are being applied:
Comment #11
xjmI can confirm the trailing comma is there in the file, but it's not raising an error somehow.
Comment #12
xjmOh! 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.Comment #13
almaudoh commentedYeah, the sniff would need to be updated to catch that.
Comment #14
klausiThe 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.
Comment #15
xjm@klausi Ah, that's helpful, thanks! I'll add note of that to the committer instructions as well.