Skip to content

[ClassLoader][Routing] Fix namespace parsing on php 8 #37783

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

Merged
merged 1 commit into from
Aug 10, 2020

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Aug 9, 2020

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

The way namespace declarations are parsed has changed in php 8 (see php/php-src#5827). This PR should fix the corresponding issues in the ClassLoader and Routing components.

Note that Doctrine Annotations suffers from the same issue (doctrine/annotations#339). I had to run the Routing tests locally with doctrine/annotations#344 applied.

@derrabus derrabus force-pushed the bugfix/php8-namespace-parsing branch from fa01dde to 3d293b2 Compare August 9, 2020 11:28
@fabpot
Copy link
Member

fabpot commented Aug 10, 2020

Thank you @derrabus.

@fabpot fabpot merged commit 1dcb67e into symfony:3.4 Aug 10, 2020
@derrabus derrabus deleted the bugfix/php8-namespace-parsing branch August 10, 2020 07:55
@@ -216,6 +216,11 @@ public static function fixNamespaceDeclarations($source)
$inNamespace = false;
$tokens = token_get_all($source);

$nsTokens = [T_WHITESPACE => true, T_NS_SEPARATOR => true, T_STRING => true];
if (\defined('T_NAME_QUALIFIED')) {
$nsTokens[T_NAME_QUALIFIED] = true;
Copy link
Member

Choose a reason for hiding this comment

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

This also misses T_NAME_FULLY_QUALIFIED

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you give an example that would result in \T_NAME_FULLY_QUALIFIED being emitted here?

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.

4 participants