Introduce two new convenience methods: registerSharedService, extendService by igorw · Pull Request #62 · silexphp/Pimple

igorw · 2013-01-13T20:53:55Z

Registering a shared service for a certain class that has certain deps happens very often. I'd consider this the primary use case for pimple. As such, it makes sense to provide some shortcuts to make this easier to use.

registerSharedService addresses the common use case of:

$c['foo'] = $c->share(function ($c) {
    return new Foo($c['bar']);
});

The shorter version of that is:

$c->registerSharedService('foo', 'Foo', array('bar'));

The array of service ids is optional and can be omitted if the service constructor has no args.

While not such a common use case, when extending a service, you usually want to re-share and assign it. And the code we have to write for that is clunky and verbose. It would make sense to have a dedicated method that makes things easier.

extendService is a shorthand for:

$c['foo'] = $c->share($c->extend('foo', function ($foo, $c) {
    $foo->addBar(new Bar());
    return $foo;
}));

The new version is:

$c->extendService('foo', function ($foo, $c) {
    $foo->addBar(new Bar());
    return $foo;
});

I'm open to splitting this PR if one of the proposed additions is controversial.

…ervice registerSharedService addresses the common use case of: $c['foo'] = $c->share(function ($c) { return new Foo($c['bar']); }); extendService is a shorthand for: $c['foo'] = $c->share($c->extend('foo', function ($foo, $c) { $foo->addBar(new Bar()); return $foo; });

GromNaN · 2013-01-13T21:20:55Z

Interesting.

extendService 👍 It avoid forgeting the share when extending a shared service. The extendService method may be named extendShared to match naming of other methods. An alternative could be to add a 3rd parameter to the extend method to make it shared.
registerSharedService 👎 I don't really see the gain of registerSharedService. If someone need to configure services with an array of options, he should use Symfony DI.

igorw · 2013-01-13T21:39:53Z

I wasn't so happy about the naming inconsistency myself, so maybe the best solution is indeed to just use the longer version of extendSharedService.

As for registerSharedService, I'm not sure I understand your concerns. There is nothing new added here, it's really just a shorter way of writing the exact same thing. And injecting parameters into services is the essence of DI.

GromNaN · 2013-01-13T21:49:30Z

It's not really shorter with registerSharedService, but it is really less explicit in context of Pimple.
The only use case I see is to load services from a config file (where you can't define Closures).

dominikzogg · 2013-01-13T22:31:41Z

extendService 👍
registerSharedService 👎 seems inconsistent to me, i prefere the closure

simensen · 2013-01-13T22:57:59Z

👍 for both though registerSharedService is maybe not something I would do often? It is a pretty common so I might end up finding it pretty useful.

Thanks for including extendService. :)

davedevelopment · 2013-01-14T00:29:39Z

lib/Pimple.php

Do we really need to pull this out of registerSharedService? Putting it back in there, would save polluting the public API with a method that is fairly useless?

davedevelopment · 2013-01-14T00:35:19Z

I like extendService, but I think I'd call it sharedExtend, almost concatentating the $app->share($app->extend(.

My only gripe with the registerSharedService is that I'd have to type the FQCN.

igorw · 2013-01-14T00:44:07Z

re FQCN: I agree that that sucks. We'll have to wait for PHP core to give us a definitive fix. I still think it's an improvement over what we have now.

ghost · 2013-01-14T03:53:58Z

Just my two cents: I think one of the "coolness" factors of Pimple is that it's a solid DIC in ~200 lines of code. Short and sweet. I'm wondering if these could be implemented as Pimple traits? The registerSharedService could be Pimple\InfectionTrait or Pimple\ContagiousTrait as a couple of odd/funny examples. Cheers!

simensen · 2013-02-12T20:02:30Z

I'm open to splitting this PR if one of the proposed additions is controversial.

@igorw Maybe that would be a good idea. It seems like this has stalled out and some people have mixed feelings on one or the other methods.

I'm personally excited for extendShare or shareExtend. Would love to see if it would be merged in more quickly as its own PR.

igorw · 2013-02-12T20:12:50Z

@fabpot you haven't commented on this PR yet, can you please share your thoughts?

igorw · 2013-03-05T02:43:58Z

Bump.

igorw · 2013-09-26T23:01:43Z

Bump.

dominikzogg · 2013-09-27T06:14:56Z

I would to have this, with this method name (consistent)

$c['foo'] = $c->shareExtend('foo', function ($foo, $c) {
    $foo->addBar(new Bar());
    return $foo;
});

For the other case, i would prefere somethin like this

$c['foo'] = $c->shareArray(array('bar'), array('setAnotherBar' => array('anotherbar')));

This PR was merged into the master branch. Discussion ---------- [2.0] changed services to be shared by default Now that we have several years of experience using Pimple, I think everyone agrees that having to call `share` everytime you need to define a service is cumbersome. And it is even error-prone when calling `extend` as one needs to call `share` again. #62 proposes a shortcut for the extend use case, but I want to go one step further in Pimple 2.0: remove the need to call `share` altogether. The reasoning is quite simple: services are 99.9% shared (I don't even have an example for a prototype service). So, this PR makes closure shared by default. If you need to define a prototype service, call `prototype`. This change is almost backward compatible as the `share` method is kept as a no-op method. The only BC break is if you have a prototype service in which case you need to wrap it with a `prototype` call. Of course, Pimple 2.0 will be used in Silex 2.0, which will make things much easier to read and shorter. TODO: - [x] update docs and website for 2.0 Commits ------- e5309ac changed services to be shared by default

fabpot · 2013-11-11T07:51:24Z

Closing as I'm not convinced registerSharedService is a good thing, and extendService is not needed anymore in 2.0.

igorw added 3 commits January 13, 2013 20:30

Rename {resolveDeps => resolveServiceIds}, add docblocks

2ab50a0

Update README to include docs for convenicence methods

e79d346

davedevelopment reviewed Jan 14, 2013
View reviewed changes

igorw mentioned this pull request Sep 16, 2013

Adds support for class name Service Controller silexphp/Silex#790

Closed

fabpot mentioned this pull request Nov 10, 2013

[2.0] changed services to be shared by default #83

Merged

1 task

fabpot closed this Nov 11, 2013

Conversation

igorw commented Jan 13, 2013

Uh oh!

GromNaN commented Jan 13, 2013

Uh oh!

igorw commented Jan 13, 2013

Uh oh!

GromNaN commented Jan 13, 2013

Uh oh!

dominikzogg commented Jan 13, 2013

Uh oh!

simensen commented Jan 13, 2013

Uh oh!

davedevelopment Jan 14, 2013

Choose a reason for hiding this comment

Uh oh!

davedevelopment commented Jan 14, 2013

Uh oh!

igorw commented Jan 14, 2013

Uh oh!

ghost commented Jan 14, 2013

Uh oh!

simensen commented Feb 12, 2013

Uh oh!

igorw commented Feb 12, 2013

Uh oh!

igorw commented Mar 5, 2013

Uh oh!

igorw commented Sep 26, 2013

Uh oh!

dominikzogg commented Sep 27, 2013

Uh oh!

fabpot commented Nov 11, 2013

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants