Skip to content

[TypeInfo] Add PhpDocAwareReflectionTypeResolver #57618

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.

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

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Jul 1, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues
License MIT

Add a PhpDocAwareReflectionTypeResolver that resolves type on reflections prioritizing PHP documentation.
The same feature already exists in the PropertyInfo component and improves DX a lot.
Installing phpstan/phpdoc-parser automatically enables this feature both using the type-info component standalone and the fullstack framework.

@carsonbot carsonbot added this to the 7.2 milestone Jul 1, 2024
@OskarStark OskarStark changed the title [TypeInfo] Add PhpDocAwareReflectionTypeResolver [TypeInfo] Add PhpDocAwareReflectionTypeResolver Jul 1, 2024
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

with minor comments

@wouterj
Copy link
Member

wouterj commented Jul 1, 2024

Isn't this outside the scope of TypeInfo and exactly the scope for the PropertyType component? Or what is the difference between these 2 components if TypeInfo is also going to read type information from different sources outside of PHP's native types?

@chalasr
Copy link
Member

chalasr commented Jul 1, 2024

TypeInfo is about getting the type of any PHP element including class properties but also methods, functions and their parameters. It does it by reading through the most common sources that are built-in types but also docblocks until we don't need them anymore for types.
It’s a low-level brick that PropertyInfo builds on to provide more high-level features about class properties introspection.
That low-level API is more powerful and flexible than the one PropertyInfo used to embed, which mostly needs to be deprecated in favor of a TypeInfo integration as it appeared to be not good enough for some use cases, notably other on-going work such as json-encoder.

@chalasr
Copy link
Member

chalasr commented Jul 1, 2024

To illustrate more, this change unlocks making MapRequestPayload able to guess the inner type of a list of objects mapped from the request by extracting it from the controller action's docblock (see #54385 (comment))

@mtarld mtarld force-pushed the feat/phpdoc-aware-type-resolver branch from 00d8043 to 10fc1c4 Compare July 2, 2024 12:10
@mtarld mtarld requested a review from dunglas as a code owner July 2, 2024 12:10
@mtarld mtarld force-pushed the feat/phpdoc-aware-type-resolver branch from 10fc1c4 to 57a5c8a Compare July 2, 2024 13:03
@mtarld mtarld force-pushed the feat/phpdoc-aware-type-resolver branch from 57a5c8a to c6698ce Compare July 3, 2024 06:46
Copy link
Contributor

@Korbeil Korbeil left a comment

Choose a reason for hiding this comment

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

Looks awesome and makes phpDoc handling way easier !
I tried to make a phpDoc example from scratch here: symfony/symfony-docs#20014 (comment) (see "Then we extract the phpDoc we want to parse" part), it is very complex without such wrapper !

@chalasr
Copy link
Member

chalasr commented Jul 22, 2024

Unless I'm mistaken the build failure should disappear once merged in 7.2 and is due to 7.2.x-dev being installed as part of property-info's test suite, while this breaks BC in the experimental TypeInfo.

@fabpot
Copy link
Member

fabpot commented Jul 25, 2024

Thank you @mtarld.

@fabpot fabpot merged commit 0234e05 into symfony:7.2 Jul 25, 2024
7 of 10 checks passed
@mtarld mtarld deleted the feat/phpdoc-aware-type-resolver branch July 25, 2024 09:33
fabpot added a commit that referenced this pull request Jul 26, 2024
This PR was merged into the 7.1 branch.

Discussion
----------

[PropertyInfo] fix tests

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

following #57618

Commits
-------

7192dd1 fix tests
@fabpot fabpot mentioned this pull request Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants