WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.56k stars 487 forks source link

AbstractClassRestrictionsSniff fails to consider T_SELF, T_STATIC, maybe others before double-colon #2184

Closed anomiex closed 1 year ago

anomiex commented 1 year ago

Bug Description

This code https://github.com/WordPress/WordPress-Coding-Standards/blob/dcbb3ec50911a554c9a4a7876dccb11419dda4e9/WordPress/AbstractClassRestrictionsSniff.php#L138-L142 is attempting to extract the class name from a static call like Class::method(), by looking for preceding string tokens. However, when the call is like self::method(), the "self" is a T_SELF token rather than a string token. Same for static::method(), the "static" is a T_STATIC.

In these cases, the logic skips over the T_SELF or T_STATIC (and various other tokens) until it finds a string, and uses that instead.

I don't know whether there are other tokens that might also appear before a double-colon token and cause similar problems.

Minimal Code Snippet

I'm not sure if this is the best reproduction, but it's where I started in tracking this down.

The issue happens when running this command:

vendor/bin/phpcs -s --standard=WordPress --sniffs=WordPress.WP.ClassNameCase test.php

... over a file containing this code:

<?php

class Foo {
    public function bar() {
        wp_styles();
        self::foo();

        wp_styles();
        static::foo();
    }
}

Error Code

FILE: /tmp/test.php
------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 6 | WARNING | It is strongly recommended to refer to classes by their properly cased name. Expected: WP_Styles Found: wp_styles
   |         | (WordPress.WP.ClassNameCase.Incorrect)
 9 | WARNING | It is strongly recommended to refer to classes by their properly cased name. Expected: WP_Styles Found: wp_styles
   |         | (WordPress.WP.ClassNameCase.Incorrect)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Custom ruleset

n/a

Environment

Question Answer
PHP version 8.0.26
PHP_CodeSniffer version 3.7.1
WPCS version dev-develop dcbb3ec
WPCS install type Composer project local
IDE (if relevant) None

Additional Context (optional)

I don't think the $local parameter to findPrevious() on line 139 will be sufficient here, as it would still FP on something like func( wp_styles(), self::method() ).

Tested Against develop branch?

GaryJones commented 1 year ago

Hi @anomiex,

Your PHPCS output snippet includes "Expected: WP_Styles Found: wp_styles" - so there's no issue with the self::foo() and static::foo(), and there's a unit test which covers exactly that. The logic for it is here:

https://github.com/WordPress/WordPress-Coding-Standards/blob/4fc18bd20314a837172c7a4d5e3652a3254c0b5e/WordPress/AbstractClassRestrictionsSniff.php#L153-L156

But...I think you may have found an issue though - in this case, when there's a valid function (wp_styles()) that matches the name of one of the listed WordPress classes, then it seems to have thrown the Warning.

anomiex commented 1 year ago

@GaryJones I think when you start investigating your "when there's a valid function that matches the name" idea that you'll come to the same conclusion that I did.

jrfnl commented 1 year ago

FYI: I agree with @anomiex and have a fix lined up for this issue.

GaryJones commented 1 year ago

Having re-read the OP with fresh eyes, I now understand it better. Thanks for raising!

jrfnl commented 1 year ago

FWIW: there's a lot more wrong with this abstract. For now, I'm just going to fix the most pertinent problems.

We'll be switching to an abstract from PHPCSUtils at some point after the 3.0.0 release and that abstract will be written from scratch and should fix the rest.