Fix token parsing on latest PHP 8 version, fixes #339#344
Fix token parsing on latest PHP 8 version, fixes #339#344shyim wants to merge 1 commit intodoctrine:1.10.xfrom shyim:fix-token-parser-with-new-tokens
Conversation
This comment has been minimized.
This comment has been minimized.
greg0ire
left a comment
greg0ire
left a comment
There was a problem hiding this comment.
Your commit message should mention https://wiki.php.net/rfc/namespaced_names_as_token , otherwise it's hard to understand why we need this. Also, please try to reduce the amount of copy/paste, it makes it hard to see what changes between the 2 methods, so it does not help understanding the commit.
Good job though, looks like it fixes the errors 👍
|
More importantly, link that RFC from the code, so that when we return it a few years later, we know what happened. |
|
I'm attempting to address the |
| { | ||
| $name = ''; | ||
| while (($token = $this->next()) && ($token[0] === T_STRING || $token[0] === T_NS_SEPARATOR)) { | ||
| while (($token = $this->next()) && ($token[0] === T_STRING || $token[0] === T_NS_SEPARATOR || (\PHP_VERSION_ID >=80000 && $token[0] === T_NAME_QUALIFIED))) { |
There was a problem hiding this comment.
isn't T_NAME_FULLY_QUALIFIED also allowed in a namespace declaration ?
|
@greg0ire I don't think we can easily avoid the copy-paste. The common code is the handling of group use statements (cases involving |
|
For reference, here is the diff between both methods: --- a/v7.php
+++ b/v8.php
@@ -3,7 +3,7 @@ class TokenParser
{
// Other code here. this is a file extract
- private function parseUseStatementV7(): array
+ private function parseUseStatementV8(): array
{
$groupRoot = '';
$class = '';
@@ -12,14 +12,22 @@ class TokenParser
$explicitAlias = false;
while (($token = $this->next())) {
$isNameToken = $token[0] === T_STRING || $token[0] === T_NS_SEPARATOR;
- if (!$explicitAlias && $isNameToken) {
+
+ if ($token[0] === T_NAME_QUALIFIED || $token[0] === T_NAME_FULLY_QUALIFIED) {
$class .= $token[1];
- $alias = $token[1];
- } else if ($explicitAlias && $isNameToken) {
- $alias .= $token[1];
+
+ $classSplit = explode('\\', $token[1]);
+ $alias = $classSplit[count($classSplit) - 1];
+ } else if($token[0] === T_NS_SEPARATOR) {
+ $class .= '\\';
} else if ($token[0] === T_AS) {
$explicitAlias = true;
$alias = '';
+ } else if ($isNameToken && !$explicitAlias) {
+ $class = $token[1];
+ $alias = $token[1];
+ } else if ($isNameToken && $explicitAlias) {
+ $alias = $token[1];
} else if ($token === ',') {
$statements[strtolower($alias)] = $groupRoot . $class;
$class = ''; |
| } else if ($token[0] === T_AS) { | ||
| $explicitAlias = true; | ||
| $alias = ''; | ||
| } else if ($isNameToken && !$explicitAlias) { |
There was a problem hiding this comment.
this should use $token[0] === T_STRING rather than $isNameToken, as the case of $token[0] === T_NS_SEPARATOR would never reach that case anyway.
Same for the next condition
|
btw, I think the case of |
|
I believe that this is one of the few rare cases when copy&paste is the right thing to do. However, it would be nice to add a few comments to mark the differences (and group them together). Also, the conditions should be as similar to the original as possible, e.g., keeping the order of |
At that time, I'm almost sure that the maintained version of doctrine/annotation will not support PHP 7 anyway so the duplicate code would be gone. |
|
You're right, I didn't realize the diff was so important. A good commit message, or, as @jkufner pointed out, comments in the code would help. |
|
@greg0ire as I said, cleaning the PHP 7 code could allow reducing it (for instance, we cannot have |
…errabus) This PR was merged into the 3.4 branch. Discussion ---------- [ClassLoader][Routing] Fix namespace parsing on php 8 | 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. Commits ------- 3d293b2 [ClassLoader][Routing] Fix namespace parsing on php 8.
|
I created a separate PR in #347 to keep a single method for both PHP 7 and PHP8. |
|
I'm closing this PR in favor of #347 |
4 participants
This fixes the failing tests from TokenParser on PHP 8.
There are 10 left Failing tests. But it should be fixed by an PHPUnit Update
fixes #339