#41057 closed task (blessed) (fixed)
Update PHP codebase per WordPress PHP Coding Standards
Reported by: | netweb | Owned by: | pento |
---|---|---|---|
Milestone: | 5.1 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | early commit |
Focuses: | coding-standards | Cc: |
Description (last modified by )
โhttps://make.wordpress.org/core/handbook/contribute/code-refactoring/
"That said, we want to be internally consistent and follow our own rules. Code is poetry, and our code should be beautiful."
โhttps://make.wordpress.org/core/handbook/best-practices/coding-standards/php/
As a first pass there are 8 files to this ticket for the initial patch, 4 patches, and 4 file lists of the files to be patched, these files lists exclude and 3rd party external PHP libraries so that the only files changed are WordPress' PHP files
41057-src-root.diff
generated from the41057-filelist-src-root.txt
PHP file list41057-src-wp-admin.diff
generated from the41057-filelist-src-wp-admin.txt
PHP file list41057-src-wp-content.diff
generated from the41057-filelist-src-wp-content.txt
PHP file list41057-src-wp-includes.diff
generated from the41057-filelist-src-wp-includes.txt
PHP file list
Each of the above diff/patches are generated programmatically by phpcbf
part of phpcs
using the WordPress-Core
version 0.11.0
PHP Codesniffer ruleset from โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards
phpcbf --file-list=41057-filelist-src-root.txt --standard=WordPress-Core --extensions=php
phpcbf --file-list=41057-filelist-src-wp-admin.txt --standard=WordPress-Core --extensions=php
phpcbf --file-list=41057-filelist-src-wp-content.txt --standard=WordPress-Core --extensions=php
phpcbf --file-list=41057-filelist-src-wp-includes.txt --standard=WordPress-Core --extensions=php
Attachments (26)
Change History (151)
This ticket was mentioned in โSlack in #core by pento. โView the logs.
7 years ago
#3
follow-up:
โย 5
@
7 years ago
We sure we want to be doing this? It's not currently done in core with any consistency, and seems noisy. Any good code editor does brace highlighting (and collapsing).
}// End foreach().
#4
follow-up:
โย 7
@
7 years ago
Pinging the โhttps://github.com/orgs/WordPress-Coding-Standards team members:
cc @westonruter, @garyj, @jdgrimes, @rarst, and @jrf
As noted in the ticket above the patches have been generated using WPCS 0.11.0
There's currently 8 open issues for the โ`0.12.0` milestone
What's the WPCS' team thoughts on this?
Options:
- Patch WordPress now per
0.11.0
and then follow up further once0.12.0
is released
or
- Wait until
0.12.0
is released and update the patches here?
#5
in reply to:
โย 3
;
follow-up:
โย 12
@
7 years ago
Replying to markjaquith:
We sure we want to be doing this? It's not currently done in core with any consistency, and seems noisy. Any good code editor does brace highlighting (and collapsing).
}// End foreach().
That rule goes all the way back to the โoriginal coding standards, 12 years old tomorrow. It was pretty clearly added for a time when code editors didn't have nice features like bracket matching.
I think we can safely remove it.
#6
@
7 years ago
Looking at 41057-src-wp-content.diffโ and array indentation not being correct via WPCS 0.11.0 lets switch to the develop branch so that we can include fixes such as โ#941: New sniff to verify correct array indentation that are part of the upcoming 0.12.0
milestone
Using "wp-coding-standards/wpcs": "dev-develop"
in your composer.json
should get you the WPCS develop
branch
#7
in reply to:
โย 4
;
follow-up:
โย 8
@
7 years ago
Replying to netweb:
Pinging the โhttps://github.com/orgs/WordPress-Coding-Standards team members:
cc @westonruter, @garyj, @jdgrimes, @rarst, and @jrf
As noted in the ticket above the patches have been generated using WPCS
0.11.0
There's currently 8 open issues for the โ`0.12.0` milestone
What's the WPCS' team thoughts on this?
Options:
- Patch WordPress now per
0.11.0
and then follow up further once0.12.0
is releasedor
- Wait until
0.12.0
is released and update the patches here?
WPCS will not cover everything in the handbook yet either way. It looks like the main relevant feature in develop
is array indentation.
The rules from the handbook that aren't covered yet are noted in โWordPress-Core/ruleset.xml. A glance didn't reveal anything that looked particularly important.
#8
in reply to:
โย 7
@
7 years ago
Replying to jdgrimes:
The rules from the handbook that aren't covered yet are noted in โWordPress-Core/ruleset.xml. A glance didn't reveal anything that looked particularly important.
Agreed, I'm fine with the missing rules.
#10
@
7 years ago
The commit here should include a phpcs.xml
that does the requisite exclusions for 3rd-party PHP, as well as declaring that the WordPress-Core
standard is being used.
#12
in reply to:
โย 5
@
7 years ago
Replying to pento:
I think we can safely remove it.
I agree with Gary & Mark about not continuing this one. Code editors 12 years ago did have bracket matching, but back then it was popular to document the beginning & end of large HTML blocks, and this was carried over from that.
As far as large coding standards related changes go:
- We can minimize the number of times theyโre necessary by enforcing the standards for every comimit after these go in.
- We can evaluate the impact of new coding standards changes regularly and keep the codebase up to date with them.
- We shouldnโt be afraid to to do either of those things.
Thereโs nothing egregious in our current coding standards โ that I can find โ that is reason enough to block this. Itโs a million times better, and I expect the positive response when this goes in will be huge.
#13
follow-up:
โย 15
@
7 years ago
The โCoding Standards now no longer have a rule about blocks longer than 35 characters.
#15
in reply to:
โย 13
@
7 years ago
Replying to pento:
The โCoding Standards now no longer have a rule about blocks longer than 35 characters.
There's โa ticket for fixing the ruleset.
#16
follow-up:
โย 17
@
7 years ago
1) grunt precommit
needs to be updated to take this into account. I would consider this a blocker.
2) Can we get a pre commit hook on the svn server to block commits that don't have the proper format?
#17
in reply to:
โย 16
;
follow-ups:
โย 18
โย 19
@
7 years ago
Replying to jorbin:
2) Can we get a pre commit hook on the svn server to block commits that don't have the proper format?
This feels like something we might want to wait on, as we fine-tune our WPCS preferences. That is, we should probably keep the ability to have exceptions to the rules while we become accustomed to the rules.
#18
in reply to:
โย 17
;
follow-up:
โย 20
@
7 years ago
Replying to boonebgorges:
Replying to jorbin:
2) Can we get a pre commit hook on the svn server to block commits that don't have the proper format?
This feels like something we might want to wait on, as we fine-tune our WPCS preferences. That is, we should probably keep the ability to have exceptions to the rules while we become accustomed to the rules.
If we need exceptions, we can mark that in the code with comments. see โhttps://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-parts-of-a-file
#19
in reply to:
โย 17
@
7 years ago
Replying to boonebgorges:
This feels like something we might want to wait on, as we fine-tune our WPCS preferences. That is, we should probably keep the ability to have exceptions to the rules while we become accustomed to the rules.
I was just typing this, too. I agree that a grace period is probably a good idea, even with the exception handling nuances.
If it is full-on right away, someone should be available to tune it as issues show up. The bounce-back from a commit rejection is jarring, and the feedback isn't always obvious, and may not be 100% accurate while we are still in the implementation phase.
#20
in reply to:
โย 18
@
7 years ago
Replying to jorbin:
If we need exceptions, we can mark that in the code with comments. see โhttps://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-parts-of-a-file
This is probably good enough, as long as grunt precommit
runs the sniffs and provides enough info to committers so that they can avoid the frustration of getting blocked by SVN.
#21
follow-up:
โย 22
@
7 years ago
@netweb Thanks for the ping.
Just a couple of notes:
- Please make sure you use at least PHPCS 2.9.0 or 2.9.1 when generating the patch files. These versions contain a fix for an issue with tabs vs spaces to indent lines which could be relevant. See โPHPCS PR 1404. Those versions also contain numerous other fixes which make the sniffs WPCS uses from upstream as well as some WPCS sniffs which use token arrays as defined in PHPCS, better, so could yield better results.
- Yes, please use the current WPCS
develop
branch as it contains two notable new sniffs which are likely to be relevant: as mentioned before โWPCS PR 941 for the array indentation, but also โWPCS PR 942 which checks that spaces are used for mid-line alignment.
- I'd strongly advise visual verification of the generated patches. I already noticed some fixes which don't look right in the above attached patches - for instance in
src/wp-content/themes/twentyeleven/404.php
-. As I'm unclear what versions of PHPCS has been used to generated these patches, these issues may have been addressed already, but that would need to be verified. Please report any bugs you find to โWPCS and I'll / we'll try to address them as soon as possible.
The commit here should include a
phpcs.xml
that does the requisite exclusions for 3rd-party PHP, as well as declaring that the WordPress-Core standard is being used.
- You may want to consider adding the
WordPress-Docs
ruleset as well. This ruleset is not complete yet, but what it does cover is in the Core PHP Documentation Handbook page and a number of sniffs included there also include auto-fixers.
If we need exceptions, we can mark that in the code with comments. see โhttps://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-parts-of-a-file
- I'd strongly advise to use these type of comments as sparingly as possible. This will turn PHPCS off completely for those lines, not just for the one sniff which may be problematic.
- As a side-note to this: PHPCS 3.x (with which WPCS is not - yet - compatible) contains a command line option
--ignore-annotations
to ignore any such annotations. This will be useful in the future to review from time to time if any such annotations are used and if so, if these are still needed.
Should we also update
.travis.yml
to start running PHPCS as well?
- Not all issues which will be identified can be auto-fixed. As this ticket - so far - only contains patches for things which can be auto-fixed, there will probably still be a list of violations after these patches have gone in. Those would need to be addressed before activating an SVN commit hook / travis build script change, otherwise all builds will fail / commits be rejected.
- Protip: please run PHPCS only once per build preferably against a reasonably high PHP version (PHP 5.5 or up). All sniffs are unit tested against all PHP versions, so running PHPCS against each build would just return the same results each time. For an example setup, see: โhttps://github.com/jrfnl/make-phpcs-work-for-you/tree/master/travis-examples
- Side-note: PHPCS 3.x contains the ability to create filters and comes with a build-in filter
gitmodified
. While this is not that useful while WP development is still using SVN, this may be good to keep in mind for if/when development would move to a Git-based system. The filter causes PHPCS to only report on violations found in newly added and changed files within a commit.
There's a ticket for fixing the ruleset.
- And a PR has been created & merged since, so the "long conditional end comments" sniff has effectively been removed from WPCS
develop
.
Can we get a (grunt) pre commit hook on the svn server to block commits that don't have the proper format?
- The WPCS repository contains an example for a pre-commit hook which might be useful as inspiration for this: โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/bin/pre-commit
#22
in reply to:
โย 21
@
7 years ago
Replying to jrf:
@netweb Thanks for the ping.
No problem, and thank you @jrf for the momentous contributions to WPCS
Just a couple of notes:
- Please make sure you use at least PHPCS 2.9.0 or 2.9.1 when generating the patch files. These versions contain a fix for an issue with tabs vs spaces to indent lines which could be relevant. See โPHPCS PR 1404. Those versions also contain numerous other fixes which make the sniffs WPCS uses from upstream as well as some WPCS sniffs which use token arrays as defined in PHPCS, better, so could yield better results.
- Yes, please use the current WPCS
develop
branch as it contains two notable new sniffs which are likely to be relevant: as mentioned before โWPCS PR 941 for the array indentation, but also โWPCS PR 942 which checks that spaces are used for mid-line alignment.
- I'd strongly advise visual verification of the generated patches. I already noticed some fixes which don't look right in the above attached patches - for instance in
src/wp-content/themes/twentyeleven/404.php
-. As I'm unclear what versions of PHPCS has been used to generated these patches, these issues may have been addressed already, but that would need to be verified. Please report any bugs you find to โWPCS and I'll / we'll try to address them as soon as possible.
The patches I attached here were generated using PHPCS 2.9.1
and WPCS 0.11.0
, we've now switched to using WPCS develop
which addresses the nested arrays issue you noticed in Twenty Eleven's 404.php
Using WPCS develop
a few files are no longer fixable on my local setup that were fixed when using WPCS 0.11.0
, currently the two instances of this is for src/wp-content/themes/twentyeleven/404.php
and src/wp-content/themes/twentyeleven/functions.php
files. I'll go through and audit this list of files later today
The commit here should include a
phpcs.xml
that does the requisite exclusions for 3rd-party PHP, as well as declaring that the WordPress-Core standard is being used.
For PHPUnit we use the .dist
configuration file, likewise we should use PHPCS' phpcs.xml.dist
to allow developers to define/test any alternate configurations locally.
- You may want to consider adding the
WordPress-Docs
ruleset as well. This ruleset is not complete yet, but what it does cover is in the Core PHP Documentation Handbook page and a number of sniffs included there also include auto-fixers.
Let's not add this at this stage, the PHP Docs can be modified at anytime, code churn is not an issue for the docs.
Should we also update
.travis.yml
to start running PHPCS as well?
- Not all issues which will be identified can be auto-fixed. As this ticket - so far - only contains patches for things which can be auto-fixed, there will probably still be a list of violations after these patches have gone in. Those would need to be addressed before activating an SVN commit hook / travis build script change, otherwise all builds will fail / commits be rejected.
- Protip: please run PHPCS only once per build preferably against a reasonably high PHP version (PHP 5.5 or up). All sniffs are unit tested against all PHP versions, so running PHPCS against each build would just return the same results each time. For an example setup, see: โhttps://github.com/jrfnl/make-phpcs-work-for-you/tree/master/travis-examples
Initially we can add a single new job to Travis CI that is "allowed to fail" using PHP 7.0
- Side-note: PHPCS 3.x contains the ability to create filters and comes with a build-in filter
gitmodified
. While this is not that useful while WP development is still using SVN, this may be good to keep in mind for if/when development would move to a Git-based system. The filter causes PHPCS to only report on violations found in newly added and changed files within a commit.
Excellent, we can still utilise this as part of #34694
Can we get a (grunt) pre commit hook on the svn server to block commits that don't have the proper format?
- The WPCS repository contains an example for a pre-commit hook which might be useful as inspiration for this: โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/bin/pre-commit
As there are core committers using SVN and Git via git svn dcommit
to commit a pre-commit hook should this into consideration, and hopefully support both methods
This ticket was mentioned in โSlack in #core by pento. โView the logs.
7 years ago
#24
@
7 years ago
It was asked in Slack whether this will be back ported to to avoid the need for multiple patches for point releases? What are your plans @pento
I'm for back porting to 4.8, FWIW. I don't think it's necessary to go back any further.
#25
follow-up:
โย 29
@
7 years ago
It won't be backported at all. If we backport it to the 4.8 branch, then 4.8.1 will be a huge minor release, with a significantly increased failure rate.
Unfortunately, that does mean that most patches from now on will need to be rewritten to go further back than 4.8. After 4.9 is released, that will only be security patches.
#26
@
7 years ago
phpcs.xml.distโ is what I have so far. The WordPress.Arrays.ArrayIndentation
rule is to work around โthis issue, but it seems there are a few more edge cases to be tackled.
If you run phpcbf
with WPCS develop and PHPCS 2.9.1, you'll see a handful of files that aren't fixed, it seems due to clashing indentation rules.
There've been some instances of a line in an else {}
clause being moved up to the same line as the else
, you can see them with this search: ack 'else {.+$' --php
.
I've also noticed some cases of massive indents being added, which isn't ideal.
#27
@
7 years ago
@pento May I suggest to document the exclusions (both files as well as rules) to make life easier for the next person ?
Also, you may want to change this part:
<rule ref="WordPress-Core" /> <rule ref="WordPress.Arrays.ArrayIndentation"> <exclude-pattern>*</exclude-pattern> </rule>
to:
<rule ref="WordPress-Core"> <!-- Exclude one rule, but only for the fixer. --> <!-- Bug report: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/968 --> <exclude phpcbf-only="true" name="WordPress.Arrays.ArrayIndentation" /> </rule>
For some more tips & tricks, you may want to leaf through this slide deck: โhttps://speakerdeck.com/jrf/dont-work-for-phpcs-make-phpcs-work-for-you-1
#28
@
7 years ago
All:
FYI: I've made a small start with a visual inspection of the fixes and - while the majority of the fixes is good -, I've already found a number of additional issues other than the one previously reported.
I will open issues for each of these in the GH repos of WPCS and - where relevant - upstream in PHPCS.
Regarding the fixes needed for PHPCS native sniffs, the timing is not ideal.
WPCS is currently only compatible with PHPCS 2.x, but as PHPCS 3.x has been released last month, it may well be that fixes will no longer be accepted for the PHPCS 2.x branch.
I'll open a discussion about this in the PHPCS repo and will refer to this issue as the reason why we'd like to see the fixes be accepted into the PHPCS 2.x branch.
I've created a branch in my GH fork of WP to keep track of the fixes I deem correct/(partially) faulty which can be found here: โhttps://github.com/jrfnl/develop.wordpress/commits/test/WPCS-autofixing
It would be great if more people could spend some time on visual inspections & reporting any bugs found, as that would allow me to spend time fixing things instead.
New issues opened so far:
โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/970Fixedโhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/971PR open upstream
[Edit] and here are some more:
- โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/972 will fix issue โWPCS #970 for now
- โhttps://github.com/squizlabs/PHP_CodeSniffer/issues/1512 - to discuss how to permanently fix issue โWPCS #970 upstream
- โhttps://github.com/squizlabs/PHP_CodeSniffer/pull/1514 and โhttps://github.com/squizlabs/PHP_CodeSniffer/pull/1515 will fix issue โWPCS #971 upstream in both the PHPCS 2.x as well as the3.x branch.
- โPHPCS PR #1514 contains the discussion opener on accepting these fixes into PHPCS 2.x
[Edit 2] and yet more:
- โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/973
- โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/974
โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/975Fixedโhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/976PR open- โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/977
And just in case someone is now thinking "Cricky, PHPCS is really buggy, shouldn't we use something else ?"... The majority of the fixes - I'd wager > 95% - is correct. Using PHPCS on WP core is, however, in a way a stress-test of PHPCS, we're really getting into all the nooks and crannies, so I'm not surprised we're finding bugs.
Current status of visual verification:
โhttps://github.com/jrfnl/develop.wordpress/commits/test/WPCS-autofixing-with-2-upstream-fixes
[Edit 3]:
- โWPCS PR #972 has been merged in the mean time and fixes โWPCS #970
- โWPCS PR #980 has just been pulled and will fix โWPCS #975
[Edit 4]:
- โWPCS PR #980 has been merged and fixes โWPCS #975 - This does require that the
WordPress.CodeAnalysis.EmptyStatement
sniff be (temporarily) added to thephpcs.xml.dist
ruleset as that sniff is not included in theWordPress-Core
ruleset by default (see the PR for the reasoning).
[Edit 5]:
- โWPCS PR #981 has just been pulled and will fix โWPCS #976
#29
in reply to:
โย 25
;
follow-up:
โย 30
@
7 years ago
Replying to pento:
It won't be backported at all. If we backport it to the 4.8 branch, then 4.8.1 will be a huge minor release, with a significantly increased failure rate.
Unfortunately, that does mean that most patches from now on will need to be rewritten to go further back than 4.8. After 4.9 is released, that will only be security patches.
Since the update zips include files with any changes, we can just run this specifically on the files that will be modified by a security backport. That way the backport is much more likely to apply cleanly.
#30
in reply to:
โย 29
@
7 years ago
Replying to markjaquith:
Since the update zips include files with any changes, we can just run this specifically on the files that will be modified by a security backport. That way the backport is much more likely to apply cleanly.
This is a really good idea!
#31
@
7 years ago
Thanks for your work on this so far, @jrf!
Would you mind posting your updates as new comments, rather than editing comments? Comment edits aren't emailed to ticket subscribers, so I only noticed your updates by chance.
And just in case someone is now thinking "Cricky, PHPCS is really buggy, shouldn't we use something else ?"... The majority of the fixes - I'd wager > 95% - is correct. Using PHPCS on WP core is, however, in a way a stress-test of PHPCS, we're really getting into all the nooks and crannies, so I'm not surprised we're finding bugs.
Agreed, WordPress has a huge variety of code, so I'm not surprised that we're running into edge cases, either.
Is there are definitive method for running PHPCS/WPCS with the various PRs? Being fairly unfamiliar with both projects, it's a bit tricky to figure out whether bugs are being caused by know or unknown issues.
Also, while I remember, another thing noticed - multibyte characters seem to cause invalid PHP to be produced. If you remove /tests/
from the excluded locations, tests/phpunit/tests/pomo/po.php
and tests/phpunit/tests/pomo/mo.php
will have curly braces immediately following a multibyte string missing, after reformatting.
#32
@
7 years ago
I've setup a Scrutinizer-ci.com job, based on a PR of my mirror:
Scrutinizer should be able to run phpcbf
to then create pull requests to the source repo with the changes, hopefully this can work as a saner way to view, and share the latest changes between everyone playing along with this ticket :)
It's currently running PHPCS against the latest dev-master
WPCS sniffs without error.
Next up, is to try and have it create the phpcbf
pull requests, but alas, out of time available today.
#33
follow-up:
โย 34
@
7 years ago
Would you mind posting your updates as new comments, rather than editing comments? Comment edits aren't emailed to ticket subscribers, so I only noticed your updates by chance.
@pento I didn't want to spam the participants of this thread with the details of individual PHPCS/WPCS issues found/fixed. If someone is interested to follow that in detail, I suggest "watching" the โWPCS repo instead to get notifications about the individual issues/fixes.
Latest issues identified based on visual review of some of the patches:
- โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/982
- โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/983
- โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/985
- โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/986
- โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/988
Latest PRs fixing some of these things:
- โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/981
- โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/987
- โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/989
Is there are definitive method for running PHPCS/WPCS with the various PRs? Being fairly unfamiliar with both projects, it's a bit tricky to figure out whether bugs are being caused by know or unknown issues.
PRs to WPCS are generally merged quite quickly, so using the latest develop
branch should work. Unless I've accidentally done something silly, I expect the above three mentioned PRs to be merged within the next 24 hours.
There are no open PRs in PHPCS at this moment related to this ticket, so using the PHPCS 2.9
branch should be ok.
When figuring out where issues are coming from, the following PHPCS tricks can be useful:
- Use the
-s
flag when runningphpcs
to see which sniff is reporting issues on which line. - Running
phpcs -p -s ./filename.php --standard=WordPress-Core --report-diff -vv
will give you a full report on all the fixes which PHPCS tries to apply in the various loops without the file actually being changed. (report is very long, but the relevant bit is at the bottom anyway)
Also, while I remember, another thing noticed - multibyte characters seem to cause invalid PHP to be produced. If you remove /tests/ from the excluded locations, tests/phpunit/tests/pomo/po.php and tests/phpunit/tests/pomo/mo.php will have curly braces immediately following a multibyte string missing, after reformatting.
The default encoding PHPCS 2.x uses for interpreting files is ISO-8859-1
. This can be changed by setting --encoding=utf-8
on the command line or using <config name="encoding" value="utf-8"/>
in a custom ruleset.
AFAIK, this can not be changed for an individual file, so will affect how all files are read.
If so desired, we can add this directive to the WordPress-Core
ruleset in WPCS.
It's currently running PHPCS against the latest dev-master WPCS sniffs without error.
@netweb Without error ? From what I've seen so far, hardly any individual file patches are 100% bug-free, so please be very careful for now when pulling patches.
#34
in reply to:
โย 33
@
7 years ago
Replying to jrf:
It's currently running PHPCS against the latest dev-master WPCS sniffs without error.
@netweb Without error ? From what I've seen so far, hardly any individual file patches are 100% bug-free, so please be very careful for now when pulling patches.
My bad, I meant I've got Scrutinizer working without error, patches are not being generated yet.
My plan is just to make available the patches available in the easiest way possible to get eyes on and examine.
This ticket was mentioned in โSlack in #core-coding-standards by garyj. โView the logs.
7 years ago
#36
follow-ups:
โย 37
โย 39
โย 40
โย 45
@
7 years ago
WPCS currently doesn't (I think) change whether a break
is aligned with it's case
, or indented from it. The format isn't defined in the coding standards, the normal usage for core is to indent the break
one tab from the case
statement.
$ ag --php --stats '^(\t*)case(.*\n){0,5}\1break' wp-includes/ wp-admin/ | ag '^\d+ matches$' 34 matches $ ag --php --stats '^(\t*)case(.*\n){0,5}\1\tbreak' wp-includes/ wp-admin/ | ag '^\d+ matches$' 613 matches
Any objections to formalising the latter as the correct standard?
#37
in reply to:
โย 36
;
follow-up:
โย 38
@
7 years ago
Replying to pento:
the normal usage for core is to indent the
break
one tab from thecase
statement.
FWIW, this is what โPSR-2 uses too (albeit, with spaces).
#38
in reply to:
โย 37
@
7 years ago
Replying to GaryJ:
FWIW, this is what โPSR-2 uses too (albeit, with spaces).
Let's not spread it around that we've adopted anything from PSR-2. ;-)
#39
in reply to:
โย 36
@
7 years ago
Replying to pento:
the normal usage for core is to indent the
break
one tab from thecase
statement.
I personally prefer not to indent the break
(to match the pattern for blocks that are enclosed in brackets, the closer is indented the same level as the opener), but I'm clearly in the minority. :-)
#40
in reply to:
โย 36
@
7 years ago
Replying to pento:
WPCS currently doesn't (I think) change whether a
break
is aligned with it'scase
, or indented from it. The format isn't defined in the coding standards, the normal usage for core is to indent thebreak
one tab from thecase
statement.
Related issue already open in WPCS: โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/982
#41
@
7 years ago
Status update time!
phpcs.xml.2.distโ is the current config I'm working with. I would not recommend running it at the moment, there are ~160 files still failing that I haven't classified and excluded - it seems they mostly fall under โWPCS#986, the related โWPCS#998, or โWPCS#1000. Not being excluded means that phpcbf
takes well over 30 minutes to run on my computer, instead of the usual ~13 minutes, hence the recommendation not to run it at the moment.
The good news is, there are very few new issues showing up in my testing, they all seem to be related to known issues. The known issues are under investigation, or have fix in progress. Many props to the WPCS crew, and @jrf in particular, for driving these. :-)
Finally, for a mildly entertaining testing note, Tests_Pluggable::getDefinedPluggableFunctions()
and Tests_User_Capabilities::testMetaCapsTestsAreCorrect()
will need to updated in the final commit - they both assume code formatting that will change.
This ticket was mentioned in โSlack in #core-coding-standards by pento. โView the logs.
7 years ago
#45
in reply to:
โย 36
@
7 years ago
Replying to pento:
WPCS currently doesn't (I think) change whether a
break
is aligned with it'scase
, or indented from it. The format isn't defined in the coding standards, the normal usage for core is to indent thebreak
one tab from thecase
statement.
$ ag --php --stats '^(\t*)case(.*\n){0,5}\1break' wp-includes/ wp-admin/ | ag '^\d+ matches$' 34 matches $ ag --php --stats '^(\t*)case(.*\n){0,5}\1\tbreak' wp-includes/ wp-admin/ | ag '^\d+ matches$' 613 matchesAny objections to formalising the latter as the correct standard?
This has now been added to the PHP Coding Standards: โhttps://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#indentation
This ticket was mentioned in โSlack in #glotpress by ocean90. โView the logs.
7 years ago
#47
@
7 years ago
phpcs.xml.3.distโ is the current PHPCS settings I'm testing with, for anyone wanting to play around it. :-)
#48
follow-up:
โย 49
@
7 years ago
@pento FYI: You can remove the encoding
directive from your ruleset. This was added to the WordPress-Core
ruleset in โPR #992 a couple of weeks back.
#49
in reply to:
โย 48
@
7 years ago
Replying to jrf:
@pento FYI: You can remove the
encoding
directive from your ruleset. This was added to theWordPress-Core
ruleset in โPR #992 a couple of weeks back.
phpcs.xml.4.distโ is a refresh of phpcs.xml.3.distโ with the encoding
directive removed
#51
@
7 years ago
โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/releases/tag/0.12.0
With the release of WPCS 0.12.0
should we switch to using 0.12.0
rather than develop
?
#52
@
7 years ago
With the release of WPCS 0.12.0 should we switch to using 0.12.0 rather than develop?
For the purposes of this ticket, I would suggest not to switch as develop
will always contain the bleeding edge fixes.
Secondly, the next version of WPCS which is currently being developed will be compatible with PHPCS 3.x, so staying on develop
will be the quickest way to also get access to upstream fixes which haven't made it into PHPCS 2.x.
There are still a couple of issues open regarding bugs found in the fixers - all to do with upstream bugs in PHPCS itself.
I've added a label to all the issues & PRs which relate to this ticket, to make it easier to get an โoverview of the current status.
Further reviews of the auto-fixes made & bug reports and such would be very helpful to move this ticket along, so anyone who has some time to review, please do!
On another note - one of the (merged) PRs covers something which is not covered by the Handbook, but would solve an issue I saw while reviewing some auto-fixes in core, so @pento / @netweb may want to add it to the custom ruleset being used: WordPress.CodeAnalysis.EmptyStatement
. See โPR 980 for more info.
#53
@
7 years ago
Regarding the current ruleset being used: as long as the PHPCS 2.9 dev branch is used, all sniff based exclusions can be removed as they have been fixed.
Both the fix for Squiz.PHP.EmbeddedPhp.SpacingBeforeClose
as well as for Squiz.Strings.DoubleQuoteUsage.NotRequired
will be included in PHPCS 2.9.2 (not yet released) and are already included in PHPCS 3.0.2.
#54
follow-up:
โย 55
@
7 years ago
Current status:
- PHP CodeSniffer 3.0.2 was released 17 days ago.
- WordPress Coding Standards (which now has support for PHPCS 3.*) โ0.13.0 was released about 11 hours ago.
The latest patch here (phpcs.xml.4.distโ) excludes a load of files because of โWPCS#1017, but this is now marked as closed. The duplicate of that ticket (โWPCS#1065), also closed, says that it was fixed upstream in PHPCS 3.0.2. Can some or all of these file patterns now be removed from the proposed phpcs.xml.dist
?
With that done, how close are the rest of the files in being patched?
#55
in reply to:
โย 54
@
7 years ago
Replying to GaryJ:
Current status:
- PHP CodeSniffer 3.0.2 was released 17 days ago.
- WordPress Coding Standards (which now has support for PHPCS 3.*) โ0.13.0 was released about 11 hours ago.
๐
The latest patch here (phpcs.xml.4.distโ) excludes a load of files because of โWPCS#1017, but this is now marked as closed. The duplicate of that ticket (โWPCS#1065), also closed, says that it was fixed upstream in PHPCS 3.0.2. Can some or all of these file patterns now be removed from the proposed
phpcs.xml.dist
?
I've removed those exclusions in phpcs.xml.5.distโ
With that done, how close are the rest of the files in being patched?
Another round of testing and review using phpcs.xml.5.distโ would be up next
This ticket was mentioned in โSlack in #buddypress by netweb. โView the logs.
7 years ago
#58
@
7 years ago
While preparing my talk for WC Nijmegen this weekend, I did another test run with PHPCS master
and WPCS develop
- I'll attach the ruleset I used as phpcs6.xml.dist
.
This yielded 26 files which failed to fix due to fixer conflicts.
I have investigated the most persistent one - see โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/1112 - and pulled a fix in PHPCS upstream: โhttps://github.com/squizlabs/PHP_CodeSniffer/pull/1633
Re-running everything with the fix applied only leaves one last file with a fixer conflict src\wp-admin\theme-install.php
.
I think we need to start recruiting more people to help with reviewing the patches ?
#59
follow-ups:
โย 61
โย 118
@
7 years ago
This is looking really good @jrf! There don't appear to be any major issues that I've run into so far, most of it is just tweaking.
I've submitted a handful of reports for indentation issues to the WPCS repo: โWPCS#1118, โWPCS#1119, โWPCS#1120, โWPCS#1121.
Array elements aren't aligning at the =>
. It's mentioned (and shown in the examples) in the โHandbook, but I can make that more explicit if necessary. More โinfo in Slack, and โWPCS#1122.
What's the condition for splitting a long array declaration into multiple lines? The arrays at the top of admin-ajax.php
and class-wp-list-table.php
are split correctly, but those in class-wp.php
are not.
There are a couple of instances of a comment associated with the while
of a do/while
being indented, when it probably shouldn't be (the behaviour is not defined in the handbook), but I'm not worried about them - two instances in compat.php
isn't worth getting excited about.
The inline XML in xmlrpc.php
is currently indented with spaces, at 2 spaces per level. This isn't being correctly fixed - instead of replacing each 2 space indent with a tab, it's replacing groups of 4 spaces with a tab. Again, I'm not worried about this - we can manually fix it. A quick read wp-includes/feed-*.php
shows all of them being indented correctly.
WP_Admin_Bar::_render_item()
probably needs to be rewritten, because that kind of abuse of white space collapsing is just not on. Conforming to the coding standards breaks the HTML output.
Generally, I'm finding everything to be looking good, and much easier to read. The bits that are still hard to read are the long lines that we don't have a standard for splitting up. The usually fall into these categories:
- Multiple conditions in an
if()
- Concatenating several strings together
- Embedded HTML, usually with multiple attributes
- HTML inside a string
- DB queries in a string
For the first two, I think we can just define a line length to split at, then they'll obey their relevant indent rules.
For embedded HTML, I don't know if we can automatically split - adding whitespace between elements can cause layout bugs, but adding whitespace inside of elements (ie, putting element attributes on separate lines) won't.
For the last two, I don't think we can sanely automatically fix them, but we could certainly throw a warning.
I'm leaning towards using a modified version of the Docs line split rule - split as close to column 100 as possible, do not go over column 120. I'm open to arguments for or against this, the exact numbers to split at, and whether a split should cause all "units" in that line to split. For example:
If you're splitting an if
condition, should you split just at the split point, at every logical operator in the current scope, or at every logical operator?
Given this example (and assuming the split point is at $d
):
if ( $a || ( $b && $c ) || $d )
Which of these should it become?
if ( $a || ( $b && $c ) || $d )
if ( $a || ( $b && $c ) || $d )
if ( $a || ( $b && $c ) || $d )
Same thing for string concatenation. Split at every .
, or just the ones we need to?
And for HTML elements, should we split at every attribute, or just the ones we need to? Should we put newlines between all elements where there was previously no whitespace, just a specific set of elements, or should we throw a warning? (cc @peterwilsoncc for your opinion on adding whitespace, and what kind of layout issues that might cause.)
#60
@
7 years ago
@pento
What's the condition for splitting a long array declaration into multiple lines? The arrays at the top of admin-ajax.php and class-wp-list-table.php are split correctly, but those in class-wp.php are not.
This currently works as follows:
- If an array is already multi-line, each item should start on a new line.
- If an array is single-line, but contains associative keys and more than one array item, it will be forced to become multi-line.
I.e. single line non-associative arrays, like in class-wp.php
, are left as is.
The inline XML in xmlrpc.php is currently indented with spaces, at 2 spaces per level. This isn't being correctly fixed - instead of replacing each 2 space indent with a tab, it's replacing groups of 4 spaces with a tab. Again, I'm not worried about this - we can manually fix it.
Sniffs can not determine whether something is intentional precision alignment or unintentional incorrect alignment. This is why this is not being fixed.
The PrecisionAlignment sniff which was merged today should warn about this, but will not auto-fix it. See: โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/1099
WP_Admin_Bar::_render_item() probably needs to be rewritten, because that kind of abuse of white space collapsing is just not on. Conforming to the coding standards breaks the HTML output.
I agree. Rewriting this method would be the better solution.
I recall having a conversation with someone about these type of situations - multi-line embedded PHP in HTML snippets - before, but can't find the link at this moment. The consensus from that conversation was that - if at all possible - variables should be created before echo-ing out the HTML string when more than simple logic is needed.
Or alternatively, the whole string should be build up within a PHP block and only echo-ed out at the end.
Generally, I'm finding everything to be looking good, and much easier to read. The bits that are still hard to read are the long lines that we don't have a standard for splitting up.
There is a sniff which can warn about this - Generic.File.LineLength
-, but this cannot be auto-fixed easily as in most cases it will need a human eye to decide the best course of action.
The above mentioned sniff is not in any ruleset at this moment as this is not covered in the handbook.
As per your examples, there may be specific situations in which it can be auto-fixed, but that would still not be an easy sniff to create and I would expect it will need extensive test-cases to get it right.
#61
in reply to:
โย 59
@
7 years ago
Replying to pento:
For embedded HTML, I don't know if we can automatically split - adding whitespace between elements can cause layout bugs, but adding whitespace inside of elements (ie, putting element attributes on separate lines) won't.
[... snip]
And for HTML elements, should we split at every attribute, or just the ones we need to? Should we put newlines between all elements where there was previously no whitespace, just a specific set of elements, or should we throw a warning?
Please don't make any meaningful changes to white-space, any instances of HTML tags butted against each other will need to remain. It will mainly cause problems with the inline & inline-block display types, see โhttps://jsbin.com/jewayex/edit?html,css,output
With very few exceptions, a developer can set any type of element to any type of display type. For some history, a meaningful change to white-space lead to #35107 & reverting [34321].
For instances where two HTML tags have some white-space between them, these can be tidied up with line breaks and tabs to meet the coding standards. This would be dandy.
For breaking at attributes, I'd suggest only where needed as any line breaks that make it into the rendered HTML will make it less readable.
This ticket was mentioned in โSlack in #core-coding-standards by pento. โView the logs.
7 years ago
#63
@
7 years ago
Array elements aren't aligning at the
=>
. It's mentioned (and shown in the examples) in the Handbook, but I can make that more explicit if necessary.
Handbook says
Exception: if you have a block of code that would be more readable if things are aligned, use spaces:
While I do agree that it helps with readability (I do it myself while coding), I don't think that it would mean much if we cannot automatically fix it with phpcbf
, no? I definitely wouldn't put it as a requirement, but a recommendation.
This ticket was mentioned in โSlack in #core by jrf. โView the logs.
7 years ago
This ticket was mentioned in โSlack in #core-coding-standards by jdgrimes. โView the logs.
7 years ago
#66
follow-up:
โย 67
@
7 years ago
It was just โpointed out in #core-coding-standards on Slack that Hello Dolly doesn't follow the standards completely either. I think that it should also be updated as part of this effort.
#67
in reply to:
โย 66
@
7 years ago
Replying to jdgrimes:
It was just โpointed out in #core-coding-standards on Slack that Hello Dolly doesn't follow the standards completely either. I think that it should also be updated as part of this effort.
It was originally included in the first set of patches, in the .2
patches right through to the recent .6
patch /src/wp-content/plugins/*
was added as an exclusion, I've removed that exclusion in phpcs.xml.7.distโ so that Hello Dolly is included as part of this ticket along with #30120 (Internationalize Hello Dolly) in the 4.9 milestone.
#69
@
7 years ago
41057.diffโ rewrites the WP_Admin_Bar
render methods to be a little less awful.
#70
@
7 years ago
- Milestone changed from 4.9 to Future Release
Not wild about committing this during beta, let's leave it for 5.0.
#71
@
7 years ago
41057.2.diffโ makes wp_widget_control()
less bad.
#72
@
7 years ago
I had thought that 41057.2.diffโ was just a one off. As it turns out, it is not - I found 44 instances using this:
ack --php '<[^?](?:[^>]+|<\?php[^>]+\?>)+(<\?php\s+if)' src
I don't really expect WPCS to handle that, because that's really bad code, and we should feel bad about it. These instances will need to be manually fixed before phpcbf
can be run across the codebase.
@
7 years ago
Further iteration on the ruleset with more fine-grained exclude patterns for select sniffs
This ticket was mentioned in โSlack in #core by pento. โView the logs.
7 years ago
#78
@
7 years ago
Another error in line 509. Noticed because it's breaking the plugin directory screens on WordPress.org.
`
foreach ( $attributes as $attribute ) {
if ( ! empty( $node->meta[ $attribute ] ) ) {
echo " $attribute='" . esc_attr( $node->meta[ $attribute ] ) . '"';
}
}
`
Note the mismatched quoting on the HTML attribute. It starts with a single quote, but closes with a double quote.
Change the echo to this to fix:
`
echo " $attribute='" . esc_attr( $node->meta[ $attribute ] ) . "'";
`
#82
@
7 years ago
- Keywords commit added
- Exclude the
vendor
directory. - Exclude the config and config-sample files from the
Generic.Functions.FunctionCallArgumentSpacing
test. This allows the files to maintain the unorthodox (but imo, significantly nicer) spacing of the KEY/SALT constants.
In order to more easily test with the latest PHPCS and WPCS, I've been using this composer.json
, in case you're after an easy way to reproduce the current state of things:
{ "require-dev": { "dealerdirect/phpcodesniffer-composer-installer": "*", "wp-coding-standards/wpcs": "dev-develop", "squizlabs/php_codesniffer": "dev-master" } }
Despite there still being โa handful of outstanding issues, I think this is good enough to commit. If you want to review the changes online, see โthis PR.
Prior to running phpcbf
, phpcs
reports 73201 errors and 21327 warnings in 1149 files (of a total of 1290 files scanned).
After the phpcbf
run, there are 3387 errors and 1606 warnings in 576 files. It looks like all of these errors and warnings require manual fixing, which is definitely best done in followup commits.
Unless there are show-stopping issues (minor formatting issues aren't show-stoppers, they can be reported to the โWPCS repo), I'm planning on committing this early next week.
This ticket was mentioned in โSlack in #core-committers by pento. โView the logs.
7 years ago
#84
follow-up:
โย 89
@
7 years ago
Side note: I don't plan on committing the composer.json
, we still need write a grunt
task to do the phpcbf
and phpcs
runs. We can make phpcbf
run a required Travis job immediately (run phpcbf
, fail if there are changes). phpcs
will have to stay as an allowed_failure
job until all of the manual fixes are done.
#85
follow-up:
โย 86
@
7 years ago
phpcs will have to stay as an allowed_failure job until all of the manual fixes are done.
So I guess that's planned for the following week then. ;)
Will these manual fixes be handled in this ticket or in different tickets (one per family of changes, individual tickets, ...) ? It would be great to discuss a consistent workflow for these, as that work will probably include a lot of "good first issues" and would be great (WCUS) contributor day material.
#86
in reply to:
โย 85
@
7 years ago
Replying to schlessera:
phpcs will have to stay as an allowed_failure job until all of the manual fixes are done.
So I guess that's planned for the following week then. ;)
๐
Will these manual fixes be handled in this ticket or in different tickets (one per family of changes, individual tickets, ...) ? It would be great to discuss a consistent workflow for these, as that work will probably include a lot of "good first issues" and would be great (WCUS) contributor day material.
New tickets, I'm inclined to group them by family - if a contributor day lead wants to reserve a ticket, they can pretty easily split the ticket by file (or whatever they prefer, really) among the contributors they're guiding.
#87
@
7 years ago
From an earlier comment:
You may want to consider adding the WordPress-Docs ruleset as well. This ruleset is not complete yet, but what it does cover is in the Core PHP Documentation Handbook page and a number of sniffs included there also include auto-fixers.
#89
in reply to:
โย 84
;
follow-up:
โย 110
@
7 years ago
Replying to pento:
... we still need write a
grunt
task to do thephpcbf
andphpcs
runs.
Added Grunt tasks grunt phpcs
and grunt phpcbf
in 41057-grunt.diffโ
We can make
phpcbf
run a required Travis job immediately (runphpcbf
, fail if there are changes).phpcs
will have to stay as anallowed_failure
job until all of the manual fixes are done.
Will do the Travis CI side of things in a separate patch
#90
@
7 years ago
phpcs.xml.8.distโ adds WordPress-Docs.
Before: 90696 errors and 21582 warnings in 1231 files.
After: 19297 errors and 1888 warnings in 1108 files.
It seems like only a handful of the docs problems are fixed by phpcbf
.
#91
follow-up:
โย 92
@
7 years ago
I think the docs ruleset should be left out for now. Fixing docs is a much larger task than fixing code formatting.
#92
in reply to:
โย 91
@
7 years ago
Replying to johnbillion:
I think the docs ruleset should be left out for now. Fixing docs is a much larger task than fixing code formatting.
Agreed - the thought of manually fixing 4493 violations is a much nicer thought than fixing 21185 in one go.
#93
@
7 years ago
@pento Just a couple of notes/suggestions before you commit this:
- Previously discussed in various issues in WPCS - it would be highly recommended to manually fix the issues the
WordPress.WhiteSpace.PrecisionAlignment
sniff throws up before running the big patch for improved results. - There is one โopen PR upstream in PHPCS which solves one of the open issues as well as fixes space indented comments (currently not being fixed).
A branch including this can be found here: โhttps://github.com/jrfnl/PHP_CodeSniffer/tree/WP-Core/test-run - I will update that branch with the latest PHPCS master this weekend.
#94
@
7 years ago
FYI: I've (force) pushed the updated โhttps://github.com/jrfnl/PHP_CodeSniffer/tree/WP-Core/test-run branch. I recommend using that branch instead of PHPCS/master to create the final patch.
#96
@
7 years ago
I agree that Docs should be left out for now. Hopefully some more rules can be auto-fixed, because manually fixing 15000+ errors and warnings is a bit of an overwhelming problem. ๐
- Remove the
WordPress-Docs
rule. - Exclude the entire
tests/phpunit/data
directory - fixtures may intentionally include incorrectly formatted code.
@jrf's PR has been merged, so I've updated the โWordPress PR, run against WPCS dev-develop
and PHPCS dev-master
.
Before wpcbf
: 72816 errors and 21044 warnings in 1117 files.
After wpcbf
: 3387 errors and 1242 warnings in 541 files.
#97
@
7 years ago
@pento You beat me to it, I was just about to post that the PR has been merged ;-)
There is one more open PR which may or may not be of interest to this patch. It's a bug in the same sniff (DisallowSpaceIndent
) which I discovered yesterday. Basically if there are files which start with inline HTML and only have the PHP open tag later on, the indentation on the lines above the first PHP open tag will not be fixed. This would typically be the case in views without File DocBlock header.
PR in PHPCS: โhttps://github.com/squizlabs/PHP_CodeSniffer/pull/1762
Just in case this comes into play here, I have updated the โhttps://github.com/jrfnl/PHP_CodeSniffer/tree/WP-Core/test-run branch to the current PHPCS master
+ PR 1762.
#98
@
7 years ago
As a side-note: I do still intend to completely revamp the WPCS Docs ruleset to both detect + auto-fix a lot more and better, but some other things (like fixing the bugs we found while working on this patch) have had higher priority.
Fingers crossed I can make in-roads on the Docs ruleset in Q1 of 2018.
#99
@
7 years ago
On a clean run with the WP-Core/test-run
branch, wpcs
reports the same number of errors and warnings, and a clean phpcbf
run doesn't change the state of the final result.
#102
@
7 years ago
A good followup for [42249] would be to rewrite the regexen as single strings, using the x
regex modifier. This will have to be done fairly carefully, however, as these regexen are quite complex.
Further discussion: โhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/1243
#103
@
7 years ago
phpcs.xml.10.distโ excludes the tools
directory, as it really should be an external to โi18n.svn.
โThe PR is updated.
Before wpcbf
: 71611 errors and 20816 warnings in 1104 files.
After wpcbf
: 3339 errors and 1238 warnings in 530 files.
@jrf is working on a fix for โmulti-line comments not being indented correctly - once that's done, it's time to commit!
#104
in reply to:
โย 101
;
follow-up:
โย 105
@
7 years ago
Replying to pento:
In 42249:
General: Add inline PHPCS options to leave regex indentation.
The comment format used in this commit is only being โintroduced in PHP_CodeSniffer 3.2.0. Is that intentional?
#105
in reply to:
โย 104
@
7 years ago
Replying to GaryJ:
The comment format used in this commit is only being โintroduced in PHP_CodeSniffer 3.2.0. Is that intentional?
Yup. Currently running again PHPCS master
and WPCS develop
due to fixes in both - we'll hopefully get back to running against release versions when PHPCS 3.2 and WPCS 0.16 land.
#107
@
7 years ago
@jrf has kindly pushed her โin-progress work on the multi-line comment indenting fix - docblocks appear to be working correctly, but other multi-line comments are still in progress.
I'm not particularly fussed about the latter, it can be fixed somewhere down the road. In the mean time, I'm planning on landing this in a few hours. โThe PR is updated, in case you feel like doing some more perusing through code, though it's worth remembering that this isn't the only formatting fix that will ever be committed - we can do more, on a more regular basis, as WPCS is updated to catch the weird edge cases.
Before wpcbf
: 74775 errors and 20816 warnings in 1107 files.
After wpcbf
: 3554 errors and 1235 warnings in 535 files.
#110
in reply to:
โย 89
@
7 years ago
Replying to netweb:
Replying to pento:
... we still need write a
grunt
task to do thephpcbf
andphpcs
runs.
Added Grunt tasks
grunt phpcs
andgrunt phpcbf
in 41057-grunt.diffโ
We can make
phpcbf
run a required Travis job immediately (runphpcbf
, fail if there are changes).phpcs
will have to stay as anallowed_failure
job until all of the manual fixes are done.
Will do the Travis CI side of things in a separate patch
Replying to pento:
@jrf has kindly pushed her โin-progress work on the multi-line comment indenting fix - docblocks appear to be working correctly, but other multi-line comments are still in progress.
I've now got phpcbf
and phpcs
patches for Travis CI jobs and Grunt tasks using the above WPCS branch refreshed here locally, thinking these should be split to another ticket, I'll create that soonish, ping me if you'd like a peek at the patches in the meantime.
#111
follow-up:
โย 113
@
7 years ago
Is there a follow-up ticket to fix the issues that phpcbf wasn't able to? Or will each issue be addressed in individual tickets?
This ticket was mentioned in โSlack in #core by johnbillion. โView the logs.
7 years ago
#113
in reply to:
โย 111
@
7 years ago
Replying to johnbillion:
Is there a follow-up ticket to fix the issues that phpcbf wasn't able to? Or will each issue be addressed in individual tickets?
Each issue in a new ticket please, working on one PHPCS sniff at a time seems to be my current workflow for which I'm then creating a new ticket for.
#115
@
7 years ago
Related: Also see the new coding-standards focus tickets
This ticket was mentioned in โSlack in #core-coding-standards by mboynes. โView the logs.
7 years ago
#118
in reply to:
โย 59
@
7 years ago
Replying to pento:
Array elements aren't aligning at the
=>
. It's mentioned (and shown in the examples) in the โHandbook, but I can make that more explicit if necessary. More โinfo in Slack, and โWPCS#1122.
I'm late to the party, but didn't see this mentioned anywhere. One of the problems with vertical alignment is that future changes might require other lines get updated if the alignment changes. That is, if you add a new variable or array key which is longer than the others, you have to change the whitespace on the other lines to align them. This leads to "messy" diffs and more potential for merge conflicts, and contradicts the coding standard recommendation for trailing commas:
this is recommended because it... makes for cleaner diffs when new items are added
It was discussed in this ticket that vertical alignment be made a requirement; I would recommend against that for this reason.
Before we do this, I would like us to take a moment and review the Coding Standards. Is there anything we'd like to change? I don't want to make such a huge commit if we're going to follow it up with another huge commit for some standards tweak.