Add PHPCS

[DJPaul]

In order to help ensure a baseline quality of new code contributions and changes, I'm suggesting we think about adopting PHP Code Sniffer (PHPCS).

PHP_CodeSniffer is a PHP script that tokenises and "sniffs" PHP... to detect violations of a defined coding standard. It is an essential development tool that ensures your code remains clean and consistent. It can also help prevent some common semantic errors made by developers.

There are pretty good (though not perfect) WordPress-specific "sniffs" at ​https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards

In a perfect world, we would have these as a pre-commit hook so Committers don't get lazy, but initially, I assume they'd be hooked up as a Grunt task. I also imagine we'd run with a very minimal set of sniffs so we don't annoy everyone with trivial stuff until we get used to it and/or we all see the value.

Probably the biggest issue is having these code sniffs only report issues with new/changed code, not existing code (our older code is generally not up to the standards of our more recent code), but there are some solutions for this, especially if operating on a Git checkout (see ​https://github.com/xwp/wp-dev-lib/) though there might be something we can do for SVN checkouts as well.

What do people think?

See also :​#WP41057

[netweb]

+1

I believe @tw2113's awesome work on the docs was helped along using PHPCS and ​https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards, maybe wiring up PHPCS for only docs initially would also help in getting it up and running without annoying everyone, then start enabling some sniffs and plucking what's needed from the also awesome "wp-dev-lib" library.

[tw2113]

Most definitely was, awesome tool to help me out.

[DJPaul]

@tw2113 want to share the sniffs config you had for that?

[tw2113]

I just used the copies that come with VVV, no custom tweaking done to them.

[DJPaul]

At Human Made, we've had issues with wp-dev-lib and it being quite brittle. Given that wp-dev-lib would be a key part of this - every file would undoubtably through warnings if we weren't running this only on new changes - i'm going to close this ticket for now and if we/Human Made come up with something a bit more reliable and there's interest, we can re-open.

Or switch to Github and have Travis-CI run PHPCS on pull requests. :p

[netweb]

Replying to DJPaul:

At Human Made, we've had issues with wp-dev-lib and it being quite brittle. Given that wp-dev-lib would be a key part of this - every file would undoubtably through warnings if we weren't running this only on new changes - i'm going to close this ticket for now and if we/Human Made come up with something a bit more reliable and there's interest, we can re-open.

I agree, it's also about to become a lot less generic, it's currently implementing coding standards changes that differ to WordPress Coding Standards.

Or switch to Github and have Travis-CI run PHPCS on pull requests. :p

Or just run PHPCS across the entire codebase and be done with it, WordPress itself is doing it in ​#WP41057

WordPress will also be adding pre-commit hooks as part of this, I've some initial work done for this that looks promising

Anyways, I'll keep BP in mind as I will do for bbPress as this WordPress initiative progresses, when I've got some solid plans I'll reopen this ticket with a proposal.

[DJPaul]

To help audit the Nouveau templates, I've ran a modified version of WP's PHPCS config on it. My current config is at ​https://gist.github.com/paulgibbs/9d6c1f1cfc5d4765852c0e38a711e1e5

There are some temporary rule exclusions purely to make the rest more manageable, but there's still a bunch of style issues I'm not sure what the correct way to resolve.

e.g. the indentation for the paired template "bp_get_x" functions, and:

-                               $data = wp_parse_args( $style, array(
-                                       'dependencies' => array(),
-                                       'version'      => $this->version,
-                                       'type'         => 'screen',
-                               ) );
+                               $data = wp_parse_args(
+                                       $style, array(
+                                               'dependencies' => array(),
+                                               'version'      => $this->version,
+                                               'type'         => 'screen',
+                                       )
+                               );

-<h2 class="bp-screen-title <?php if ( bp_is_group_create() ) { echo esc_attr( 'creation-step-name' ); } ?>">
+<h2 class="bp-screen-title
+<?php
+if ( bp_is_group_create() ) {
+       echo esc_attr( 'creation-step-name' );
+}
+?>
+">

This is not going to be a trivial thing to adopt to BuddyPress.

[johnjamesjacoby]

I'm not sure there's a way to share my feelings on PHPCS without unintentionally discouraging working on it, so just know that isn't my intent – if this is important to you, you should work on it regardless of what I say here. 🤣

My issue with PHPCS in BuddyPress is it is yet-another hurdle that contributors will trip and fall on. I don't think adding it makes building BuddyPress easier or more fun.

I do not like that installing it is neither simple nor obvious, implementing it into an editor or IDE is up to each person to figure out, and nested projects aren't very well supported in most IDEs so they will collide with WordPress core. PHPCS is almost always frustrating to setup, work with, maintain, and then the codebase ends up littered with ugly exception comments anyways – the antithesis of cleaner code.

What I don't want BuddyPress to end up with is ​what WordPress has – its own massive project that takes someone who wants to work with it to 5 different out-of-date websites with conflicting information, and requires this small team to maintain huge rulesets.

I don't mean that as a criticism of WPCS or PHPCS; it's my own strong dislike against a growing library of fragile tooling that cumulatively makes projects like BuddyPress unfun to contribute to. BuddyPress needs a 1-step developer setup experience with crystal clear documentation about dependencies, environments, and be platform friendly and agnostic. And PHPCS specifically needs a functional purpose beyond making the code fit a valueless standard.

It needs to be up to committers to run PHPCS when committing, and patches shouldn't be rejected only because of PHPCS issues. This is a committer tool for a small group of people to take on added responsibility, not a community expectation that one group who can uses against another group who cannot.

I'm not real supportive. I will be critical. Excited to see progress. 😁

[espellcaste]

Thanks John! Your feedback is not discouraging at all, on the contrary. I have a different take and goal about PHPCS for BuddyPress.

I see PHPCS not as a valueless standard, but as an opportunity to catch bugs, or at the very least, to be aware of them and create a plan to fix them. In the long run, we will be making them visible and hopefully fixing them. This is also a good opportunity to set, or follow, some good patterns and standards across the projects.

Some of your concerns I'm not sure we can solve. IDE setup, for example, is very tricky and personal. BuddyPress can't, nor should, be responsible or be concerned with that. Like you said, this should be for each person for figure it out.

... the codebase ends up littered with ugly exception comments anyways – the antithesis of cleaner code.

Another good point. I believe that's inevitable with a project with such a history as BuddyPress. But I'd caution here that the goal is not perfection or following the valueless standard by the book. But to thrive for a better code base, and at the very least, catch ERRORS as soon as possible.

Will we silent or remove some rules? Yes. But ideally, they would be warnings, not errors. But the most important point I could make here is: we need to know where the problem lies. Today, only checking against PHPCompatibilityWP, we can't.

It needs to be up to committers to run PHPCS when committing, and patches shouldn't be rejected only because of PHPCS issues.

I disagree here partly. I think that patches, or commits in Github projects, should not go through if they have errors in their code. They can have warnings, but not errors.

We can run phpcs: checking for errors, in changed files only. And run it completely in the CI. We can also see warnings in the CI and in the terminal but not fail the build/commit/release/etc.

Again, the goal here is to be proactive and catch errors early. They might be annoying but they are necessary for a better code base. This is not only about following rules or standards.

---

Overall, I don't see this as disruptive as you seem to be. Ideally, people will continue to contribute code as they always have been. They might only be bothered if they have an error in their code, something I presume to be a good thing to catch beforehand.

My proposal is the creation of a custom BuddyPress ruleset with packages and rules that should be shared between the BuddyPress projects (including BP itself).

The projects themselves can exclude or add custom rules as needed or desired. But currently everyone is doing or installing their own thing.

The BP REST API uses a strict WordPress ruleset catching all errors, but BuddyPress doesn't. The BP WP-CLI package does their thing, etc. I mean, everyone is doing something differently. Having a common standard should improve overall code quality by following sensible rules.

Sharing future changes across the projects will be easier as well with every project using this custom package/ruleset.

Overall, I think BuddyPress would benefit from it. In the same way we encourage unit tests as much as possible, we should be encouraging following PHPCS as well.

:)

[imath]

I understand @johnjamesjacoby's concerns and I strongly agree with him when he says

and patches shouldn't be rejected only because of PHPCS issues

We should always welcome new contributors, if a patch needs to be improved to comply with code standards, then I believe it's on us members of the team to make this happen.

I believe we can make it an opportunity to welcome new contributors: if we look back in some suggested patches, some were only about applying some WPCS rules.

I believe, we need to lead by example but do it in a nice way: only keep the good of the rules and always welcome contributors patches, we can try to encourage them to make it comply with WP/BP Code standards but we should never make it a blocker for contributors or make it an excuse for us to not review a patch.

Let's take an example of how it can be useful: we agreed for Nouveau to introduce 2 informations into the PHP header's docblock of template packs:

/**
 * Long description etc..
 * @since 3.0.0 (when it was introduced)
 * @version 7.0.0 (the last time it was edited)
 */

I believe if a rule would check this information is accurate it would help us not forget to update it (I often do 😬), and it would help BP advanced users to be aware they need to update their custom templates.

This example is in favor of doing custom sniffs/ruleset. I believe we should be the closest to WP rules so that we can link to their documentation for the wide majority of rules.

[espellcaste]

That's interesting because the example given is one where I don't think a patch should be blocked from being sent/pushed/committed.

It's also important to remember that PHPCS doesn't check for code styling and rules only. There are several levels of severity for errors and warnings and we should strive to not allow errors with severity of 1. Those are the ones that should be blocked from being patched/sent.

If your code has a level 1 severity error, that's something that we should not be committing to the project, regardless if you are a new contributor or a seasoned one.

Today, there is no guardrail for that. Anyone can push whatever they want and there is no indication of problematic code being commited to the project. No only of bugs but also of code styling or standards. Even from an awareness point of view: there is nothing. Literally a wild west!

Here we are relying on the contributors and on the code reviewers, which are error proned since those are humans, to ensure code quality. But again, those are error proned, however how talent or smart they are, or think they are.

We can do better and PHPCS is one sensible step in that direction. :)

[imath]

@renato what about testing PHPCS on a restricted area to begin, like the src/bp-template directory only ?

[espellcaste]

@imath I think that's what's going to happen, eventually. Fixing issues slowling, choosing folders and fixing major issues in them.

I'd rather we don't hardcode verified folders, but exclude the fixed ones.

That's harder to do but ideally, we should be deciding in the beginning the ruleset we will be applying.

I'm mostly concerned with ERRORs, for now. But even that is prone to interpretation.

[imath]

We still need to discuss about this subject. Moving the ticket out of 8.0.0.

[espellcaste]

I'd love to make any progress in this ticket. Specifically, what are the concerns? What can I do to ease the introduction of this tool in the project without disrupting the team or contributions?

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/7228

[espellcaste]

In 13324:

Testing BuddyPress against its own/new PHP Code Sniffer (PHPCS).

The plugin is using its own custom PHP Code Sniffer (​https://github.com/buddypress/bp-coding-standards)

A non-blocking Github Action was introduced to test against this new code sniffer. Actual PHPCS fixes will be introduced in future commits.

Closes ​https://github.com/buddypress/buddypress/pull/21
See #7228

[imath]

Future improvements about PHPCS will wait a bit as 11.0.0's first beta is happening very soon.

• As titled.
• XProfile component only.

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/7228

• As titled.

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/7228

• As titled;
• XProfile component only.

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/7228

• As titled;
• XProfile component only.

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/7228

[espellcaste]

In 13392:

PHPCS: adding a little bit of space between brackets.

Closes ​https://github.com/buddypress/buddypress/pull/44
See #7228

[espellcaste]

In 13393:

PHPCS: adding space after an operator in a if statement.

Closes ​https://github.com/buddypress/buddypress/pull/45
See #7228

[espellcaste]

In 13394:

PHPCS: adding comma to the last array item.

Closes ​https://github.com/buddypress/buddypress/pull/46
See #7228

I think I need to investigate this a bit further.

See ​1 and ​2 for possible issues.

[espellcaste]

I released the 0.1.0 version a few weeks ago: ​https://github.com/buddypress/bp-coding-standards/releases/tag/v0.1.0

There will be a new version soon for the new WCS changes.

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/7228

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/7228

[imath]

In 13799:

Members component: improve PHP code standards using WPCS

See #7228 (trunk)

[imath]

In 13800:

Members component: improve PHP code standards using WPCS

See #7228 (branch 12.0)

[espellcaste]

In 13801:

Upgrade the WordPress Coding Standards for PHP_CodeSniffer for the latest 3.0 version.

See #7228

Commited here: ​https://buddypress.trac.wordpress.org/changeset/13801

[espellcaste]

Upgraded the WordPress Coding Standards for PHP_CodeSniffer for the latest 3.0 version in the BP REST API: ​https://github.com/buddypress/BP-REST/pull/505

[imath]

In 13802:

Blogs component: improve PHP code standards using WPCS

See #7228 (trunk)

[imath]

In 13803:

Blogs component: improve PHP code standards using WPCS

See #7228 (branch 12.0)

[imath]

In 13804:

Friends component: improve PHP code standards using WPCS

See #7228 (trunk)

[imath]

In 13805:

Friends component: improve PHP code standards using WPCS

See #7228 (branch 12.0)

[imath]

In 13806:

xProfile component: improve PHP code standards using WPCS

See #7228 (trunk)

[imath]

In 13807:

xProfile component: improve PHP code standards using WPCS

See #7228 (branch 12.0)

[imath]

In 13808:

Groups component: improve PHP code standards using WPCS

See #7228 (trunk)

[imath]

In 13809:

Groups component: improve PHP code standards using WPCS

See #7228 (branch 12.0)

[imath]

In 13810:

Messages component: improve PHP code standards using WPCS

See #7228 (trunk)

[imath]

In 13811:

Messages component: improve PHP code standards using WPCS

See #7228 (branch 12.0)

[imath]

In 13812:

Notifications component: improve PHP code standards using WPCS

See #7228 (trunk)

[imath]

In 13813:

Notifications component: improve PHP code standards using WPCS

See #7228 (branch 12.0)

[imath]

In 13814:

Settings component: improve PHP code standards using WPCS

See #7228 (trunk)

[imath]

In 13815:

Settings component: improve PHP code standards using WPCS

See #7228 (branch 12.0)

[imath]

In 13816:

Activity component: improve PHP code standards using WPCS

See #7228 (trunk)

[imath]

In 13817:

Activity component: improve PHP code standards using WPCS

See #7228 (branch 12.0)

[imath]

In 13818:

Core: improve PHP code standards using WPCS

See #7228 (trunk)

[imath]

In 13819:

Core: improve PHP code standards using WPCS

See #7228 (branch 12.0)

[imath]

In 13820:

BP Nouveau: improve PHP code standards using WPCS

See #7228 (trunk)

[imath]

In 13821:

BP Nouveau: improve PHP code standards using WPCS

See #7228 (branch 12.0)

[imath]

In 13822:

BP Legacy: improve PHP code standards using WPCS

See #7228 (trunk)

[imath]

In 13823:

BP Legacy: improve PHP code standards using WPCS

See #7228 (branch 12.0)

[imath]

In 13824:

Deprecated functions: improve PHP code standards using WPCS

See #7228 (trunk)

[imath]

In 13825:

Deprecated functions: improve PHP code standards using WPCS

See #7228 (branch 12.0)

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/7228

[imath]

Hi @espellcaste

I just had a look at ​PR #282, it's looking great, thanks for your work on it, let's have it in!

[espellcaste]

In 13850:

Improve handing of PHP code standards using WPCS

Until we have all WPCS warnings and errors fixed, we block pull requests, and releases, with code with escaping or/and PHP Compatibility issues.

For local development only, we set WPCS severity to 1 to see everything that is not effectively turned off.

Run composer run phpcs for showing all WPCS issues.
Run composer run phpcs-escape for showing escaping issues.
Run composer run phpcompat for showing PHP Compatibility issues.

See #7228
Closes ​https://github.com/buddypress/buddypress/pull/282/