8000 [DX] [PropertyInfo] Filter PropertyInfo Extractors with Interfaces by zanbaldwin · Pull Request #16890 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DX] [PropertyInfo] Filter PropertyInfo Extractors with Interfaces #16890

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
Closed
wants to merge 1 commit into from
Closed

[DX] [PropertyInfo] Filter PropertyInfo Extractors with Interfaces #16890

wants to merge 1 commit into from

Conversation

zanbaldwin
Copy link
Member
Q A
Bug fix? No
New feature? No
BC breaks? Yes
Deprecations? No
Tests pass? Yes
Fixed tickets N/A
License MIT
Doc PR symfony/symfony-docs#5974

To improve developer experience, require just one collection of extractors that filters by the interfaces that have been implemented on each extractor.

  • Simplifies registering new extractors in the service container.
  • Reduces duplication of semantics (intent defined by interfaces implemented, instead of interfaces implemented and service container tag).
  • Prevents Call to undefined method fatal errors for extractors defined with the wrong service container tag.

However, this does break backwards-compatibility for:

I'm suggesting this change purely in the unlikely event that the PropertyInfo component can be classed as internal, since documentation is still pending.

/cc @dunglas

[DX] Filter PropertyInfo Extractors with Interfaces
159fd83 
Use a single service tag to filter a central source of information extractor instances.
Since PropertyInfo requires PHP 5.5+, use ::class constant to negate the use of class constants or strings.
@dunglas
Copy link
Member
dunglas commented Dec 7, 2015

It is not possible because it doesn't handle every use cases, see #15858 (comment) and my reply for details.

@zanbaldwin
Copy link
Member Author

You're absolutely right, I didn't take into account different ordering for different types.
I learnt more about the FrameworkBundle whilst writing this PR, so at least it wasn't a waste 😄

@zanbaldwin zanbaldwin closed this Dec 7, 2015
@dunglas
Copy link
Member
dunglas commented Dec 7, 2015

But as you're the second person asking why after @fabpot, it can be a good idea to mention it in the doc :-)

@zanbaldwin
Copy link
Member Author

I'll update the documentation PR to include the explanation in your comment, since it's an important but not immediately obvious decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zanbaldwin @dunglas @carsonbot
0