search

squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.66k stars 1.48k forks source link

Squiz.PHP.DisallowMultipleAssignments error on code with comparison and assignment #1679

Closed paulschreiber closed 6 years ago

paulschreiber commented 6 years ago

I updated PHPCS to 3.1.0 and I’m now getting a lot of Assignments must be the first block of code on a line errors.

They’re on code like this:

if ( ( $key = array_search( 'tag', $classes, true ) ) !== false ) {

Are you intending to prohibit these sorts of expressions? If so, it would be good to use a different type of error message. If not, can you fix this?

jrfnl commented 6 years ago

Updated from what version ? This sniff has not been changed in v 3.1.0.

paulschreiber commented 6 years ago

Upgraded from 2.7.1.

gsherwood commented 6 years ago

That code construct has always been banned by the Squiz.PHP.DisallowMultipleAssignments sniff.

I have checked this on version 3.1.0 (latest), master, 2.7.1, and even went back to version 1.5.5. All versions have the same check and show the same error.

So I don't think your new errors are coming from the upgrade itself. Are you using a custom coding standard? Did it change? Can you make it available for testing?

paulschreiber commented 6 years ago

I'm using the WordPress-VIP coding standard. Odd that I hadn't caught these before. I've fixed them in my code.

If this is intended to be disallowed, PHPCS should do a better job of distinguishing between these two types of errors:

Multiple assignments:

$a = $b = false;

Assign and compare

if ( ( $key = array_search( 'tag', $classes, true ) ) !== false ) {
gsherwood commented 6 years ago

PHPCS should do a better job of distinguishing between these two types of errors

The sniff reporting the error doesn't make a distinction because it doesn't need to. If you think the WordPress coding standard should make a distinction, you could ask over there.

I'll close this as the sniff is working as intended and the behaviour has not changed during the upgrade. It's likely that the coding standard itself changed.

paulschreiber commented 6 years ago

It's likely my ruleset/exclusions changed.

I don't think WPCS should make a distinction here, as its rule isn't the one being violated. Since it's a Squiz.PHP rule, PHPCS would be a better place to make that distinction.