Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[PropertyInfo] AddPhpStanExtractor#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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:5.4fromKorbeil:feature/phpstan-extractor
Oct 30, 2021

Conversation

@Korbeil
Copy link
Contributor

@KorbeilKorbeil commentedMar 12, 2021•
edited
Loading

QA
Branch?5.4
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#38093
LicenseMIT
Doc PRsymfony/symfony-docs#...

This PR will add a PhpStanExtractor that is based onphpstan/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 ifphpstan/phpdoc-parser is present)
  • Update CHANGELOG

Related PR:

dunglas, OskarStark, chalasr, bastnic, damienalexandre, norkunas, Kocal, sstok, welcoMattic, and kayneth reacted with heart emoji
@OskarStark
Copy link
Contributor

I would propose to name it PHPStanExtractor

wouterj reacted with thumbs down emoji

@dunglas
Copy link
Member

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

derrabus and BafS reacted with thumbs up emoji

@OskarStark
Copy link
Contributor

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 ....

sstok reacted with eyes emoji

@chalasr
Copy link
Member

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

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 namedPhpDocExtractor, the PHPUnit bridge resides inSymfony\Bridge\PhpUnit… I think, I'm team@dunglas here. 🙃

Oh well, naming things is hard. 😓

dunglas, Korbeil, OskarStark, sstok, and zmitic reacted with laugh emoji

@chalasr
Copy link
Member

Fair enough :) consistency first.

@OskarStark
Copy link
Contributor

Stay consistent, because of PhpUnit

dunglas reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

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

@nicolas-grekasnicolas-grekas added this to the5.x milestoneMar 16, 2021
@Korbeil
Copy link
ContributorAuthor

How does this compare to usingphpdocumentor/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 atSymfony\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.

nicolas-grekas reacted with thumbs up emoji

@KorbeilKorbeilforce-pushed thefeature/phpstan-extractor branch 2 times, most recently from32e2af3 to2884390CompareMarch 16, 2021 17:27
@KorbeilKorbeilforce-pushed thefeature/phpstan-extractor branch 3 times, most recently from4f57313 to560cd06CompareMarch 22, 2021 09:40
@KorbeilKorbeilforce-pushed thefeature/phpstan-extractor branch 2 times, most recently from4a5236f toaf62a30CompareJune 25, 2021 15:03
@OskarStarkOskarStark changed the title[PropertyInfo] Add PhpStanExtractor[PropertyInfo] AddPhpStanExtractorAug 1, 2021
@KorbeilKorbeilforce-pushed thefeature/phpstan-extractor branch fromcdc9c56 to8bdb91fCompareOctober 29, 2021 12:58
@KorbeilKorbeilforce-pushed thefeature/phpstan-extractor branch from8bdb91f to4651d61CompareOctober 29, 2021 14:54
Copy link
Member

@nicolas-grekasnicolas-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)

Copy link
Member

@fabpotfabpot 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 :)

@fabpotfabpotforce-pushed thefeature/phpstan-extractor branch from68300c7 to9931c37CompareOctober 30, 2021 09:35
@fabpot
Copy link
Member

Thank you@Korbeil.

@fabpotfabpot merged commit5745b43 intosymfony:5.4Oct 30, 2021
weaverryan added a commit to weaverryan/ux that referenced this pull requestMar 22, 2023
symfony-splitter pushed a commit to symfony/ux-live-component that referenced this pull requestMar 22, 2023
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull requestJun 12, 2023
… (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
jcrombez pushed a commit to jcrombez/ux that referenced this pull requestAug 16, 2023
fabpot added a commit that referenced this pull requestJan 5, 2025
…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       | MITAs 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@OskarStarkOskarStarkOskarStark left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@dunglasdunglasAwaiting requested review from dunglasdunglas is a code owner

+1 more reviewer

@ondrejmirtesondrejmirtesondrejmirtes left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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
[8]ページ先頭
©2009-2025 Movatter.jp