Skip to content

[PropertyInfo] Add PhpStanExtractor #40457

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
fabpot merged 1 commit into from
Oct 30, 2021
Merged

[PropertyInfo] Add PhpStanExtractor #40457

fabpot merged 1 commit into from
Oct 30, 2021

Conversation

@Korbeil
Copy link
Contributor

@Korbeil Korbeil commented Mar 12, 2021 •
edited
Loading

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #38093
License MIT
Doc PR symfony/symfony-docs#...

This PR will add a PhpStanExtractor that is based on phpstan/phpdoc-parser library.
The PhpStan library allows us to manage union types in collection key values that we don't manage today.

Todo

  • PhpStanExtractor
  • Add tests for unions types
  • Add FrameworkBundle glue (use this extractor if phpstan/phpdoc-parser is present)
  • Update CHANGELOG

Related PR:

@Korbeil Korbeil requested a review from dunglas as a code owner March 12, 2021 15:46
@Korbeil Korbeil force-pushed the feature/phpstan-extractor branch 6 times, most recently from aeb982f to 2e08a5b Compare March 12, 2021 17:14
@OskarStark
Copy link
Contributor

OskarStark commented Mar 13, 2021

I would propose to name it PHPStanExtractor

@dunglas
Copy link
Member

dunglas commented Mar 14, 2021

@OskarStark IIRC the current name is more in sync with our coding conventions.

@OskarStark
Copy link
Contributor

OskarStark commented Mar 14, 2021

I know for sure, but this could be hard to read/understand because of the word "Php" and the word "Stan", additionally PHPStan is a proper name.

IIRC we use sth like ORM instead of Orm, but not 100% sure. I am currently on a phone 📱

Anyway it was just a proposal and a personal opinion, otherwise I would go with PhpstanExtractor then ....

@chalasr
Copy link
Member

chalasr commented Mar 14, 2021

IIRC we use sth like ORM instead of Orm, but not 100% sure. I am currently on a phone 📱

We do :) I agree with Oscar here.

@derrabus
Copy link
Member

derrabus commented Mar 14, 2021

I know for sure, but this could be hard to read/understand because of the word "Php" and the word "Stan", additionally PHPStan is a proper name.

How so we deal with other PHPSomething project names? The extractor for PHPDoc is named PhpDocExtractor, the PHPUnit bridge resides in Symfony\Bridge\PhpUnit… I think, I'm team @dunglas here. 🙃

Oh well, naming things is hard. 😓

@chalasr
Copy link
Member

chalasr commented Mar 14, 2021

Fair enough :) consistency first.

@OskarStark
Copy link
Contributor

OskarStark commented Mar 15, 2021

Stay consistent, because of PhpUnit

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 16, 2021

How does this compare to using phpdocumentor/reflection-docblock? Can we drop one for the other?

@nicolas-grekas nicolas-grekas added this to the 5.x milestone Mar 16, 2021
@Korbeil
Copy link
Contributor Author

Korbeil commented Mar 16, 2021

How does this compare to using phpdocumentor/reflection-docblock? Can we drop one for the other?

Actually, phpdocumentor/reflection-docblock implementation supports 3 interfaces:

  • Symfony\Component\PropertyInfo\PropertyDescriptionExtractorInterface
  • Symfony\Component\PropertyInfo\PropertyTypeExtractorInterface
  • Symfony\Component\PropertyInfo\Extractor\ConstructorArgumentTypeExtractorInterface

I do support 2 of them. I'll try to take a look at Symfony\Component\PropertyInfo\PropertyDescriptionExtractorInterface later but if I support it aswell we can plan to remove the extractor later.
It would be nice to make some benchmark to see if it doesn't have too much impact on performance aswell.

@Korbeil Korbeil force-pushed the feature/phpstan-extractor branch 2 times, most recently from 32e2af3 to 2884390 Compare March 16, 2021 17:27
nicolas-grekas
nicolas-grekas reviewed Mar 16, 2021
View reviewed changes
@Korbeil Korbeil force-pushed the feature/phpstan-extractor branch 3 times, most recently from 4f57313 to 560cd06 Compare March 22, 2021 09:40
@dunglas dunglas mentioned this pull request Jun 1, 2021
Closed
@Korbeil Korbeil force-pushed the feature/phpstan-extractor branch 2 times, most recently from 4a5236f to af62a30 Compare June 25, 2021 15:03
@OskarStark OskarStark changed the title [PropertyInfo] Add PhpStanExtractor [PropertyInfo] Add PhpStanExtractor Aug 1, 2021
@Korbeil Korbeil force-pushed the feature/phpstan-extractor branch from cdc9c56 to 8bdb91f Compare October 29, 2021 12:58
@Korbeil Korbeil force-pushed the feature/phpstan-extractor branch from 8bdb91f to 4651d61 Compare October 29, 2021 14:54
nicolas-grekas
nicolas-grekas approved these changes Oct 29, 2021
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas 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 cs comments)

@Korbeil Korbeil force-pushed the feature/phpstan-extractor branch from 4651d61 to 68300c7 Compare October 29, 2021 14:56
fabpot
fabpot approved these changes Oct 30, 2021
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

LGTM, I hope you're ready to maintain it as this is a large piece of code :)

@fabpot fabpot force-pushed the feature/phpstan-extractor branch from 68300c7 to 9931c37 Compare October 30, 2021 09:35
@fabpot
Copy link
Member

fabpot commented Oct 30, 2021

Thank you @Korbeil.

@fabpot fabpot merged commit 5745b43 into symfony:5.4 Oct 30, 2021
This was referenced Oct 30, 2021
Closed
Merged
Merged
@nicolas-grekas nicolas-grekas mentioned this pull request Dec 2, 2021
Closed
@vlastv vlastv mentioned this pull request Dec 2, 2021
Closed
weaverryan added a commit to weaverryan/ux that referenced this pull request Mar 22, 2023
@weaverryan
[Live] Bumping min versions of 2 packages to fix tests
4fcb213 
* doctrine/persistence to doctrine/persistence#286
* symfony/property-info to get symfony/symfony#40457
symfony-splitter pushed a commit to symfony/ux-live-component that referenced this pull request Mar 22, 2023
@weaverryan
[Live] Bumping min versions of 2 packages to fix tests
95151c4 
* doctrine/persistence to doctrine/persistence#286
* symfony/property-info to get symfony/symfony#40457
@colinodell colinodell mentioned this pull request Jun 9, 2023
Merged
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Jun 12, 2023
@javiereguiluz
minor symfony#18399 [PropertyInfo] Fix PhpStanExtractor added version…
db084b4 
… (colinodell)

This PR was merged into the 6.2 branch.

Discussion
----------

[PropertyInfo] Fix PhpStanExtractor added version

`PhpStanExtractor` was actually added in 5.4: symfony/symfony#40457 (comment)

Commits
-------

d47d1fc Fix PhpStanExtractor added version
@alamirault alamirault mentioned this pull request Jul 25, 2023
Closed
jcrombez pushed a commit to jcrombez/ux that referenced this pull request Aug 16, 2023
@weaverryan @jcrombez
[Live] Bumping min versions of 2 packages to fix tests
24a57ef 
* doctrine/persistence to doctrine/persistence#286
* symfony/property-info to get symfony/symfony#40457
@alexandre-le-borgne alexandre-le-borgne mentioned this pull request Apr 11, 2024
Closed
@Korbeil Korbeil mentioned this pull request May 1, 2024
Closed
@mtarld mtarld mentioned this pull request Jul 3, 2024
Merged
fabpot added a commit that referenced this pull request Jan 5, 2025
@fabpot
feature #57632 [PropertyInfo] Add `PropertyDescriptionExtractorInterf…
f86c095 
…ace` to `PhpStanExtractor` (mtarld)

This PR was merged into the 7.3 branch.

Discussion
----------

[PropertyInfo] Add `PropertyDescriptionExtractorInterface` to `PhpStanExtractor`

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

As mentioned in #40457, the `PhpStanExtractor` should at some point implement the `PropertyDescriptionExtractorInterface`.

This PR adds that implementation.

Commits
-------

bd80f29 [PropertyInfo] Add `PropertyDescriptionExtractorInterface` to `PhpStanExtractor`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@OskarStark OskarStark OskarStark left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

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

+1 more reviewer

@ondrejmirtes ondrejmirtes ondrejmirtes left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature PropertyInfo Status: Reviewed

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

[PropertyInfo] Support multiple types (union types) for collection values

9 participants

@Korbeil @OskarStark @dunglas @chalasr @derrabus @nicolas-grekas @fabpot @ondrejmirtes @carsonbot