Skip to:
Content

BuddyPress.org

Opened 5 years ago

Last modified 33 hours ago

#7228 reopened enhancement

Add PHPCS

Reported by: DJPaul Owned by:
Milestone: Up Next Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords:
Cc: contato@…

Description (last modified by netweb)

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

Change History (28)

#1 @netweb
5 years ago

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

#2 @tw2113
5 years ago

Most definitely was, awesome tool to help me out.

#3 @DJPaul
5 years ago

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

#4 @tw2113
5 years ago

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

#5 follow-up: @DJPaul
4 years ago

  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from new to closed

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

#6 in reply to: ↑ 5 @netweb
4 years ago

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.

This ticket was mentioned in Slack in #buddypress by netweb. View the logs.


4 years ago

#8 @espellcaste
4 years ago

  • Cc contato@… added

#9 @DJPaul
4 years ago

  • Milestone set to Awaiting Contributions
  • Resolution maybelater deleted
  • Status changed from closed to reopened

#10 @DJPaul
4 years ago

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.

#11 @netweb
4 years ago

  • Description modified (diff)

This ticket was mentioned in Slack in #buddypress by slaffik. View the logs.


11 months ago

#13 @imath
10 months ago

  • Milestone changed from Awaiting Contributions to 8.0.0

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


9 months ago

#15 @johnjamesjacoby
9 months ago

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

#16 @espellcaste
9 months ago

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.

:)

This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.


9 months ago

#18 @imath
9 months ago

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.

#19 @espellcaste
9 months ago

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

This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.


9 months ago

#21 @imath
7 months ago

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

#22 @espellcaste
7 months ago

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

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


7 months ago

#24 @imath
7 months ago

  • Milestone changed from 8.0.0 to Up Next

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

This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.


4 months ago

#26 @espellcaste
4 months ago

  • Milestone changed from Up Next to 10.0.0

This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.


5 weeks ago

#28 @espellcaste
33 hours ago

  • Milestone changed from 10.0.0 to Up Next
Note: See TracTickets for help on using tickets.