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

[7.x] Provide syntactic sugar for contextually binding tagged services #32514

Merged
merged 1 commit into from
Apr 23, 2020

Conversation

simensen
Copy link
Contributor

@simensen simensen commented Apr 23, 2020

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:

$this->app->when(WantFactory::class)
    ->needs('$wantConfigurers')
    ->give(function ($container) {
        return iterator_to_array($container->tagged('want_configurer'));
    });

There are some weird oddities with Container::tagged, though. So, even if one figures out they can use iterator_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:

if (! isset($this->tags[$tag])) {
    return [];
}

So, to be safe, one would need to be more specific by doing something like this:

$this->app->when(WantFactory::class)
    ->needs('$wantConfigurers')
    ->give(function ($container) {
        $services = $container->tagged('want_configurer');

        return is_array($services) ? $services : iterator_to_array($services);
    });

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:

$this->app->when(WantFactory::class)
    ->needs('$wantConfigurers') // primitive binding w/ array type
    ->giveTagged('want_configurer');

Building on the new variadic constructor argument support, this can also be used to satisfy typed variadic constructor arguments as well:

$this->app->when(WantFactory::class)
    ->needs(WantConfigurer::class) // variadic binding w/ WantConfigurer type
    ->giveTagged('want_configurer');

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:

$this->app->when(WantFactory::class)
    ->needs(WantConfigurer::class) // variadic binding w/ WantConfigurer type
    ->giveTagged(WantConfigurer::class);

Taking it to the next level, if someone wants to giveTagged and haven't set a needs, we can use the tag as the assumed abstract. That looks like this:

~~$this->app->when(WantFactory::class)->giveTagged(WantConfigurer::class); ~~

This PR makes these last four three 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.

@driesvints driesvints changed the title Provide syntactic sugar for contextually binding tagged services [7.x] Provide syntactic sugar for contextually binding tagged services Apr 23, 2020
@simensen
Copy link
Contributor Author

To clarify the issue with iterator_to_array on an empty array, one gets the following:

iterator_to_array([]);

TypeError: Argument 1 passed to iterator_to_array() must implement interface Traversable, array given

It is entirely possible I've missed some other way to handle these in a nicer way. If there is a better way than is_array($services) ? $services : iterator_to_array($services) I'd like to update this PR. Even if this PR is ultimately rejected, it would be nice for me to know for my own purposes since I have this code in a lot of places currently. :)

@taylorotwell
Copy link
Member

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)
Copy link
Contributor

@X-Coder264 X-Coder264 Apr 23, 2020

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

Copy link
Member

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) {
Copy link
Contributor

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.

@simensen
Copy link
Contributor Author

@taylorotwell How best to move forward based on @X-Coder264 and @GrahamCampbell comments regarding the ContextualBindingBuilder interface?

Also: Removed the extra functionality. :)

@taylorotwell
Copy link
Member

It can be added to the interface on master. But, the feature itself could go in here.

@taylorotwell taylorotwell merged commit 27db810 into laravel:7.x Apr 23, 2020
@simensen simensen deleted the give-tagged branch April 24, 2020 13:38
@simensen
Copy link
Contributor Author

@taylorotwell For docs, should it go on master or 7.x?

@driesvints
Copy link
Member

@simensen this was merged into 7.x so the same branch on docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants