The general setup looks awesome
There’s some nice separation of concerns, and the flow looks good.
I don’t really like the communication through public properties though as that couples classes together and can lead to hard to debug errors, I would suggest changing that to returning response objects/arrays.
Basically instead of keeping state in services and then passing those services for other services to read that state, keep state completely out of the services and pass state around as method arguments instead, using the output of one service as an input for the next.
Let’s go through all classes one by one.
Application
This looks fine, however I would not call $this->router->route()
in the constructor, as it’s generally considered bad practice to actually do stuff in constructors, as you can never instantiate the object without stuff happening - which is fine in this case, but take for example a database class, if it connects to the database when you instantiate it, but then you never use it, you’ve made that connection for nothing.
Anothing thing is that I wouldn’t create a response in the Application
yet, as a Response
is an excellent candidate for an Immutable object. You create it once and then you never have to change it, makes it a lot easier to debug stuff.
So then the constructor would just be:
public function __construct() {
$this->request = new Request();
$this->router = new Router();
$this->dispatcher = new Dispatcher();
}
For more flexibility I would create interfaces for the Router
and the Dispatcher
and optionally accept those in the constructor:
public function __construct(RouterInterface $router, DispatcherInterface $dispatcher) {
$this->request = new Request();
$this->router = $router ?: new Router();
$this->dispatcher = $dispatcher ?: new Dispatcher();
}
That way you get the default classes when you do new Application()
but you could override the Router
or Dispatcher
if you wanted to - or both.
These interfaces may look something like this:
interface Router
{
public function route(Request $request): array;
}
interface Dispatcher
{
public function dispatch(array $route): Response;
}
I would change the run
method of the Application
as well, but for that we’d need to discuss the Router
and the Dispatcher
first.
Router
You haven’t posted the code for your Router
, but from the looks of it it takes Request
as argument and then sets some public properties on the Router
. I would recommend against that. What if you want to support sub-request at some point, how are you going to separate the two? It’s going to be a real mess.
Instead I would do what I suggested in my RouterInterface
above and return an array with routing information from the Router.
Such an array may look like:
[
'controller' => [FooBar::class, 'someAction'],
'params' => [
'foo' => 'bar',
]
]
And then instead of passing the entire Router
to the dispatcher, you would only need to pass it this array, decoupling the implementation of the Router
from the Dispatcher
Dispatcher
Since we’ve changed the router, we need to change the Dispatcher
as well to take these changes into account:
public function run(array $route, Response $response)
{
[$controller, $action] = $route['controller'];
if (!method_exists($controller, $action)) {
throw new RuntimeException('Unable to route '.$controller.'::'.$action.' - no such method exists');
}
// may want to think about constructor arguments for the controller here
// maybe use a Dependency Injection Container
$instance = new $controller();
$instance->response = $response;
call_user_func_array([$instance, $action], $params);
}
But, remember the bit about using an immutable response? That would the change this bit to look like this:
public function run(array $route): Response
{
[$controller, $action] = $route['controller'];
if (!method_exists($controller, $action)) {
throw new RuntimeException('Unable to route '.$controller.'::'.$action.' - no such method exists');
}
// may want to think about constructor arguments for the controller here
// maybe use a Dependency Injection Container
$instance = new $controller();
$response = call_user_func_array([$instance, $action], $params);
return $response;
}
So now instead of passing the controller an existing Response
we expect the Controller
to create a Response
and return it to us. Again, when you start doing sub-requests this will make your life a lot easier than trying to use one global Response
object.
Response
To make the Response
actually immutable, change it like so:
class Response{
private $body;
public function __construct(string $body)
{
$this->body = $body;
}
/* Send
* Outputs the view.
=================================================== */
public function send() {
echo $this->body;
}
}
Controller
Now the controller changes to this:
protected function renderView(string $view, $viewData = []) {
//Instantiate a new view
$view = new View($view, $viewData);
//Create and return a Response
return new Response($view->render());
}
Application (Continued)
And lastly we can change the run
method of the Dispatcher
:
public function run() {
/* Call the run method from the dispatcher
* Takes the Router and Response objects for use in the method. */
$response = $this->dispatcher->run($this->router->route($this->request), $this->response);
/* Send the reponse
* Outputs the view. */
$response->send();
}
Additionally you could instantiate the Request
outside of the Application
and then pass it to the Application::run
method instead.
And then that’s it
Basically all these changes boil down to: instead of setting properties on objects and then use those properties in other objects, pass around result objects/arrays instead. That lowers coupling between classes and improves encapsulation. And as a result the code will be easier to understand, because the result of calling a method on a service is no longer dependent on the state of the service, so less information to keep track off. And also it will be much easier to test using automated testing.