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

[5.4] Bringing back app()->make() parameters #391

Closed
sebastiaanluca opened this Issue Jan 27, 2017 · 33 comments

Comments

Projects
None yet
@sebastiaanluca

sebastiaanluca commented Jan 27, 2017

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 while allowing us to pass our own parameters.

See laravel/framework#17556 for further information and the initial discussion.

Short version: app()->make() parameters have been removed. Some alternatives have been proposed, but these are not suitable for every situation. I'm still wondering why it has been removed in the first place and not leave it to the developer to decide when to use it (and when not).

@greabock

This comment has been minimized.

greabock commented Jan 28, 2017

Well, I can understand why the method make() should not have additional parameters.
Looking ahead to psr-11... method make can be easy renamed to get. It's ok. But!
Why cut method build()? I very often and in many places use it as abstract factory. And I was very upset by the fact that now I have to reinvent the wheel.

@aedart

This comment has been minimized.

aedart commented Jan 28, 2017

I too am sad about this change. I have 33+ packages at work, many of which I do make use of a single parameter to pass in some kind of configuration or data (for data transfer objects mostly).

It would be nice if Laravel could offer an automatic "populate" or "configure" alternative, e.g.:

A new interface that can handle object configuration (thus, nothing to do with illuminate/config)

<?php

interface Configurable
{
    /**
     * Configures this component
     * 
     * @param array $configuration [optional] 
     *  
     * @return void 
     */
    public function configure(array $configuration = []);
}

When you bind an instance, you shouldn't care about anything other then returning your desired concrete instance.

$ioc->bind(CategoryInterface::class, function($ioc){
    // instance of Configurable
    return new Category();
});

But when you attempt to obtain that instance, the Container could perhaps allow for "configuration" data, by just passing it on to the concrete instance, if it is an instance of Configurable (or whatever interface would be accepted).

// Some kind of configuration - can originate
// from anywhere...
$config = [
    'showInMenu'  => false,
    'allowSearch' => true,
    'cache'       => true
];

// IoC creates bound instance and passes on the $config array,
// but only if concrete instance is configurable!
$category = $ioc->make(CategoryInterface::class, $config);

Please do not get hooked on the whole "category" object, as it is just an example.

Allowing for this kind of behaviour can save us a few factories, when we are dealing with small and simple components. Furthermore, the Container would only require a few additional lines of code, all of which have nothing to do with ReflectionClass and or parameters checking. In other words, it should not decrease the container's make / build performance in any significant manner.

@czim

This comment has been minimized.

czim commented Jan 28, 2017

I agree. This is a very useful feature of the service container that facilitates clean dependency inversion in many of my projects at work.

It really helps with unit testing. I'm currently refactoring some code to make it compatible with Laravel 5.4, and this is how it's turning out:

Before

I have a simple rest service class that uses guzzle:

RestService

class RestService extends AbstractService
{
    public function __construct(array $guzzleConfig = [])
    {
        $this->client = app(Client::class, [ $guzzleConfig ]);

        // Parent constructor call, other setup here...
    }
}

And a RestServiceTest for unit testing it:

class RestServiceTest extends TestCase
{
    /** @test */
    function it_returns_mocked_data_as_service_response()
    {
        $guzzleMock = $this->getMockBuilder(Client::class)->getMock();

        // Mock preparation code here...

        app()->instance(Client::class, $guzzleMock);

        // Tests here..
    }
}

(The only thing ugly about this, to me, is that the Client::class reference in the app() call should have been to the ClientInterface, but since the constructor is not in that interface, maybe not.)

After

To make this compatible with Laravel 5.4, I could not simply replace the App::make() with a tight coupling new Client() instantiation, even if I wanted to, since that would prevent me from testing/mocking it.

Instead, I had to take a roundabout way to achieve the same thing, with a factory:

RestService

class RestService extends AbstractService
{
    public function __construct(array $guzzleConfig = [])
    {
        $this->client = $this->createGuzzleClient($guzzleConfig);

        // Parent constructor call, other setup here...
    }

    /**
     * @param array $config
     * @return \GuzzleHttp\ClientInterface
     */
    protected function createGuzzleClient(array $config)
    {
        /** @var GuzzleFactoryInterface $factory */
        $factory = app(GuzzleFactoryInterface::class);

        return $factory->make($config);
    }
}

Requiring an additional GuzzleFactory (to keep things neat, with an additional GuzzleFactoryInterface):

class GuzzleFactory implements GuzzleFactoryInterface
{
    /**
     * Makes a Guzzle client instance.
     *
     * @param array  $config    guzzle constructor configuration
     * @return \GuzzleHttp\Client
     */
    public function make(array $config = [])
    {
        return new \GuzzleHttp\Client($config);
    }
}

In the package's ServiceServiceProvider, this required another binding for the interface:

    public function register()
    {
        // Other bindings...

        $this->app->bind(GuzzleFactoryInterface::class, GuzzleFactory::class);
    }

Finally, I had to adjust the test mock setup:

RestServiceTest

class RestServiceTest extends TestCase
{
    /** @test */
    function it_returns_mocked_data_as_service_response()
    {
        $guzzleMock = $this->getMockBuilder(Client::class)->getMock();

        // Mock preparation code here...

        $factoryMock = $this->getMockBuilder(GuzzleFactoryInterface::class)->getMock();
        $factoryMock->method('make')->willReturn($guzzleMock);
        
        app()->instance(GuzzleFactoryInterface::class, $factoryMock);

        // Tests here..
    }
}

This seems clunky, overwrought and arbitrary. I'm just adding code to be able to keep an approach to DI and testing that I don't see as 'too smelly' to use.

Of course, if I'm missing some solution to this that is much neater, but equally powerful to the above, I'm all ears. I'm also curious as to the whole story behind the 'code smell' comment; what was so terrible about App::make()'s second parameter when used with care?

@greabock

This comment has been minimized.

greabock commented Jan 29, 2017

a little sugar guys
https://github.com/greabock/maker

@pilot911

This comment has been minimized.

pilot911 commented Jan 31, 2017

Hmm... strange idea to remove construct's parameters. Or we cant understand something...

@garygreen

This comment has been minimized.

garygreen commented Jan 31, 2017

Honestly when I saw this in the changelog I really didn't get why it was removed. It's simple functionality and removing it breaks a lot of peoples workflow.

@pilot911

This comment has been minimized.

pilot911 commented Jan 31, 2017

okey, but what instead of ->make() ? How make coding ?

@unstoppablecarl

This comment has been minimized.

unstoppablecarl commented Feb 3, 2017

There was a lot of relevant discussion here laravel/framework#17556

@unstoppablecarl

This comment has been minimized.

unstoppablecarl commented Feb 3, 2017

I have a use case I cannot find a way to accomplish in 5.4.

class FooServiceProvider extends ServiceProvider {

    public function register() {
        // defined/overridden by user elsewhere
        $this->app->bind(FooContract::class, Foo::class);

        $this->app->bind('foo', function ($app) {

            $dep    = $this->app->make(Dep::class);
            $active = false;
            $Class  = ''; // get user defined implementation Foo::class from contract FooContract::class;

            return new $Class($dep, $active);
        });

    }

    public function boot() {
        dd($this->app->make('foo'));
    }
}

class Dep {
}

interface FooContract {
    public function __construct(Dep $dep, $active);
}

class Foo implements FooContract {
    public function __construct(Dep $dep, $active) {

    }
}

The FooContract constructor function is not resolvable by the service container because of the required $active argument. OK thats reasonable, so I make a binding for it and inject the args myself, But there is no way to figure out the Foo class from the FooContract (that I can find). If I am correct and there is no way to do it this seems like a serious limitation.

@sebastiaanluca

This comment has been minimized.

sebastiaanluca commented Feb 4, 2017

It seems that a lot of people want this back, either because they find the alternatives lacking or there aren't any for their use-case. Depending on a third-party package for such a small, core feature isn't exactly ideal either, I would say, but better than nothing (thanks btw, @greabock!).

I'm sure many are willing to create a PR to revert this back to its previous functionality, but can we get some input from you, @taylorotwell?

In the future, I think it'd be wise to mark such change as deprecated as described in this proposal, so it can be discussed beforehand instead of just deleting it and leaving many stranded.

@greabock

This comment has been minimized.

greabock commented Feb 5, 2017

@sebastiaanluca I think that the best solution would be to return to the build method the old functionality. In reality, there is no need to do this for make method because you never want to get singleton or instance when you passing parameters.

@unstoppablecarl

This comment has been minimized.

unstoppablecarl commented Feb 6, 2017

@sebastiaanluca I think that the best solution would be to return to the build method the old functionality. In reality, there is no need to do this for make method because you never want to get singleton or instance when you passing parameters.

Surprisingly build is not in the Illuminate\Contracts\Container\Container contract.

@Blizzke

This comment has been minimized.

Blizzke commented Feb 8, 2017

I use this functionality as well to keep fetching configuration values out of my classes as much as possible. My ServiceProviders create instances with the required values. Now I have to start adding setters for each of those values and use some kind of fluent syntax to set everything afterwards.
If adding an array of parameters is a code smell, imo calling 5 or more setters afterwards smells a lot worse. Please bring the functionality back!

@RyanThompson

This comment has been minimized.

RyanThompson commented Feb 15, 2017

I agree - I feel like this was poorly thought out. It's one thing to advise against poor practices but another to abolish a useful method because it can be abused. We use this extensively in lower level classes as a way of elegantly providing extendability to developers.. would prefer not to bind our own container for this :-/

@pilot911

This comment has been minimized.

pilot911 commented Feb 16, 2017

So - back returned ?

@nicksnellockts

This comment has been minimized.

nicksnellockts commented Feb 17, 2017

I have just come across this problem and as a result, we simply cannot consider moving to 5.4. It may be considered to be 'bad practice' from some, but looking at it from other angles, construction of an instance that is not populated with valid data and the consequent requirement for setters that should not be present is, in my opinion, worse.

@mickaelpch

This comment has been minimized.

mickaelpch commented Feb 17, 2017

i'm also waiting to see where this issue is going as it is an important problem for us unfortunately :/

@unlike777

This comment has been minimized.

unlike777 commented Feb 24, 2017

I agree that in an ideal world, you can do without this possibility and even necessary! But in reality it is not.

Example: I need to set up a class Monolog\Formatter\LineFormatter, but there's no setter to change $format, it can only pass a parameter to the constructor.

@RyanThompson

This comment has been minimized.

RyanThompson commented Feb 24, 2017

Has @taylorotwell chimed in on this anywhere? I would really love to know his stance on it if he's already made it apparent anywhere after the release.

@sebastiaanluca

This comment has been minimized.

sebastiaanluca commented Feb 24, 2017

@RyanThompson I pinged him a few times here and in the other issue, but no response yet. Of course he's super busy, so I'm patient, but would love to get his feedback on this too so we know where to go from here.

@etiennemarais

This comment has been minimized.

etiennemarais commented Feb 28, 2017

I feel this might be more of a configuration problem and where to do that in the correct place than the DI container not receiving params.

I am unsure now how to compose configuration options in a correct way so that when the app(Foobar::class) gets run, the Foobar class will already have it's config set on the object itself. Also this makes it incredibly hard to test now. Some insights will be greatly appreciated 🙌

@aedart

This comment has been minimized.

aedart commented Feb 28, 2017

@etiennemarais You are very right - one can certainly argue that this is more a configuration issue rather than dependency injection. And from what I can tell, from these arguments, many have been used to use the container as a shortcut for a factory (myself included).

I still believe that having such possibility would be great, simply because it can reduce some factories in the long run. Of course it can always be abused, but so can a normal factory class. Yet, I guess that most are just sad about not being offered an alternative, out of the box. Furthermore, that this feature was completely removed, rather than being deprecated - you know, being given able time to deal with this.

Regardless, I still hope that my previous post could become a part of Laravel, yet I have not had the time to create a new issue, pull requests, etc. If more people are interested in that solution, I will make time for it...

@etiennemarais

This comment has been minimized.

etiennemarais commented Mar 1, 2017

Thanks @aedart for the kind words. If you look at the referenced ticket our use case already uses a factory to transform the configs of the various pubsub clients. To keep the nice provides aliases vs extracting the configs out to like a config provider is probably a tradeoff we have to make at some point.

Otherwise we end up using a FactoryFactory and that is the real code smell imho. I have to echo your thoughts about this feature not being deprecated at least for a version. I think the right thing here would've been to possibly deprecate it for 5.4 and remove it for 5.5 which is the new LTS version.

@czim czim referenced this issue Mar 5, 2017

Closed

Laravel 5.4 #1

@zeroonnet

This comment has been minimized.

zeroonnet commented Mar 7, 2017

This is the only thing that makes all my code break and cannot upgrade to 5.4 .
Still don't see a reason to remove parameters from make().

Typically, you can always construct the object in another way that is more intuitive.

What is more intuitive then $record = app($this->recordClass, $attr) ?

@lasergoat

This comment has been minimized.

lasergoat commented Mar 8, 2017

Can confirm, I've been using 5.1 and 5.2 for a while and did a hackathon even last weekend, I used 5.4 obvs and couldn't figure out why my make calls weren't working. So I gave up and hurriedly used the new operator instead. Now I know!

@sebastiaanluca

This comment has been minimized.

sebastiaanluca commented Mar 8, 2017

Guess we're all waiting on a reply from @taylorotwell, but as @syhol pointed out, I too think it's a "case closed" situation (as with almost every breaking change in Laravel) and we'd better look at some alternatives like that make package.

¯\_(ツ)_/¯

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Mar 8, 2017

If someone can provide me a very clear use case they don't think is solvable without this functionality I will gladly offer how I would solve it. It must use real class names and be a real example... no "Foo" stuff.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Mar 8, 2017

Here's just one example from higher up in the thread:

class RestService extends AbstractService
{
    public function __construct(array $guzzleConfig = [])
    {
        $this->client = app(Client::class, [ $guzzleConfig ]);

        // Parent constructor call, other setup here...
    }
}

How I would setup bindings to solve this now...

$app->bind(Client::class, function () {
    return function ($config) {
        return new Client($config);
    }
});

$client = app(Client::class)($config);

I still find this whole situation sort of a strange design in the first place. You already know the concrete type you want... you don't need the container at all.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Mar 8, 2017

You know what, if someone wants to make a clean PR to add this in - go for it. I don't have time to bicker back and forth about it. I have said my part that I find it a code smell and abuse of the container but if the community is absolutely hell-bent on introducing such things into their code than so be it and I'll evaluate the PR when it comes in. Thanks.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Mar 8, 2017

I'll even have a go at re-implementing this really simply since I feel challenged about it now 🤣

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Mar 8, 2017

OK I have it re-implemented cleanly using a makeWith method I will try to get in tomorrow.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Mar 8, 2017

@laravel laravel locked and limited conversation to collaborators Mar 8, 2017

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Mar 14, 2017

This was done.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.