symfony: [PropertyInfo] Bug/BC break? - Getter return type seems to have higher priority than property type

Symfony version(s) affected: 5.1.6, 5.1.7

Description
Hello!

In one of our projects we have a class similar to this:

final class ExampleDTO
{
    private ?string $example;

    public function __construct(?string $example)
    {
        $this->example = $example;
    }

    public function getExample(): string
    {
        return $this->example ?? 'fallback';
    }
}

It’s used in the context of api platform, where incoming data is being deserialized to this object.

Before with Symfony 5.1.5 the example property was detected as type ?string, after upgrading to 5.1.6/5.1.7 it’s only detected as type string. Now when the incoming data contains a null value, it will fail with The type of the "example" attribute must be "string", "NULL" given.

I did some tests and was able to track this new behaviour down to the property-info component. I suspect this change to be the reason: https://github.com/symfony/symfony/pull/38041

It looks like now the return type of getExample() is used instead of the property type hint (?string).

So I would like to ask if this is the expected behaviour and if yes, how can I make the old way work again (somehow influence the priorities?). Thanks in advance!

How to reproduce
I created a reproducer here: https://github.com/rogamoore/property-info-bug

Steps:

Checkout https://github.com/rogamoore/property-info-bug/releases/tag/old-behaviour (property-info 5.1.5)
Run composer install
Run symfony server:start
Go to http://127.0.0.1:8000/
Result:

^ array:1 [▼
  0 => Symfony\Component\PropertyInfo\Type {#116 ▼
    -builtinType: "string"
    -nullable: true
    -class: null
    -collection: false
    -collectionKeyType: null
    -collectionValueType: null
  }
]

Checkout https://github.com/rogamoore/property-info-bug/releases/tag/new-behaviour (property-info 5.1.6)
Run composer install
Run symfony server:start
Go to http://127.0.0.1:8000/
Result:

^ array:1 [▼
  0 => Symfony\Component\PropertyInfo\Type {#116 ▼
    -builtinType: "string"
    -nullable: false
    -class: null
    -collection: false
    -collectionKeyType: null
    -collectionValueType: null
  }
]

Additional context
PHP 7.4.10

About this issue

Original URL
State: closed
Created 4 years ago
Reactions: 7
Comments: 15 (6 by maintainers)

Most upvoted comments

Another situation where this seems to be a problem. Let’s say you have a DTO like the following, that to me seems to be a perfect normal use case.

final class Product
{
    private string $currency;

    public function __construct(int $amount, string $currency)
    {
        $this->currency = $currency;
    }

    public function getCurrency(): Currency
    {
        return Currency::fromString($this->currency);
    }
}

final class Currency
{
    private string $currency;

    private function __construct(string $currency)
    {
        $this->currency = $currency;
    }

    public static function fromString(string $currency): self
    {
        return new self($currency);
    }
}

With this last change to the property accessor, if you try to deserialize something into this class there will be an error because it tries to create a Currency instance

dpinheiro on Mar 18, 2021

I think I may have found a workaround for this

I discovered this PR https://github.com/symfony/symfony/pull/30335 (available for Symfony 5.2) and although there’s no documentation yet for it this could be used to give priority to extract the types from constructor, internally this new ConstructorExtractor will call the method getTypesFromConstructor of the other extractors. To use it we just have to declare it has a service and add the proper the tag, something like this:

Symfony\Component\PropertyInfo\Extractor\ConstructorExtractor:
        arguments:
            - { param1: "@property_info.php_doc_extractor", param2: "@property_info.reflection_extractor" }
        tags:
            - { name: "property_info.type_extractor", priority: -999 }

dpinheiro on May 14, 2021

@stof But would you agree, that the behaviour should be somehow stable/predictable and not change between minor releases? Looks like there are no tests covering this, otherwise they would have failed when changing it in https://github.com/symfony/symfony/pull/38041

Regarding expectations - as this component is called property-info I would argue that the actual type of the class property should come first and any other hints as constructor/getter/setter should be used just as a fallback.

rogamoore on Dec 1, 2020