[Docs] Constructor arguments conflict with class fields

[rvanlaak]

API Platform version(s) affected: x.y.z 4.1.17

Description

When a constructor argument gets defined that has the same name as the class's field, in which the constructor argument gets used to instantiate values in the constructor, the OpenAPI docs then type the field as array<string> items while they actually should be typed as the classes that just were instantiated.

How to reproduce

The collection is fairly straight forward

class Foo {
   # ...

  #[ORM\OneToMany(targetEntity: Bar::class, mappedBy: 'foo', fetch: 'EXTRA_LAZY')]
  #[ORM\OrderBy(['level' => 'ASC'])]
  public Collection $bars;

  public __construct(array $bars) 
  {
     $this->bars = new ArrayCollection(array_map( ... , $bars));   
  }

And when the constructor argument is named differently, the bar field on foo is correctly documented as object with it's related fields again:

class Foo
{

    /**
     * @var Bar[]
     */
    #[ORM\OneToMany(targetEntity: PackagingLayer::class, mappedBy: 'packagingHierarchy', fetch: 'EXTRA_LAZY')]
    public Collection $bars;

    public function __construct(
-        array $bars,
+        array $inputBars,
    ) {
-     $this->bars = new ArrayCollection(array_map( ... , $bars));   
+     $this->bars = new ArrayCollection(array_map( ... , $inputBars));   
    }
}

Possible Solution

The typing of the class field should take precedence over the typing of the constructor parameter. Especially as the constructor parameter won't be available after construction, which is how it will be read by APIP. That makes this classify as bug.

Renaming the constructor argument is a workaround for this, as the argument then doesn't relate to the class field anymore.

Additional Context

Probably this is related to an earlier change for promoted properties. That check should be enhanced, by adding a check whether that constructor argument actually does have a class field with the same name. The class field should then be used over the constructor parameter.

[...] is Bar also a resource?

see https://github.com/api-platform/core/issues/6173

I've tested this by either attributing both classes or none of the classes as ApiResource, but that does not change the behavior. The Foo class is a field on another entity (let's call it Baz), which is an ApiResource, so on normalization these three layers should result in a json graph like Baz -> Foo -> Bar[]

[soyuka]

https://github.com/api-platform/core/blob/main/src/Metadata/Property/Factory/PropertyInfoPropertyNameCollectionFactory.php probably there, therefore its probably and issue inside Symfony.

[rvanlaak]

Thanks for the pointer to the src. Great point to start diving deeper into this. This issue likely is related to the way how the metadata gets handled in the process of creating documentation out of it, not to the way how the property info component gets used to retrieve context (as that component is not aware that the intent is documenting the class as openapi scheme).

[VincentLanglet]

This is related to this Symfony PR which prefer the ConstructorExtractor of the Property ones. https://github.com/symfony/symfony/pull/30335/files

There is also test about this controversial behavior.

The solution might be to pass 'enable_constructor_extraction' => false in the context of every

• PropertyTypeExtractorInterface::getTypes
• PropertyTypeExtractorInterface::getType

calls in ApiPlatform\Metadata\Property\Factory\PropertyInfoPropertyMetadataFactory I assume @rvanlaak

[rvanlaak]

Thanks a lot for referring to these PRs @VincentLanglet . An approach here could be to re-wire the priorities of the property and constructor argument extractors? Solely passing enable_constructor_extraction as context does not seem sufficient, as some situations actually expect preferring properties over their constructor argument, while some other properties still should get extracted from the constructor arguments.

Probably, this asks for a prefer_constructor_over_property_extraction sort-of option on the extractor context.

It also seems like some more is going on, since 7.3 enabled the constructor extractor by default: https://github.com/symfony/symfony/issues/60795

[VincentLanglet]

An approach here could be to re-wire the priorities of the property and constructor argument extractors?

The existing priority was used for usecase like

private object $foo;

public function __construct(Foo $foo) {}

but this could still be wrong, cause you might have a setFoo(object $object).

Solely passing enable_constructor_extraction as context does not seem sufficient, as some situations actually expect preferring properties over their constructor argument, while some other properties still should get extracted from the constructor arguments.

I don't see a usecase where constructor_extraction would be preferred

[rvanlaak]

I don't see a usecase where constructor_extraction would be preferred

Probably for BC, and for writing scenarios, e.g. when DTOs are marked as ApiResource, sent through messenger (async), and processed on the entities on the background?

class SetUserFooDTO 
{
   public string $foo;
   public function __construct(FooEnum $foo) {
        $this->foo = $foo->value;
   # ...

class User {
   private FooEnum $foo;
   
   public function setFoo(string $foo) {
        $this->foo = FooEnum::from($foo);

[soyuka]

We're moving to TypeInfo so this should be tested on 4.2 as getTypes is now deprecated.

[rvanlaak]

Will that be opt in, and are you considering to prefer class fields over constructor arguments (given that potentially could be a BC break)?

Anything I can do to help with implementing it?

[soyuka]

I want to favor new PHP syntax's especially for api resources:

public __construct(public array $bars) {}

Therefore I'm not a huge fan of preferring class fields. However I'm always in favor of adding an option to enable that!

[VincentLanglet]

public __construct(public array $bars) {}

I don't think it's against class field (but it should be tested).

Symfony rely on constructor to override the class field, so in the following example

public Collection $bars;

public __construct(ArrayCollection $bars)

Symfony will say $bars is an ArrayCollection (coming from constructor), even it's partially true (unless some set the property manually)

In the example

private Collection $bars;

public __construct(ArrayCollection $bars)

it will more often true, unless the setter allow more types.

And in the example of the issue, it's totally wrong.

I assume public __construct(public array $bars) {} correctly understand the property as array since it's defined as a property.

A good way to check this @rvanlaak would be to write tests of the cases, and to check the difference we get with enable_constructor_extraction => false

[rvanlaak]

Exactly, the issue lies in the scenario where the constructor argument is not a promoted property, but is rewritten in the constructor method and then set on the class field.

So, not even:

public Collection $bars;

public __construct(ArrayCollection $bars)

... but ...

/* @var Bar[] /*
public Collection $bars;

public __construct(iterable $bars)
{ 
    $this->bars = new ArrayCollection(array_map(... do some magic ..., $bars));

In the last situation, the API Platform metadata considers the documentation as a string[] where Bar[] is expected.

Does TypeInfo differentiate constructor argument from promoted property?

[soyuka]

I'm not sure as its the Property Extractor that probably does this