Skip to content

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

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
Aug 26, 2020
Merged

[PropertyInfo] ConstructorExtractor which has higher priority than PhpDocExtractor and ReflectionExtractor #30335

merged 1 commit into from
Aug 26, 2020

Conversation

karser
Copy link
Contributor

@karser karser commented Feb 21, 2019

Q A
Branch? master
Bug fix? yes
New feature? yes
BC breaks? hopefully no
Deprecations? no
Tests pass? yes
Fixed tickets #30053
License MIT

Supersedes #30056 #30128

In short, when using PhpDocExtractor, it ignores the constructor argument type, although argument 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 added getTypesFromConstructor to PhpDocExtractor and ReflectionExtractor - they implement ConstructorArgumentTypeExtractorInterface 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

@karser karser mentioned this pull request Feb 21, 2019
Closed
@nicolas-grekas nicolas-grekas added this to the next milestone Feb 21, 2019
@fabpot
Copy link
Member

fabpot commented Mar 4, 2019

Is it still WIP? ping @dunglas

@karser
Copy link
Contributor Author

karser commented Mar 4, 2019

There are still controversial cases, I need more feedback. This discussion.

@fabpot
Copy link
Member

fabpot commented Apr 8, 2019

/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 ?

joelwurtz
joelwurtz suggested changes Apr 8, 2019
View reviewed changes
@karser
Copy link
Contributor Author

karser commented Apr 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

fabpot commented Apr 8, 2019

@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?

@karser karser force-pushed the constructor-extractor branch 2 times, most recently from 51ebfd7 to 95e2d3f Compare April 8, 2019 16:20
@karser karser changed the title [WIP][PropertyInfo] ConstructorExtractor which has higher priority than PhpDocExtractor and ReflectionExtractor [PropertyInfo] ConstructorExtractor which has higher priority than PhpDocExtractor and ReflectionExtractor Apr 8, 2019
@karser
Copy link
Contributor Author

karser commented Apr 8, 2019

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

@karser karser force-pushed the constructor-extractor branch from 95e2d3f to 3c21a3e Compare April 8, 2019 16:39
@fabpot fabpot force-pushed the constructor-extractor branch from 3c21a3e to 5049e25 Compare August 26, 2020 07:35
@karser karser requested a review from dunglas as a code owner August 26, 2020 07:35
@fabpot
Copy link
Member

fabpot commented Aug 26, 2020

Thank you @karser.

@fabpot fabpot mentioned this pull request Aug 26, 2020
Closed
@fabpot fabpot merged commit 4a89215 into symfony:master Aug 26, 2020
@ogizanagi ogizanagi mentioned this pull request Aug 28, 2020
Merged
fabpot added a commit that referenced this pull request Aug 28, 2020
@fabpot
bug #37983 [PropertyInfo] Fix ReflectionExtractor::getTypesFromConstr…
6c094cc 
…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/A

Fixes https://travis-ci.org/github/symfony/symfony/jobs/721953907#L4274-L4296

Relates to #30335

Commits
-------

6423d8a [PropertyInfo] Fix ReflectionExtractor::getTypesFromConstructor
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
@pixelplan-digitalweb pixelplan-digitalweb mentioned this pull request May 14, 2021
Closed
@HypeMC HypeMC mentioned this pull request May 16, 2023
Merged
fabpot added a commit that referenced this pull request Jan 6, 2025
@fabpot
feature #50334 [FrameworkBundle][PropertyInfo] Wire the `ConstructorE…
0aed9fa 
…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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@joelwurtz joelwurtz joelwurtz approved these changes

@maxhelias maxhelias maxhelias approved these changes

@dunglas dunglas Awaiting requested review from dunglas

Assignees
No one assigned
Labels
Bug Feature PropertyInfo Status: Needs Review
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