Skip to content
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

Adds support for autowire of __construct for wildcards #616

Merged
merged 7 commits into from Aug 25, 2018

Conversation

jasonrm
Copy link
Contributor

@jasonrm jasonrm commented Aug 25, 2018

In 5.x it was possible to have auto wiring of wildcard definitions via reflection, this PR enables that functionality again.

The "fix" might be a bit hacky however, as I wasn't able to find a cleaner way for ReflectionBasedAutowiring to be aware of what class to use reflection on, unless I sent the wildcard class through the normalizer a second time after the derived implementation class name was set.

@mnapoli
Copy link
Member

mnapoli commented Aug 25, 2018

Thanks for the PR, you have actually spotted a bug! Wildcard definitions with autowiring are supposed to work in 6.x, and they work in simple scenarios. Your PR fixes the bug which is great.

I think however there is a more elegant solution because your solution involves "normalizing" the definition twice. I've opened a PR on your PR in your repository ;) -> ArtFireCom#1 If you merge it into your branch then this pull request here will be updated and good to merge I think.

@jasonrm
Copy link
Contributor Author

jasonrm commented Aug 25, 2018

Excellent! This was exactly the best case scenario I'd hoped for where,

  1. it wasn't me using the library incorrectly
  2. with your deep understanding of the code you could provide an even better solution that didn't resort to a hacky fix

@mnapoli
Copy link
Member

mnapoli commented Aug 25, 2018

Great. And woops it seems I missed an unused import (see https://github.com/PHP-DI/PHP-DI/pull/616/checks?check_run_id=13995093) would you have time to remove it? That way the build will be green and I'll be able to merge and release.

@mnapoli
Copy link
Member

mnapoli commented Aug 25, 2018

Perfect thanks!

@mnapoli mnapoli merged commit 61ef5c9 into PHP-DI:master Aug 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants