Split the ClassInstantiation sniff

[jrfnl]

Rationale

WPCS currently has its own ClassInstantiation sniff.
Since PHPCS 3.5.0, the PSR12 standard in PHPCS itself also contains a PSR12.Classes.ClassInstantiation sniff.

I've done a comparison of the two sniffs (and some others in other external standards) to see if the sniff could be swopped out and/or moved to PHPCSExtra.

These are the results, which are in line with a previous comparison documented inline in the ruleset since #1587:

Checks	WPCS sniff	PSR12	Slevomat	Symfony
Class instantiated using parentheses	✔️ (has bug with variable variable class instantiation)	✔️	✔️	✅ (very buggy)
.. including anonymous classes	✔️	❌	❌	✔️
.. including JS classes	✔️	❌	❌	❌
No space between the class keyword and the open parenthesis	✔️	❌ (partially covered via the PEAR.Functions.FunctionCallSignature sniff which is included in WPCS)	❌	❌
No new by reference	✔️	❌	❌	❌

Conclusions:

• PHPCSExtra will not support sniffs which include JS checks, so moving it there as-is, is not an option.
• Using the PSR12 sniff would remove the JS check + the anon class check + the new by reference check, so is not (yet) an option.

Other considerations:

• The "new by reference" check is a bit of an odd one out.
  PHPCompatibility covers this, so we could consider removing that check from WPCS or moving it out of this sniff to a separate sniff.
• PHPCS 4.0 will no longer support JS tokenizing, so once WPCS starts supporting PHPCS 4.0, we need to remove the JS check anyway.
  We could choose to remove that check already as with the improvements made to the JS tooling over time, WPCS is not used for JS that much anymore anyway.
• PHPCS has other sniffs which (largely) cover the spacing before the parentheses (though not completely).
• An attempt could be made to see if adding the parentheses check for anon classes to the upstream sniff would be accepted, but I have my doubts as it is not explicit in PSR12.
  Alternatively that check could be split off to a separate sniff.

Proposal:

• In WPCS 3.0.0 split the sniff in 2/3:
  1. ClassInstantiation
  2. ClassInstantiationSpacing
  3. NewByReference (if we decide to keep this check)
• At some point (not necessarily 3.0.0) add a sniff which covers anon class declarations and move the parentheses check for anon classes there.
  A sniff like that should probably go into PHPCSExtra.

Doing the initial splitting of the sniff now, will allow us to swop out the sniff for the PSR12 one once PHPCS 4.x has been released and WPCS starts supporting it.

Alternatively, this can be done in the WPCS version which would start to support PHPCS 4.x, but as it's a breaking change, it would have to be done in a major release.

Open questions

• Do we want to keep the "new by reference" check ?
• Do we want to keep the support for checking JS in this sniff until PHPCS 4.x has been released ?
• Split now or later ?

Depending on the answers this issue could be added to the 3.0.0 milestone.

References

• https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.5.0
• Loosely related issue: Creating a new Anonymous class require Parenthesis. #1721

Action Checklist

• tbd

[dingo-d]

My two cents:

Do we want to keep the "new by reference" check ?

Isn't that like PHP4 thing? You've mentioned that PHP compatibility already has this check, so I'd remove that altogether.

Do we want to keep the support for checking JS in this sniff until PHPCS 4.x has been released ?

I'm all for removing JS checks from WPCS. It just adds unnecessary complexity, and the JS tooling like ES Lint is better suited for checking JS files than PHPCS imo.

Split now or later?

Now if the maintenance burden will be lessened in the future 🙂

[jrfnl]

Update: PHPCSExtra already contains a sniff to require parentheses for anonymous class instantiations (will be included in the alpha4 release), so that part is already sorted.

So if we drop the "new by reference" check and drop support for JS, then all that's left to do would be to:

• Create a sniff for the spacing between the parenthesis and/or find an appropriate existing sniff which handles that.
• Remove the WPCS native sniff.
• Include the PSR12 sniff + the PHPCSExtra sniff.

[jrfnl]

PHPCSExtra alpha 4 will now also contain a Universal.WhiteSpace.AnonClassKeywordSpacing sniff which specifically handles the whitespace between the class keyword and the open parenthesis. The sniff is configurable, so we can set the spacing to 0 (= default value anyhow).

I've also now pulled a PR to PHPCS upstream to improve the PEAR.Functions.FunctionCallSignature sniff to also handle the object instantiation for anonymous classes: squizlabs/PHP_CodeSniffer#3636

Providing we drop support for JS and the "new by reference" check (which is a parse error as of PHP 7.0 anyhow), all prelim requirements for this issue to be actioned have now been fulfilled.