8000 [PropertyInfo] fix tests by xabbuh · Pull Request #57837 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[PropertyInfo] fix tests #57837

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
Merged
merged 1 commit into from
Jul 26, 2024
Merged

[PropertyInfo] fix tests #57837

merged 1 commit into from
Jul 26, 2024

Conversation

xabbuh
Copy link
Member
@xabbuh xabbuh commented Jul 26, 2024
Q A
Branch? 7.2
Bug fix? no
New feature? no
Deprecations? no
Issues
License MIT

@xabbuh xabbuh requested a review from dunglas as a code owner July 26, 2024 09:36
@carsonbot carsonbot added this to the 7.2 milestone Jul 26, 2024
@xabbuh
Copy link
Member Author
xabbuh commented Jul 26, 2024

@mtarld @chalasr I could need your help here as I am not sure that this is really the best way to fix the test failures. Basically, this PR reverts #57825. On the 7.1 branch this change was necessary as the ReflectionExtractor does not pass any resolvers to TypeResolver::create() which in turn means that the new PhpDocAwareReflectionTypeResolver will be used. On the 7.2 branch we are explicit and this means that the new PhpDocAwareReflectionTypeResolver is not used (is that intended)?

@chalasr
Copy link
Member
chalasr commented Jul 26, 2024

@xabbuh The reasoning behind not wiring the PhpDocAwareReflectionTypeResolver in PropertyInfo is that PropertyInfo has its own implementation for extracting types from phpdoc (PhpStanTypeHelper). I'm not entirely sure but as far as I understand that helper is only used by the to-be-deprecated PropertyTypeExtractorInterface::getTypes(), not the new type-info based getType(). If I'm right, then we should probably just always wire PhpDocAwareReflectionTypeResolver and keep relying on TypeResolver::create() on 7.2.

Mathias is on vacation for 2 weeks so I suggest merging this PR, he will revisit the topic once it gets back.
Thanks for the PRs

@xabbuh xabbuh merged commit da90fe7 into symfony:7.2 Jul 26, 2024
10 checks passed
@xabbuh xabbuh deleted the pr-57618 branch July 26, 2024 11:24
@mtarld
Copy link
Contributor
mtarld commented Aug 13, 2024

Hey @xabbuh,
The ReflectionExtractor of PropertyInfo is by definition not aware of phpdoc.
This is the wiring (ie: the service definition) that enable that feature using decoration.

Therefore, we cannot give the PhpDocAwareTypeResolver to the ReflectionExtractor, because if so, it won't behave at it used to be.

This is strange that tests are failing in 7.1 though. This might be because of cross-component weird dependencies (ie: property-info in "raw 7.1" and type-info in 7.1 with the PhpDocAwareTypeResolver)

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

@fabpot fabpot fabpot approved these changes

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

Assignees
No one assigned
Labels
PropertyInfo Status: Reviewed
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.
5 participants
@xabbuh @chalasr @mtarld @fabpot @carsonbot
0