BetterPhpDocParser does not handle inline tags

[andrewnicols]

Bug Report

Subject	Details
Rector version	Rector 2.0.6

We have some code which makes use of inline PHPDoc tags within other tags, for example the @copyright line in the following example contains an inline @link tag:

/**
 * Unit tests for stored_progress_bar_cleanup
 *
 * @package   core
 * @copyright Some Person <person@example.com> {@link https://example.com}
 * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
 * @covers    \core\task\stored_progress_bar_cleanup_task
 */

When the BetterDocsParser parses this docblock, it stops at the Copyright line before the inline link. It does not process any further docblock tags, and therefore fails to enact some of the subsequent rules.

My first quick dive into the code led me to think that this was an issue with PHPStan's phpdoc-parser and I raised phpstan/phpdoc-parser#261 but turns out I was wrong.

I've just spent a bit of time trying to track the true cause down and I believe it's coming from the DoctrineAnnotationDecorator's mergeNestedDoctrineAnnotations method.

https://github.com/rectorphp/rector/blob/main/src/BetterPhpDocParser/PhpDocParser/DoctrineAnnotationDecorator.php#L96-L159

Digging further here I can't quite tell if this is an issue with rector incorrectly testing the close bracket count here:

https://github.com/rectorphp/rector/blob/main/src/BetterPhpDocParser/PhpDocParser/DoctrineAnnotationDecorator.php#L319-L321

Or whether it's an issue with the phpstan lexer.

In the above example the Lexer generates tokens as follows:

  0 => 'Some',
  1 => ' ',
  2 => 'Person',
  3 => ' ',
  4 => '<',
  5 => 'person',
  6 => '@example',
  7 => '.com>',
  8 => ' ',
  9 => '{',
  10 => '@link',
  11 => ' ',
  12 => 'https',
  13 => ':',
  14 => '//example.com}',
  15 => '',

As you can see the token at index 9 is an opening brace and therefore matches the isCurrentTokenType test for Lexer::TOKEN_OPEN_CURLY_BRACKET
https://github.com/rectorphp/rector/blob/main/src/BetterPhpDocParser/PhpDocParser/DoctrineAnnotationDecorator.php#L316

However the close brace is found on index 14 appended to part of the URI: //example.com}.
This fails to match the isCurrentTokenType test for Lexer::TOKEN_CLOSE_CURLY_BRACKET because it is a part of the string.
The str_contains test only checks for a close paren ()) so this does not match either.

https://github.com/rectorphp/rector/blob/main/src/BetterPhpDocParser/PhpDocParser/DoctrineAnnotationDecorator.php#L319

As a result the isClosedContent test does not match, and the tag is entirely removed.

In subsequent iterations of the loop we of course append to the $genericTagValueNode->value using the composed content, and so the isClosedContent parsing continues to fail until we run out of tags to parse.

https://github.com/rectorphp/rector/blob/main/src/BetterPhpDocParser/PhpDocParser/DoctrineAnnotationDecorator.php#L137-L139

Minimal PHP Code Causing Issue

https://getrector.com/demo/e27ca69b-ed81-475a-a3a0-17f097610c26

Expected Behaviour

The end brace of inline tag should not be treated as a nested tag, and should therefore capture all subsequent tags in the block to be parsed and the rules correctly applied.

[TomasVotruba]

Thanks for detailed reporting.
I'm curious, what rule is broken here or what migration are you trying to achieve?

We might not be able to fix this as it's trade off for other parts to work, that's why I'm asking.

[TomasVotruba]

Ah, it's about the CoversAnnotationWithValueToAttributeRector here then. Could you send a minimal failing test fixture for this rule to https://github.com/rectorphp/rector-phpunit ?

[andrewnicols]

Thanks for the quick reply.

As shown in the demo we're trying to update PHPUnit Coverage rules.

[andrewnicols]

Ah, it's about the CoversAnnotationWithValueToAttributeRector here then. Could you send a minimal failing test fixture for this rule to https://github.com/rectorphp/rector-phpunit ?

I've been trying to write a test for the DoctrineAnnotationDecorator but have been struggling to do so because that's where we're seeing the bug and it applies to all rules which impact docblock annotations, not just this one. I'm happy to write a test against the PHPUnit ruleset - that would be much simpler! I'll get on that first thing tomorrow (00:33 here).

Thanks!

[TomasVotruba]

It's easier for contributors to add single rule with simple test fixture, instead of deep diving into docblock parser. I don't want you to bother with that, as it could take few weeks to understand :)

Single rule test case should do. The smaller test fixture the better.

[andrewnicols]

I've quickly created a PR for the test
rectorphp/rector-phpunit#440

And a PR which fixes the issue for me: rectorphp/rector-src#6670