laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.55k stars 11.03k forks source link

[Proposal] Don't force Container::extend() to be called after Container::share() #1020

Closed franzliedke closed 11 years ago

franzliedke commented 11 years ago

Right now, when extending a service in a service provider via $this->app->share($this->app->extend( ... )), the service needs to be registered already for this to work (as it's instantiated and then passed into another closure).

The way I understand the service providers, no instantiation should take place in ther register() methods - only closures being set up.

Does this mean that extending should take place in the boot() method of our service providers, or should we refactor the extend method to work without instantiating the extended object immediately? An error would still be thrown (but when instantiating, not when registering the extension). Also, the interface of the extend() method could be simplified (no duplicate $app->share() call).

This would also allow us to have packages optionally provide integrations with other packages, whether or not those are installed.

(Inspired by recent problems with the MtHaml package and Twig integration.)

taylorotwell commented 11 years ago

Extend should be used in the boot method, and you can check "bound" before calling extend if you're not sure the package exists.

On Apr 19, 2013, at 6:13 PM, Franz Liedke notifications@github.com wrote:

Right now, when extending a service in a service provider via $this->app->share($this->app->extend( ... )), the service needs to be registered already for this to work (as it's instantiated and then passed into another closure).

The way I understand the service providers, no instantiation should take place in ther register() methods - only closures being set up.

Does this mean that extending should take place in the boot() method of our service providers, or should we refactor the extend method to work without instantiating the extended object immediately? An error would still be thrown (but when instantiating, not when registering the extension). Also, the interface of the extend() method could be simplified (no duplicate $app->share() call).

This would also allow us to have packages optionally provide integrations with other packages, whether or not those are installed.

(Inspired by recent problems with the MtHaml package and Twig integration.)

— Reply to this email directly or view it on GitHub.

franzliedke commented 11 years ago

Then what's the point of using extend() at all? I can just retrieve the instance in the boot() method and do anything I would otherwise do in the closure passed to that method...

taylorotwell commented 11 years ago

Because that would instantiate the object. Extend would defer, meaning if its not used on that request it wouldn't be instantiated.

On Apr 19, 2013, at 7:19 PM, Franz Liedke notifications@github.com wrote:

Then what's the point of using extend() at all? I can just retrieve the instance in the boot() method and do anything I would otherwise do in the closure passed to that method...

— Reply to this email directly or view it on GitHub.

franzliedke commented 11 years ago

Would you think it's worth it to call the share() method implicitly when calling extend()?

taylorotwell commented 11 years ago

Hmm I don't think that would make sense since extend sometimes wouldn't instantiate anything but only modify. I think things are fine as is.

On Apr 20, 2013, at 7:03 AM, Franz Liedke notifications@github.com wrote:

Would you think it's worth it to call the share() method implicitly when calling extend()?

— Reply to this email directly or view it on GitHub.

jasonlewis commented 11 years ago

Can I just chip in here. What exactly is the purpose of extend Taylor? You don't intend it to be used to replace functionality of say the router do you? I'm finding that I need to actually overwrite the bound instance with something like this.

$this->app['router'] = $this->app->share();

It also seems that share doesn't actually register the abstract as shared, which would be hard to do since you're binding it with ArrayAccess.

Any pointers here?

taylorotwell commented 11 years ago

For an example of extend, let's say you just want to use setter injection to inject a different implementation of something into a class. So, for example, say Foo can use an instance of Bar, but via setter injection instead of constructor:

$app->extend('Foo', function($foo)
{
     $bar = new MyBarImplementation;

     $foo->setBar($bar);
});
jasonlewis commented 11 years ago

Ah right, that makes more sense. It's not there to actually extend an entire implementation but instead its dependencies. Makes sense since you get its resolved instance as the first parameter. Cheers for that. On Apr 22, 2013 10:49 PM, "Taylor Otwell" notifications@github.com wrote:

For an example of extend, let's say you just want to use setter injection to inject a different implementation of something into a class. So, for example, say Foo can use an instance of Bar, but via setter injection instead of constructor:

$app->extend('Foo', function($foo) { $bar = new MyBarImplementation;

 $foo->setBar($bar);

});

— Reply to this email directly or view it on GitHubhttps://github.com/laravel/framework/issues/1020#issuecomment-16783696 .