Override callback resolver

PHP-DI / Silex-Bridge

PHP-DI integration in Silex

http://php-di.org/doc/frameworks/silex.html

MIT License

24 stars 8 forks source link

Override callback resolver #10

Closed praswicaksono closed 8 years ago

praswicaksono commented 8 years ago

this PR will solve issue #9

mnapoli commented 8 years ago

Thank you for the PR! Could you also add a functional test in this class to cover the behavior you described in #9? i.e. that something like this works:

$app->get('/{name}', HelloName::class)
    ->convert('req', HelloNameConverter::class);

I have a few other comments that I'll write inline the diff.

mnapoli commented 8 years ago

I don't have much time right now to check, do you know if the callback_resolver service is used elsewhere in Silex (other than for param converters)? I'm asking that because I'm wondering:

could this change add support for "PHP-DI callables" on other features of Silex (which would be great)?
but could this change also break/change the behavior of something else?

Anyway I'm looking forward to merge and release this!

praswicaksono commented 8 years ago

Silex use callback_resolver on MiddlewareListener, ExceptionListener, ConverterListener and ViewListener so PHP-DI Callable will useful on those feature too. I don't think this will break them or other feature but anyway I will add functional test to all those feature.

mnapoli commented 8 years ago

:+1: awesome

praswicaksono commented 8 years ago

ready for review :)

jdreesen commented 8 years ago

I'm going to take a deeper look when I'm at home this evening.

jdreesen commented 8 years ago

Some small nits and style consistency thingies. Otherwise it's fine, I think :)

mnapoli commented 8 years ago

That was fast ;) That looks all good to me! @jdreesen?

jdreesen commented 8 years ago

:+1: from me :)

mnapoli commented 8 years ago

:+1: thanks @Atriedes

mnapoli commented 8 years ago

I'll create a release tonight with a documentation update