Override ControllerResolver as a shared service

[jdreesen]

I just wanted to give this bridge a try with a Silex 2 project and stumbled upon a bug with the overridden ControllerResolver being not registered as a Pimple service but as a parameter.

As this bridge doesn't support Silex 2/Pimple 3 by default (because there are some breaking changes in Pimple 3 and Silex 2) and I have no Silex 1/Pimple 1 project at hand I cannot surely confirm that this is a bug there too, but as Silex itself registeres its ControllerResolver (and every other class) as a (shared) service and not as a parameter I think we should do here, too.

This was the exception I ran into:

Fatal error: Uncaught exception 'InvalidArgumentException' with message 'Identifier "resolver" does not contain an object definition.' in \vendor\pimple\pimple\src\Pimple\Container.php:232
Stack trace:
#0 \vendor\silex\silex\src\Silex\Provider\ServiceControllerServiceProvider.php(24): Pimple\Container->extend('resolver', Object(Closure))
#1 \vendor\pimple\pimple\src\Pimple\Container.php(273): Silex\Provider\ServiceControllerServiceProvider->register(Object(Silex\Application))
#2 \vendor\silex\silex\src\Silex\Application.php(132): Pimple\Container->register(Object(Silex\Provider\ServiceControllerServiceProvider), Array)
#3 \src\Silex\Application.php(51): Silex\Application->register(Object(Silex\Provider\ServiceControllerServiceProvider))
#4 \bin\console(14): Silex\Application->__construct('\vendor\pimple\pimple\src\Pimple\Container.php on line 232

[jdreesen]

Please let me know if you want a pull request for the changes needed to make this bridge compatible to Silex 2 / Pimple 3, too.

[mnapoli]

Thank for the pull request! I did this change this morning (replacing the closure with directly setting the instance) because I thought why not: the controller resolver will (almost) always be instantiated so we might as well avoid the closure overhead (very little optimization of course, but again why not).

All tests are green with Silex/Pimple v1 so I guess it currently works with this version. The fact that it's a bug for Silex 2/Pimple 3 means we are definitely better off merging that!

The work you did for compatibility with the new versions is great! Are you sure it's possible to make this package compatible with both versions at the same time? If it is, then I'd love to merge all your changes back.

By the way do you know when Silex 2 will be released? I didn't even see a beta, are you using it on a project?

[jdreesen]

I just took a closer look at how Pimple works in the inside. And even though the way how Pimple works has changed quite a bit from version 1 to 3, this specific behaviour has remained the same.

The answer to "why not" is, that it's only possible to extend() services in Pimple if they are closures/callables (specifically: objects implementing __invoke()). That's why everything that's more than just a simple parameter should be added as a closure.

Therefore this should be a bug in version 1, too. But it only arises if you try to extend() the resolver definition, which is not covered by the tests yet, as far as I can see.

I'm registering Silex's ServiceControllerServiceProvider which is exactly doing this one thing: extend()ing the resolver definition.

[jdreesen]

Regarding the compatibility for both Silex/Pimple versions at the same time: I'm not sure if this will work because Pimple switched it's behaviour regarding shared/factory service definitions. I will take a closer look and play around a bit with both versions in the next days to see if it works, before sending a pull request.

The problem is, that in Pimple 1 you create a factory service by simply setting a closure. If you want a shared service instead, you have to wrap it with $container->share(...). In the contrary, starting with Pimple 2, when you simply set a closure, it will be a shared service. If you want a factory service instead, you have to wrap the closure with $container->factory(...).

When I think about it now, we may simply check for method_exists($this, 'share') when setting the resolver, but I don't know if that's a desirable way.

Sadyl I don't know when Silex 2 will be released. There is a Silex 2 roadmap issue which was closed a while ago, but no one seems to know when a final release will eventually happen. I'm using it in a private project which is in its early stages, so I hope there will be a release within the next couple of month or so (maybe together with Symfony3?)

[mnapoli]

Thanks for the info. I'll be merging this now and releasing then. For Silex 2 support there's no rush since there is not really information about this :/

[jdreesen]

After I thought a little bit more about this, I don't think it would be wise to support Pimple 1 and 3 at the same time because of its twist regarding the behaviour of registering shared / factory services. Therefore we should handle this in different branches and make the "v2" branch the default one once Silex 2 has been released.

I'm going to track Silex 2 development in my own fork and create a pull request when it's released, unless you want me to create it now.

[mnapoli]

Sounds good to me!