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

Make constructor argument override the property docblock in PhpDocExtractor #30056

Open
wants to merge 2 commits into
base: 3.4
from

Conversation

Projects
None yet
6 participants
@karser
Copy link
Contributor

karser commented Feb 1, 2019

Q A
Branch? 3.4 up to 4.2
Bug fix? yes
New feature? no
BC breaks? hopefully no
Deprecations? no
Tests pass? yes
Fixed tickets #30053
License MIT

The original ticket is #30053
In short, when using PhpDocExtractor, it ignores the constructor argument type, although argument types from constructor are the only types that are valid for the class instantiation.
This PR adds the constructor phpdoc support to PhpDocExtractor.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Feb 1, 2019

if (!$reflectionConstructor) {
return null;
}
if (!preg_match('/\@param.+?\$'.$property.'/', $reflectionConstructor->getDocComment(), $matches)) {

This comment has been minimized.

@dunglas

dunglas Feb 5, 2019

Member

Can't we delegate the extraction of the parameter name to phpdocumentor/reflection-docblock? It would make the code more solid.

This comment has been minimized.

@karser

karser Feb 5, 2019

Author Contributor

Done. Now I parse the phpdoc params using reflection-docblock, then filter docblock.tags so that only the requested property param is kept and then recreate the docblock object. The code became more verbose though.

@xabbuh xabbuh added the PropertyInfo label Feb 5, 2019

karser added a commit to karser/symfony that referenced this pull request Feb 5, 2019

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Feb 5, 2019

Will this also solve the reported case where the constructor has real type hints instead of a docblock? If so, we should also add a test for that case.

@karser

This comment has been minimized.

Copy link
Contributor Author

karser commented Feb 5, 2019

@xabbuh Nope, because PhpDocExtractor has higher priority than ReflectionExtractor. So if the constructor argument is not phpdoc-annotated, it'll return the property type.
ReflectionExtractor will get control only in case if both constructor argument and the property are not phpdoc-annotated.

@dunglas

dunglas approved these changes Feb 5, 2019

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Feb 5, 2019

But how will this solve the linked issue then which is about a constructor not having a docblock. If I got you right, the ReflectionExtractor will not do anything in that case.

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Feb 5, 2019

The same kind of patch must be added to ReflectionExtractor.

@karser

This comment has been minimized.

Copy link
Contributor Author

karser commented Feb 5, 2019

Yep, adding phpdoc to the constructor would solve the linked issue.

@xabbuh How do you imagine this issue should be solved properly? Should we first check constructors for both PhpDocExtractor and ReflectionExtractor, and only after that properties?

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Feb 5, 2019

IMO we should only evaluate the property docblock (and in future PHP versions its type) if we were not able to infer the type from the constructor. The constructor is the gatekeeper here and can convey different information than the property if it performs some type conversion (like in the linked issue).

karser added a commit to karser/symfony that referenced this pull request Feb 6, 2019

Added ConstructorExtractor which has higher priority than PhpDocExtra…
…ctor and ReflectionExtractor. So that first attempt is to extract type from the constructor and otherwise from the property symfony#30056
@karser

This comment has been minimized.

Copy link
Contributor Author

karser commented Feb 6, 2019

@xabbuh Please take a look. I added ConstructorExtractor which has higher priority than PhpDocExtractor and ReflectionExtractor. So that first attempt is to extract the type from the constructor and otherwise from the property

karser added a commit to karser/symfony that referenced this pull request Feb 6, 2019

karser added a commit to karser/symfony that referenced this pull request Feb 6, 2019

karser added a commit to karser/symfony that referenced this pull request Feb 6, 2019

@karser

This comment has been minimized.

Copy link
Contributor Author

karser commented Feb 6, 2019

Travis passes on php5 but gets an error on 7.2 / 7.3: Class 'Symfony\Component\PropertyInfo\Extractor\ConstructorExtractor' not found. It happens only for functional tests:

1) Symfony\Bundle\FrameworkBundle\Tests\Functional\PropertyInfoTest::testPhpDocPriority
Error: Class 'Symfony\Component\PropertyInfo\Extractor\ConstructorExtractor' not found
...
7) Symfony\Bundle\FrameworkBundle\Tests\Functional\SerializerTest::testDeserializeArrayOfObject
Error: Class 'Symfony\Component\PropertyInfo\Extractor\ConstructorExtractor' not found

I tried php 7.2 / 7.3 locally but I'm not able to reproduce this issue. Maybe autoloading / bytecode cache issue, or something else?

@karser

This comment has been minimized.

Copy link
Contributor Author

karser commented Feb 6, 2019

Thanks @xabbuh this fixes tests for php 7.3 but the error is still there for 7.2.
Still not fully following why this error happening and why it uses composer.json dependencies instead of the current symfony revision.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 7, 2019

could you please squash all commits in one?

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

That's a lot of new public API for a bug fix, isn't this a new feature really?
How does that play with #25605?

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Feb 7, 2019

I wonder if we really need all the new classes (I haven't had time to read the code yet). But shouldn't the existing extractors take into account the constructor by default if the property they analyse is private?

@karser

This comment has been minimized.

Copy link
Contributor Author

karser commented Feb 7, 2019

That's a lot of new public API for a bug fix, isn't this a new feature really?

Does it mean it should be targeted to master?

How does that play with #25605?

@nicolas-grekas Unfortunately, I haven't seen this code because I was working in 3.4 branch, but it's different:

  1. It only fallbacks to constructor extraction (if getter/setter don't have types)
  2. It doesn't solve this issue (#30053) because if property is phpdoc-annotated, PhpDocExtractor will return non-null result and it will not even reach ReflectionExtractor. Please see below my explanation.

@xabbuh Let's review it once again:
When PropertyInfoExtractor::getTypes is called, it tries to infer the type using one of the extractors in this order: [PhpDocExtractor::getTypes, ReflectionExtractor::getTypes].

Originally I modified PhpDocExtractor::getTypes so that it tries to extract the argument DocBlock from the constructor first and otherwise, fallbacks to the property one. It didn't solve the issue though (@xabbuh has noticed that) because it never called ReflectionExtractor::getTypes since the property was phpdoc annotated.

That's why I added a separate extractor - ConstructorExtractor which is called first (-999) and it attempts to extract the type from constructor only, either from PhpDoc or using reflection.
I added getTypesFromConstructor to PhpDocExtractor and ReflectionExtractor - they implement ConstructorArgumentTypeExtractorInterface interface. ConstructorExtractor aggregates those extractors using compiler pass.

So the flow of control looks like this:

PropertyInfoExtractor::getTypes:
    - ConstructorExtractor::getTypes
        - PhpDocExtractor::getTypesFromConstructor
        - ReflectionExtractor::getTypesFromConstructor
    - PhpDocExtractor::getTypes
    - ReflectionExtractor::getTypes

@karser karser force-pushed the karser:phpdocextractor-constructor-overrides-property-docblock branch from 5d94e8d to 6eade3b Feb 7, 2019

@karser

This comment has been minimized.

Copy link
Contributor Author

karser commented Feb 7, 2019

What type should be returned by PropertyInfoExtractor::getTypes for these examples?
Currently, ConstructorExtractor will be executed first and return string.

class DTO1 {
    /** @var UuidInterface */
    private $uuid;

    public function __construct(string $uuid) {
        $this->uuid = new Uuid($uuid);
    }

    /**
     * @return UuidInterface
     */
    public function getUuid()
    {
        return $this->uuid;
    }
}
class DTO2 {
    /** @var UuidInterface */
    public $uuid;

    public function __construct(string $uuid) {
        $this->uuid = new Uuid($uuid);
    }
}
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 7, 2019

OK, no opinion on that, maybe @dunglas @lyrixx would have good advice.
How would this PR look like on 4.2? Can you have a look and maybe submit one?

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Feb 9, 2019

I'm not sure this is solvable as is...

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Feb 11, 2019

Yes, We knew there was some limitations to this system :/ IMHO, we need typed property to really fix this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment