WordPress-Coding-Standards icon indicating copy to clipboard operation
WordPress-Coding-Standards copied to clipboard

Fixer for missing parentheses adds them to the wrong place in one case

Open GaryJones opened this issue 3 years ago • 7 comments

Bug Description

The fixer for the missing parentheses sniff adds the parentheses to the wrong place for a particular statement.

Minimal Code Snippet

$this->current_provider = new $this->providers->{$this->current_provider_slug}['provider'];

The issue happens when running this command:

phpcs phpcs-test.php --standard=WordPress-Extra -sp --basepath=.

... over a file containing this code (minimal example, extracted from the real code):

<?php
$this->current_provider = new $this->providers->{$this->current_provider_slug}['provider'];

The file was auto-fixed via phpcbf to:

<?php
$this->current_provider = new $this->providers->(){$this->current_provider_slug}['provider'];
// ---------------------------------------------^ ❌ 

... while I expected the code to be fixed to:

<?php
$this->current_provider = new $this->providers->{$this->current_provider_slug}['provider']();
// ---------------------------------------------------------------------------------------^ ✅  

Error Code

WordPress.Classes.ClassInstantiation.MissingParenthesis

Environment

Question Answer
PHP version 8.0.5
PHP_CodeSniffer version Latest git pull of master today (51335eb46)
WPCS version Latest git pull of develop today (a93d2970)
WPCS install type Composer
IDE (if relevant) N/A - CLI.

Additional Context (optional)

Tested Against develop branch?

  • [x] I have verified the issue still exists in the develop branch of WPCS.

GaryJones avatar Mar 16 '22 09:03 GaryJones

When running the fixer again on an already fixed line:

$this->current_provider = new $this->providers->{$this->current_provider_slug}['provider']();

...then the mid-line "fix" gets added again, leading to:

$this->current_provider = new $this->providers->(){$this->current_provider_slug}['provider']();

(two sets of ()).

GaryJones avatar Mar 16 '22 09:03 GaryJones

As a workaround, changing it to:

$class = $this->providers->{$this->current_provider_slug}['provider'];
$this->current_provider = new $class;

results in it being correctly fixed to:

$class                  = $this->providers->{$this->current_provider_slug}['provider'];
$this->current_provider = new $class();

GaryJones avatar Mar 16 '22 09:03 GaryJones

This sniff is on my list of things which need looking into anyway for WPCS 3.0.0 as the rule it applies a candidate for moving to the Core ruleset.

The WPCS sniff does not handle variable variables with new. There is an upstream PSR12 sniff which does, however, that sniff doesn't handle anonymous classes and there are some more differences as also documented in the Extra ruleset:

https://github.com/WordPress/WordPress-Coding-Standards/blob/a93d2970a3694e683588fedaa0f8a901fe148d3b/WordPress-Extra/ruleset.xml#L130-L139

There is already an issue open to discuss this: https://github.com/WordPress/WordPress-Coding-Standards/issues/1884 Might be good to have a look through that and leave an opinion ?

jrfnl avatar Mar 16 '22 10:03 jrfnl

For the fun of it, I just added

		$this->classname_tokens[ \T_OPEN_CURLY_BRACKET ]       = \T_OPEN_CURLY_BRACKET;
		$this->classname_tokens[ \T_CLOSE_CURLY_BRACKET ]      = \T_CLOSE_CURLY_BRACKET;

To the list of classname tokens and ran the phpcbf and got the correct result 😅

Note that I didn't run the tests or anything so this could be a one off fix that breaks other things, so...

EDIT: Added a test and it breaks anonymous classes:

-$util->setLogger( new class() {} );
+$util->setLogger( new class {}() );

So, not really a fix.

dingo-d avatar Mar 16 '22 10:03 dingo-d

@dingo-d Yup, sniffing is tricky... there was a reason variable variables weren't supported (yet) 😉

jrfnl avatar Mar 16 '22 10:03 jrfnl

This is an open source project, which means that this is the personal fiefdom, not really an open community. It's weird that it's included in the WordPress organization. At any rate, there hasn't been a published update in forever, and probably will never be one. But, these assholes will be rude as fuck and call you rude if you ask when it'll be updated and published. Fuck WordPress.

WraithKenny avatar Apr 09 '22 00:04 WraithKenny

@WraithKenny You have been told before and I'm telling you again: this kind of behaviour and language is not tolerated here. Your behaviour is disruptive, rude, abusive and highly demotivating and discouraging for existing contributors (whether currently active or not).

You are put on warning and will be blocked from participating in this repo if you continue.

jrfnl avatar Apr 09 '22 01:04 jrfnl