Skip to content

[PropertyInfo] ConstructorExtractor can read from promoted properties #62385

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.

Sign up for GitHub

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

Jump to bottom
Open
xavierleune wants to merge 3 commits into
base: 7.3
Choose a base branch
from
Open

[PropertyInfo] ConstructorExtractor can read from promoted properties #62385

xavierleune wants to merge 3 commits into from
+237 −14

Conversation

@xavierleune
Copy link
Contributor

@xavierleune xavierleune commented Nov 13, 2025

Q A
Branch? 7.3
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #60795
License MIT

The issue #60795 and the discussion #62045 describes a BC Break with symfony 7.3 regarding the ConstructorExtractor. Old behavior could extract types from promoted properties docblock, while the new behavior only works with the function docblock. As the last ConstructorExtractor is ReflectionExtractor, the extraction fails to provide an accurate description typed arrays, leading to incorrect deserialization.
This PR adds a fallback to read docblock from promoted properties too, to restore the original behavior.

@xavierleune xavierleune requested a review from dunglas as a code owner November 13, 2025 13:00
@carsonbot carsonbot added this to the 7.3 milestone Nov 13, 2025
@xavierleune xavierleune force-pushed the fix/constructor-extractor branch 4 times, most recently from 19fe14f to fae062d Compare November 13, 2025 14:37
@HypeMC
Copy link
Member

HypeMC commented Nov 13, 2025

@xavierleune Why not just disable the ConstructorExtractor? If you're not using it, there's no need for it to stay enabled. It also feels odd to read property PHPDoc through a method named getTypesFromConstructor().

@xavierleune
Copy link
Contributor Author

xavierleune commented Nov 13, 2025

@HypeMC disabling it globally feels like a bit harsh, moreover the current implementation already reads the PHPDoc, but for the function only and not for the promoted props. So the expected behavior ("read the data from the constructor") is incomplete and a BC break.
I really think this should be considered as a bug and should be fixed on 7.3, not by asking to disable the new extractor that can be useful and is enabled by default.

mtarld
mtarld reviewed Nov 14, 2025
View reviewed changes
Copy link
Contributor

@mtarld mtarld left a comment

Choose a reason for hiding this comment

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

I think that this fix might be needed.
Indeed, the PhpDocAwareReflectionTypeResolver of TypeInfo does the same.

@xavierleune xavierleune changed the title [PropertyInfo] ConstructorExtractor now fallbacks on promoted properties [PropertyInfo] ConstructorExtractor can reads from promoted properties Nov 14, 2025
@xavierleune xavierleune changed the title [PropertyInfo] ConstructorExtractor can reads from promoted properties [PropertyInfo] ConstructorExtractor can read from promoted properties Nov 14, 2025
@xavierleune xavierleune force-pushed the fix/constructor-extractor branch from 7cbfa54 to cbfd37a Compare November 17, 2025 09:56
@xavierleune
Copy link
Contributor Author

xavierleune commented Nov 17, 2025

@nicolas-grekas thanks for the review, code is up to date.

@OskarStark OskarStark changed the title [PropertyInfo] ConstructorExtractor can read from promoted properties [PropertyInfo] ConstructorExtractor can read from promoted properties Nov 17, 2025
HypeMC
HypeMC reviewed Nov 17, 2025
View reviewed changes
src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php
Comment on lines +187 to 195
if (!$docBlock = $this->getDocBlockFromPromotedProperty($class, $property)) {
if (!$docBlock = $this->getDocBlockFromConstructor($class, $property)) {
return null;
}

if (!$docBlock) {
return null;
$tags = $docBlock->getTagsByName('param');
} else {
$tags = $docBlock->getTagsByName('var');
}
Copy link
Member

@HypeMC HypeMC Nov 17, 2025

Choose a reason for hiding this comment

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

Perhaps this would be a bit more readable:

if ($docBlock = $this->getDocBlockFromPromotedProperty($class, $property)) {
    $tags = $docBlock->getTagsByName('var');
} elseif ($docBlock = $this->getDocBlockFromConstructor($class, $property)) {
    $tags = $docBlock->getTagsByName('param');
} else {
    return null;
}

Copy link
Contributor

@webda2l webda2l Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think I'll go for:

    $tags = $this->getDocBlockFromPromotedProperty($class, $property)?->getTagsByName('var')
        ?? $this->getDocBlockFromConstructor($class, $property)?->getTagsByName('param');
        
   if (null === $tags) {
     return null;
  }

src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php
public static function constructorPromotedPropertyWithDocblockProvider(): iterable
{
// ReflectionExtractor can only extract native PHP types, not generic types from docblocks
yield ['dates', Type::array()];
Copy link
Member

@HypeMC HypeMC Nov 17, 2025

Choose a reason for hiding this comment

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

No need for the data provider if there's only one case.

src/Symfony/Bundle/FrameworkBundle/Tests/Functional/SerializerTest.php

public function provideDeserializeArrayOfObjectData(): array
{
return [
Copy link
Member

@HypeMC HypeMC Nov 17, 2025

Choose a reason for hiding this comment

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

Why not yield here as well?

src/Symfony/Bundle/FrameworkBundle/Tests/Functional/SerializerTest.php
/**
* @dataProvider provideDeserializeArrayOfObjectData
*/
public function testDeserializeArrayOfObject(string $expectedClass, bool $usePromotedProperties, bool $withConstructorExtractor)
Copy link
Member

@HypeMC HypeMC Nov 17, 2025

Choose a reason for hiding this comment

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

Lowest-deps build is failing because it's using PropertyInfo v6.4.0. Even though the with_constructor_extractor option was added in v7.3, the actual constructor extractor logic was added in v5.2.

It might make sense to split the PR and apply the PropertyInfo part of the fix to 6.4, but I'm not completely sure about that.

mtarld
mtarld reviewed Dec 5, 2025
View reviewed changes
src/Symfony/Component/PropertyInfo/Tests/Fixtures/Php80PromotedDummyWithDocblock.php
class Php80PromotedDummyWithDocblock
{
/**
* @param \DateTime[] $datesWithIncoherentDocBlock
Copy link
Contributor

@mtarld mtarld Dec 5, 2025

Choose a reason for hiding this comment

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

If you are testing with Type::list, you might want to write list<\DateTime> instead. Otherwise, tests will fail after #62388 being merged.

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

Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@HypeMC HypeMC HypeMC left review comments

@mtarld mtarld mtarld left review comments

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

+1 more reviewer

@webda2l webda2l webda2l left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug PropertyInfo Status: Needs Review

Projects

None yet

Milestone

7.3

Development

Successfully merging this pull request may close these issues.

6 participants

@xavierleune @HypeMC @nicolas-grekas @webda2l @mtarld @carsonbot