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.

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

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

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants