PHP Dependency Injections w/ Router

Question

I've been working on a personal from-scratch project for the purpose of learning, and so far I've been able to successfully build a DI container and a simple URI router. It works as expected right now, but from my perspective there's a lot that could potentially be improved.

So, from the tip top, my router is loosely based on Laravel's implementation, and is begun by injecting it with a hacky dependency container.

$container = new s\Container(new s\JsonFactory('../src/dependencies.json'));
$router    = new Router($container);

Afterwards, I define routes in a series of formats. Namely, you can pass the URI and an "action" to a series of public functions in the Router; primarily get() and post(). As expected, each of these functions handle their own HTTP request types.

$router->get('/user/register', function() {
    $view = new v\User\Register();
    $view->output();
});
$router->post('/user/register', 'user/Register@register');

My container reads from a JSON file to know what dependencies to inject:

{
    "classes": [
        {
            "id":    "TestClass",
            "class": "Poseidon\\TestClass",
            "arguments": [
                "Database"
            ]
        }
    ]
 }

When a class is requested from the container, the Register pulls from an associative array containing both class paths and the ids which point to each array from the JSON file.

Once the URI matches one of my routes (through a series of redundant checks) I analyze the "action" of the route:

protected function prepAction($action)
{
    if (is_string($action) && preg_match('/^[\w]+(?:\/)?[\w]+\@[\w]+$/', $action)) {
        // If the function is in the format 'class@method'
        return explode('@', $action);
    } elseif (is_array($action) || is_callable($action)) {
        // If the function is a class-method array or a callable
        return $action;
    }

    throw new \Exception("The action [$action] is invalid!");
}

At which time, the Container will resolve the dependencies of the action passed to it:

public function analyse($function)
{
    $method = null;

    if (is_array($function)) {
        $class = $this->get($function[0]);
        if ($class) {
            $method = new ReflectionMethod($class, $function[1]);
        }
    } elseif (is_callable($function)) {
        $method = new ReflectionFunction($function);
    }

    return $method;
}

public function resolve($function)
{
    $params        = $function->getParameters();
    $result        = [];
    $indentifiable = [];

    foreach ($params as $param) {
        if ($param->getClass() !== null) {
            $indentifiable[] = $param;
        }
    }

    foreach ($indentifiable as $param) {
        $result[$param->name] = $this->get($param->getClass()->name);
    }

    return $result;
}

And then I proceed to have my Router convert the Reflection to a closure:

protected function closureify($method)
{
    if ($method instanceof \ReflectionFunction) {
        return $method->getClosure();
    } elseif ($method instanceof \ReflectionMethod) {
        return $method->getClosure($this->container->get($method->class));
    } elseif ($method instanceof \Closure) {
        return $method;
    }

    throw new \Exception("Whatever [$method] is, it's not valid.");
}

And then it's simply called:

public function work()
{
    // Get the route info from our routes, if it's there
    $retrieved = $this->retrieve($this->currentURI);

    // If it is there, go ahead and run it's action here
    if (!empty($retrieved)) {
        $this->run($retrieved[2], $retrieved[1]);
        return;
    }

    throw new \Exception("No routes match the current URI. [$this->currentURI]");
}

protected function run($action, $args)
{
    $method = $this->container->analyse($this->prepAction($action));
    $params = $this->container->resolve($method);
    $result = array_merge($params, $args);

    call_user_func_array($this->closureify($method), $result);
}

That's my current way of doing things!! I need to know what I can do better and how I can do it.

Answer 1

All in all I would say that this is a solid start, but I think there are some details that could use some straightening out.

1. DI

This first comment comes from me putting on my "purist" hat:

$container = new s\Container(new s\JsonFactory('../src/dependencies.json'));
$router    = new Router($container);

Namely, you are injecting your dependency injection container directly into your object. That makes this the service locator design pattern rather than typical dependency injection, which is generally frowned upon as an anti-pattern. In your case you (presumably) aren't doing this throughout your application, but rather to bootstrap your application. That makes it a more forgivable. To some extent it is unavoidable here because you are relying upon method-level dependency injection (i.e. you are using type-hinting to specify dependencies at the method-level), while your dependency-injection container usually only knows about dependencies specified at the constructor level. This isn't really a surprise at all, because this is how laravel behaves, but I've personally never been a fan of method-level dependency injection. It can cause some wonkiness in the long-run. Suffice it to say, if you relied upon constructor-level DI only, then you wouldn't have to inject your DI container into your router. Rather, you could do a simple:

$router = $container->resolve( Router::class );
$router->execute()

(or something along those lines). Hopefully that all makes sense. Obviously you are doing things much like how laravel does things (a very popular framework built with best-practices in mind), so I wouldn't go so far as to say that this is wrong, but I did want to point out that there are other ways to do these things.

DI Configuration

This part about your JSON file confuses me:

My container reads from a JSON file to know what dependencies to inject:

Typically the reason why you have to configure your dependency injection container as such is because your type-hinted dependencies specify interfaces, rather than classes, and you need some config to map interfaces to concrete classes to build and inject. This is one way of granting finer-grained control over your injection process: in your testing environment you can tell the container about a different concrete class you would like it to inject when a particular interface is requested. However, that's not really what I see in your JSON configuration. In particular, I don't understand what the arguments array in your JSON configuration would be for. I would expect something much simpler: just a map from interface name to concrete class name. Maybe I'm misunderstanding your intent though.

Calling your method

I have a couple quick suggestions here, the first has to do with a potential bug that will probably be hard to find if you run into it in practice. The run method pulls together arguments from two different locations to execute the method being called: any arguments that come from the DI container via type-hinting, and any arguments passed in from the URL. The trouble is that your arguments aren't properly ordered. The analyze method which builds up an array of arguments resolved via the DI container skips any arguments which it can't find via type-hinting. These arguments are then added before any data that comes in from the URL. The result is that your arguments can easily end up out-of-order by the time you call call_user_func_array. Here is a laravel-esque controller method that (I think) will break when used with your current implementation:

class userController {
    public function update( int $userId, UserValidator $validator ) {
    }
}

When you try to route this via a URL like http://example.com/user/update/5 the run method will be called like run( [ $userControllerInstance, 'update' ], [ 5 ] ). So $args will be an array with one parameter: 5. After you call $this->container->resolve($method) the $params variable will be populated with a UserValidator instance. You then merge these two together with $args last. As a result, you will end up calling:

$userControllerInstance->update( $userValidatorInstance, 5 )

Our method specifies its parameters in the opposite order, so what you will end up with is a type error because the arguments don't match the type-hinting. Currently things will work fine as long as your DI-provided parameters come first, but eventually someone will break that rule and end up with a very hard to find error. You need to make sure your argument building respects the proper method order.

You also need to handle the possibility that you can't fulfill the requested arguments. For instance, what if the method is expecting two required parameters from the URL, but only one parameter comes in from the URL? That should return a 404 error, but I suspect that with your current implementation it will result in an actual PHP error. However, that depends on you code that matches URLs to routes, which isn't included in this code-review. So it may not be a problem.

This one is more of a nitpick, but as long as you are injecting your DI container, I would suggest a small tweak to better separate out your concerns. Your router needs to know a lot about the DI container in order to do its job: the two are tightly coupled. I would make a more generic method in your DI container for your router to call so that it doesn't have to know as much detail. As an example to get you moving in the right direction, imagine that your router worked exclusively with classes (i.e. skip closures for routers). You could imagine making a method like this:

$this->container->run( $class_name, $method_name, $args );

Then your router doesn't have to know anything at all. The DI container then becomes completely responsible for instantiating the object, building any dependencies needed for its constructor, and calling the method and providing any dependencies declared at the method-level. Presumably, you would make sure it is smart enough to mix in your additional arguments, per the above discussion. I would then also have the run method throw exceptions of various kinds (such as if a required argument wasn't passed, or if dependency can't be built for some reason). Your router can then catch those exceptions and either return a 404 (if a required argument was missing), or let your error/exception handler deal with other problems that the developer has to deal with.

Nitpicks

1. Your analyze method doesn't actually analyze anything: it converts a callable to a Reflection container. I would change its name to reflect this
2. Ditto your work method. That name doesn't give any good hints on what it actually does.
3. Depending on your needs I would probably just ditch the ability to attach closures directly to routes. IMO, this leads to people putting controllers in the routing configuration, a very poor separation of concerns. I always avoided that use-case with laravel, and I think that be allowing it you will just cause more trouble in the long run, both in terms of making your router more complicated, and making your application less organized.