Fix NamespaceDirectoryNameSniff misreading strict_types as namespace name#333
Open
ewartx wants to merge 1 commit into
Open
Fix NamespaceDirectoryNameSniff misreading strict_types as namespace name#333ewartx wants to merge 1 commit into
ewartx wants to merge 1 commit into
Conversation
…name The sniff calls findNext(T_STRING, 0) on line 33 to locate the namespace name, which starts searching from position 0 of the file. When the file opens with a declare(strict_types=1) statement before its namespace declaration, the first T_STRING token in the file is strict_types (PHP tokenizes the declare directive name as T_STRING), so the sniff reads strict_types as the namespace and reports a false NameMismatch or ExtraDirs error against the actual directory. Change the search start position to $stackPtr so the namespace name is read starting from the namespace keyword the sniff is currently processing. Adds a fixture (strict-types-namespace.php) that combines strict_types with a namespace declaration; the test fails on the current main and passes with this fix. Fixes humanmade#319.
Author
|
@kadamwhite tagging you for visibility — this addresses the upstream NamespaceDirectoryNameSniff bug discussed in our wharton-humanmade PR #23 thread. Fix matches the resolution proposed in #319; local test run confirms it. Happy to iterate on anything. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #319.
NamespaceDirectoryNameSniffcallsfindNext( T_STRING, 0 )on line 33 to locate the namespace name, which starts searching from position 0 of the file. When the file opens with adeclare(strict_types=1);statement before its namespace declaration, the firstT_STRINGtoken in the file isstrict_types(PHP's tokenizer emits the declare directive name asT_STRING), so the sniff readsstrict_typesas the namespace and reports a falseNameMismatchorExtraDirserror against the actual directory.Fix
Change the
findNextstart position from0to$stackPtr, so the search begins at theT_NAMESPACEtoken the sniff is currently processing rather than the beginning of the file.This matches the resolution proposed in #319 directly.
Why this is safe
The fix produces the same result as the existing code in every case except the buggy one:
<?php namespace Foo;(nodeclare): the firstT_STRINGin the file is the namespace name itself, so the original (from position 0) and the fix (from $stackPtr) land on the same token.<?php declare(strict_types=1); namespace Foo;: original readsstrict_types; fix correctly readsFoo.namespace A {} namespace B {}files: original readsAfor both invocations (the file-firstT_STRING); fix correctly reads each block's own namespace name.Only
declare()directives can legally place aT_STRINGtoken beforeT_NAMESPACE; PHP requiresnamespaceto be the first non-declare statement, so other cases aren't reachable in real code.Test
Adds
HM/Tests/Files/NamespaceDirectoryNameUnitTest/inc/standards/strict-types-namespace.phpto the existing fixture tree and adds its basename to the$passlist inNamespaceDirectoryNameUnitTest::getErrorList(). The fixture is the minimal repro from the issue:Verified locally with
./vendor/bin/phpunit --filter NamespaceDirectoryNameUnitTest:Expected 0 error(s) in strict-types-namespace.php but found 1 error(s). Directory /standards for namespace strict_types found; nested too deep. (HM.Files.NamespaceDirectoryName.ExtraDirs)NamespaceDirectoryNameUnitTestpasses.