[5.4] Removal of app()->make() parameters provides no alternatives

[sebastiaanluca]

So Laravel 5.4 removed the ability to use app()->make(MyService::class, ['parameter1', 'parameter2, 'parameter3']), yet provides no alternatives to resolve a bound class or interface from the IoC container when we have to pass our own parameters.

A simple example that doesn't work anymore:

// Bind an interface to an implementation in a registered service provider
$this->app->bind(MyServiceInterface::class, function ($app, $parameters) {
    return new MyServiceA(...$parameters);
});

// Instantiate whatever class is bound to the interface with the given constructor arguments
$myService = app(MyServiceInterface::class, ['parameter1', 'parameter2', 'parameter3']);

This type of instantiation was useful for a various number of reasons:

• providing default constructor parameters while having the ability to override them (even partially)
• binding interfaces to classes and passing parameters (say DatabaseRepositoryInterface with a default returning an EloquentRepository
• simply exchanging the returned class by e.g. a cached class without changing any code throughout your application
• ability to test better
• et cetera

All in all I find it a nice alternative to using new in some cases. The upgrade docs specify that this is a 'code smell' and thus it has been removed completely. Why exactly and what's the alternative for the above situation? Also, and I know it's a never ending discussion about what to enforce and what to leave to the developer, but why completely remove it given the listed pros?

—

I opened a thread on Laracasts to discuss a possible alternative, but wanted to ping here too about the why and how.

[mariomka]

Make factories can be a good solution.

[taylorotwell]

Give the real example for your situation and describe it. Don't give an example with "MyServiceA", etc

[sebastiaanluca]

Thanks for the replies!

@mariomka They can, but not in every situation. Sometimes they require a lot more redundant, boilerplate classes and code which can done in a better way using the IoC container.

@taylorotwell The given example is actually something I use quite often. Could be anything really, a repository for instance:

$this->app->bind(UserRepositoryInterface::class, function ($app, $parameters) {
    // For the sake of an example, we'll pass a model to the repository here.
    // This could be any dependency, without limiting them in quantity.
    $model = User::class;

    // Base configuration ($model) + external parameters that are unique per instantiation.
    // Could be an Eloquent, MongoDB, Doctrine, or any other type of repository.
    return new UserEloquentRepository($model, ...$parameters);
});

// Get a new instance of whatever class is bound to the interface. Pass additional
// parameters that are either optional or required by the class implementing the
// interface. Again, anything goes.
$users = app(UserRepositoryInterface::class, ['id', 'array']);

Here's another one:

<?php

// Default binding to an interface
$this->app->bind(FacebookApiService::class, function ($app, $parameters) {
    $client = new \GuzzleHttp\Client();

    // We can switch this out for any another type of class that
    // implements FacebookApiService at any time, without changing
    // another line of code in our application.
    return new CurlFacebookApiService($client, ...$parameters);
});

// Using DI binding allows us to easily switch out the default provider
// for a cached service for instance.
$this->app->bind(FacebookApiService::class, function ($app, $parameters) {
    $client = new \GuzzleHttp\Client();
    $cache = $app->make(\Illuminate\Cache\Repository::class);

    return new CachedFacebookApiService($client, $cache, ...$parameters);
});

// Whichever implementation of the Facebook API service we use,
// all require a client ID and secret. In this particular case,
// credentials cannot be read from the config inside the class
// itself as per usual, but are unique to the environment
// wherein it's instantiated and/or the current user.
$facebook = app(FacebookApiService::class, [$clientId, $clientSecret]);

I hope this makes it clear what kind of use cases are affected after removing the ability to pass parameters to make. The discussion itself has been made a few times over the years before here on Github and the result was the (re)introduction of being able to pass parameters to app()->make(). Now it got removed again, so we're back to square one ;p

I personally find it a core feature and think it should be kept in line with the new keyword allowing you to instantiate a class or interface ánd pass parameters. Not every situation allows you to pre-bind a class/interface with a default, never-changing set of parameters. I'd love to hear the reasoning behind the removal of it, maybe there's something I'm missing. And as I don't have any alternatives for now, I'm kind of depending on this feature. I'm willing to create a PR reverting the change if you give your go-ahead.

[KennedyTedesco]

You have a point @sebastiaanluca. This looks useful when you need, for example, inject just a specific dependency and let the container take care of the others.

class MyClass
{
   public function __construct(Foo $foo, Bar1 $bar1, Bar2 $bar2, Bar3 $bar3)
   {
      //
   }
}

$foo = new Foo('foo');

$myClassObject = app(MyClass::class, [$foo]); // Success

Without this "feature" you would need a setter method, for example, to inject this specific dependency. 🤔

[sebastiaanluca]

The sky is the limit (in reasonable terms of course) :) And yes, you'd need to create numerous setters, one for each variable you're depending on to mimic the same workflow or use factories, yet both have limitations and are not ideal / seem hacky.

I'm very pro passing dependencies through the constructor so you know what your class needs and does. Also, not every class allows customization (to add setters) or even using setters at all (if you're doing some actual construction in the constructor, heh). I also like to automize everything, so the less manual work I have to do to create a class, the better. That's also the gist of DI, I think.

Even if the majority of Laravel users wouldn't need this feature due to the specific use cases, it'd be great if it'd get reintroduced. Not all Laravel methods are used by all its users all the time anyway, you just need to be able to.

[KennedyTedesco]

Yeah, I can see some use cases for this.

[taylorotwell]

@sebastiaanluca I would create a UserRepositoryFactory that has a ->make($driver, $parameters) method and let that create the driver how you want.

[GrahamCampbell]

I had this issue in laravel auto-presenter, and resolved it with a setter.

laravel-auto-presenter/laravel-auto-presenter@ed61b3d

[sebastiaanluca]

I agree those are all viable alternatives, yet think one is more suited for (and applicable in) a specific situation than the other. But why remove this feature though? Why make things difficult and have us look for alternatives when this was perfectly fine and did the job? I want to understand the reasoning behind it besides the notion that it's a "code smell".

[unstoppablecarl]

I was not a fan of the few cases I saw where this was abused and make it difficult to reason about some object creation code. I personally only ever used it for optional config settings that were primitives and not strictly required (although it can be unclear weather an arg is a dependency or config). I have mixed feelings about this change.

Comparing different ways to handle this for the typical cases I used it.

// 5.3
class Foo {
    protected $cache = false;
    protected $dep;

    public function __construct(Dep $dep, $cache = false) {
        $this->dep    = $dep;
        $this->cache = $cache;
    }
}

app(Foo::class, ['cache' => true]);


// 5.4 solution 1
$this->app->bind(Foo::class, function ($app) {
    $depClass = app(Dep::class);
    $cache   = config('package.cache');
    return new Foo($depClass, $cache);
});

app(Foo::class);



// 5.4 solution 2
class Foo {
    protected $cache = false;
    protected $dep;

    public function __construct(Dep $dep, \Illuminate\Config\Repository $config) {
        $this->dep    = $dep;
        $this->cache = $config->get('package.cache');
    }
}

app(Foo::class);


// 5.4 solution 3 (ideal for lots of injected dependencies)
class Foo {
    protected $cache = false;
    protected $dep;
    protected $dep2;

    public function __construct(Dep $dep, Dep2 $dep2) {
        $this->dep = $dep;
        $this->dep2 = $dep2;
    }

    public function cache($value = null) {
        if ($value !== null) {
            $this->cache = $value;
        }
        return $this->cache;
    }
}

$this->app->bind(Foo::class, function ($app) {
    $obj    = app(Foo::class);
    $cache = config('package.cache');
    $obj->cache($cache);
});

It seems like solution 2 has the least resistance but I find it to be the most sloppy.

What would a proper factory look like for this example?

[lukepolo]

I have done something like this before,

    $this->app->bind(
        LaraCart::HASH,
        function ($app, $data) {
            return md5(json_encode($data));
        }
    );

    // In another area of the app
    app(LaraCart::HASH, $cartItemArray);

I made my life simple, as I did not have to create a class to pass a parameter

[GrahamCampbell]

Closing since this is a not a bug report. Feel free to discuss on the forums, and/or the internals repo.