Update PHP codebase per WordPress PHP Coding Standards

[netweb]

​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 the 41057-filelist-src-root.txt PHP file list
• 41057-src-wp-admin.diff generated from the 41057-filelist-src-wp-admin.txt PHP file list
• 41057-src-wp-content.diff generated from the 41057-filelist-src-wp-content.txt PHP file list
• 41057-src-wp-includes.diff generated from the 41057-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

[pento]

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.

[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().

[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:

1. Patch WordPress now per 0.11.0 and then follow up further once 0.12.0 is released

or

2. Wait until 0.12.0 is released and update the patches here?

[pento]

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.

[netweb]

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

[jdgrimes]

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:

1. Patch WordPress now per 0.11.0 and then follow up further once 0.12.0 is released

or

2. 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.

[pento]

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.

[johnbillion]

#41066 was marked as a duplicate.

[westonruter]

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.

[westonruter]

Should we also update .travis.yml to start running PHPCS as well?

[johnjamesjacoby]

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.

[yahil]

improved coding standards for user.php

[pento]

The ​Coding Standards now no longer have a rule about blocks longer than 35 characters.

[pento]

phpcs.xml

Yes.

.travis.yml

Yes.

[pento]

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.

[jorbin]

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?

[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.

[jorbin]

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

[johnjamesjacoby]

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.

[boonebgorges]

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.

[jrf]

@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

[netweb]

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

[peterwilsoncc]

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.

[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.

[pento]

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.

[jrf]

@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>

Ref: ​https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-ruleset.xml#selectively-applying-rules

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

[jrf]

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/970 Fixed
• ​https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/971 PR 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/975 Fixed
• ​https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/976 PR 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 the phpcs.xml.dist ruleset as that sniff is not included in the WordPress-Core ruleset by default (see the PR for the reasoning).

[Edit 5]:

• ​WPCS PR #981 has just been pulled and will fix ​WPCS #976

[markjaquith]

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.

[pento]

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!

[pento]

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.

[netweb]

I've setup a Scrutinizer-ci.com job, based on a PR of my mirror:

• ​https://scrutinizer-ci.com/g/ntwb/wordpress/
• ​https://github.com/ntwb/wordpress/pull/2

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.

[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.

@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 running phpcs 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.

[netweb]

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.

[pento]

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?

[GaryJ]

Replying to pento:

the normal usage for core is to indent the break one tab from the case statement.

FWIW, this is what ​PSR-2 uses too (albeit, with spaces).

[pento]

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. ;-)

[jdgrimes]

Replying to pento:

the normal usage for core is to indent the break one tab from the case 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. :-)

[jrf]

Replying to pento:

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.

Related issue already open in WPCS: ​https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/982

[pento]

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.

[netweb]

#41181 was marked as a duplicate.

[desrosj]

#41213 was marked as a duplicate.

[netweb]

Replying to pento:

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?

This has now been added to the PHP Coding Standards: ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#indentation

[pento]

phpcs.xml.3.dist​ is the current PHPCS settings I'm testing with, for anyone wanting to play around it. :-)

[jrf]

@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.

[netweb]

Replying to jrf:

@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.

phpcs.xml.4.dist​ is a refresh of phpcs.xml.3.dist​ with the encoding directive removed

[netweb]

#41342 was marked as a duplicate.

[netweb]

​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?

[jrf]

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.

[jrf]

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.

[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?

With that done, how close are the rest of the files in being patched?

[netweb]

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

[netweb]

#41749 was marked as a duplicate.

[jrf]

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 ?

[pento]

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.)

[jrf]

@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.

[peterwilsoncc]

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.

[dingo_bastard]

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.

[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.

[netweb]

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.

[johnbillion]

In 41635:

Build/Test tools: Update some test cases in anticipation of code formatting corrections.

These two tests assume a certain level of indentation in code which does not conform to core's coding
standards and will hopefully be corrected at some point in #41057.

See #41057

[pento]

41057.diff​ rewrites the WP_Admin_Bar render methods to be a little less awful.

[pento]

Not wild about committing this during beta, let's leave it for 5.0.

[pento]

41057.2.diff​ makes wp_widget_control() less bad.

[pento]

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.

[jrf]

Further iteration on the ruleset with more fine-grained exclude patterns for select sniffs

[pento]

#42297 was marked as a duplicate.

[pento]

In 42128:

Admin Bar: Reformat the render methods.

The admin bar render methods use some cute tricks which don't come close to the WordPress coding standards. So that we can more easily apply automated code fixing to the codebase, these tricks need to be removed.

See #41057.

[pento]

In 42129:

Admin Bar: Fix a HTML error introduced in [42128].

Props swissspidy.
See #41057.

[Otto42]

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 ] ) . "'"; 

[pento]

In 42131:

Admin Bar: Fix another HTML error introduced in [42128].

Props Otto42.
See #41057.

[pento]

In 42217:

General: Reformat inline if () statements inside HTML tags.

This pattern occurs a handful of times across the codebase:

<div class="foo<?php if ( $bar ) { echo ' baz'; } ?>">

Unfortunately, it doesn't really play nicely with phpcbf, so all instances need to be removed in preperation for auto code formatting.

See #41057.

[pento]

In 42218:

Setup: Allow for wp-config-sample.php to be formatted according to coding standards.

When the setup process reads wp-config-sample.php, it assumes that there are no spaces inside the brackes of the define()s. Unfortunately, this doesn't match our coding standards, so will no longer work correctly once we start enforcing them.

This also improves coding standards of the generated wp-config.php file.

See #41057.

[pento]

In phpcs.xml.6.dist​:

• Exclude the vendor directory.
• 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.

[pento]

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.

[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.

[pento]

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.

[GaryJ]

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.

[pento]

Oh, good idea. I'll add that and update the PR a bit later.

[netweb]

Replying to pento:

... we still need write a grunt task to do the phpcbf and phpcs runs.

Added Grunt tasks grunt phpcs and grunt phpcbf in 41057-grunt.diff​

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.

Will do the Travis CI side of things in a separate patch

[pento]

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.

[johnbillion]

I think the docs ruleset should be left out for now. Fixing docs is a much larger task than fixing code formatting.

[GaryJ]

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.

[jrf]

@pento Just a couple of notes/suggestions before you commit this:

1. 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.
2. 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.

[jrf]

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.

[pento]

In 42228:

General: Fix some precision alignment formatting warnings.

The WPCS WordPress.WhiteSpace.PrecisionAlignment rule throws warnings for a bunch of code that will likely cause issues for wpcbf. Fixing these manually beforehand gives us better auto-fixed results later.

See #41057.

[pento]

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. 🙃

In phpcs.xml.9.dist​:

• 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.

[jrf]

@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.

[jrf]

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.

[pento]

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.

[jrf]

@pento 👍 (and PR 1762 has just been merged now anyway, so the point is moot).

[pento]

In 42249:

General: Add inline PHPCS options to leave regex indentation.

We have a handful of super long regexen that are written over multiple lines, as a collection of strings concatenated together. Each string is indented appropriately for the regex, but PHPCS doesn't recognised this, so defaults to removing the extra whitespace.

Disabling the Squiz.Strings.ConcatenationSpacing.PaddingFound rule for these blocks stops the extra whitespace from being removed.

See #41057.

[pento]

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

[pento]

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!

[GaryJ]

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?

[pento]

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.

[pento]

In 42342:

Tests: Fix a couple of weirdly formatted comments.

See #41057.

[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'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.

[pento]

In 42343:

Code is Poetry.
WordPress' code just... wasn't.
This is now dealt with.

Props jrf, pento, netweb, GaryJ, jdgrimes, westonruter, Greg Sherwood from PHPCS, and everyone who's ever contributed to WPCS and PHPCS.
Fixes #41057.

[pento]

In 42346:

Coding Standards: Add the phpcs.xml.dist file.

See #41057.

[netweb]

Replying to netweb:

Replying to pento:

... we still need write a grunt task to do the phpcbf and phpcs runs.

Added Grunt tasks grunt phpcs and grunt phpcbf in 41057-grunt.diff​

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.

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.

[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?

[netweb]

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.

[netweb]

Related: Also see the new coding-standards focus tickets

[johnbillion]

In 42786:

Upgrade/Install: Fix the format of the upgrader_process_complete actions after [42343].

More info: ​https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/1323

See #41057, #42505

[mboynes]

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.

[ocean90]

In 42843:

Pinking shears.

See #41057.

[rnaby]

#41989 was marked as a duplicate.

[rnaby]

#41988 was marked as a duplicate.

[peterwilsoncc]

In 45559:

Code is Poetry: CS Fixes required in wp-cron.php.

See #47396, #41057.