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.

@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.

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

The same kind of patch must be added to ReflectionExtractor.

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?

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).

@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

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::testPhpDocPriorityError: Class 'Symfony\Component\PropertyInfo\Extractor\ConstructorExtractor' not found...7) Symfony\Bundle\FrameworkBundle\Tests\Functional\SerializerTest::testDeserializeArrayOfObjectError: 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?

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.

could you please squash all commits in one?

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?

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 ([AbstractObjectNormalizer][PropertyInfo] AbstractObjectNormalizer/PropertyInfoExtractor does not respect constructor type hinting. #30053) because if property is phpdoc-annotated,PhpDocExtractor will return non-null result and it will not even reachReflectionExtractor. Please see below my explanation.

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

Originally I modifiedPhpDocExtractor::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 hasnoticed that) because it never calledReflectionExtractor::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 addedgetTypesFromConstructor toPhpDocExtractor andReflectionExtractor - they implementConstructorArgumentTypeExtractorInterface 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

What type should be returned byPropertyInfoExtractor::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);    }}

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?

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

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

Looks like this is a new feature indeed (fixing a "know limitation").@dunglas@lyrixx Do you agree?

@fabpot I agree this is a new feature, yes

FYI the same PR but targeted to 4.2#30128

Closing this one then.