Skip to content

Commit 4c8f919

Browse files
committed
Generic/ConstructorName: fix bug with nested anonymous classes and more
Based on the bug report and test case in issue 2178, I've had a look at this sniff. Turns out the `processTokensWithinScope()` method gets called for each whitelisted scope a `T_FUNCTION` token exists in. In effect, this means that the first time it gets called for the `nested()` method, it is because it is within the scope of the `Nested` class, the second time the function gets called, it is for the method being in the scope of the anonymous class. The fix I've now applied checks that the scope for which the method is called is actually the deepest scope first and bows out if it's not. This fixes the false positive. Additionally, the `loadFunctionNamesInScope()` method would examine every token within the class and the resulting function list would therefore include the names of functions which were nested. This would lead to false negatives as a `__construct()` method in a nested class would be regarded as the `__construct()` method of the higher level class and the expected error would not be thrown. I've fixes this by having the `loadFunctionNamesInScope()` method skip to the end of a function after adding the function name to the list, which effectively will prevent the names of nested functions from getting into the list. This should also make the method more efficient as it now examines a lot less tokens. Additional notes: * Class and method names in PHP are case _in_sensitive. While the sniff tried to take that into account by using `strcasecmp()`, the `in_array()` check and the check for the extended parent class name were *not* done in a case-insensitive manner leading - again - to incorrect results. This has now been fixed by lowercasing both the class name as well as all function names before comparing them. * Use strict comparison for `in_array()` to prevent issues with type juggling. * Includes minor fix to an existing unit test - the sniff logic contains a "caching" mechanism for the list of function names gathered which is based on the name of the class. As the name of the class on line 32 was the same as the name of the class for the preceding unit test case, the function name list did not get refreshed and therefore the unit test wasn't testing things correctly. Includes unit tests for all fixes. Fixes 2178
1 parent 6ad2835 commit 4c8f919

File tree

3 files changed

+55
-16
lines changed

3 files changed

+55
-16
lines changed

src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -56,41 +56,49 @@ public function __construct()
5656
*/
5757
protected function processTokenWithinScope(File $phpcsFile, $stackPtr, $currScope)
5858
{
59-
$className = $phpcsFile->getDeclarationName($currScope);
59+
$tokens = $phpcsFile->getTokens();
60+
61+
// Determine if this is a function which needs to be examined.
62+
$conditions = $tokens[$stackPtr]['conditions'];
63+
end($conditions);
64+
$deepestScope = key($conditions);
65+
if ($deepestScope !== $currScope) {
66+
return;
67+
}
68+
69+
$className = strtolower($phpcsFile->getDeclarationName($currScope));
6070
if ($className !== $this->currentClass) {
6171
$this->loadFunctionNamesInScope($phpcsFile, $currScope);
6272
$this->currentClass = $className;
6373
}
6474

65-
$methodName = $phpcsFile->getDeclarationName($stackPtr);
75+
$methodName = strtolower($phpcsFile->getDeclarationName($stackPtr));
6676

67-
if (strcasecmp($methodName, $className) === 0) {
68-
if (in_array('__construct', $this->functionList) === false) {
77+
if ($methodName === $className) {
78+
if (in_array('__construct', $this->functionList, true) === false) {
6979
$error = 'PHP4 style constructors are not allowed; use "__construct()" instead';
7080
$phpcsFile->addError($error, $stackPtr, 'OldStyle');
7181
}
72-
} else if (strcasecmp($methodName, '__construct') !== 0) {
82+
} else if ($methodName !== '__construct') {
7383
// Not a constructor.
7484
return;
7585
}
7686

77-
$tokens = $phpcsFile->getTokens();
78-
79-
$parentClassName = $phpcsFile->findExtendedClassName($currScope);
80-
if ($parentClassName === false) {
87+
// Stop if the constructor doesn't have a body, like when it is abstract.
88+
if (isset($tokens[$stackPtr]['scope_closer']) === false) {
8189
return;
8290
}
8391

84-
// Stop if the constructor doesn't have a body, like when it is abstract.
85-
if (isset($tokens[$stackPtr]['scope_closer']) === false) {
92+
$parentClassName = strtolower($phpcsFile->findExtendedClassName($currScope));
93+
if ($parentClassName === false) {
8694
return;
8795
}
8896

8997
$endFunctionIndex = $tokens[$stackPtr]['scope_closer'];
9098
$startIndex = $stackPtr;
9199
while (($doubleColonIndex = $phpcsFile->findNext(T_DOUBLE_COLON, $startIndex, $endFunctionIndex)) !== false) {
92100
if ($tokens[($doubleColonIndex + 1)]['code'] === T_STRING
93-
&& $tokens[($doubleColonIndex + 1)]['content'] === $parentClassName
101+
&& strtolower($tokens[($doubleColonIndex + 1)]['content']) === $parentClassName
94102
) {
95103
$error = 'PHP4 style calls to parent constructors are not allowed; use "parent::__construct()" instead';
96104
$phpcsFile->addError($error, ($doubleColonIndex + 1), 'OldStyleCall');
@@ -136,8 +144,12 @@ protected function loadFunctionNamesInScope(File $phpcsFile, $currScope)
136144
continue;
137145
}
138146

139-
$next = $phpcsFile->findNext(T_STRING, $i);
140-
$this->functionList[] = trim($tokens[$next]['content']);
147+
$this->functionList[] = trim(strtolower($phpcsFile->getDeclarationName($i)));
148+
149+
if (isset($tokens[$i]['scope_closer']) !== false) {
150+
// Skip past nested functions and such.
151+
$i = $tokens[$i]['scope_closer'];
152+
}
141153
}
142154

143155
}//end loadFunctionNamesInScope()

src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.inc

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ class TestClass extends MyClass
88
}
99

1010
function __construct() {
11-
parent::MyClass();
11+
parent::MYCLASS();
1212
parent::__construct();
1313
}
1414

@@ -29,7 +29,7 @@ class MyClass
2929

3030
}
3131

32-
class MyClass extends \MyNamespace\SomeClass
32+
class MyOtherClass extends \MyNamespace\SomeClass
3333
{
3434
function __construct() {
3535
something::MyNamespace();
@@ -64,3 +64,29 @@ foo(new class extends MyClass
6464
}
6565

6666
});
67+
68+
class OlderClass
69+
{
70+
function OlderClass() {}
71+
72+
function __CONSTRUCT() {}
73+
}
74+
75+
76+
// Issue #2178.
77+
class Nested extends Another {
78+
public function getAnonymousClass() {
79+
return new class() extends Something {
80+
public function nested() {
81+
echo 'In method nested!';
82+
parent::Another(); // OK.
83+
}
84+
85+
public function __construct() {
86+
parent::Another(); // OK.
87+
}
88+
};
89+
}
90+
91+
abstract public function nested();
92+
}

src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ public function getErrorList()
3030
11 => 1,
3131
47 => 1,
3232
62 => 1,
33+
91 => 1,
3334
];
3435

3536
}//end getErrorList()

0 commit comments

Comments
 (0)