Skip to content
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

Ignore any tokens within classes when looking for namespaces #9

Merged
merged 1 commit into from
Jul 18, 2015

Conversation

asgrim
Copy link
Contributor

@asgrim asgrim commented Jul 14, 2015

This should resolve issue #8

Basically, this proposed solution looks for classes, looks for the first opening brace, then keeps iterating until the same-level closing brace is found, then we continue.

I may've missed a use-case with this, but I think on the surface this should work - happy for any feedback given :)

@asgrim
Copy link
Contributor Author

asgrim commented Jul 14, 2015

Not sure why Scrutinizer thinks the tests failed.. looks okay on Travis I think?

$braceLevel = 0;
$firstBraceFound = false;
while ($tokens->valid() && ($braceLevel > 0 || !$firstBraceFound)) {
if ($tokens->current() === '{') {
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Member

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

Copy link

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 and T_DOLLAR_OPEN_CURLY_BRACES.

Copy link
Member

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

Copy link
Contributor Author

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 a class within the same namespace, see: http://3v4l.org/uiEWd :/

Copy link

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.

Copy link
Contributor Author

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):

namespace A {
  class Foo {
    function a() {
      echo 'A\Foo';
    }
  }
}

namespace B {
  class Foo extends Bar{
    function a() {
      echo 'B\Foo';
    }
  }
  use A\Foo as Bar;
}

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.

Copy link

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:

class A extends X {}
use Foo\X;
class B extends X {}

Then class A will extend X, while class B extends Foo\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.)

Copy link
Contributor Author

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.

@stof
Copy link
Contributor

stof commented Jul 18, 2015

Regarding the scrutinizer issue, this is because the first job to finish was the PHP 7 job, which was not able to generate code coverage (no XDebug on PHP 7 for now as it is not migrated yet). Given that you still ask ocular to upload the code coverage on PHP 7, it notified Scrutinizer that the coverage data were missing (the message in the scrutinizer UI is a bit misleading as it is not about test failing but about coverage not being available)

@@ -88,6 +88,26 @@ public function testReadsAliasesFromProvidedNamespaceAndContent()

$this->assertSame($expected, $context->getNamespaceAliases());
}

public function testTraitUseIsNotDetectedAsNamespaceUse()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am missing an @covers statement with this test; meaning that coverage will potentially show a skewed number of lines that were covered

@mvriel
Copy link
Member

mvriel commented Jul 18, 2015

@asgrim @Ocramius I am going to merge this item since it does not seem to contain any functional issues. I would however appreciate it if the comments mentioned above can be resolved in a follow up PR

mvriel added a commit that referenced this pull request Jul 18, 2015
Ignore any tokens within classes when looking for namespaces
@mvriel mvriel merged commit 83e3125 into phpDocumentor:master Jul 18, 2015
@asgrim asgrim mentioned this pull request Jul 19, 2015
mvriel added a commit that referenced this pull request Dec 2, 2015
@asgrim asgrim deleted the ignore-class-trait-usage branch April 16, 2017 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants