pennyphp / penny

:moneybag: Php Framework made of other components
http://penny.gianarb.it/
Other
40 stars 7 forks source link

Fixes #148 #149

Closed samsonasik closed 8 years ago

samsonasik commented 8 years ago

Actually, the check should happen for Dispatcher instead of RouteInfo as if Dispatcher class executed, it must return RouteInfo or throw Exception in the switch case at https://github.com/pennyphp/penny/blob/master/src/Dispatcher.php#L64-L73, so, placing check should be

   if (!$dispatcher instanceof Dispatcher) {
       throw here....
   } 

at getDispatcher() method. Hereby a provided fixes ;). In next, the implementation for custom dispatcher must extends Penny\Dispatcher class with mixed $request in __invoke($request), or we create new interface for Dispatcher ;)

gianarb commented 8 years ago

mmm I don't understand.. Why are you extend Dispatcher? It is Penny\Dispatch is only the "default" dispatcher for penny.. And it is an invokable class

samsonasik commented 8 years ago

the problem is we need a router and container in the invokable class, and we need to push user to use it.

fntlnz commented 8 years ago

ping @gianarb

samsonasik commented 8 years ago

@gianarb If we have DispatcherInterface, we can have:

namespace Penny\Dispatcher;

interface DispatcherInterface
{
    /**
     * @param mixed $request
     * @throws RouteNotFoundException       If the route is not found.
     * @throws MethodNotAllowedException    If the method is not allowed.
     * @throws Exception                    If no one case is matched.
     *
     * @return RouteInfoInterface
     */
    public function __invoke($request);
}

The Dispatcher consumer will implement dispatch with required request no matter need container or not. It's pretty flexible imo ;)

gianarb commented 8 years ago

@samsonasik this interface is pointless at the moment.

is_callable() checks this stuff.. At the moment I would maintain the current approach.. :)

gianarb commented 8 years ago

I close this PR, at the moment I think that a closure is a good way to implement it..

Thanks @samsonasik