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

Pick up docs for filters in condition expressions #186

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JDGrimes
Copy link
Contributor

PHPParser loops over the body of a conditional statement (the part between the curly brackets), before looping over each node in the expression. Because we were only saving the last docblock that wasn't associated with a documentable element, if there was also a documented filter within the condition body, the docblock for that filter would be picked up, and the docblock for the first filter in the condition expression would be discarded. So by the time we got to the node for the filter in the condition expression, there was no docblock saved to associate with it.

This is now fixed by keeping a stack of stray docblocks for non-documentable elements, so that instead of the docblock for the first filter being discarded it is just pushed down the stack. The docblock for the second filter within the condition block will be pushed on top of it, and then popped off once it has been used. So when we come around to actually looping over the node for the filter in the conditional expression, its docblock will be at the tip of the stack once again, and can be associated with the filter as expected.

See #185

PHPParser loops over the body of a conditional statement (the part between the curly brackets), before looping over each node in the expression. Because we were only saving the last docblock that wasn't associated with a documentable element, if there was also a documented filter within the condition body, the docblock for that filter would be picked up, and the docblock for the first filter in the condition expression would be discarded. So by the time we got to the node for the filter in the condition expression, there was no docblock saved to associate with it.

This is now fixed by keeping a stack of stray docblocks for non-documentable elements, so that instead of the docblock for the first filter being discarded it is just pushed down the stack. The docblock for the second filter within the condition block will be pushed on top of it, and then popped off once it has been used. So when we come around to actually looping over the node for the filter in the conditional expression, its docblock will be at the tip of the stack once again, and can be associated with the filter as expected.

See WordPress#185
Discovered while working on WordPress#185.
@JDGrimes
Copy link
Contributor Author

It looks like the tests are failing on PHP 7, but I don't think that is related to this PR. If I get a chance I'll investigate further, and see if I can find the cause of the problem.

@JDGrimes
Copy link
Contributor Author

The error the tests are getting is this:

TypeError: Argument 3 passed to phpDocumentor\Reflection\DocBlock::__construct() must be of the type array, object given, called in /home/travis/build/WordPress/phpdoc-parser/vendor/phpdocumentor/reflection/src/phpDocumentor/Reflection/FileReflector.php on line 246

/home/travis/build/WordPress/phpdoc-parser/vendor/phpdocumentor/reflection/src/phpDocumentor/Reflection/FileReflector.php:246
/home/travis/build/WordPress/phpdoc-parser/vendor/nikic/php-parser/lib/PHPParser/NodeTraverser.php:49
/home/travis/build/WordPress/phpdoc-parser/vendor/phpdocumentor/reflection/src/phpDocumentor/Reflection/Traverser.php:52
/home/travis/build/WordPress/phpdoc-parser/vendor/phpdocumentor/reflection/src/phpDocumentor/Reflection/FileReflector.php:163
/home/travis/build/WordPress/phpdoc-parser/lib/runner.php:56
/home/travis/build/WordPress/phpdoc-parser/tests/phpunit/includes/export-testcase.php:31
/home/travis/build/WordPress/phpdoc-parser/tests/phpunit/includes/export-testcase.php:44

However, \phpDocumentor\Reflection\DocBlock::__construct()'s declaration is thus in version 2.0.4 (which is what is being installed on Travis):

    public function __construct(
        $docblock,
        Context $context = null,
        Location $location = null
    ) {

So an object is definitely being expected here. The only thing that I can think is that this error stems from a bug in PHP 7.

@grappler
Copy link
Member

As PHP 7.1 has been released it would be good to add it to the travis list and see if it passes there.

@JDGrimes
Copy link
Contributor Author

This does not pass on PHP 7.1 either, has the same issues. I don't know what is going on here, I cannot reproduce this locally:

$ php --version
PHP 7.0.14 (cli) (built: Dec 31 2016 10:05:11) ( ZTS )
Copyright (c) 1997-2016 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2016 Zend Technologies
    with Zend OPcache v7.0.14, Copyright (c) 1999-2016, by Zend Technologies
    with Xdebug v2.5.0, Copyright (c) 2002-2016, by Derick Rethans

Which is the same configuration of PHP running on Travis. So I'm stumped.

@ntwb
Copy link
Member

ntwb commented Dec 31, 2016

I've just added PRs for PHP 7.1 and to HHVM (Update HHVM to the latest)

@grappler
Copy link
Member

grappler commented Jan 1, 2017

@JDGrimes The error is not coming from your code but from existing code. You can see this in the PR #187 which is only adding PHP7.1 to travis.

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.

3 participants