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] Add container support for variadic constructor arguments #32454

Merged
merged 1 commit into from
Apr 20, 2020

Conversation

simensen
Copy link
Contributor

This PR adds container support for variadic constructor arguments.

See these Twitter discussions for the catalysts for this PR:

The Problem

There is currently no way to support classes that have variadic constructor arguments without breaking out and providing a full factory for a class.

For example:

interface Logger { }

class MyLogger implements Logger { }

interface Filter { }

class NullFilter implements Filter { }
class ProfanityFilter implements Filter { }
class TooLongFilter implements Filter { }

class Firewall
{
    public $logger;
    public $filters;

    public function __construct(Logger $logger, Filter ...$filters)
    {
        $this->logger = $logger;
        $this->filters = $filters;
    }
}

The only way to do this currently:

app()->singleton(Logger::class, MyLogger::class);
app()->bind(Firewall::class, function ($c) {
    return new Firewall(
        $c->make(Logger::class),
        ...[
            $c->make(NullFilter::class),
            $c->make(ProfanityFilter::class),
            $c->make(TooLongFilter::class),
        ]
    );
});

It feels heavy handed since we have to manually make all four dependencies for Firewall when all of them should be able to be autowired. Likewise, add another constructor argument, and the container configuration needs to change, even if the new constructor argument could be autowired.


The Solution

My ideal situation is to configure classes like this using contextual binding. I only want to specify the dependencies that cannot be autowired.

To me, that looks like this:

app()->singleton(Logger::class, MyLogger::class);
app()
    ->when(Firewall::class)
    ->needs(Filter::class)
    ->give(function ($c) {
        return [
            $c->make(NullFilter::class),
            $c->make(ProfanityFilter::class),
            $c->make(TooLongFilter::class),
        ];
    });

Or... better yet...

app()->singleton(Logger::class, MyLogger::class);
app()
    ->when(Firewall::class)
    ->needs(Filter::class)
    ->give([
        NullFilter::class,
        ProfanityFilter::class,
        TooLongFilter::class,
    ]);

This PR makes both of these last two configuration styles possible.


Notable Changes

  • Container::getContextualConcrete can now return an array (only a doc change, but changed expectation)
  • Container::resolveClass can now return an array (docs already have it returning mixed, but changed expectation)
  • Container::resolveClass will now return an empty array ([]) if a value is not defined (better fits with how PHP handles variadic functions when no variadic values are actually passed)
  • ContextualBindingBuilder::give can now accept an array (only a doc change, but changed expectation)

Other Thoughts on Implementation

  • Unclear if reusing needs/give is best vs adding needsVariadic/givesVariadic or something along those lines. This looks cleanest and probably easiest to adopt, but happy to discuss this.
  • Unclear if there are performance impact on some of the places I changed the code. I'm most worried about Container::resolveClass. I started down resolveVariadic but quickly scaled back as it was duplicating a lot of code and wasn't sure it was worth it.
  • Unclear what other impacts there might be by adding the option to return an array in some of these cases. The tests all pass, but it is changing some expected behavior...

Related to #26950.

@GrahamCampbell GrahamCampbell changed the title Add container support for variadic constructor arguments [7.x] Add container support for variadic constructor arguments Apr 20, 2020
@driesvints
Copy link
Member

I guess this won't work for situations like this?

public function __construct(Logger $logger, Filter $mainFilter, Filter ...$filters)

@simensen
Copy link
Contributor Author

Thanks for jumping in, @driesvints!

I guess this won't work for situations like this?

public function __construct(Logger $logger, Filter $mainFilter, Filter ...$filters)

Gah, you are right. :(

That said, I'm not sure how big of an issue it is? As far as I understand it, you cannot contextually bind a class that has more than one instance of the same type. For example, I don't think you can do the following without dropping to a full factory:

public function __construct(Logger $logger, Filter $mainFilter, Filter $backupFilter)

The solution I had hoped to find already in place was being able to contextually bind based on parameter name instead of type. This seems to currently only work for binding primitives.

For example, I'd hoped I'd be able to do this but it does not:

interface Logger { }

class MyLogger implements Logger { }

interface Filter { }

class NullFilter implements Filter { }
class ProfanityFilter implements Filter { }
class TooLongFilter implements Filter { }

class Firewall
{
    public $logger;
    public $filters;

    public function __construct(Logger $logger, Filter $mainFilter, Filter $backupFilter)
    {
        $this->logger = $logger;
        $this->filters = [$mainFilter, $backupFilter];
    }
}

app()->singleton(Logger::class, MyLogger::class);
app()->when(Firewall::class)->needs('$mainFilter')->give(NullFilter::class);
app()->when(Firewall::class)->needs('$backupFilter')->give(ProfanityFilter::class);

$firewall = app(Firewall::class);

My vote:

This is a separate issue/limitation with the current Container contextual binding functionality.

This PR would enable support for a common variadic case which would be a huge win on its own.

If we want to support $namedConstructorArguments to allow for contextually binding multiple instances of the same type, it should work for the variadic solution in this PR out of the box.

@driesvints
Copy link
Member

@simensen let's see what Taylor says :)

@taylorotwell taylorotwell merged commit 45519f2 into laravel:7.x Apr 20, 2020
@taylorotwell
Copy link
Member

I agree the multiple instances of same type is kind of a separate issue. Do you mind sending in a laravel/docs PR for this?

@simensen
Copy link
Contributor Author

I'll see if I can get on the docs, sure!

@simensen simensen deleted the container-variadic-support branch April 20, 2020 15:22
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.

3 participants