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

Support for Group Use Statements #69

Merged
merged 5 commits into from
Oct 24, 2016
Merged

Conversation

poldridge
Copy link
Contributor

@Ocramius Ocramius added this to the v1.3.0 milestone Jan 14, 2016
@guilhermeblanco
Copy link
Member

I remembered why I didn't merge this patch before. I need to check against zend_language_parser.y to verify the potential acceptance of:

use Foo\{Bar, Woo}, Bar\{Fuzz, Lala};

If this is not accepted, it's actually a bug in the language grammar, because this is supported:

use Foo\Bar, Foo\Woo;

And I'd have to provide a patch to PHP, which would further require an update in this patch.

@blowski
Copy link

blowski commented Apr 8, 2016

@guilhermeblanco It's a good point, and no it doesn't work. The original RFC doesn't mention it.

@tristanbes
Copy link

tristanbes commented Oct 10, 2016

ping, that prevent other libraries using doctrine/annotations to be compliant with use group features :(

What's missing inside this PR so it gets merged ?

$groupRoot = $class;
$class = '';
} else if ($token === '}' ) {
continue;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If PHP's grammar is modified to accept: use Foo\{Bar, Woo}, Bar\{Fuzz, Lala}; then replace continue; with $groupRoot = $class = $alias = '';

Copy link
Contributor Author

@poldridge poldridge Oct 10, 2016

Choose a reason for hiding this comment

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

Also read the next token and handle it (if it's a comma, skip over it and continue parsing, otherwise it is a semicolon or invalid PHP, and we're done):

if ( $this->next() == ',' ) {
    continue;
} else {
    break;
}

@marcioAlmada
Copy link

marcioAlmada commented Oct 11, 2016

So, this PR was opened 10 months ago. What's preventing it from getting merged?

The claims the new syntax is "too hard to parse" is not sustainable, the patch looks very simple. It's not as simple as a regular expression, but so many aspects of PHP can't be parsed with regex. This is not the best reasoning when we have libraries like PhpParser and manually parsing a list inside a block is not really a challenge.

The DocParser.php itself has ~1,138 lines of code handling many kinds of syntax edge cases, adding two else ifs on TokenParser won't make any substantial increase in complexity here.

If this is not accepted, it's actually a bug in the language grammar, because this is supported:
use Foo\Bar, Foo\Woo;
And I'd have to provide a patch to PHP, which would further require an update in this patch.

@guilhermeblanco this feels 100% opinion based to me. It also sounds a bit authoritative to consider you have to go fix the language first before merging a PR. It's probably not the intention, but that's the impression it might yield.

Think collectively, the language feature wasn't merged in core by a single person the cowboy way - it was discussed, reviewed, improved, voted, released.

PS: I apologize in advance in case the long wait is happening purely due to lack of availability from maintainers, we all have tons of things to do.

@Ocramius
Copy link
Member

LGTM. Merging and preparing 1.3.0!

Thanks @poldridge!

@marcioAlmada yes, it was mostly lack of time delaying merges/reviews, sorry.

@Ocramius Ocramius merged commit 1adda86 into doctrine:master Oct 24, 2016
@Ocramius Ocramius self-assigned this Oct 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants