WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 5 days 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 (15)

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

Change History (71)

#1 @pento
2 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.


2 months ago

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

#41066 was marked as a duplicate.

#10 @westonruter
2 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
2 months ago

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

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

improved coding standards for user.php

#13 follow-up: @pento
2 months ago

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

#14 @pento
2 months ago

phpcs.xml

Yes.

.travis.yml

Yes.

#15 in reply to: ↑ 13 @pento
2 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
2 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
2 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
2 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
2 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
2 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
2 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 2 months ago by jrf (previous) (diff)

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


2 months ago

#24 @peterwilsoncc
2 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
2 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
2 months ago

#26 @pento
2 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
2 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
2 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]:

Version 9, edited 2 months ago by jrf (previous) (next) (diff)

#29 in reply to: ↑ 25 ; follow-up: @markjaquith
2 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
2 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
2 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
2 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
2 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
2 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.


2 months ago

#36 follow-ups: @pento
2 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 2 months ago by pento (previous) (diff)

#37 in reply to: ↑ 36 ; follow-up: @GaryJ
2 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
2 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
2 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
2 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
2 months ago

#41 @pento
2 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
8 weeks ago

#41181 was marked as a duplicate.

#43 @desrosj
8 weeks ago

#41213 was marked as a duplicate.

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


7 weeks ago

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


7 weeks ago

@pento
6 weeks ago

#47 @pento
6 weeks 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
6 weeks 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
6 weeks ago

#49 in reply to: ↑ 48 @netweb
6 weeks 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
5 weeks ago

#41342 was marked as a duplicate.

#51 @netweb
5 weeks 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
5 weeks 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
5 weeks 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
3 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
3 weeks ago

#55 in reply to: ↑ 54 @netweb
3 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 days ago

Note: See TracTickets for help on using tickets.