[property-info] Detect invalid tags and throw error accordingly #36850

drupol · 2020-05-18T09:27:32Z

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #36049 and api-platform/core#3565
License	MIT

I'm doing this as a work in progress.

This PR:

Update composer.json and support phpdocumentor/reflection-docblock version ^3, ^4 and ^5.
Add a new test to check if an invalid tag returns the proper exception
Tests are passing when building with lowest deps (I don't know how to deal with that yet)
Tests are passing using highest deps

Questions:

How to deal with the test when the InvalidTag class is not available ?
Throwing an exception might be a BC break
If instead of throwing an InvalidArgumentException, we ignore it, then the following tests are failing:

1) Symfony\Component\PropertyInfo\Tests\Extractor\PhpDocExtractorTest::testExtract with data set #27 ('emptyVar', null, null, null)

Error: Call to undefined method phpDocumentor\Reflection\DocBlock\Tags\InvalidTag::getType()

/home/devlin/dev/git/symfony/src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php:151
/home/devlin/dev/git/symfony/src/Symfony/Component/PropertyInfo/Tests/Extractor/PhpDocExtractorTest.php:40

drupol · 2020-05-18T10:14:58Z

@nicolas-grekas Hi! Should I rebase this for branch 4.4 ?

nicolas-grekas · 2020-05-18T10:27:44Z

Should I rebase this for branch 4.4 ?

this looks like a bug fix to me, so yes please. Check 3.4 before, in case it makes sense on 3.4.

nicolas-grekas · 2020-05-18T12:19:50Z

composer.json

@@ -100,7 +100,7 @@
        "egulias/email-validator": "~1.2,>=1.2.8|~2.0",
        "symfony/phpunit-bridge": "^3.4.31|^4.3.4|~5.0",
        "symfony/security-acl": "~2.8|~3.0",
-        "phpdocumentor/reflection-docblock": "^3.0|^4.0"
+        "phpdocumentor/reflection-docblock": "^3.0||^4.0||^5.0"


please use a single | for consistency

nicolas-grekas · 2020-05-18T12:21:14Z

Tests are passing using highest deps

not the case anymore, failures are on all jobs :)

dunglas · 2020-05-18T15:27:53Z

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php

@@ -83,6 +83,10 @@ public function getShortDescription($class, $property, array $context = [])
        }

        foreach ($docBlock->getTagsByName('var') as $var) {
+            if (is_a($var, 'phpDocumentor\Reflection\DocBlock\Tags\InvalidTag')) {


Suggested change

if (is_a($var, 'phpDocumentor\Reflection\DocBlock\Tags\InvalidTag')) {

if (is_a($var, InvalidTag::class)) {

(add the appropriate use too)

The InvalidTag is introduced in v5 of phpdocumentor/reflection-docblock. A use statement would be a BC break with v3 and v4 since those are autoloaded?

no. Use statements don't imply that a class exist. And ::class does not either (it is not an actual constant access on the class, but a compile-time thing)

Ha I did't know that.

dunglas · 2020-05-18T15:28:37Z

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php

@@ -137,6 +141,10 @@ public function getTypes($class, $property, array $context = [])
        $types = [];
        /** @var DocBlock\Tags\Var_|DocBlock\Tags\Return_|DocBlock\Tags\Param $tag */
        foreach ($docBlock->getTagsByName($tag) as $tag) {
+            if (is_a($tag, 'phpDocumentor\Reflection\DocBlock\Tags\InvalidTag')) {


Suggested change

if (is_a($tag, 'phpDocumentor\Reflection\DocBlock\Tags\InvalidTag')) {

if (is_a($tag, InvalidTag::class)) {

dunglas · 2020-05-18T15:29:05Z

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php

@@ -137,6 +141,10 @@ public function getTypes($class, $property, array $context = [])
        $types = [];
        /** @var DocBlock\Tags\Var_|DocBlock\Tags\Return_|DocBlock\Tags\Param $tag */
        foreach ($docBlock->getTagsByName($tag) as $tag) {
+            if (is_a($tag, 'phpDocumentor\Reflection\DocBlock\Tags\InvalidTag')) {
+                return null;


Shouldn't we throw here too?

Throwing here would break applications with invalid docblocks.

nicolas-grekas · 2020-06-14T10:31:39Z

Closing in favor of #36975, thanks @drupol

carsonbot added Status: Needs Review Bug Feature labels May 18, 2020

nicolas-grekas added this to the 4.4 milestone May 18, 2020

nicolas-grekas removed the Feature label May 18, 2020

drupol changed the base branch from master to 3.4 May 18, 2020 10:43

Update composer.json.

d83fa01

nicolas-grekas modified the milestones: 4.4, 3.4 May 18, 2020

nicolas-grekas reviewed May 18, 2020

View reviewed changes

drupol added 3 commits May 18, 2020 15:51

Update composer.json for consistency.

4664618

Add new test.

07cfd92

Update PhpDocExtractor.

b5f1423

dunglas requested changes May 18, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels May 18, 2020

xabbuh added the PropertyInfo label May 21, 2020

DerManoMann mentioned this pull request May 28, 2020

[PropertyInfo] Make PhpDocExtractor compatible with phpDocumentor v5 #36975

Merged

nicolas-grekas closed this Jun 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[property-info] Detect invalid tags and throw error accordingly #36850

[property-info] Detect invalid tags and throw error accordingly #36850

drupol commented May 18, 2020 •

edited

drupol commented May 18, 2020

nicolas-grekas commented May 18, 2020 •

edited

nicolas-grekas May 18, 2020

nicolas-grekas commented May 18, 2020

dunglas May 18, 2020

jaapio May 20, 2020

stof May 20, 2020

jaapio May 20, 2020

dunglas May 18, 2020

dunglas May 18, 2020

jaapio May 20, 2020

nicolas-grekas commented Jun 14, 2020

	if (is_a($var, 'phpDocumentor\Reflection\DocBlock\Tags\InvalidTag')) {
	if (is_a($var, InvalidTag::class)) {

	if (is_a($tag, 'phpDocumentor\Reflection\DocBlock\Tags\InvalidTag')) {
	if (is_a($tag, InvalidTag::class)) {

[property-info] Detect invalid tags and throw error accordingly #36850

[property-info] Detect invalid tags and throw error accordingly #36850

Conversation

drupol commented May 18, 2020 • edited

drupol commented May 18, 2020

nicolas-grekas commented May 18, 2020 • edited

Choose a reason for hiding this comment

nicolas-grekas commented May 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jun 14, 2020

drupol commented May 18, 2020 •

edited

nicolas-grekas commented May 18, 2020 •

edited