Splitting the Core ruleset into Code style & Core

[jrfnl]

Following up on #676:

I've made a start at splitting the ruleset, but would like a second opinion on what should go where.

N.B. the below list includes rules which are currently not covered yet!

Current proposal:

Proposal	Section	Sub-section	Reasoning
Code Style	Single and Double Quotes	quote style
Core	Single and Double Quotes	use of esc_attr
Code Style	Indentation
Code Style	Brace Style
Code Style	Use elseif, not else if
Core	Regular Expressions	PCRE not POSIX
Core	Regular Expressions	/e switch
Code style	Regular Expressions	single quotes
Core + Code style	No Shorthand PHP Tags		Can lead to bugs cross-PHP version.
Code style	Remove Trailing Spaces
Code style	Space Usage
Code style	Formatting SQL statements	capitalize SQL parts
Core	Formatting SQL statements	update vs slashes
Core	Formatting SQL statements	$wpdb->prepare placeholders
Core	Formatting SQL statements	escaping
Core	Database Queries
Code style	Naming Conventions
Core	Self-Explanatory Flag Values for Function Arguments
Code style	Interpolation for Naming Dynamic Hooks	interpolation instead of concatenation
Code style	Interpolation for Naming Dynamic Hooks	use curly braces
Core	Interpolation for Naming Dynamic Hooks	dynamic value names
Core	Ternary Operator
Code style	Yoda Conditions
Tbd	Clever Code		Currently no rules covering this, depending on what will be checked for the ruleset should be determined.
Core	Error Control Operator @
Core	Don’t extract()
Core + Code style	--	BOM
Code style	--	Line ending style
Code style	--	New line end of file
Code style	--	Lowercase constants
Code style	--	Lowercase keywords
Code style	--	Class opening brace
Core	--	I18n

Opinions ?

Practical implications

From a practical point of view, I think we'll need to split the ruleset into two new rulesets - I propose WordPress-Code-Style and WordPress-Core-Basic (or should this be WordPress-Core-Best-Practices ?).

WordPress-Core would then include both.
WordPress-VIP would only include WordPress-Core-Basic

The reason for this is that while we can include a standard and subsequently exclude all rules coming from a specific other standard, as the rules contained in the WP rulesets come from a lot of different standards, this will not work smoothly AFAICS.
By splitting it into two new rulesets, we can overcome this.

[jrfnl]

N.B. One thing we should be aware off when this will be pulled, is that sniffs which now rely in their token walking on code being formatted according to the WP code style rules will no longer work as expected for those rulesets where the code style rules won't be included.

In effect this means that all (non-code style) sniffs will have to be reviewed critically for this.

Example of what could go wrong:

// Line taken from the `is_in_isset_or_empty()` method in WordPress.Sniff
return in_array( $this->tokens[ ( $open_parenthesis - 1 ) ]['code'], array( T_ISSET, T_EMPTY ), true );

This relies on the code being formatted like so:

if ( isset( $something ) && ! empty( $something['key'] ) ) {}

And will not catch code formatted like this - note the extra space before the open parenthesis for isset/empty:

if ( isset ($something) && ! empty ($something['key']) ) {}

The original line would have to be rewritten to read like this for the functionality to be maintained:

$previous_token = $this->phpcsFile->findPrevious( PHP_CodeSniffer_Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true,  null, true);
return in_array( $this->tokens[ $previous_token ]['code'], array( T_ISSET, T_EMPTY ), true );

[JDGrimes]

From a practical point of view, I think we'll need to split the ruleset into two new rulesets - I propose WordPress-Code-Style and WordPress-Core-Basic (or should this be WordPress-Core-Best-Practices ?).

WordPress-Core-Best-Practices is more descriptive, Basic could mean almost anything.

---

On the whole, I think this idea makes sense. But I think we need to be clear about exactly why this is useful/who this benefits. People who are contributing to WordPress core will still need to use the WordPress-Core ruleset (we should probably make that very clear in the docs). So the question is, why do people need just the style rules or just the best practices?

I doubt that many people who are using WordPress code styles will want to exclude all of the best practices, but maybe I'm wrong. It seems that mainly this is geared toward people who don't want the code-style rules but do want to know about best practices, like i18n, SQL escaping, etc. However, those best practices that people who don't care about code-style are likely to care about are a small minority of the rules. I doubt they'll worry about the small stuff, just the larger stuff like i18n and query formatting, etc. Just like 3-5 sniffs. So it would be really easy for these picky people to pick what they want and leave the rest.

If that's the case, would we really be splitting this as they would want or not? I get the feeling that it would start some debate about what goes where, and people would still end up excluding rules from the best practices ruleset.

As far as I can see, the main benefit to splitting this ourselves rather than each person having to construct their own ruleset is that this way they don't have to update their custom ruleset each time WPCS is updated. They'll automatically get the best practice (or code-style) sniffs based on what they want. I'm just not sure that best practices is really going to offer them exactly what they want. If they don't care about the code styles in the handbook, they probably aren't going to care about all of the best practices in the handbook. So if the rule as to what is Extra and what is Core-Best-Practices is what's in the handbook, I think it is probably going to still end up including more than they want. So they'll still end up having us add rules to the ruleset that they want to ignore. Just maybe a bit less.

So again, I'm not against this, it sounds very reasonable, but I'm not entirely convinced where to split this to be the most help to people. I think really we need some more input from the community to know if there is a pattern that has emerged in what people include/exclude as far as the Core ruleset goes. That would give us something more than just hypothetical to go on. If it is something as simple as "what's in the handbook", then I can see us maintaining a split like this. If it is more complex than that, it might be better for various sub-communities to maintain their own rulesets based on their specific needs. If a simple split will help them do that, let's do it, but I'm not convinced that it will.

[jrfnl]

@JDGrimes Thanks for your thoughtful response.

Anyone got ideas of who we should ping for opinions on whether or not to split the ruleset ?

[JDGrimes]

Since #1067 provides an example use-case, I'm happy for us to go ahead with this. 👍

[GaryJones]

I previously said:

I'm not convinced of the value in splitting up WP Core into yet another subgroup. If a rule is in the handbook, then it's in the handbook, regardless of whether it's for security, whitespace or general formatting. The Core standard is already somewhat broken up, by the directories the Sniffs are stored in, and excludes can be done on that basis.

However, I now do think there is value, as folks could choose to follow PSR-2, or PSR-12, or some company code style rules, but can also use a WordPress-Core ruleset that doesn't contain code style checks, to ensure input is sanitized, output is escaped, args are present etc.

As @jrfnl proposed, having the default be to include the code style checks, then we're still recommended that they be followed, but we make it more flexible for developers to at least embrace the really important bits of the checks.

[jrfnl]

Closing as killed by #1157 (proposal 1) not getting enough support.