PHPCS 3.0.0 compatibility.

[mattheu]

The new version of PHP CodeSniffer has some nice features, but introduces breaking changes which mean that the WordPress coding standards are not compatible.

I had a quick look, and I think the minimum required is to namespace the WPCS codebase and import dependencies to ensure everything is autoloaded.

For reference:

• Alpha release notes with more info on new features
• Upgrade guide
• RC1 release notes

[westonruter]

The namespace requirement means we'll need to drop support for PHP 5.2. I'm not heartbroken by that, but Travis builds on PHP 5.2 will need to skip running PHPCS.

[grappler]

It seems they have a minimum requirement as PHP 5.4. Do we know when 3.0 is planned to be released?

[jrfnl]

I'd be cautious of upping the minimum PHPCS to 3.0 for now as other standard libraries - for example PHPCompatibility - which may be used in conjunction with this one may not be compatible with it (yet) and this could cause issues.

Might it be an idea to create a separate PHPCS 3.0 branch (similar to how PHPCS itself is currently doing it) instead for now ?

Do we know when 3.0 is planned to be released?

AFAIK there is no public release planning for the new version, so probably "when it's ready"

[JDGrimes]

I'd be cautious of upping the minimum PHPCS to 3.0 for now as other standard libraries - for example PHPCompatibility - which may be used in conjunction with this one may not be compatible with it (yet) and this could cause issues.

Might it be an idea to create a separate PHPCS 3.0 branch (similar to how PHPCS itself is currently doing it) instead for now ?

That sounds like work. 😄 I'd say wait until 3.0 is actually released, and then plan to be compatible with it in our next release after that. Anybody who needs to use sniffs that aren't compatible with PHPCS 3.0 yet can just use an older version of WPCS for a bit. We can back-port more bugfixes if we want to, but I don't fancy juggling two branches and backporting all new features. If it is worth it to particular folks, of course, they are welcome to do so, but I have a feeling it would just get tedious after a while.

[GaryJones]

I think the consensus is that yes, we want it, but adding it in before 3.0 is released, is not desired.

PHPCS 3.0 RC2 has been tagged, so it won't be long.

[sandrodz]

I'm running phpcs inside docker container, so I need new --basepath= option to run it correctly. Its only available in 3.x version.

Is there any action plan regarding phpcs 3.0 support? thanks.

[jrfnl]

@sandrodz As stated above: once PHPCS 3.0 has been released, i.e. is no longer in alpha/beta/RC.

[jrfnl]

FYI: PHPCS 3.0 has just been released. https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.0.0

[westonruter]

PHPCS 3.0.0 broke WPCS on the core-media-widgets plugin. See xwp/wp-dev-lib#234

[JDGrimes]

As I understand it, in order to be compatible with 3.0.0, the changes necessary would be so great that we'd no longer be compatible with 2.*, am I right? It doesn't really matter to me, but I just want to be clear about it.

I suggest that we release 0.12.0, and then make 0.13.0 the release that just updates to be compatible with 3.0, not really adding any other features.

The update to 3.0 will break most open PRs, so ideally we would try to get them merged into 0.12.0, I guess.

Does anybody else have thoughts?

[jrfnl]

As I understand it, in order to be compatible with 3.0.0, the changes necessary would be so great that we'd no longer be compatible with 2.*, am I right?

Generally speaking: yes.

However.... for the PHPCompatibility standard we're going to attempt to make the standard compatible with both PHPCS 2.x as well as PHPCS 3.x. See PHPCompatibility/PHPCompatibility#367
If we get that working, I could implement the same for WPCS 😎

The reason - at least for PHPCompatibility - to try and be compatible with both, is that it is often used on servers where the users does not have the rights to upgrade the PHPCS install.

As for WPCS, if it is possible and not too hard to do, it would be good to have dual PHPCS compatibility for at least the next six months or so, as it can be a lot of work to upgrade a custom standard, so people who use WPCS combined with other custom standards, may not be able to upgrade to PHPCS 3.x yet and would run into issues using two standards with different requirements.

I suggest that we release 0.12.0, and then make 0.13.0 the release that just updates to be compatible with 3.0, not really adding any other features.

👍 It would be nice if #826, #840, #858 and possibly #843 could be coached to be ready for merge so these could still go in that release, though the current state of 0.12.0 is quite feature rich/ready for release already.

The update to 3.0 will break most open PRs, so ideally we would try to get them merged into 0.12.0, I guess.

That would depend on whether the dual compatibility would be possible/considered for WPCS. If it is, current PRs should probably still be fine with only some minor adjustments.

[grappler]

I suggest that we release 0.12.0, and then make 0.13.0 the release that just updates to be compatible with 3.0, not really adding any other features.

Is there a reason not to do a major version update as it is breaking change? e.g. version 1.0

[JDGrimes]

Is there a reason not to do a major version update as it is breaking change? e.g. version 1.0

There is general agreement that 1.0.0 should denote that WPCS is feature complete as regards WordPress's core coding standards. See #519 (comment). I don't think that we are there yet, although I did have a similar thought about a potential 1.0 here.

[jrfnl]

There is general agreement that 1.0.0 should denote that WPCS is feature complete as regards WordPress's core coding standards. See #519 (comment). I don't think that we are there yet, although I did have a similar thought about a potential 1.0 here.

Maybe it is time to revisit that discussion ? Looking at the annotated WP Core ruleset, we currently cover ~90% and some of the remaining things can not easily be covered by static analysis tools.

[JDGrimes]

However.... for the PHPCompatibility standard we're going to attempt to make the standard compatible with both PHPCS 2.x as well as PHPCS 3.x. See PHPCompatibility/PHPCompatibility#367
If we get that working, I could implement the same for WPCS 😎

OK, I'd be in favor of that. I had considered the possibility of a shim, but wasn't sure if it was possible/worth it. If you believe that it is, then by all means lets give it a try. If what is needed is mostly a shim, then couldn't it be developed separately from any one standard, and then people that needed it could use it with WPCS or custom standards or whatever they needed to use it with? Although I gather that some things might not be possible just via a shim, so WPCS might still need to be updated to be compatible with the shim.

Anyway, I guess that for right now WPCS will just wait to see what is possible in that regard, and then make a decision as to what to support and how long. In the mean time we can try to get some of those PRs merged.

[jrfnl]

WPCS might still need to be updated to be compatible with the shim.

Correct. WPCS would still need to be updated, the precise details I can't yet tell you, but I expect they will be along the lines of:

• namespacing all sniffs
• removing/replacing the type hints referring to upstream classes (that is the most difficult issue to solve for dual compatibility as it causes function signature mismatch issues)
• possibly changing some of the references to upstream classes to references to the shim classes.

More info will be available once I've had time to attempt this for PHPCompatibility.

[jrfnl]

I've started looking into this and while I think I can get it working, I would suggest waiting a bit as I've already found numerous bugs in the 3.0/3.0.1 release which should be fixed (merged) first as they will probably impact users of WPCS as well.
squizlabs/PHP_CodeSniffer#1450
squizlabs/PHP_CodeSniffer#1451

Edit:

And another one which is a prerequisite for getting cross-version compat working:
squizlabs/PHP_CodeSniffer#1452

Edit:

And one more: squizlabs/PHP_CodeSniffer#1453

In other words: solutions to all four issues will need to be merged upstream and we need to wait for an upstream release containing these fixes before we can release a PHPCS 3.x compatible version.

[jrfnl]

Short update about my current line of thinking.

• Create a PHPCS cross-version compatible version using the namespace WordPress (as otherwise it will not work with PHPCS 2.x)
• Announce that PHPCS 2.x support will be dropped approx 1 year after the release of the PHPCS 3.x compatible version, August 31, 2018 at the latest. This allows other standards which build onto WPCS plenty of time to upgrade their standard.
• 1 year after the release, drop PHPCS 2.x support and prefix the namespace with WordPressCS.

That way, while using the WordPress namespace is "dirty" for now (but the only possible solution for cross-version compatibility), we will have released the namespace again by the time WP itself potentially would start to use that namespace.

@WordPress-Coding-Standards/wpcs-collaborators Please leave an opinion 👍 / 👎 about this.

[jrfnl]

I need some help. Bright ideas welcome.

We currently extend the upstream Squiz\Sniffs\Arrays\ArrayDeclarationSniff, though recently I have split off quite a few things which were original done by that sniff into separate sniffs.

We overload one method in the WPCS version: processMultiLineArray() and exclude quite a few of the errorcodes from the other methods.

In PHPCS 2.x the function declaration of the method we overload is:

public function processMultiLineArray(PHP_CodeSniffer_File $phpcsFile, $stackPtr, $arrayStart, $arrayEnd) {}

In PHPCS 3.x the type hint for the $phpcsFile parameter has been dropped.

If I use a typehint (aliased or not), PHPCS 3.x will throw a Declaration of ... should be compatible with ... warning.
If I don't use a typehint, PHPCS 2.x will throw that warning.

Anyone has any bright ideas how to get round this ?

If worse comes to worst, we'll need to uncouple from the upstream sniff and have one or more WPCS native sniffs for the few things which are still left over in the sniff:

• WordPress.Arrays.ArrayDeclaration.SpaceAfterKeyword
• WordPress.Arrays.ArrayDeclaration.NoSpaceBeforeDoubleArrow
• WordPress.Arrays.ArrayDeclaration.NoSpaceAfterDoubleArrow
• WordPress.Arrays.ArrayDeclaration.CloseBraceNewLine
• WordPress.Arrays.ArrayDeclaration.SpaceInEmptyArray
• WordPress.Arrays.ArrayDeclaration.FirstIndexNoNewline
• WordPress.Arrays.ArrayDeclaration.SpaceBeforeDoubleArrow
• WordPress.Arrays.ArrayDeclaration.SpaceAfterDoubleArrow
• WordPress.Arrays.ArrayDeclaration.FirstValueNoNewline

[grappler]

The idea that I came up with is to create two different sniffs for each version. It would require loading a specific class depending on the version. I have not tested this. Don't worry about telling me I am wrong.

class WordPress_Sniffs_Arrays_ArrayDeclarationSniff extends Squiz_Sniffs_Arrays_ArrayDeclarationSniff {
	public function newProcessMultiLineArray( $phpcsFile, $stackPtr, $arrayStart, $arrayEnd ) {
		// ... all of the code ...
	}
}

class WordPress_Sniffs_Arrays_ArrayDeclaration29Sniff extends WordPress_Sniffs_Arrays_ArrayDeclarationSniff {
	public function processMultiLineArray( PHP_CodeSniffer_File $phpcsFile, $stackPtr, $arrayStart, $arrayEnd ) {
		$this->newProcessMultiLineArray( $phpcsFile, $stackPtr, $arrayStart, $arrayEnd );
	}
}

class WordPress_Sniffs_Arrays_ArrayDeclaration30Sniff extends WordPress_Sniffs_Arrays_ArrayDeclarationSniff {
	public function processMultiLineArray( $phpcsFile, $stackPtr, $arrayStart, $arrayEnd ) {
		$this->newProcessMultiLineArray( $phpcsFile, $stackPtr, $arrayStart, $arrayEnd );
	}
}

[jrfnl]

@grappler Thanks for taking the time to think about this.

Unfortunately I don't think this will get us to a solution.

We cannot have one ruleset for PHPCS 2.x and another for PHPCS 3.x, so that means that both sniffs would have to be included in the WordPress-Core ruleset - with the full list of exclusions for the sniffs - and all three sniffs would automatically be included in the WordPress ruleset.

So that still doesn't get us round the problem, which is that PHP will throw the warning when it reads in the class - not when the function is called. As all three classes would always be loaded, that means we'd still have one or the other throw the Declaration of ... should be compatible with ... warning.

See: https://3v4l.org/Elptb

[Rarst]

Without looking through the code my quick guess would be that one way or another this will error out if sticking with inheritance/overloading.

Not sure without trying if composition makes sense? Use upstream class internally within WPCS class, without subclassing?

[jrfnl]

@Rarst Thanks you too for helping me think of other solution directions.

Just to make sure I understand you correctly, you mean doing something like this ?

class WPCS_Class implements Sniff {
 	protected $upstream;

 	protected function get_upstream_object() {
 	 	$this->upstream = new Squiz_Class();
 	 	// Use the object here or in other methods.
 	}
}

The upstream class has four methods:

• register() - which we could just call in that case
• process() - which we could not call as it will call the next two methods automatically if we do and we don't want that. This method contains two checks we want and some process logic.
• processSingleLineArray() - yes, this method could be called, however, out of the 9 checks in that method, we exclude 5 from the ruleset. I'm also not sure (I'd need to test this), whether the error codes would still identify as the WPCS sniff in that case or whether they would be thrown as if they were errors from the upstream sniff (most likely). If the latter, that would break custom rulesets referencing these error codes. Not necessarily an issue, but something we need to be aware of.
• processMultiLineArray() - this is the method we currently overload. This method in the upstream sniff contains 17 checks. Our overloaded method comments out 12 of these, leaving 5 checks still active.

So having said that, in effect, it would only be the 4 checks in processSingleLineArray() which could still be taken from upstream. Everything else would be duplicated already. And of those 4, the error code would most likely change using this method.

So I think in that case, it would make more sense having one or more WPCS native sniffs to sort this out.

[GaryJones]

So I think in that case, it would make more sense having one or more WPCS native sniffs to sort this out.

Using only 9, out of the 26 upstream errors, would suggest the WPCS native sniffs would be the way to go here.

Wild idea: Could we change the PHP error reporting level to remove E_STRICT right before the method signature line / before the file is loaded, and restore it afterwards?

[Rarst]

Don’t mess with error level, it’s not worth it and is major code smell.

[jrfnl]

Wild idea: Could we change the PHP error reporting level to remove E_STRICT right before the method signature line / before the file is loaded, and restore it afterwards?

@GaryJones I like the wickedness of the idea, unfortunately that would not help us.
In PHP 5.6 this throws an E_STRICT, but in PHP 7+, it throws an E_WARNING. See: https://3v4l.org/Elptb
Fun fact: the warning will actually be removed in PHP 7.2... See: https://wiki.php.net/rfc/parameter-no-type-variance

[jrfnl]

Status update:

Sub-project	PHPCS 2.x	PHPCS 3.x
Running the sniffs	✅	✅
Running the unit tests	❌	✅

Upcoming:

Things which will need to be addressed before PHPCS 3.x compatibility can succeed;

• Test bleed through - PR Unit tests: Prevent bleed through between unit tests. #1042
• Minor refactor of part of two of the abstract sniffs. These use a function which will no longer be available in PHPCS 3.x - ready: can be pulled once Unit tests: Prevent bleed through between unit tests. #1042 has been merged PR PHPCS 3.x prep: work round a function which will no longer exist #1045
• Array Declaration sniff refactor - WIP PR Remove the ArrayDeclaration sniff #1044

PHPCS 3.x compatibility:

• The big one - 90% ready, working except for the unit tests in combination with PHPCS 2.x PR Make WPCS compatible with PHPCS 3.x #1047
• Remove PHP 5.2 work-arounds and test skipping (no longer needed as PHP 5.2 support will be dropped) - ready to be pulled after the "big one" PR PHPCS 3.x compat: remove php 5.2 work arounds and PHP 5.2 specific code #1050
• Respect --ignore-annotations (Respect --ignore-annotations #862) - ready to be pulled after the "big one" PR PHPCS 3.x compat: Implement recognition of the --ignore-annotations setting #1051

[jrfnl]

Status Update:

Sub-project	PHPCS 2.x	PHPCS 3.x
Running the sniffs	✅	✅
Running the unit tests	✅ 🎉	✅

Once #1042 and #1044 are merged, I'll pull the third prep PR.
Once that one is also merged, I will rebase my WIP branches & pull the other three in turn.

Technical notes:

The problem with the PHPCS 2.x unit tests was that the PHPCS 2.x unit test suite can not deal with namespaced unit test classes, which is a requirement for PHPCS 3.x.

The way I've solved this now is a bit hacky:

• Some minor changes in two places in the PHPCS 2.x unit test code solves this.
• However I highly doubt that a change like that would still be allowed into PHPCS 2.x. Quite apart from the fact that we'd then need to wait for PHPCS 2.9.2 to come out before we can release a WPCS version which is compatible with PHPCS 3.x and that unit testing with earlier 2.9.x versions which we currently still support, will no longer be possible.
• So having a WPCS version of select classes from the PHPCS 2.x test suite which overload the relevant methods looked like the only way to get this working.
• However, obviously (sigh) one of the methods which needed overloading was declared as final... so instead of just selectively overloading two methods, we need a full copy of one of the classes.

This also means that the commands to run the unit tests will not be the same for PHPCS 2.x / 3.x while we still support both. I will update the contributing.md file as part of the PR with the new instructions.

Creative ideas for alternative solutions to this are welcome and who knows we might find a more elegant way, but I can't think of any at this moment. Just glad that I (finally) got the last bit working 😎

[GaryJones]

#1042 and #1044 are now merged.

[jrfnl]

Thanks. #1045 is the next one.

[GaryJones]

#1045 is merged.

[jrfnl]

Thanks. Rebasing & tidying up the main PR now. This may take a little while.

[jrfnl]

Rewriting history: should or shouldn't @since comments which refer to the "old" class names of sniffs be fixed ? Opinions ?

N.B. @use, @see and @param comments have been fixed, but that's a different matter.

[JDGrimes]

What I do sometimes in refactoring like this:

Before:

/**
 * @since 0.12.0
 */

After:

/**
 * @since 0.12.0 As WordPress_Arrays_ArrayDeclarationSpacingSniff.
 * @since 0.13.0
 */

I think that preserving the history while also noting when the sniff itself was originally introduced is helpful. This means that you can also keep the other @since comments that document changes, if they are still useful.

[jrfnl]

@JDGrimes I'm not sure we're talking about the same thing here...?

I meant more, what to do about comments like:

 * @since   0.11.0 Extends the WordPress_Sniff class.

Should this be changed to the below ?

 * @since   0.11.0 Extends the \WordPress\Sniff class.

As all the class names have been changed, a lot of comments still refer to the old classnames. Should those be updated or not ?
Or maybe if it's more about the sniff than about the class, changed to WordPress.Category.SniffName format ?

A secondary question would be: should all classes get a @since 0.13.0 This class is now namespaced. comment ?

[GaryJones]

Should this be changed to the below ?

No - leave as the original @since, with the original comment (don't change class names to namespaced FQCN).

should all classes get a @since 0.13.0 This class is now namespaced. comment ?

I'd initially say No, but https://github.com/phpDocumentor/fig-standards/blob/master/proposed/phpdoc.md#717-since suggests otherwise.

[jrfnl]

I'd initially say No, but https://github.com/phpDocumentor/fig-standards/blob/master/proposed/phpdoc.md#717-since suggests otherwise.

Which is why I am in two minds about it. It just feels a bit redundant adding the same comment in every single class (118 x).

[JDGrimes]

Should this be changed to the below ?

No.

should all classes get a @since 0.13.0 This class is now namespaced. comment ?

I'd say yes. I mean, it is technically a different class, because it has a different FQCN. The PHPDoc is not for the sniff, it is for the class itself. So it should document the version that the FQCN was introduced. It is redundant now, but in the future as new sniffs are introduced anybody will be able to see at a glance which classes were namespaced from the start and which were not.

This is what I was answering above, I should have read your question more closely. 😄