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

Avoid empty first part for global namespace in NameResolver::resolveC… #444

Closed
wants to merge 1 commit into from

Conversation

VolCh
Copy link

@VolCh VolCh commented Nov 16, 2017

…lassName

During investigate goaop/parser-reflection#80 and goaop/parser-reflection#83 I found that root of issue is in Name::$parts[0] === '' in global namespace which append by NameResolver::resolveClassName during resolve class names using in global namespace like fetch class const.

@VolCh VolCh changed the base branch from master to 3.x November 16, 2017 11:23
@nikic
Copy link
Owner

nikic commented Nov 16, 2017

Do you have a reproduce script for this? This looks like a usage mistake to me, as the parser shouldn't ever generate a namespace for which toString() is ''.

@VolCh
Copy link
Author

VolCh commented Nov 22, 2017

As far I understand it generated by new Namespace_(new FullyQualified(''), ...) at https://github.com/goaop/parser-reflection/blob/master/src/NodeVisitor/RootNamespaceNormalizer.php#L47
during parsing file without any namespace, i. e. global namespaces

P.S. What is expected behavior for (new Namespace_(new FullyQualified('')))->toString() ?

@nikic
Copy link
Owner

nikic commented Nov 22, 2017

That's the problem then. The global namespace is represented using new Namespace(null, ...), not new Namespace_(new FullyQualified(''), ...). Generally names with empty components are always invalid. I guess it would be good to add an explicit check + exception in this case.

@nikic
Copy link
Owner

nikic commented Dec 1, 2017

I've added an exception on empty names in master: b507fa4 Not changing this in 3.x for BC reasons. It looks like goaop/parser-reflection already has an open PR to fix this issue: goaop/parser-reflection#68

@nikic nikic closed this Dec 1, 2017
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.

2 participants