woohoolabs / harmony

A simple and flexible PHP middleware dispatcher based on PSR-7, PSR-11, and PSR-15
MIT License
164 stars 16 forks source link

Update DispatcherMiddleware.php #4

Closed userlond closed 9 years ago

userlond commented 9 years ago

If callable is not "directly" callable, try to get it's instance from container.

kocsismate commented 9 years ago

Thanks for your PR, I like the idea very much! However, give me some time to think if the addition really makes sense. And before merging, please fix the build error: https://travis-ci.org/woohoolabs/harmony/jobs/89164883.

userlond commented 9 years ago

Thanks for reply. I'v fixed some small errors.

The idea is based on https://github.com/PHP-DI/demo.

Controllers in demo app have some dependencies, which are injected via autowiring (http://php-di.org/doc/autowiring.html), example of controller is https://github.com/PHP-DI/demo/blob/master/src/SuperBlog/Controller/ArticleController.php

Autowiring is elegant and requires no config, but you need to get route handler via $container->get() to iject dependencies.

Example of my route (I use default harmony dispatcher with fastroute):

<?php 
$r->addRoute('GET', '/', \App\Controller\IndexController::class);

Example of controller:

<?php

namespace App\Controller;

use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;

class IndexController
{
    /**
     * @Inject
     * @var \Twig_Environment
     */
    private $twig;

    /**
     * @param \Psr\Http\Message\ServerRequestInterface $request
     * @param \Psr\Http\Message\ResponseInterface $response
     * @return ResponseInterface|null
     */
    public function __invoke(ServerRequestInterface $request, ResponseInterface $response)
    {
        $response->getBody()->write(
            $this->twig->render('index.twig', [
                'message' => 'Main page'
            ])
        );
        return $response;
    }
}

Without patch app raises exception Warning: call_user_func() expects parameter 1 to be a valid callback, function 'App\Controller\IndexController' not found or invalid function name in <...>\vendor\woohoolabs\harmony\src\Middleware\DispatcherMiddleware.php on line 43

With patch it works :).

P.S. It is possible to inject controller before pass to dispatcher:

<?php
/** @var ContainerInterface $c */
$r->addRoute('GET', '/', $c->get(\App\Controller\IndexController::class));

But this solition lacks of caching routes (some troubles with dumping closures).

kocsismate commented 9 years ago

I also use PHP-DI with auto-wired constructor injection and invokable classes as controllers and the current dispatcher works well for me! So I am pretty sure that you only notice this error because of your route handler definition in FastRoute:

Instead of

$r->addRoute('GET', '/', \App\Controller\IndexController::class);

you could write:

$r->addRoute('GET', '/', [\App\Controller\IndexController::class, "__invoke"]);

This way, everything would work as expected! However, I was also wondering not long ago if the definition for invokable controllers could have been simplified. Unfortunately I couldn't find a solution, but now, you gave me the answer! I will merge the PR shortly.

P.S. Injecting the controller directly into the route definition not only prevents caching of your routes, but instantiates all your controllers for every single request (unless you use PHP-DI with lazy loading)!

userlond commented 9 years ago

:+1: