WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 2 weeks ago

#41057 accepted task (blessed)

Update PHP codebase per WordPress PHP Coding Standards

Reported by: netweb Owned by: pento
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: Cc:

Description (last modified by pento)

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

Attachments (16)

41057-filelist-src-root.txt (256 bytes) - added by netweb 3 months ago.
41057-filelist-src-wp-admin.txt (7.6 KB) - added by netweb 3 months ago.
41057-filelist-src-wp-content.txt (12.2 KB) - added by netweb 3 months ago.
41057-filelist-src-wp-includes.txt (10.5 KB) - added by netweb 3 months ago.
41057-src-root.diff (102.5 KB) - added by netweb 3 months ago.
41057-src-wp-admin.diff (1.8 MB) - added by netweb 3 months ago.
41057-src-wp-content.diff (325.2 KB) - added by netweb 3 months ago.
41057-src-wp-includes.diff (2.1 MB) - added by netweb 3 months ago.
41057-src-wp-includes-user.patch (24.0 KB) - added by yahil 3 months ago.
improved coding standards for user.php
phpcs.xml.dist (2.2 KB) - added by pento 3 months ago.
phpcs.xml.2.dist (4.1 KB) - added by pento 3 months ago.
41057-src-wp-includes-template.patch (6.7 KB) - added by Dency 3 months ago.
phpcs.xml.3.dist (8.9 KB) - added by pento 2 months ago.
phpcs.xml.4.dist (8.9 KB) - added by netweb 2 months ago.
phpcs.xml.5.dist (2.5 KB) - added by netweb 7 weeks ago.
phpcs6.xml.dist (2.4 KB) - added by jrf 3 weeks ago.

Change History (80)

#1 @pento
3 months ago

  • Description modified (diff)
  • Owner set to pento
  • Status changed from new to accepted

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.

This ticket was mentioned in Slack in #core by pento. View the logs.


3 months ago

#3 follow-up: @markjaquith
3 months 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: @netweb
3 months 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:

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

or

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

#5 in reply to: ↑ 3 ; follow-up: @pento
3 months 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 @netweb
3 months 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: @jdgrimes
3 months 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:

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

or

  1. 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 @pento
3 months 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.

#9 @johnbillion
3 months ago

#41066 was marked as a duplicate.

#10 @westonruter
3 months 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.

#11 @westonruter
3 months ago

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

#12 in reply to: ↑ 5 @johnjamesjacoby
3 months 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.

@yahil
3 months ago

improved coding standards for user.php

#13 follow-up: @pento
3 months ago

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

#14 @pento
3 months ago

phpcs.xml

Yes.

.travis.yml

Yes.

#15 in reply to: ↑ 13 @pento
3 months 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: @jorbin
3 months 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: @boonebgorges
3 months 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: @jorbin
3 months 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 @johnjamesjacoby
3 months 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 @boonebgorges
3 months 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: @jrf
3 months 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.
  • 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?

Last edited 3 months ago by jrf (previous) (diff)

#22 in reply to: ↑ 21 @netweb
3 months 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.

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?

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.


3 months ago

#24 @peterwilsoncc
3 months 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: @pento
3 months 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.

@pento
3 months ago

#26 @pento
3 months 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 @jrf
3 months 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>

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

#28 @jrf
3 months 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:

[Edit] and here are some more:

[Edit 2] and yet more:

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

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

Last edited 3 months ago by jrf (previous) (diff)

#29 in reply to: ↑ 25 ; follow-up: @markjaquith
3 months 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 @pento
3 months 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 @pento
3 months 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 @netweb
3 months 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: @jrf
3 months 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:

Latest PRs fixing some of these things:

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.

#34 in reply to: ↑ 33 @netweb
3 months 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.


3 months ago

#36 follow-ups: @pento
3 months 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?

Last edited 3 months ago by pento (previous) (diff)

#37 in reply to: ↑ 36 ; follow-up: @GaryJ
3 months ago

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

#38 in reply to: ↑ 37 @pento
3 months 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 @jdgrimes
3 months ago

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

#40 in reply to: ↑ 36 @jrf
3 months ago

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
3 months ago

#41 @pento
3 months 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.

#42 @netweb
3 months ago

#41181 was marked as a duplicate.

#43 @desrosj
3 months ago

#41213 was marked as a duplicate.

This ticket was mentioned in Slack in #core-coding-standards by pento. View the logs.


3 months ago

#45 in reply to: ↑ 36 @netweb
3 months ago

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

This ticket was mentioned in Slack in #glotpress by ocean90. View the logs.


3 months ago

@pento
2 months ago

#47 @pento
2 months ago

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

#48 follow-up: @jrf
2 months 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.

@netweb
2 months ago

#49 in reply to: ↑ 48 @netweb
2 months ago

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

#50 @netweb
2 months ago

#41342 was marked as a duplicate.

#51 @netweb
2 months 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 @jrf
2 months 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 @jrf
2 months 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: @GaryJ
7 weeks 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?

@netweb
7 weeks ago

#55 in reply to: ↑ 54 @netweb
7 weeks 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.


5 weeks ago

#57 @netweb
3 weeks ago

#41749 was marked as a duplicate.

#58 @jrf
3 weeks 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 ?

@jrf
3 weeks ago

#59 follow-up: @pento
2 weeks 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 @jrf
2 weeks 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 @peterwilsoncc
2 weeks 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.


2 weeks ago

#63 @dingo_bastard
2 weeks 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.


2 weeks ago

Note: See TracTickets for help on using tickets.