Repository: WordPress/WordPress-Coding-Standards (php)

Issue:
AbstractClassRestrictionsSniff fails to consider T_SELF, T_STATIC, maybe others before double-colon
<!--
PLEASE FILL OUT THE TEMPLATE COMPLETELY.
BUG REPORTS WHICH CANNOT BE REPRODUCED BASED ON THE INFORMATION PROVIDED WILL BE CLOSED.
-->

## Bug Description
<!--
Please provide a clear and concise description of what the bug is.

What did you expect to happen? What actually happened?
-->
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
<!-- Please provide example code that allows us to reproduce the issue. Do NOT paste screenshots of code! -->

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:
```bash
vendor/bin/phpcs -s --standard=WordPress --sniffs=WordPress.WP.ClassNameCase test.php
```

... over a file containing this code:
```php
<?php

class Foo {
    public function bar() {
        wp_styles();
        self::foo();

        wp_styles();
        static::foo();
    }
}
```

## Error Code
<!--
The error code for the sniff that is (or should be) being triggered (you
can see the sniff error codes by running `phpcs` with the `-s` flag).
e.g. `WordPress.PHP.NoSilencedErrors.Discouraged`

You can leave this section empty if you are reporting a false negative.
-->
```
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
<!--
If the issue cannot be reproduced when using `--standard=WordPress` on the command line,
please post the relevant part of your custom ruleset here.
-->

n/a

## Environment
<!--
To find out the versions used:
* PHP: run `php -v`.
* PHPCS: run `[vendor/bin/]phpcs --version`
* WPCS: run `composer [global] info` for a Composer install.
-->

| 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)
<!-- Add any other context about the problem here. -->
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?
- [x] I have verified the issue still exists in the `develop` branch of WPCS.


Pull request:
AbstractClassRestrictionsSniff: implement PHPCSUtils and various bug fixes
Note: there is a lot more that needs improving in this abstract, but as we intend to switch to an abstract from PHPCSUtils in the near future and as WP itself doesn't currently use these features very often, if at all, I'm leaving the other shortcoming for the switch to the PHPCSUtils abstract in a future 3.x release.

For the record, the sniff currently has false negatives on:
* `instanceof ClassName`
* Use of exception classes in a `catch()`.
* Use of class (or interface) names in type declarations in any form (plain, union, intersect, DNF / parameter, return or property types / constructor property promotion).
* Use of attribute classes in PHP 8.0+ attributes.

The sniff also does not distinguish correctly between classes and interfaces, i.e. it will examine `implements...` statements as if the names found are classes (while they are interfaces).

For `implements` statements, it does not handle multi-implements - `implements InterfaceA, InterfaceB` - correctly, nor does it handle interface declarations which potentially `extend` multiple interfaces correctly.

For now, I've just implemented PHPCSUtils and fixed the most pertinent bugs I could find in the functionality of the abstract as it is.

----

### AbstractClassRestrictionsSniff::get_namespaced_classname(): efficiency tweak

Instead of doing a preliminary search for the `T_NAMESPACE` token, just let the PHPCSUtils `Namespaces::determineNamespace()` method handle the namespace determination.

Finding the previous `T_NAMESPACE` token can be quite token walking intensive as if the token isn't used, it would mean walking all the way back to the start of the file before `false` is returned.

While the PHPCSUtils method will also do this, it will do this in a much more efficient and precise manner. Additionally, it will cache the results, meaning that it will only do this token walking once per file per PHPCS run (at most).

### AbstractClassRestrictionsSniff::get_namespaced_classname(): bug fix - namespace operator is case-insensitive

The `namespace` keyword as an operator was not being recognized correctly when not in lowercase, while PHP treats the keyword insensitively, so so should the sniff.

Fixed now.

Includes unit test.

### AbstractClassRestrictionsSniff::is_targetted_token(): bug fix - false positives icw hierarchy keywords

When a hierarchy keyword or a variable would be before the double colon, the sniff would potential walk too far back and identify an unrelated `T_STRING` token as the class name.

Includes unit tests (which would throw two errors on each line without the fix).

Fixes #2184

### AbstractClassRestrictionsSniff::is_targetted_token(): use PHPCSUtils [1]

... for the name determination based on a `::`.

Note: the condition checking for `T_NS_SEPARATOR` is not necessary as the `get_namespaced_classname()` method checks this as well and will return straight away in that case.

### AbstractClassRestrictionsSniff::is_targetted_token(): bug fix - false negative on class instantiation ending on close tag

Includes unit test.

### AbstractClassRestrictionsSniff::is_targetted_token(): bug fix - false negative on class instantiation with method chaining

Note: the `->` or `?->` object operators can only be used in this context if the `new ...` is wrapped in parentheses, so the `T_OBJECT_OPERATOR` was redundant in the search.

Includes unit tests.

### AbstractClassRestrictionsSniff: add an extra test

### AbstractClassRestrictionsSniff::is_targetted_token(): bug fix - sniff does not bow out early on hierarchy keywords

The abstract class turns every found name into a fully qualified name, so, as things are, the hierarchy keywords would be prefixed with a `\` too, so the `in_array()` comparison would never match.

While this doesn't lead to false positives, it should still be fixed for performance reasons.

Fixed now.

Includes unit test.

### AbstractClassRestrictionsSniff::is_targetted_token(): bug fix - hierarchy keywords are case-insensitive

... so should be matched in a case-insensitive manner.

While this doesn't lead to false positives, it should still be fixed for performance reasons.

Fixed now.

Includes (adjusted) unit tests.

### AbstractClassRestrictionsSniff::is_targetted_token(): use PHPCSUtils [2]

... for the name determination based on a `new ...` etc.

Note: the condition checking for `T_NS_SEPARATOR` is not necessary as the `get_namespaced_classname()` method checks this as well and will return straight away in that case.

### AbstractClassRestrictionsSniff::is_targetted_token(): add tests with PHP 7.0+ anonymous classes

### AbstractClassRestrictionsSniff::is_targetted_token(): add tests with PHP 8.1+ enums

... which can also `implement`.

Patch:
diff --git a/WordPress/AbstractClassRestrictionsSniff.php b/WordPress/AbstractClassRestrictionsSniff.php
index 22a0609c..bd3b7aa9 100644
--- a/WordPress/AbstractClassRestrictionsSniff.php
+++ b/WordPress/AbstractClassRestrictionsSniff.php
@@ -9,6 +9,9 @@
 
 namespace WordPressCS\WordPress;
 
+use PHP_CodeSniffer\Util\Tokens;
+use PHPCSUtils\Tokens\Collections;
+use PHPCSUtils\Utils\GetTokensAsString;
 use PHPCSUtils\Utils\Namespaces;
 use WordPressCS\WordPress\AbstractFunctionRestrictionsSniff;
 use WordPressCS\WordPress\Helpers\RulesetPropertyHelper;
@@ -122,28 +125,25 @@ abstract class AbstractClassRestrictionsSniff extends AbstractFunctionRestrictio
 
 		if ( \in_array( $token['code'], array( \T_NEW, \T_EXTENDS, \T_IMPLEMENTS ), true ) ) {
 			if ( \T_NEW === $token['code'] ) {
-				$nameEnd = ( $this->phpcsFile->findNext( array( \T_OPEN_PARENTHESIS, \T_WHITESPACE, \T_SEMICOLON, \T_OBJECT_OPERATOR ), ( $stackPtr + 2 ) ) - 1 );
+				$nameEnd = ( $this->phpcsFile->findNext( array( \T_OPEN_PARENTHESIS, \T_WHITESPACE, \T_SEMICOLON, \T_CLOSE_PARENTHESIS, \T_CLOSE_TAG ), ( $stackPtr + 2 ) ) - 1 );
 			} else {
 				$nameEnd = ( $this->phpcsFile->findNext( array( \T_CLOSE_CURLY_BRACKET, \T_WHITESPACE ), ( $stackPtr + 2 ) ) - 1 );
 			}
 
-			$length    = ( $nameEnd - ( $stackPtr + 1 ) );
-			$classname = $this->phpcsFile->getTokensAsString( ( $stackPtr + 2 ), $length );
-
-			if ( \T_NS_SEPARATOR !== $this->tokens[ ( $stackPtr + 2 ) ]['code'] ) {
-				$classname = $this->get_namespaced_classname( $classname, ( $stackPtr - 1 ) );
-			}
+			$classname = GetTokensAsString::noEmpties( $this->phpcsFile, ( $stackPtr + 2 ), $nameEnd );
+			$classname = $this->get_namespaced_classname( $classname, ( $stackPtr - 1 ) );
 		}
 
 		if ( \T_DOUBLE_COLON === $token['code'] ) {
-			$nameEnd   = $this->phpcsFile->findPrevious( \T_STRING, ( $stackPtr - 1 ) );
-			$nameStart = ( $this->phpcsFile->findPrevious( array( \T_STRING, \T_NS_SEPARATOR, \T_NAMESPACE ), ( $nameEnd - 1 ), null, true, null, true ) + 1 );
-			$length    = ( $nameEnd - ( $nameStart - 1 ) );
-			$classname = $this->phpcsFile->getTokensAsString( $nameStart, $length );
-
-			if ( \T_NS_SEPARATOR !== $this->tokens[ $nameStart ]['code'] ) {
-				$classname = $this->get_namespaced_classname( $classname, ( $nameStart - 1 ) );
+			$nameEnd = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true );
+			if ( \T_STRING !== $this->tokens[ $nameEnd ]['code'] ) {
+				// Hierarchy keyword or object stored in variable.
+				return false;
 			}
+
+			$nameStart = ( $this->phpcsFile->findPrevious( Collections::namespacedNameTokens(), ( $nameEnd - 1 ), null, true ) + 1 );
+			$classname = GetTokensAsString::noEmpties( $this->phpcsFile, $nameStart, $nameEnd );
+			$classname = $this->get_namespaced_classname( $classname, ( $nameStart - 1 ) );
 		}
 
 		// Stop if we couldn't determine a classname.
@@ -152,7 +152,7 @@ abstract class AbstractClassRestrictionsSniff extends AbstractFunctionRestrictio
 		}
 
 		// Nothing to do if one of the hierarchy keywords - 'parent', 'self' or 'static' - is used.
-		if ( \in_array( $classname, array( 'parent', 'self', 'static' ), true ) ) {
+		if ( \in_array( strtolower( $classname ), array( '\parent', '\self', '\static' ), true ) ) {
 			return false;
 		}
 
@@ -222,26 +222,18 @@ abstract class AbstractClassRestrictionsSniff extends AbstractFunctionRestrictio
 		}
 
 		// Remove the namespace keyword if used.
-		if ( 0 === strpos( $classname, 'namespace\\' ) ) {
+		if ( 0 === stripos( $classname, 'namespace\\' ) ) {
 			$classname = substr( $classname, 10 );
 		}
 
-		$namespace_keyword = $this->phpcsFile->findPrevious( \T_NAMESPACE, $search_from );
-		if ( false === $namespace_keyword ) {
+		$namespace = Namespaces::determineNamespace( $this->phpcsFile, $search_from );
+		if ( '' === $namespace ) {
 			// No namespace keyword found at all, so global namespace.
 			$classname = '\\' . $classname;
 		} else {
-			$namespace = Namespaces::determineNamespace( $this->phpcsFile, $search_from );
-
-			if ( ! empty( $namespace ) ) {
-				$classname = '\\' . $namespace . '\\' . $classname;
-			} else {
-				// No actual namespace found, so global namespace.
-				$classname = '\\' . $classname;
-			}
+			$classname = '\\' . $namespace . '\\' . $classname;
 		}
 
 		return $classname;
 	}
-
 }


Test patch:
diff --git a/WordPress/Tests/DB/RestrictedClassesUnitTest.1.inc b/WordPress/Tests/DB/RestrictedClassesUnitTest.1.inc
index 60d70609..4b8a5e32 100644
--- a/WordPress/Tests/DB/RestrictedClassesUnitTest.1.inc
+++ b/WordPress/Tests/DB/RestrictedClassesUnitTest.1.inc
@@ -64,3 +64,43 @@ $db9 = new \My\DBlayer; // Error.
 
 echo mysqli::$affected_rows; // Error.
 class YourMysqliC extends \mysqli {} // Error.
+
+// Bug fix: namespace keyword as operator is case-insensitive.
+class YourMysqliC extends NameSpace\mysqli {} // Error.
+
+// Issue #2184 Prevent false positives when searching for the class name before a double colon.
+class NotOurTarget extends SomethingElse {
+	public function doSomething() {
+		echo mysqli();
+		if (self::$property === static::$property
+			|| $obj::$property === parent::$property
+		) {}
+	}
+}
+
+// Bug fix: false negative when an object instantiation ends on a PHP close tag without whitespace.
+$db1 = new mysqli?><!-- Error. -->
+<div>Something</div>
+<?php
+
+// Bug fix: prevent false negatives on object instantiation with chaining.
+$db10 = (new PDO)->exec(); // Error.
+$db11 = (new PDO)?->exec(); // Error.
+
+// Safeguard handling of class which extends and implements.
+class MyMysqliWithArrayAccess extends mysqli implements ArrayAccess {} // Error.
+
+// Safeguard handling of new with hierarchy keywords.
+$obj = new self();
+$obj = new Parent();
+$obj = new STATIC();
+
+// Safeguard handling of PHP 7.0+ anonymous classes.
+$anon = new class {
+	public function PDO() {} // OK.
+};
+
+$anon = new class extends PDOStatement {}; // Error.
+
+// PHP 8.1: enums can implement.
+enum MysqliEnum implements mysqli {} // Error.
diff --git a/WordPress/Tests/DB/RestrictedClassesUnitTest.php b/WordPress/Tests/DB/RestrictedClassesUnitTest.php
index 1ee99d08..40fc1006 100644
--- a/WordPress/Tests/DB/RestrictedClassesUnitTest.php
+++ b/WordPress/Tests/DB/RestrictedClassesUnitTest.php
@@ -71,31 +71,38 @@ final class RestrictedClassesUnitTest extends AbstractSniffUnitTest {
 		switch ( $testFile ) {
 			case 'RestrictedClassesUnitTest.1.inc':
 				return array(
-					17 => 1,
-					18 => 1,
-					19 => 1,
-					20 => 1,
-					22 => 1,
-					23 => 1,
-					24 => 1,
-					25 => 1,
-					26 => 1,
-					27 => 1,
-					29 => 1,
-					30 => 1,
-					32 => 1,
-					33 => 1,
-					35 => 1,
-					36 => 1,
-					37 => 1,
-					39 => 1,
-					40 => 1,
-					42 => 1,
-					51 => 1,
-					52 => 1,
-					63 => 1,
-					65 => 1,
-					66 => 1,
+					17  => 1,
+					18  => 1,
+					19  => 1,
+					20  => 1,
+					22  => 1,
+					23  => 1,
+					24  => 1,
+					25  => 1,
+					26  => 1,
+					27  => 1,
+					29  => 1,
+					30  => 1,
+					32  => 1,
+					33  => 1,
+					35  => 1,
+					36  => 1,
+					37  => 1,
+					39  => 1,
+					40  => 1,
+					42  => 1,
+					51  => 1,
+					52  => 1,
+					63  => 1,
+					65  => 1,
+					66  => 1,
+					69  => 1,
+					82  => 1,
+					87  => 1,
+					88  => 1,
+					91  => 1,
+					103 => 1,
+					106 => 1,
 				);
 
 			case 'RestrictedClassesUnitTest.2.inc':