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] ConstructorExtractor which has higher priority than PhpDocExtractor and ReflectionExtractor#30335

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:masterfromkarser:constructor-extractor
Aug 26, 2020

Conversation

karser
Copy link
Contributor

QA
Branch?master
Bug fix?yes
New feature?yes
BC breaks?hopefully no
Deprecations?no
Tests pass?yes
Fixed tickets#30053
LicenseMIT

Supersedes#30056#30128

In short, when using PhpDocExtractor, it ignores the constructor argument type, althoughargument types from the constructor are the only types that are valid for the class instantiation.

This PR adds a separate extractor -ConstructorExtractor which is called first (-999) and it attempts to extract the type from constructor only, either from PhpDoc or using reflection.
I addedgetTypesFromConstructor toPhpDocExtractor andReflectionExtractor - they implementConstructorArgumentTypeExtractorInterface interface.ConstructorExtractor aggregates those extractors using compiler pass.

So the flow of control looks like this:

PropertyInfoExtractor::getTypes:    - ConstructorExtractor::getTypes        - PhpDocExtractor::getTypesFromConstructor        - ReflectionExtractor::getTypesFromConstructor    - PhpDocExtractor::getTypes    - ReflectionExtractor::getTypes

@fabpot
Copy link
Member

Is it still WIP? ping@dunglas

@karser
Copy link
ContributorAuthor

There are still controversial cases, I need more feedback. Thisdiscussion.

@fabpot
Copy link
Member

/cc@joelwurtz

@joelwurtz
Copy link
Contributor

I think it make sense to have something like this, however i'm not sure about the current priority system. To be clear, this is how it should be, but for BC purpose adding a new extractor on top of other would maybe induce a different behavior in existing application.

What would be really nice is to split extraction for __constructor and attributes list, which is something that would be possible with#30818 (by passing a different extractor to the instantiator, and another one for the property list extraction)

For the moment can we add this extractor without adding it to the flow of control (DI) ? So we don't break any behaviour but still allow people to use it ?

WDYT@dunglas ?

@karser
Copy link
ContributorAuthor

karser commentedApr 8, 2019•
edited
Loading

Thank you@joelwurtz for the code review. I applied all the suggestions and removed from FrameworkBundle all the injections so it won't introduce any BC break.

@fabpot
Copy link
Member

@karser I haven't had a look at the code, but can you rebase the PR to get rid of the merge commit (we don't merge PR with merge commits). Also, is it still WIP?

@karserkarserforce-pushed theconstructor-extractor branch 2 times, most recently from51ebfd7 to95e2d3fCompareApril 8, 2019 16:20
@karserkarser changed the title[WIP][PropertyInfo] ConstructorExtractor which has higher priority than PhpDocExtractor and ReflectionExtractor[PropertyInfo] ConstructorExtractor which has higher priority than PhpDocExtractor and ReflectionExtractorApr 8, 2019
@karser
Copy link
ContributorAuthor

@fabpot I removed merges, squashed commits and removed WIP status.

@fabpot
Copy link
Member

Thank you@karser.

karser reacted with hooray emoji

@fabpotfabpot merged commit4a89215 intosymfony:masterAug 26, 2020
fabpot added a commit that referenced this pull requestAug 28, 2020
…uctor (ogizanagi)This PR was merged into the 5.2-dev branch.Discussion----------[PropertyInfo] Fix ReflectionExtractor::getTypesFromConstructor| Q             | A| ------------- | ---| Branch?       | master <!-- see below -->| Bug fix?      | yes| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets       | N/A <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->| License       | MIT| Doc PR        | N/AFixeshttps://travis-ci.org/github/symfony/symfony/jobs/721953907#L4274-L4296Relates to#30335Commits-------6423d8a [PropertyInfo] Fix ReflectionExtractor::getTypesFromConstructor
@nicolas-grekasnicolas-grekas modified the milestones:next,5.2Oct 5, 2020
@fabpotfabpot mentioned this pull requestOct 5, 2020
fabpot added a commit that referenced this pull requestJan 6, 2025
…xtractor` class (HypeMC)This PR was merged into the 7.3 branch.Discussion----------[FrameworkBundle][PropertyInfo] Wire the `ConstructorExtractor` class| Q             | A| ------------- | ---| Branch?       | 7.3| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -A sort of followup to#30335.The original PR removed the wiring to prevent creating a BC break, see#30128 (comment) &#30335 (comment).This PR proposes adding a new `framework.property_info.with_constructor_extractor` configuration to allow optionally enabling the framework integration. The configuration defaults to `false` to prevent creating a BC break. Only when it's set to `true` will the `ConstructorExtractor` be registered in the container.Commits-------2b392b1 [FrameworkBundle][PropertyInfo] Wire the `ConstructorExtractor` class
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@fabpotfabpotfabpot approved these changes

@joelwurtzjoelwurtzjoelwurtz approved these changes

@maxheliasmaxheliasmaxhelias approved these changes

@dunglasdunglasAwaiting requested review from dunglas

Assignees
No one assigned
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.

6 participants
@karser@fabpot@joelwurtz@maxhelias@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp