Allow arbitrary injection into middleware and param converters

[felixfbecker]

This is an implementation for #15
I would love to get this merged as quickly as possible to depend on it in my projects.

---

Feature

It is now possible to inject whatever is registered in the container into route param converters and middleware via type hints. Also, as long as the arguments are type-hinted, argument position of request / response / app doesn't matter anymore. If there are no type-hints, it falls back to Silex' default argument order.

Implementation

To accomplish this, ConverterListener and MiddlewareListener are overwritten and in Application the default dispatcher is overwritten to use the custom classes. For application middleware, before/after/finish are overwritten aswell.

For DRY reasons the injection logic is done in separate classes (BeforeMiddlewareCaller, AfterMiddlewareCaller, ConverterCaller). This allows the application middleware and route middleware to share the same logic and also for all event listeners to share the same instance of the caller and ParameterResolver objects.

Tests

ConverterTest and MiddlewareTest test that both arbitrary injection is possible and the old behaviour still works.

Docs

I altered the README to highlight that injection works not only for controllers but also for converters and middleware and added two examples.

BC Breaks

None

---

Any thoughts / objections?

[mnapoli]

I'm not sure I understand when this class is used by Silex? Since App::before() and App::after() are overridden, why do we need to override this too?

[felixfbecker]

There are two types of middleware, Application level middleware via $app->before() and route / controller collection level middleware like $app->get('/', ...)->before(...). Route level middleware is handled throught MiddlewareListener.

[mnapoli]

Ah right thanks!

[felixfbecker]

I will squash once you give your OK to merge

[jdreesen]

"calling a before an after middleware"

[jdreesen]

As I don't have much time right now I just took a quick look.
The PR looks good overall, just some small nits.
Although I don't really like the code duplications in all those call() methods...

[felixfbecker]

@jdreesen I addressed your comments and squashed.

Although I don't really like the code duplications in all those call() methods...

I know, I don't like it either, but every case (after middleware, before middleware, param converter, controller resolver) is just a bit different so they cannot share the exact same implementation. If you break it down it's really nothing but

• do reflection (by using the already high-level Invoker API)
• do a foreach to resolve the very special arguments needed in that case (request, response etc)
• get the arguments with fallback to the default behaviour in that case
• check if all parameters resolved by getting a diff and maybe throwing an exception.
  this is the only step (4 statements if you count) that all cases actually share and it doesn't make sense to refactor this anywhere

I'm actually quite happy that there is no duplication for route / application level middleware and that finish and after middleware can both use AfterMiddlewareCaller.

[mnapoli]

Sorry for the delay, I've taken some time to think about it.

In order to preserve BC we want to inject the request, response and app by:

• parameter name
• type-hint

The application is not a problem because it's a service, but the request & response are not so using the Invoker, even with a custom ResolverChain (to answer your question yes it's possible), will not work because type-hints are only checked for container entries.

What we could do is create a new type of ParameterResolver that checks the type-hint, but not for container entries. Example of usage:

$parameterResolver = new ResolverChain([
    new AssociativeArrayResolver, // inject request/response by name
    new TypeHintResolver, // inject request/response by type-hint
    new TypeHintContainerResolver, // dependency injection
]);

...

$invoker->call($middleware, [
    'request' => $request, // by name, used by AssociativeArrayResolver
    'response' => $response, // by name, used by AssociativeArrayResolver
    'Symfony\...\Request' => $request, // by type-hint, used by TypeHintResolver
    'Symfony\...\Response' => $request, // by type-hint, used by TypeHintResolver
]);

The TypeHintResolver could be reused by other bridges later (e.g. Slim framework). It could be added directly to the Invoker project.

What do you think?

[felixfbecker]

@mnapoli

In order to preserve BC we want to inject the request, response and app by:

• parameter name

that doesn't make much sense to me - with default Silex there is not injection by parameter name, only by parameter position. A middleware is always called with call_user_func($middleware, $request, $app). I took care of BC by falling back to a numeric parameter resolver. I get injection by parameter name for route parameters, but injecting the request by parameter name is too magic for me. It would also be quite weird to document because for consistency, all parameters including the application should then be able to get injected by name, and the application is always called $app in the docs...

What we could do is create a new type of ParameterResolver that checks the type-hint, but not for container entries

this is a very good solution, but I want to point out here that I strictly followed the existing implementation in ControllerResolver, so this has not much to do with this PR. But it would save a lot of code duplication.

What I don't get though:

$invoker->call($middleware, [
   'request' => $request, // by name, used by AssociativeArrayResolver
   'response' => $response, // by name, used by AssociativeArrayResolver
   'Symfony\...\Request' => $request, // by type-hint, used by TypeHintResolver
   'Symfony\...\Response' => $request, // by type-hint, used by TypeHintResolver
]);

Where do you pass the paramResolver here?

[mnapoli]

The parameter resolvers are passed when constructing the Invoker:

$parameterResolver = new ResolverChain([
    ...
]);
$invoker = new Invoker\Invoker($parameterResolver);

And you are right about injection of request/response by name, that's not necessary here so let's keep it as simple as we can (and it wouldn't be consistent with injection in controllers anyway).

Then you would only need something like this:

$parameterResolver = new ResolverChain([
    new TypeHintResolver, // inject request/response by type-hint
    new TypeHintContainerResolver, // dependency injection
    new NumericArrayResolver, // fallback to injecting "request, response, application"
]);
$invoker = new Invoker\Invoker($parameterResolver);

$invoker->call($middleware, [
    // parameters indexed by position for the NumericArrayResolver
    $request, $response, $application,
    // parameters indexed by the type-hint for the TypeHintResolver
    'Symfony\...\Request' => $request,
    'Symfony\...\Response' => $request,
]);

Does that make sense?

I want to point out here that I strictly followed the existing implementation in ControllerResolver, so this has not much to do with this PR

I agree and take your point. But with this PR this implementation gets duplicated multiple times and the code gets much more complex. By merging this I'll also be committing to supporting this code, I'd rather go with something as simple as possible ;)

[felixfbecker]

@mnapoli Your proposed solution makes a lot of sense. Do you want me to do a PR to Invoker?

[mnapoli]

@felixfbecker if you have time for that that would be awesome!

[felixfbecker]

I changed the implementation to use the TypeHintResolver from my PR PHP-DI/Invoker#8.

• A single class CallbackInvoker does the job of all the *Caller classes before
• It extends Invoker for type safety so MiddlewareListener etc. can depend on this specific Invoker with this specific ResolverChain.
• Tests of course fail because you need to copy the new TypeHintResolver to vendor

When the change is published on Invoker I will bump the dependency version and squash. Just wanted to share the changes now so you can review.

[felixfbecker]

Looks good?

[mnapoli]

I'm not convinced by the usefulness of this class. I think you referred to that when you said:

It extends Invoker for type safety so MiddlewareListener etc. can depend on this specific Invoker with this specific ResolverChain.

But I don't see why a MiddlewareListener should explicitly require this implementation. To me it would be better than middleware listeners or parameter converters are implementations that rely on the interface of "Invoker", and then the application passes an instance of Invoker configured as we want.

I.e. I don't see the point to couple things more than necessary.

[felixfbecker]

But then the application could theoretically pass an Invoker instance that uses a completely different ResolverChain, which would break the MiddlewareListener. Only this way can MiddlewareListener declare that it needs an Invoker that is suited for calling Silex callbacks, through the parameter names, type hints, the container and through parameter position.

[felixfbecker]

I am not attached to it though. If you feel strongly about this, let me know and I remove the class :)

[mnapoli]

Looking very good! Much better now! It's really too bad we have to copy so much code from Silex :/ but that's not your fault of course.

[felixfbecker]

Do you want me to squash so you can merge? Or do you have any other idea for the dispatcher?

[mnapoli]

Thanks for your answers. I'm good for merging, 👍 for squashing

[felixfbecker]

👍

[felixfbecker]

It should be good to go, any ETA?

[mnapoli]

A few minutes :)

[mnapoli]

1.5.0 is tagged and released! The documentation was also updated in PHP-DI's docs: http://php-di.org/doc/frameworks/silex.html

Thank you for your patience and implementing all that ;)