PSR2.Classes.PropertyDeclaration: add T_STATIC to tokens list

[jrchamp]

Description

In addition to scope modifiers, class properties currently have five modifier tokens: T_ABSTRACT, T_FINAL, T_READONLY, T_STATIC and T_VAR. The blame for this sniff does not explain why all but T_STATIC were added to the set of modifier tokens in the search list. T_VAR has been present since the file's creation and the other three were added one at a time as those modifiers were added. To that extent, perhaps T_STATIC was missed because the sniff itself uses getMemberProperties() and code to perform explicit checks on those properties? I'm not sure.

Suggested changelog entry

PSR2.Classes.PropertyDeclaration: add T_STATIC to tokens list

Related issues/external references

Found while discussing #1359

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.
• I have opened a sister-PR in the documentation repository to update the Wiki.

[jrchamp]

I wanted to add a test, but I honestly don't understand why the existing tests aren't flagging this (unless - as mentioned above - the rest of the code is compensating). 😞

[jrfnl]

@jrchamp Thanks for this PR. I'm looking at the code now and as far as I can see, the $find array is only used for determining whether the current variable is part of a multi-property declaration (and bowing out if it is):

PHP_CodeSniffer/src/Standards/PSR2/Sniffs/Classes/PropertyDeclarationSniff.php

Lines 70 to 73 in 8a54aaa

	$prev = $phpcsFile->findPrevious($find, ($stackPtr - 1));
	if ($tokens[$prev]['code'] === T_VARIABLE) {
	return;
	}

In other words, it's trying to find the previous variable if $propB or $propC would be the current $stackPtr, while trying to prevent walking back further than needed (and preventing confusion with other property declarations):

public $propA = null, $propB = 10, $propC;

Now, whether or not static is in that list doesn't make any real difference, other than in efficiency of the sniff (it comes to a conclusion earlier).

Consider the following code

class Foo {
    var $propA;
    static $propB; // <= The `$prev` token for `$stackPtr = $propB` would become the `T_SEMICOLON` on the line before instead of `T_STATIC`, but in both cases, it would be correctly recognized as not part of a multi-property declaration.
}

class Bar {
    static $propB; // <= The `$prev` token for `$stackPtr = $propB` would become the `T_OPEN_CURLY_BRACKET` on the line before instead of `T_STATIC`, but in both cases, it would be correctly recognized as not part of a multi-property declaration.
}

// And even for classes using unconventional class order, it isn't a problem:
class Foo {
	public function bar() {
		$var = 10;
	}

	static $prop = 10; // <= The `$prev` token for `$stackPtr = $prop` would become the `T_SEMICOLON` within the function body.
}

class Foo {
	abstract public function bar($param);

	static $prop = 10; // <= The `$prev` token for `$stackPtr = $prop` would become the `T_SEMICOLON` of the function.
}

class Foo {
	public function bar() {}

	static $prop = 10; // <= The `$prev` token for `$stackPtr = $prop` would become the `T_OPEN_CURLY_BRACKET` of the function.
}

And even if the class uses property hooks, (though that is not currently supported at all yet in PHPCS), it would still not get confused over the static (though it would throw other false positives):

class Foo {
    public string $lc {
        set {
            $this->lc = strtolower($value);
        }
    }

    static $propA; // <= The `$prev` token for `$stackPtr = $propA` would become the `T_SEMICOLON` for the statement in `set()`.

    public string $fullName { get => $this->first . " " . $this->last; }

    static $propB; // <= The `$prev` token for `$stackPtr = $propB` would become the `T_SEMICOLON` for the statement in `get()`.
}

Does this help explain sufficiently why the sniff was working correctly, even though it was missing the T_STATIC token ?

And yes, considering the above analysis, adding extra tests is not strictly necessary as this case is already covered by existing tests, like the test on line 10.

[jrfnl]

@jrchamp Thanks for this PR!

I'm marking this as an efficiency improvement, not a bug, as the sniff behaviour is effectively unchanged. Agreed ?

I will also adjust the changelog entry to reflect that.

[jrfnl]

@jrchamp Thinking this over some more, I think this check can be simplified significantly by just checking the token code of the previous non-empty token before the $stackPtr variable. If it is a T_COMMA, this is a multi-property declaration and we should bow out. Anything else should fall through and be handled by the sniff.

I can't think of any code sample (safe for possibly property hooks, but that's outside of the scope of this PR) in which this would break. Can you ?

That should allow for removing the complete $find array declaration.

What do you think ?

[jrfnl]

@jrchamp Just checking in to see if my review/feedback notes were clear and if you intend to continue with this PR. No pressure, just want to make sure there's nothing blocking you.

[jrchamp]

I agree, the reason this "bug" wasn't caught before is because the code works even without most of the special modifier tokens in the find array. Sorry I didn't get back to you sooner - I've been busy. It seems like you know exactly how you would like to fix this, so please move forward as I feel like my attempt to implement it would take more of your limited time ❤️