-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
[7.x] Provide syntactic sugar for contextually binding tagged services #32514
Conversation
To clarify the issue with
It is entirely possible I've missed some other way to handle these in a nicer way. If there is a better way than |
Yeah that last little tricky one is a bit confusing to me. 😆 I would remove that part but the rest looks good. |
* @param string $tag | ||
* @return void | ||
*/ | ||
public function giveTagged($tag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this also needs to be added to the ContextualBindingBuilder interface (at least on master since we cannot add it on v7)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh, adding to an interface is a major breaking change.
$this->needs = $tag; | ||
} | ||
|
||
$this->give(function ($container) use ($tag) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$container
can be typehinted with the container interface typehint for better IDE autocompletion.
@taylorotwell How best to move forward based on @X-Coder264 and @GrahamCampbell comments regarding the Also: Removed the extra functionality. :) |
It can be added to the interface on master. But, the feature itself could go in here. |
@taylorotwell For docs, should it go on |
@simensen this was merged into 7.x so the same branch on docs. |
Handling tagged services feels heavier than it needs to be.
Currently, we can use primitive binding for primitive (
array
) constructor arguments to avoid having to use a factory for the whole class. That's a plus.However, they almost always look the same:
There are some weird oddities with
Container::tagged
, though. So, even if one figures out they can useiterator_to_array
to convert the response into something that can be consumed of the receiving object's constructor, it might not always work.This is because
Container::tagged
returns an array early if nothing has been tagged with the tag in question:So, to be safe, one would need to be more specific by doing something like this:
It is easy to get it wrong. It is easy to end up in a situation where it works until it doesn't.
It could be easier.
Tagged services are baked into the Container as first-class citizens. By extending the Contextual Binding Builder with a bit of sugar, we can simplify this common task:
Building on the new variadic constructor argument support, this can also be used to satisfy typed variadic constructor arguments as well:
If you are using a variadic constructor argument and are using the type as a tag (something I've started to do more often) you can do this:
Taking it to the next level, if someone wants togiveTagged
and haven't set aneeds
, we can use the tag as the assumed abstract. That looks like this:This PR makes these last
fourthree configuration options possible.When -> Needs -> Gives
reads so nicely.When -> Gives
, not so much. As such, I'm not sold on the inclusion of the last configuration option presented. Which is why it is in an isolated commit in this PR. :) Easy to drop if the rest is acceptable.