Generics containing spaces not parsed correctly

[gronostajo]

In v4.3.2 calling \phpDocumentor\Reflection\DocBlockFactory::create for this phpDoc:

/**
 * @return array<string, Channel>
 */

will cause a RuntimeException from \phpDocumentor\Reflection\TypeResolver::parseTypes:263 with message 'A type is missing in a collection expression'.

The bug seems to be in \phpDocumentor\Reflection\DocBlock\Tags\Return_::create:50. This split:

$parts = preg_split('/\s+/Su', $body, 2);

doesn't take generics into account. array<string, is being read as type and Channel> becomes the description. Removing the space after comma solves the problem.

I've tried to determine if those spaces are actually a valid syntax and it seems like they kind-of-are:

1. Grammar on phpdoc.org doesn't allow for them (on a side note, the "phpDocumentor" link in top left leads to a 404)
2. PSR-5 grammar currently doesn't support generics too
3. However the PSR-5 draft did allow for them before Oct 16, 2018 and spaces were allowed.
4. The file that throws exception phpdocumentor/type-resolver/src/TypeResolver.php does itself use spaces on lines 64-65:

   /**
    * @var array<string, string> List of recognized keywords and unto which Value Object they map
    * @psalm-var array<string, class-string<Type>>
    */
   

So the implementation seems to be inconsistent with grammar in PSR and usage in phpDocumentor/TypeResolver.

[GrahamCampbell]

I think this was already addressed in v5 of this library?

[kuraobi]

This broke production on our project, making Symfony serializer crash. We had to lock the version to 4.3.1 to make it work again. Maybe you should rollback 4.3.2 release since it is broken, then either fix it or just wait for v5.

[jaapio]

@kuraobi I think, this was not introduced in v4.3. the tags files in v4.3 haven't been changed for years. So a more extended bug report would help instead of blaming this project without any proper feedback.

We know that the world around us is moving forward, and we are trying to do the best we can with the limited time we have to keep up with other projects that have more than 2 contributors.

Our type resolver is a bit further than this library because adding support for generics there was an easy task. The docblocks on the other side are way more complicated to extract we have to do lot of tokenizing and regexes to get the job done. To be able to deliver a good quality of code we want to use the latest tools available in the eco system. Since the docblocks of our code are not processed by phpDocumentor this isn't a problem atm.

In the end, we are not responsible for your project's production code. You are more than welcome to contribute to the project to help us moving forward. But you can never blame us like this for breaking your code. It is up to you to check before upgrading a library. Without a proper bug report from yourside I cannot produce the issue and see if something breaks the existing behavior. I do not know the internals of Symfony serializer but with a quick look at the repo, they don't even seem to reference this package at all!

[jaapio]

Possible related:
symfony/symfony#35077

[kuraobi]

I think, this was not introduced in v4.3

Yes it was. 4.3.2 has the bug, while 4.3.1 does not. I downgraded to 4.3.1 and everything works fine.

I do not know the internals of Symfony serializer but with a quick look at the repo, they don't even
seem to reference this package at all!

-> https://packagist.org/packages/symfony/serializer-pack

They rely on SemVer, and since there is a BC in the 4.3.2 fix release, it breaks. I'm not blaming you, I'm just saying that since you have a BC in your 4.3.2 release, you should roll it back to prevent it from breaking every Symfony project out there.

There is already a bug report in the original message, I was just confirming the bug and informing you that it breaks the Symfony serializer. I don't have much more than @gronostajo , except stacktraces if you want to know where Symfony calls this code:

RuntimeException: A type is missing in a collection expression
#40 /vendor/phpdocumentor/type-resolver/src/TypeResolver.php(263): phpDocumentor\Reflection\TypeResolver::parseTypes
#39 /vendor/phpdocumentor/type-resolver/src/TypeResolver.php(439): phpDocumentor\Reflection\TypeResolver::resolveCollection
#38 /vendor/phpdocumentor/type-resolver/src/TypeResolver.php(224): phpDocumentor\Reflection\TypeResolver::parseTypes
#37 /vendor/phpdocumentor/type-resolver/src/TypeResolver.php(146): phpDocumentor\Reflection\TypeResolver::resolve
#36 /vendor/phpdocumentor/reflection-docblock/src/DocBlock/Tags/Var_.php(68): phpDocumentor\Reflection\DocBlock\Tags\Var_::create
#35 [internal](0): call_user_func_array
#34 /vendor/phpdocumentor/reflection-docblock/src/DocBlock/StandardTagFactory.php(201): phpDocumentor\Reflection\DocBlock\StandardTagFactory::createTag
#33 /vendor/phpdocumentor/reflection-docblock/src/DocBlock/StandardTagFactory.php(122): phpDocumentor\Reflection\DocBlock\StandardTagFactory::create
#32 /vendor/phpdocumentor/reflection-docblock/src/DocBlockFactory.php(231): phpDocumentor\Reflection\DocBlockFactory::parseTagBlock
#31 /vendor/phpdocumentor/reflection-docblock/src/DocBlockFactory.php(96): phpDocumentor\Reflection\DocBlockFactory::create
#30 /vendor/symfony/property-info/Extractor/PhpDocExtractor.php(201): Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor::getDocBlockFromProperty
#29 /vendor/symfony/property-info/Extractor/PhpDocExtractor.php(172): Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor::getDocBlock
#28 /vendor/symfony/property-info/Extractor/PhpDocExtractor.php(123): Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor::getTypes
#27 /vendor/symfony/property-info/PropertyInfoExtractor.php(109): Symfony\Component\PropertyInfo\PropertyInfoExtractor::extract
#26 /vendor/symfony/property-info/PropertyInfoExtractor.php(74): Symfony\Component\PropertyInfo\PropertyInfoExtractor::getTypes
#25 /vendor/symfony/property-info/PropertyInfoCacheExtractor.php(118): Symfony\Component\PropertyInfo\PropertyInfoCacheExtractor::extract
#24 /vendor/symfony/property-info/PropertyInfoCacheExtractor.php(80): Symfony\Component\PropertyInfo\PropertyInfoCacheExtractor::getTypes

#33 /vendor/phpdocumentor/type-resolver/src/TypeResolver.php(263): phpDocumentor\Reflection\TypeResolver::parseTypes
#32 /vendor/phpdocumentor/type-resolver/src/TypeResolver.php(439): phpDocumentor\Reflection\TypeResolver::resolveCollection
#31 /vendor/phpdocumentor/type-resolver/src/TypeResolver.php(224): phpDocumentor\Reflection\TypeResolver::parseTypes
#30 /vendor/phpdocumentor/type-resolver/src/TypeResolver.php(146): phpDocumentor\Reflection\TypeResolver::resolve
#29 /vendor/phpdocumentor/reflection-docblock/src/DocBlock/Tags/Var_.php(68): phpDocumentor\Reflection\DocBlock\Tags\Var_::create
#28 [internal](0): call_user_func_array
#27 /vendor/phpdocumentor/reflection-docblock/src/DocBlock/StandardTagFactory.php(201): phpDocumentor\Reflection\DocBlock\StandardTagFactory::createTag
#26 /vendor/phpdocumentor/reflection-docblock/src/DocBlock/StandardTagFactory.php(122): phpDocumentor\Reflection\DocBlock\StandardTagFactory::create
#25 /vendor/phpdocumentor/reflection-docblock/src/DocBlockFactory.php(231): phpDocumentor\Reflection\DocBlockFactory::parseTagBlock
#24 /vendor/phpdocumentor/reflection-docblock/src/DocBlockFactory.php(96): phpDocumentor\Reflection\DocBlockFactory::create
#23 /vendor/symfony/property-info/Extractor/PhpDocExtractor.php(201): Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor::getDocBlockFromProperty
#22 /vendor/symfony/property-info/Extractor/PhpDocExtractor.php(172): Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor::getDocBlock
#21 /vendor/symfony/property-info/Extractor/PhpDocExtractor.php(123): Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor::getTypes
#20 /vendor/symfony/property-info/PropertyInfoExtractor.php(109): Symfony\Component\PropertyInfo\PropertyInfoExtractor::extract
#19 /vendor/symfony/property-info/PropertyInfoExtractor.php(74): Symfony\Component\PropertyInfo\PropertyInfoExtractor::getTypes
#18 /vendor/symfony/property-info/PropertyInfoCacheExtractor.php(118): Symfony\Component\PropertyInfo\PropertyInfoCacheExtractor::extract
#17 /vendor/symfony/property-info/PropertyInfoCacheExtractor.php(80): Symfony\Component\PropertyInfo\PropertyInfoCacheExtractor::getTypes
#16 /vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php(485): Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::denormalizeParameter
#15 /vendor/symfony/serializer/Normalizer/AbstractNormalizer.php(496): Symfony\Component\Serializer\Normalizer\AbstractNormalizer::instantiateObject
#14 /vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php(237): Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::instantiateObject
#13 /vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php(335): Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::denormalize
#12 /vendor/symfony/serializer/Serializer.php(191): Symfony\Component\Serializer\Serializer::denormalize

[GrahamCampbell]

Yes it was. 4.3.2 has the bug, while 4.3.1 does not.

4.3.2 and 4.3.1 are identical apart from a typehint change. The issue must be due to a phpdocumentor/type-resolver upgrade?

4.3.1...4.3.2

[kuraobi]

I think the problem comes from an incompatibility with:

"phpdocumentor/reflection-common": "^1.0.0 || ^2.0.0",
"phpdocumentor/type-resolver": "~0.4 || ^1.0.0",

phpdocumentor/type-resolver was locked to 0.4 before, which was working fine. Either phpdocumentor/type-resolver@^1.0.0 has a problem, or phpdocumentor/reflection-docblock@4.3.2 has a problem with phpdocumentor/type-resolver@^1.0.0.
When I say "problem", I mean that it behaves differently, which is a breaking change, that's why downstream projects have problems. Hence my suggestion to rollback. Then if the problem is indeed within phpdocumentor/*, it should be fixed, and if it's not a bug it's still a breaking change and should only be released as a major release (i.e. v5). I don't know why you bumped the version of the type-resolver, maybe it's safer to leave it to ~0.4 for v4.3 and just upgrade it in v5 if there is no need for the upgrade.

[GrahamCampbell]

👎 For changing the dependencies back to what they were. Changing dependencies is not a breaking change.

[GrahamCampbell]

It's too late too change them back anyway, since composer will just resolve the older version of this package in order to get the newer dependencies.

[GrahamCampbell]

Instead, we should just fix the upstream bug?

[jaapio]

Yes we should, but I don't think it is a bug in the type resolver. But a bug in this package passing wrong data to the type resolver. That needs to be checked, before any one can pass a pr to any package

[mvriel]

I just investigated the issue and neither 4.3.2 nor 4.3.1 was able to deal with this situation.

I wrote the following testcase in the ReturnTest:

    public function testFactoryMethodWithGenericWithSpace()
    {
        $descriptionFactory = m::mock(DescriptionFactory::class);
        $resolver = new TypeResolver();
        $context = new Context('');

        $type = new String_();
        $description = new Description('My Description');
        $descriptionFactory->shouldReceive('create')->with('My Description', $context)->andReturn($description);

        $fixture = Return_::create('array<string, string> My Description', $resolver, $descriptionFactory, $context);

        $this->assertSame('string My Description', (string)$fixture);
        $this->assertEquals($type, $fixture->getType());
        $this->assertSame($description, $fixture->getDescription());
    }

And in tag 4.3.1 (using TypeResolver 4.0) this resulted in:

There was 1 error:

1) phpDocumentor\Reflection\DocBlock\Tags\ReturnTest::testFactoryMethodWithGenericWithSpace
InvalidArgumentException: "\array<string," is not a valid Fqsen.

/app/vendor/phpdocumentor/reflection-common/src/Fqsen.php:48
/app/vendor/phpdocumentor/type-resolver/src/FqsenResolver.php:70
/app/vendor/phpdocumentor/type-resolver/src/FqsenResolver.php:32
/app/vendor/phpdocumentor/type-resolver/src/TypeResolver.php:264
/app/vendor/phpdocumentor/type-resolver/src/TypeResolver.php:118
/app/tests/unit/DocBlock/Tags/ReturnTest.php:163

And in 4.3.2 (using TypeResolver 1.0.1):

There was 1 error:

1) phpDocumentor\Reflection\DocBlock\Tags\ReturnTest::testFactoryMethodWithGenericWithSpace
RuntimeException: A type is missing in a collection expression

/app/vendor/phpdocumentor/type-resolver/src/TypeResolver.php:263
/app/vendor/phpdocumentor/type-resolver/src/TypeResolver.php:439
/app/vendor/phpdocumentor/type-resolver/src/TypeResolver.php:224
/app/vendor/phpdocumentor/type-resolver/src/TypeResolver.php:146
/app/tests/unit/DocBlock/Tags/ReturnTest.php:163

Conclusion: this behaviour was always unsupported by both 4.3.1 and 4.3.2. This difference seems to be the type of exception. 4.3.1 threw an InvalidArgumentException but 4.3.2 throws a RuntimeException.

My assumption is that the Symfony component silently ignores any error based on the exception type

[jaapio]

This is what I kind of expect from the earlier issue report at the start of this issue. Phpdoc expects a space to be the start of a description. This requires a lot of work to be fixed in this library.

Pr's are welcome, but I don't think we will have time to do this up front. I think we should focus first on more important issues. Since this is not a bug but a feature request.