-
-
Notifications
You must be signed in to change notification settings - Fork 51
Ignore any tokens within classes when looking for namespaces #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,6 +70,26 @@ public function createForNamespace($namespace, $fileContents) | |
case T_NAMESPACE: | ||
$currentNamespace = $this->parseNamespace($tokens); | ||
break; | ||
case T_CLASS: | ||
// Fast-forward the iterator through the class so that any | ||
// T_USE tokens found within are skipped - these are not | ||
// valid namespace use statements so should be ignored. | ||
$braceLevel = 0; | ||
$firstBraceFound = false; | ||
while ($tokens->valid() && ($braceLevel > 0 || !$firstBraceFound)) { | ||
if ($tokens->current() === '{') { | ||
if (!$firstBraceFound) { | ||
$firstBraceFound = true; | ||
} | ||
$braceLevel++; | ||
} | ||
|
||
if ($tokens->current() === '}') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above |
||
$braceLevel--; | ||
} | ||
$tokens->next(); | ||
} | ||
break; | ||
case T_USE: | ||
if ($currentNamespace === $namespace) { | ||
$useStatements = array_merge($useStatements, $this->parseUseStatement($tokens)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,6 +88,26 @@ public function testReadsAliasesFromProvidedNamespaceAndContent() | |
|
||
$this->assertSame($expected, $context->getNamespaceAliases()); | ||
} | ||
|
||
public function testTraitUseIsNotDetectedAsNamespaceUse() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am missing an |
||
{ | ||
$fixture = new ContextFactory(); | ||
|
||
$php = "<?php | ||
namespace Foo; | ||
|
||
trait FooTrait {} | ||
|
||
class FooClass { | ||
use FooTrait; | ||
} | ||
"; | ||
|
||
$fixture = new ContextFactory(); | ||
$context = $fixture->createForNamespace('Foo', $php); | ||
|
||
$this->assertSame([], $context->getNamespaceAliases()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may want to add a test where one trait is imported and used (from a different namespace) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yu sould also add a test using some interpolation inside the class (or any other syntax relying on curly braces) |
||
} | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a class constant for opening and closing brace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, no. (well, there are constants for some opening braces depending on the reason why they are opened).
Btw, this should be looked at carefully for interpolation in strings as the opening token may include more than just the opening brace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no class constant but it would not hurt to introduce it; though I do not consider it blocking for a merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relevant tokens should be
T_CURLY_OPEN
andT_DOLLAR_OPEN_CURLY_BRACES
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but those constants are string-based curlies and always include a dollar sign (see http://php.net/manual/en/tokens.php); the curly braces for functions and classes are literals and AFAIK do not have a constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea, but this wouldn't work, you can have a
use
that follows aclass
within the samenamespace
, see: http://3v4l.org/uiEWd :/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asgrim However this use would only take effect for classes defined after it. If you want to properly handle that you need to specify the class name you are targeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikic it may not be valid PHP and not be able to execute, but it seems that if you perform a
token_get_all()
on this, it will still give you valid tokens (even if it won't execute):In this edge case where tokenizer will happily tokenize this, the depth-checking method I've implemented (and improved in a soon-to-be-opened-PR) would be safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asgrim It's perfectly fine PHP and will work. What I'm saying is that use statements only affect all classes after them. So if you have this code:
Then class
A
will extendX
, while classB
extendsFoo\X
. So if you want to correctly handle this, you have to pass in the class name you're considering (rather than just its namespace name) and only return the aliases that are in effect at the time of the definition of the class.(So yes, you're right, I'm just pointing out an extra caveat why it won't work out of the box, even if you properly do the skipping here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikic Ah yes - I see what you mean, you are absolutely correct. I've reported as a separate issue in #11 as this is not something that is introduced by this PR, the issue would've existed already in TypeResolver.