Permalink
Browse files

Fix configuration of the service provider.

The recent addition of `boot()` in Silex actually changed how services
are exposed and accessible during the registration phase of service
providers.
  • Loading branch information...
nrk committed Jul 8, 2012
1 parent 9583716 commit 040fa846622d7442eead6662ee533a508cd88bd5
Showing with 37 additions and 35 deletions.
  1. +36 −35 lib/Predis/Silex/PredisServiceProvider.php
  2. +1 −0 tests/Predis/Silex/PredisServiceProvider.php
@@ -46,13 +46,6 @@ public function __construct($prefix = 'predis')
* {@inheritdoc}
*/
public function boot(Application $app)
- {
- }
-
- /**
- * {@inheritdoc}
- */
- public function register(Application $app)
{
$prefix = $this->prefix;
@@ -68,34 +61,6 @@ public function register(Application $app)
$app["$prefix.default_options"] = array();
}
- $app["$prefix.client_initializer"] = $app->protect(function($arguments) use($app, $prefix) {
- $extract = function($bag, $key) use ($app, $prefix) {
- $default = "default_$key";
- if ($bag instanceof Application) {
- $key = "$prefix.$key";
- }
- if (!isset($bag[$key])) {
- return $app["$prefix.$default"];
- }
- if (is_array($bag[$key])) {
- return array_merge($app["$prefix.$default"], $bag[$key]);
- }
-
- return $bag[$key];
- };
-
- if (is_string($arguments)) {
- $parameters = $arguments;
- $options = $app["$prefix.default_options"];
- }
- else {
- $parameters = $extract($arguments, 'parameters');
- $options = $extract($arguments, 'options');
- }
-
- return new Client($parameters, $options);
- });
-
if (isset($app["$prefix.clients"])) {
foreach ($app["$prefix.clients"] as $alias => $args) {
if (in_array($alias, self::$reserved, true)) {
@@ -129,4 +94,40 @@ public function register(Application $app)
});
}
}
+
+ /**
+ * {@inheritdoc}
+ */
+ public function register(Application $app)
+ {
+ $prefix = $this->prefix;
+
+ $app["$prefix.client_initializer"] = $app->protect(function($arguments) use($app, $prefix) {
+ $extract = function($bag, $key) use ($app, $prefix) {
+ $default = "default_$key";
+ if ($bag instanceof Application) {
+ $key = "$prefix.$key";
+ }
+ if (!isset($bag[$key])) {
+ return $app["$prefix.$default"];
+ }
+ if (is_array($bag[$key])) {
+ return array_merge($app["$prefix.$default"], $bag[$key]);
+ }
+
+ return $bag[$key];
+ };
+
+ if (is_string($arguments)) {
+ $parameters = $arguments;
+ $options = $app["$prefix.default_options"];
+ }
+ else {
+ $parameters = $extract($arguments, 'parameters');
+ $options = $extract($arguments, 'options');
+ }
+
+ return new Client($parameters, $options);
+ });
+ }
}
@@ -21,6 +21,7 @@ protected function register(Array $arguments = array(), PredisServiceProvider $p
{
$app = new Application();
$app->register($provider ?: new PredisServiceProvider(), $arguments);
+ $app->boot();
return $app;
}

5 comments on commit 040fa84

@nrk

This comment has been minimized.

Show comment Hide comment
@nrk

nrk Jul 8, 2012

Owner

I'm still not sure but I guess it's probably better to bump the version of the next release to v0.3.0 instead of v0.2.5 even if the changes does not affect publicly accessible parts of the service provider in the vast majority of cases.

Owner

nrk replied Jul 8, 2012

I'm still not sure but I guess it's probably better to bump the version of the next release to v0.3.0 instead of v0.2.5 even if the changes does not affect publicly accessible parts of the service provider in the vast majority of cases.

@indeyets

This comment has been minimized.

Show comment Hide comment
@indeyets

indeyets Jul 17, 2012

This commit doesn't feel right.

The idea of boot() and register() is, that register() registers actual service, while boot() is called after all services are registered, so that provider can do additional configuration using dependencies, which were not available earlier.

The problem is, that this commit delays service-registration till boot() and it is impossible to use $app['predis'] during app-configuration process

This commit doesn't feel right.

The idea of boot() and register() is, that register() registers actual service, while boot() is called after all services are registered, so that provider can do additional configuration using dependencies, which were not available earlier.

The problem is, that this commit delays service-registration till boot() and it is impossible to use $app['predis'] during app-configuration process

@nrk

This comment has been minimized.

Show comment Hide comment
@nrk

nrk Jul 17, 2012

Owner

The fact is, I don't like this commit too but I couldn't find a good solution either.

Leaving aside default_options and default_parameters (admittedly, their usefulness is dubious), as far as I understand now it is impossible to perform this kind of check inside register(). I like to have an easy way to define single or multiple client instances when registering the service provider in your application, with slightly more concise code to achieve the former.

Any idea?

Owner

nrk replied Jul 17, 2012

The fact is, I don't like this commit too but I couldn't find a good solution either.

Leaving aside default_options and default_parameters (admittedly, their usefulness is dubious), as far as I understand now it is impossible to perform this kind of check inside register(). I like to have an easy way to define single or multiple client instances when registering the service provider in your application, with slightly more concise code to achieve the former.

Any idea?

@indeyets

This comment has been minimized.

Show comment Hide comment
@indeyets

indeyets Jul 17, 2012

Aha! This commit is to blame: fabpot/Silex@9766faf

I guess @fabpot didn't have this case in mind.

Well, in this case the solution is to move multi-client configuration to the constructor of PredisServiceProvider. That is the BC break, but it will work

Aha! This commit is to blame: fabpot/Silex@9766faf

I guess @fabpot didn't have this case in mind.

Well, in this case the solution is to move multi-client configuration to the constructor of PredisServiceProvider. That is the BC break, but it will work

@indeyets

This comment has been minimized.

Show comment Hide comment
@indeyets

indeyets Jul 17, 2012

Additionally, for clarity, it makes sense to separate classes for single and multiple instances cases. Or just use different "static constructors"

Additionally, for clarity, it makes sense to separate classes for single and multiple instances cases. Or just use different "static constructors"

Please sign in to comment.