Skip to content
This repository was archived by the owner on Feb 16, 2019. It is now read-only.

Conversation

@erikwiffin
Copy link
Contributor

Fixes bug referenced by ezzatron in https://github.com/FriendsOfPHP/Sami/issues/99#issuecomment-57895131. NULL was being added to the ParserContext classes array because of a complicated chain of events in PHP-Parser that resulted in leaveClass getting called without a matching enterClass. This doesn't fix the root cause, but it does allow for filtering out classes (ie. if SymfonyFilter is used).

Copy link
Contributor

Choose a reason for hiding this comment

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

We need an extra blank line following this closing brace.

@aik099
Copy link
Contributor

aik099 commented Jan 16, 2015

This doesn't fix the root cause, but it does allow for filtering out classes (ie. if SymfonyFilter is used).

I'd prefer a fix for a root cause considering that it's unlikely that PHP file was tokenized wrong.

@erikwiffin
Copy link
Contributor Author

I looked into it a bit more, and this might actually be the best possible fix. The problem stems from https://github.com/nikic/PHP-Parser/blob/v1.0.2/lib/PhpParser/NodeTraverser.php and the traverseArray function. First, it loops through each node and calls $visitor->enterNode($node) on them. That makes it's way to Sami/Parser/NodeVisitor.php and the addClassOrInterface function. If a class is to be filtered out, $this->context->enterClass($class); (line 132) is never called. Since PhpParser doesn't care that you want to filter out the class, it happily continues on to call $visitor->leaveNode($node);.

It looks to me like unless you're willing to completely change how filtering classes works, isset($this->class) is going to need to be called somewhere, and leaveClass makes the most sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better to check explicitly for null, e.g. null === $this->class.

@aik099
Copy link
Contributor

aik099 commented Jan 16, 2015

@erikwiffin , I've looked at current code I now agree with you that check you've made is the right one. So if nobody entered class, then leaving non-entered class should be ignored as well.

@fabpot
Copy link
Member

fabpot commented Feb 17, 2015

Thank you @erikwiffin.

@fabpot fabpot closed this in e647453 Feb 17, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants