Skip to content

Conversation

romaricdrigon
Copy link
Contributor

Q A
Branch? master (issue introduced in master)
Bug fix? yes
New feature? no
Deprecations? no
Tickets /
License MIT
Doc PR /

Background

Since a few days ago (this commit), PropertyInfo 5.2 component supports array PhpDoc types, such as array<string,string>.
While testing SF 5.2 over my project, it was failing over:

class SomeDoctrineEntityProxy implements \Doctrine\ORM\Proxy\Proxy
{
    /**
     * @var array<string, mixed> default values of properties to be lazy loaded, with keys being the property names
     *
     * @see \Doctrine\Common\Proxy\Proxy::__getLazyProperties
     */
    public static $lazyPropertiesDefaults = array();
}

Upon investigation, it appears array<string,mixed> is causing the issue: mixed type is not handled.

Expected behavior

PropertyInfo component should not guess anything for mixed type (return NULL), but it should not throw any exception.

Comment about my fix

I added a test case (arrayOfMixed) you can use to reproduce the issue.
Since array PhpDoc types were not tested at all, I also added a test case over array<string,string>.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 2, 2020

Can you have a look at deps=low failures, please?

@romaricdrigon
Copy link
Contributor Author

Can you have a look at deps=low failures, please?

Sure, I'm ont it.

@romaricdrigon
Copy link
Contributor Author

romaricdrigon commented Oct 2, 2020

So, to sum up, the issue is coming from old PhpDocExtractor versions not supporting array<x> notations. I believe support was introduced in version 4+ of phpdocumentor/reflection-docblock (for instance this issue has a few details...).
With deps=high, we are using version 5, it works. With deps=low, we are using version 3, which won't guess any type (so return NULL, and test case won't work).

PhpDocExtractor code is quite intricated with PhpDocumentor, and it is not quite obvious something can be mocked / worked around. There was already a method to detect if we are using PhpDocumentor5, so I'm using it. That's the best option I could think of.
Another option would be not to test that type is detected, since we can't control what PhpDocumentor is outputting. So I could remove those lines, the test would only check no exception is thrown.

@@ -122,7 +122,7 @@ private function createType(DocType $type, bool $nullable, string $docType = nul
$collectionKeyType = $this->getTypes($type->getKeyType())[0];

$collectionValueTypes = $this->getTypes($type->getValueType());
if (\count($collectionValueTypes) > 1) {
if (0 === \count($collectionValueTypes) || \count($collectionValueTypes) > 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!== 1 ?

@fabpot
Copy link
Member

fabpot commented Oct 3, 2020

Thank you @romaricdrigon.

@fabpot fabpot merged commit d69b525 into symfony:master Oct 3, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@romaricdrigon romaricdrigon deleted the fix-union-type branch October 6, 2020 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants