Remove dependency on container-interop about psr7-middlewares HOT 37 CLOSED

Oh and here's a simple resolver that works with container-interop:

use Interop\Container\ContainerInterface as Container;

class ContainerResolver implements Resolver
{
    private $container;

    public function __construct(Container $container)
    {
        $this->container = $container;
    }

    public function resolve($spec)
    {
        return $this->container->get($spec);
    }
}

from psr7-middlewares.

The reason of using container-interop is to provide support of libraries currently availables. But I see the benefits of the Resolver pattern: more simple and it's easy to create adapters to other patterns. I have to think about this.
Thanks

from psr7-middlewares.

Hi, first of all thanks for this repo.

Is there any plan to merge 0c52f54 and/or add a git tag?

And just my 2 cents: being a collection of callbacks, probably it does not worth to use a container nor a resolver... make the constructor argument mandatory for middlewares that require configuration would do the trick. And would make the library less complex and usable everywhere. IMO the container should be out of any library logic and be at application level. Or don't be anywhere: applications may exist with no container at all.

from psr7-middlewares.

@Giuseppe-Mazzapica I definitely agree with you. Ideally, there should be no service location required within any middleware. However, there are scenarios that require it. For example, after routing to create the domain / controller instance.

from psr7-middlewares.

@shadowhand A container is never required, but surely can be helpful in different situations. In your example you could simply accept a callable in constructor that once executed has to return the domain / controller instance. At application level, that callable could be resolved by the container.

from psr7-middlewares.

@Giuseppe-Mazzapica I agree. That's (effectively) what the resolver allows for.

from psr7-middlewares.

@shadowhand The resolver requires a container that must to satisfy an interface. But:

• I may want to don't use a container at all
• I may want to use a container that does not satisfy that interface (so I would need to write an adapter)

simply accepting a callable make the code more flexible, IMO.

from psr7-middlewares.

@Giuseppe-Mazzapica the ResolverInterface takes a class name and returns an object. That's all it does. It does not require a container.

from psr7-middlewares.

Right. But being an interface I need to write a class that implements it. You can consider callable as an interface provided by PHP, with the difference that you are not forced to write a class, because you can use a an existent class method, a function, or even a closure. As I said, just more flexible.

If we were talking about supporting PHP 7 only, I would be agree that the resover is better because can enforce the return type, but with PHP 5 using resover you are adding a constraint (= complexity) with no real benefit in exchange.

from psr7-middlewares.

But a callable has no documentation. What the expected input? Output?

from psr7-middlewares.

Why not?

/**
 * @param callable $callable Callback that MUST return an object of `Foo/BarInterface`
 */
function __construct(callable $callable) {
}

from psr7-middlewares.

Here's some sketchs of using callables:

//Basic
Middleware::auraRouter()->from(function () {
    return new Aura\Router\RouterContainer();
});

//Using a container/resolver
Middleware::auraRouter()->from(function () use ($container) {
    return $container->get('router');
});

//Using simple class that returns a callable from a container-interop
Midleware::auraRouter()->from(ContainerResolver::get($container, 'router'));

//Using an array, and even detect whether more than two elements are included to use them as arguments:
Midleware::auraRouter()->from([$container, 'get', 'router']);

from psr7-middlewares.

The benefit of an interface is that when you're using a proper dependency injector like Auryn the interface can be type hinted and automatically resolved. A callable requires configuration.

from psr7-middlewares.

Personally I prefer interfaces over anonymous functions in just about every scenario, especially as constructor parameters. This is OOP, not functional programming, after all.

from psr7-middlewares.

The benefit of an interface is that when you're using a proper dependency injector like Auryn the interface can be type hinted and automatically resolved

Yep... I always forget about magic.

This is OOP, not functional programming, after all

As I said callable in PHP is, at any userland effects, an interface. More precisely it is a language-level interface, something like Iterator or Serializable. The only difference is that it provides more flexibility in implementation. With PHP 7 return type and anonymous classes the choice of callable makes far less sense, but in PHP 5, there are cases in which callable is better choice.

For example, you said that callable requires Auryn configuration, but an interface requires to write a class... and to write a class is more effort than to write some configuration for some container. Even because using resolver all the implementers are forced to write a class, but container configuration is something that only some users of some containers should do.

from psr7-middlewares.

For example, you said that callable requires Auryn configuration, but an interface requires to write a class... and to write a class is more effort than to write some configuration for some container.

That's not necessarily true. Once a resolver is written for a particular implementation, it can be reused infinitely. Using a closure to resolve requires writing a separate closure for every single resolution. It might appear more efficient initially but actually results in more code which means it is implicitly harder to debug and not as reuseable.

from psr7-middlewares.

I think that this depends on the implementation.

For example in Pimple I would simply do:

$pimple = new Pimple\Container();

// store  in the container the resolver callable that returns a controller class
$pimple['controllers.resolver'] = $pimple->protect( function($class = null)
{
    is_null($class) and $class = ControllerDefault::class;
    if ( ! is_subclass_of($class, ControllerInterface::class)) {
        throw new \InvalidArgumentException('Invalid Controller class name.');
   }
   return new $class();
});

After that, in the container you can use the same resolver again and again

$pimple['middlewares.foo'] = function($container)
{
   $controller = $container['controllers.resolver']('FooController');
   return new MiddlewareFoo($controller);
}

$pimple['middlewares.bar'] = function($container)
{
   $controller = $container['controllers.resolver']('BarController');
   return new MiddlewareBar($controller);
}

$pimple['middlewares.baz'] = function($container)
{
   $controller = $container['controllers.resolver'](); // this uses default class
   return new MiddlewareBaz($controller);
}

And at application level:

$dispatcher = $relay->newInstance([

   $pimple['middlewares.foo'],
   $pimple['middlewares.bar'],
   $pimple['middlewares.baz'],
]);

As you can see, my code is the version of a resolver that uses a callable as resolver.

Nice thing is that using a code like this the library could not contain any of the container-related code, leaving the container implementation to application/framework implementers.

Library can instead focus on what matters: write middleware functions, just that.

from psr7-middlewares.

Yeah, see you just wrote 4 pieces of code. This is what I would write with Auryn and an interface:

use Auryn\Injector;

class AurynResolver implements ResolverInterface
{
    private $injector;

    public function __construct(Injector $injector)
    {
        $this->injector = $injector;
    }

    public function resolve($class)
    {
        return $this->injector->make($class);
    }
}

And then I would configure it once:

$injector->alias(ResolverInterface::class, AurynResolver::class);

Done. Every since piece of code that uses a ResolverInterface will now work properly.

from psr7-middlewares.

FWIW https://github.com/relayphp/Relay.Relay/blob/1.x/src/ResolverInterface.php

from psr7-middlewares.

I wrote 3 pieces of code, where 1 is the application usage, that you did not wrote. So they are 2 pieces. And you write 2 pieces of code as well. Also, I made the example of 3 concrete usages of the resolved class, you wrote none. If I reduce my code at the level of detail of your code, my code is actually less or at least as big as your code. (I also think that mine is more readable and easier to debug because no magic, but that's a personal preference).

The point is that a library like this (a collection of middlewares) should not be coupled with any container nor to any interfaces (like ResolverInterface) that refers to a container logic.

If you look at my code again, the classes that would be part of the library (MiddlewareFoo, MiddlewareBar and MiddlewareBaz) receives the real dependency (a class implementing ControllerInterface) in the constructor and they know nothing about the way the classes implementing that interface are instantiated, which container is used or if a container is used at all.

This makes the library usable everywhere, with zero additional effort.

from psr7-middlewares.

I didn't include the usage of resolver because I felt it would be contrived. But here... the usage of the resolver in a middleware:

public function __construct(Resolver $resolver)
{
    $this->resolver = $resolver;
}

public function __invoke($request, $response, $next)
{
    $domain = $this->resolver->resolve($request->getAttribute('adr/domain'));
    ...
}

I am fairly sure I know exactly what your code is doing and it requires significantly more configuration than I want or need. Part of the problem is using an inferior dependency injector that cannot infer how classes should be resolved. The other part of the problem is lack of reuse... your example is specific to a single type of class and cannot be used universally.

from psr7-middlewares.

I'm sorry but this code is really bad for me. How do you you think that pass an instance of resolver into a middleware class is a good thing?

What you call a "resolver" here is just a "Service Locator" that is known to be a bad pattern.

In this specific case it hides a dependency: you code makes the middleware class dependant from a Resolver, but it actually isn't: the middleware does not require a Resolver to work, it requires a ControllerInterface. Why the hell should I inject a Resolver where I could inject the real dependency?

Moreover, what happen if $request->getAttribute() returns something that is not what the resolver expect? I guess that the container of choice maybe will throw an exception... but which exception? How can I answer this question without looking into Container code?

from psr7-middlewares.

@Giuseppe-Mazzapica are you suggesting that all of the fully resolved controller instances should be passed into the middleware? I assume not, which leads to...

How would you architect a situation where you have RoutingMiddleware that chooses which controller to load based on the URL and then executes the controller?

My solution is that we have injection in two places: application bootstrapping and post-routing. This is not service location because we are dealing with real class names and we are not pull them out of a locator arbitrarily within a class. This is service location:

class Controller
{
    private $db;

    public function __construct(Resolver $resolver)
    {
        $this->db = $resolver->resolve(Database::class);
    }
}

In this scenario, your dependencies have been completely hidden and that is what everyone says is bad and rightly so. It is definitely not what I was demonstrating, though it looks very similar.

from psr7-middlewares.

this is my last comment, at worst we can agree to disagree.

I'm not talking about "Controllers", I'm talking in general. And I'm saying that you should not pass to a constructor something that "resolves" something, but directly the "resolved" things. Whatever it is.

This is your code:

public function __construct(Resolver $resolver)
{
    $this->resolver = $resolver;
}

public function __invoke($request, $response, $next)
{
    $domain = $this->resolver->resolve($request->getAttribute('adr/domain'));
    ...
}

My question is: what $domain variable is? You can declare it in documentation, but it is a good thing? You can declare in documentation even in the case you said is "bad". Your dependecy is hidden.

There is no place in the class code that fails if $domain is actually not an instance of the type you need. Unless you wrote it explicilty:

public function __invoke($request, $response, $next)
{
    $domain = $this->resolver->resolve($request->getAttribute('adr/domain'));
    if ( ! $domain instanceof TheInterfaceYouNeedForReal ) {
      throw Exception('Service locator provided a wrong instance');
    }
}

this means add complexity to middleware and add a responsibility that it does not have.

A code like this:

public function __construct(DomainInterface $domain)
{
    $this->domain = $domain;
}

public function __invoke($request, $response, $next)
{
}

is more readable, more clear and with a lower complexity.

When people talks about "Dependency Injection" talk about this. To pass a "resolver" is not dependency injection. For a very simple reason: the "resolver" is not a dependency, but something that resolves a dependency. This migh be called "Dependency Resolver Injection", but there's no reason to find a name, because someone already named this pattern as "Service Locator Pattern", and nowadays is almost universally considered bad (one example for all http://blog.ploeh.dk/2010/02/03/ServiceLocatorisanAnti-Pattern/).

from psr7-middlewares.

I am talking specifically about controllers (or domain objects in ADR pattern) because this the one of very few places where service location/resolution is required. To this end, you still have not answered my question:

How would you architect a situation where you have RoutingMiddleware that chooses which controller to load based on the URL and then executes the controller?

from psr7-middlewares.

Trust me when I say that this is not theory and something I am intimately familiar with. We've built a whole framework around the ADR pattern which uses the resolver pattern in middleware with great success. For things like content negotiation and routing, I have yet to see any solution that avoids the use of service location.

from psr7-middlewares.

How would you architect a situation where you have RoutingMiddleware that chooses which controller to load based on the URL and then executes the controller?

I would create a specific ControllerFactory class (instead of a generic resolver) with a method factoryController() that returns a controller instance. That method has the responsibility to check the arguments received and type of what it returns and throw an exception if something is not right.

But that kind of middleware is very coupled with the application stack, and does not fit a library of generic and application agnostic middlewares like this.

from psr7-middlewares.

Right, which isn't significantly different from what I am doing, except that you have to create more code to do it. Earlier you stated that a closure was less code and configuration, which we now see is not the case. Instead of having the factory do type checking, I would prefer that the engine do it:

function run(Domain $domain, Input $input) { ... }

If the resolved object is of the wrong type, this code will fail the type check and blow up. This is okay because it's a developer error, a misconfiguration of routing.

It wasn't included in my example, but obviously there would be an empty check on the attribute value of adr/domain so that a proper 404 could be generated if no routes could be matched.

from psr7-middlewares.

In conclusion: both solutions work and I think that the resolver approach more generic, less code, and more efficient. It allows reuse and doesn't require any explicit configuration beyond defining the resolver implementation.

The downside is that anything is implicitly allowed, so avoiding abuse as a service locator is left up to the developer.

from psr7-middlewares.

Let me do my conclusion.

which isn't significantly different from what I am doing, except that you have to create more code to do it. Earlier you stated that a closure was less code and configuration, which we now see is not the case. Instead of having the factory do type checking, I would prefer that the engine do it:

I said callable is less code, because we were talking about the code needed to implement the library into an application. If absolutely needed, with a callable, you just need to add an entry to composer.json, with a resolver interface you need to also write a class, so more code.

After that, I looked more deeply into this repo and realized that all the middlewares that are using a resolver, could actually be rewritten to don't use any resolver, nor any callable. In fact, they already receives the real dependency in the constructor, but it is not mandatory (default to null) and the resolver is used to resolve the dependency only when the dependency is not passed to constructor.

For example look at AuraRouter middlewares, it accepts the Aura RouterContainer in the constructor.

If the constructor argument would be mandatory instead of optional, you could delete quite a lot of lines of code in this repo (removing all the container-related and the resolver-related code), without losing nothing, but actually making it more flexible.

And this is what I tryed to proof with the code blocks I posted above.

After that you asked how I would solve an application problem, and I answered saying that I would use a class, that uses the "factory method" pattern, surely something that I did not invented.
This is more code, but at application level I don't need to make the library easily and universally implementable , because I have already implemented the library, and at that level I care more of readable and predictable code than to write less code.

But in a library, more specifically in this library, there's no need of the resolver pattern, nor of any callable. The only thing that I would do is to make the constructor argument mandatory (where present) making any container-related code useless here.

from psr7-middlewares.

@Giuseppe-Mazzapica a good analysis. I would suggest that @oscarotero remove both the resolver and the container-interop dependency and not rely on any service location.

from psr7-middlewares.

Ok, I understand the confusion now, so I'm going to clarify some things:

• This library has not problems with dependencies. As pointed @Giuseppe-Mazzapica all middlewares have arguments and methods to provide the direct dependencies (instances of routers, sessions, cache, debugbar, etc). So I can do thinks like this:

$dispatcher = $relay->getInstance([
    Middleware::Cors()
        ->settings(new Neomerx\Cors\Strategies\Settings()),
    Middleware::DebugBar()
        ->debugBar(new DebugBar\StandardDebugBar()),
    Middleware::AuraSession()
        ->factory(new Aura\Session\SessionFactory()),
    Middleware::FastRouter()
        ->router(FastRoute\simpleDispatcher()),
    Middleware::Geolocate()
        ->geocoder(new Geocoder\Geocoder()),
]);

(For brevity, I omitted the code to configure these dependencies)

• The only reason to provide a container/resolver is for performance. Let's say, we add a cache middleware to our middleware stack:

$dispatcher = $relay->getInstance([
    Middleware::Cache()
        ->cache(new Psr6CachePool())
    Middleware::Cors()
        ->settings(new Neomerx\Cors\Strategies\Settings()),
    Middleware::DebugBar()
        ->debugBar(new DebugBar\StandardDebugBar()),
    Middleware::AuraSession()
        ->factory(new Aura\Session\SessionFactory()),
    Middleware::FastRouter()
        ->router(FastRoute\simpleDispatcher()),
    Middleware::Geolocate()
        ->geocoder(new Geocoder\Geocoder()),
]);

Even if the response is cached previously and the first middleware returns this cache, the dependencies of the following middlewares are instantiated anyway (cors settings, debug bar, session, etc..) So the main purpose of the container/resolver is to lazy load these dependencies and avoid to create unused instances when the middleware is not executed. I choose the container-interop because is widely used and there's adapters for many libraries.

• When @shadowhand suggested to remove the container-interop dependency and create a Resolver interface, I didn't feel bad with it. It has the only method I need (to fetch the object) and It's easy to create adapters.
• The use of callables, suggested by @Giuseppe-Mazzapica is also good for me. It's simpler and faster. I thought in this before implements the container-interop, but I didn't like to fill the middleware stack with a bunch of callables. I'd like something clean and readable.
• The other option is handle this out of the scope of this library. I know that this can be done using the resolver of Relay, for example. But not all middleware dispatchers have this feature and I like a way to do this without create a bunch of classess and resolvers.
• I'm open minded so any suggestion is welcome, but take into account that the purpose is provide a way to lazy load dependencies, not resolve them.

from psr7-middlewares.

Let me answer

The only reason to provide a container/resolver is for performance. Let's say, we add a cache middleware to our middleware stack... Even if the response is cached previously and the first middleware returns this cache, the dependencies of the following middlewares are instantiated anyway

I understand this point, but just instantiate < 10 objects is never a big deal. And really, it's hard to believe a system with more of 10 middlewares, and even in that case, having that big number of middlewares the instantiantion of object is last of the problems in the regard of performance.

Considering that only some of the middlewares accepts object to constructor, very likely there will be never more than 6 or 7 object ot instantiate, and that's not really a big deal.

Have you tried to run some sort of profiling? I do my bet: if you profile a stack with 20 middlewares that receives dependency in constructor, and a stack with same 20 middlewares that pull the dependency via a resolver, I almost sure that the difference in terms of perfomance will so trivial that can be considered zero.

That, of course, if the dependency injected don't do something "expensive" in the constructor. In that case it's more a problem with the object, because constructor should never used to do something expensive, expecially in a language like PHP where there's no object persistence.

By the way...

considering that the "resolver" is not something mandatory, but fully optional, I can use this library just like it is, and just inject things in constructor when I need.

My suggestion

Just as a suggestion, I can only say that IMO this library could be much more flexible removing any "resolver" logic and instead using Middleware class to encapsulate all the factory logic.

You are already using that class as a factory for your middlewares (via __callStatic()), so why not use that class to create "asyncronous" middlewares (with no need of "resolvers")?

To better explain what I mean, I wrote a Gist.

There I took your Middleware class and I added a method: middleware().

That method takes a callback that once called have to return a middleware and return a fully valid middleware in form of a closure. This closure instantiates the real middleware and run it, acting as a proxy. Please see the comment in the Gist for an usage example.

In this way, Middlewares that requires "expensive" instantiation are instantiated only when used, with no need of "ResolverInterface" and "ResolverTrait".

As additional benefit, the code becomes more readable, so we have the same flexibility, more readability with less code.

Note that the code is perfectly and completely backward compatible with your current code.

Another thing: you can see in my Gist that I used a private method newWithArgs() that is used inside __callStatic() to instantiate classes when they take arguments. Reason is that ReflectionClass::newInstanceArgs() is notably slow, and if you are concerned about performance you should avoid that if you can.

As a final note, in __callStatic() I added a third argument $namespace. The reason is this way is possible to use Middleware class for middlewares in other namespaces, e.g. custom application-level middlewares.

from psr7-middlewares.

Oh, I really like your suggestion!
This removes the dependency of any resolver and moves the lazy instantiation logic out of the scope of the middleware.
Thank you

from psr7-middlewares.

👍

from psr7-middlewares.

👍 I like this solution. I wouldn't personally use it, but it also doesn't get my way. It's a win-win all around.

from psr7-middlewares.

Released 3.9.0 including this change.
Thank you!

from psr7-middlewares.