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

Remove imprecise `T_IDENTIFIER` capture groups, leading to simpler token sequences #258

Conversation

@Ocramius
Copy link
Member

commented Mar 25, 2019

Note: I just need a review - will forward-port after that.

Discovered while working on #257

A T_IDENTIFIER represents:

  • a constant
  • a function name
  • a function name prefixed with \
  • a class name
  • a class name prefixed with \
  • a class constant
  • a class constant, with class name prefixed with \

The current capturing regular expression is too simplistic, and it
leads to edge cases where Foo\42.5 are captured as separate tokens.

In this patch, we deprecate DocLexer::T_NAMESPACE_SEPARATOR (which
shouldn't be looked up directly anymore), and tell the parser to work
with wider T_IDENTIFIER tokens instead.

Note that a test is also being removed. That is intentional, since the
test verifies a parser issue when parsing @Foo\3.42:

  • previously, the tokens would be [@ Foo\3.42]
  • now they are [@, Foo, \, 3.42]

This also means that the above will be parsed as @Foo.

Ocramius added some commits Mar 25, 2019

Using a more precise capturing group for `T_IDENTIFIER`
A `T_IDENTIFIER` represents:

 * a constant
 * a function name
 * a function name prefixed with `\`
 * a class name
 * a class name prefixed with `\`
 * a class constant
 * a class constant, with class name prefixed with `\`

The current capturing regular expression is too simplistic, and it
leads to edge cases where `Foo\42.5` are captured as separate tokens.

In this patch, we deprecate `DocLexer::T_NAMESPACE_SEPARATOR` (which
shouldn't be looked up directly anymore), and tell the parser to work
with wider `T_IDENTIFIER` tokens instead.

Note that a test is also being removed. That is intentional, since the
test verifies a parser issue when parsing `@foo\3.42`:

 * previously, the tokens would be `[@ Foo\3.42]`
 * now they are `[@, Foo, \, 3.42]`

This also means that the above will be parsed as `@foo`.

@Ocramius Ocramius added this to the 1.7.0 milestone Mar 25, 2019

@Ocramius Ocramius requested a review from Majkl578 Mar 25, 2019

@nicolas-grekas nicolas-grekas referenced this pull request Aug 6, 2019
22 of 23 tasks complete
@nicolas-grekas
Copy link
Member

left a comment

Makes sense (+ would fix a deprecation that blocks Symfony's CI on PHP 7.4 :) )

@@ -988,18 +988,7 @@ private function Identifier()
$this->lexer->moveNext();
$className = $this->lexer->token['value'];
while ($this->lexer->lookahead['position'] === ($this->lexer->token['position'] + strlen($this->lexer->token['value']))

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Aug 6, 2019

Member

because lookahead can be null here, this line triggers a deprecation on 7.4
that's one of the main remaining blockers we have to make Symfony's tests pass on PHP 7.4 :)

This comment has been minimized.

Copy link
@Ocramius

Ocramius Aug 8, 2019

Author Member

@guilhermeblanco let's separate the concern of fixing 7.4 compat and the issue found with this patch: we should check for the lookahead to exist in this while(), and have a test that triggers a deprecation warning on 7.4

@Tobion

Tobion approved these changes Aug 6, 2019

@Tobion

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Fixes #273

* @expectedException \Doctrine\Common\Annotations\AnnotationException
* @expectedExceptionMessage [Syntax Error] Expected Doctrine\Common\Annotations\DocLexer::T_IDENTIFIER or Doctrine\Common\Annotations\DocLexer::T_TRUE or Doctrine\Common\Annotations\DocLexer::T_FALSE or Doctrine\Common\Annotations\DocLexer::T_NULL, got '3.42' at position 5.
*/
public function testInvalidIdentifierInAnnotation()

This comment has been minimized.

Copy link
@Ocramius

Ocramius Aug 8, 2019

Author Member

Urgh, I shouldn't have removed this test. It should at least have a test declaring another sort of failure here.

@guilhermeblanco

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

As agreed, we decided to take a simplified approach and only resolve the 7.4 notice instead.
This patch is now considered invalid. Closing.

@alcaeus alcaeus removed this from the v1.7.0 milestone Aug 8, 2019

@alcaeus alcaeus removed the enhancement label Aug 8, 2019

@alcaeus alcaeus removed the request for review from Majkl578 Aug 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.