Skip to main content
Code Review

Return to Question

deleted 144 characters in body; edited tags; edited title
Source Link
Jamal
Jamal
  • 35.2k
  • 13
  • 134
  • 238

Yet another phpPHP MVC - form validation

So I'm working on my own "MVC" framework for learning purposes and for personal use. I use the "MVC" expression as a concept not a concrete design pattern. So I refer to MVC as a "separation concept" not a concrete thing since (mainly in the world of web) the implementation can be different in every framework.

I have concrete question (you can find it at the bottom of this long post - sorry for that) but first, I want to show you my current implementation. So here it is:

The htaccess rewrite rule says that everything (which is not a file or a directory) is passed to the index.php (bootstrap). A RouterRouter instance is created and asked to process the urlURL and render the page.

The specific ControllerController instance is instantiated by and its method is called by the RouterRouter. The RouterRouter extract the controller and action name from the urlURL. Example:

Example:

is "translated to":

I have a concrete ViewView class which is pretty simple, I can add, remove and get custom data which is used when the page is rendered, something like this:

The Model layer is the data representation and storage layer. I split up the persistancepersistence and representation. So I have a concrete class (like User) which represents a single user. I also have a UserServiceUserService class which is used to add/remove/modify/query user to/from the database or memory or anything (because it's an abstract class, different implementations can be created).

Because I want to write less boilerplate code I've created a simple "class inspector" class. It can be used to get (and parse) the doc comment of a class, its functions and fields. I use this inspector in the base Model class to "generate" automatic getters and setters. This place can be used to final-validate data. 

Code example:

The RouterRouter creates a new controller and call its action. The ControllerController creates the corresponding view instance and pass the model layer's data to it as a function parameter. The ViewView instance then loads the required template file, set the data which is required by the template (strong coupling).

I also created a ServiceContainerServiceContainer class which is used by the controllers. Because the Controller is instantiated by the Router I cannot use direct dependency injection and this was my solution for this problem.

Here comes a simple example of a one-way data flow. The UserControllerUserController creates the UserView and call its viewUser(id)viewUser(id) function which loads the required template, sets the "presentable" data and done.

1)

As you can see I totally separated the Model from the View Template, so the actual rendering does not know about the data in the Model layer. This looks good for me because of the separation. And also sometimes I want to preprocess the data before sending it to rendering (like creating the correct date format and so on) and this can be done in the View. Is this a good idea?

2)

The example shows a "one-way data flow" but what's about the forms? I have a register() function in the UserController as well. It asks the UserView to read the form data (and check if it was sent at all). I've done this in the UserView because the UserController does not know about the concrete form. Where should I validate the data? Before HTML5 the input fields of a form are sent as simple strings without restrictions (expect the radio buttons and these of course).

  1. As you can see I totally separated the Model from the View Template, so the actual rendering does not know about the data in the Model layer. This looks good for me because of the separation. I also sometimes want to preprocess the data before sending it to rendering (like creating the correct date format and so on) and this can be done in the View. Is this a good idea?

  2. The example shows a "one-way data flow" but what's about the forms? I have a register() function in the UserController as well. It asks the UserView to read the form data (and check if it was sent at all). I've done this in the UserView because the UserController does not know about the concrete form. Where should I validate the data? Before HTML5 the input fields of a form are sent as simple strings without restrictions (except the radio buttons and these of course).

I would put the validation in the ControllerController. If I do that in the ControllerController, I can check the incoming input with a pattern (eg. allow only letters and numbers for a username), check the length of the string (but this is determined by the model, isn't it?) and the most important aspect is that I can use the Model and Service directly to validate the data (eg. do not allow duplicated username).

Do you think it's a good idea to put the validation in the controller? My problem with the validation in the controller is that if the Model is (for some reason, so don't ask why) modified directly without using the Controller's function then the data wouldn't be validated.

Yet another php MVC - form validation

So I'm working on my own "MVC" framework for learning purposes and for personal use. I use the "MVC" expression as a concept not a concrete design pattern. So I refer to MVC as a "separation concept" not a concrete thing since (mainly in the world of web) the implementation can be different in every framework.

I have concrete question (you can find it at the bottom of this long post - sorry for that) but first, I want to show you my current implementation. So here it is:

The htaccess rewrite rule says that everything (which is not a file or a directory) is passed to the index.php (bootstrap). A Router instance is created and asked to process the url and render the page.

The specific Controller instance is instantiated by and its method is called by the Router. The Router extract the controller and action name from the url. Example:

is "translated to"

I have a concrete View class which is pretty simple, I can add, remove and get custom data which is used when the page is rendered, something like this:

The Model layer is the data representation and storage layer. I split up the persistance and representation. So I have a concrete class (like User) which represents a single user. I also have a UserService class which is used to add/remove/modify/query user to/from the database or memory or anything (because it's an abstract class, different implementations can be created).

Because I want to write less boilerplate code I've created a simple "class inspector" class. It can be used to get (and parse) the doc comment of a class, its functions and fields. I use this inspector in the base Model class to "generate" automatic getters and setters. This place can be used to final-validate data. Code example:

The Router creates a new controller and call its action. The Controller creates the corresponding view instance and pass the model layer's data to it as a function parameter. The View instance then loads the required template file, set the data which is required by the template (strong coupling).

I also created a ServiceContainer class which is used by the controllers. Because the Controller is instantiated by the Router I cannot use direct dependency injection and this was my solution for this problem.

Here comes a simple example of a one-way data flow. The UserController creates the UserView and call its viewUser(id) function which loads the required template, sets the "presentable" data and done.

1)

As you can see I totally separated the Model from the View Template, so the actual rendering does not know about the data in the Model layer. This looks good for me because of the separation. And also sometimes I want to preprocess the data before sending it to rendering (like creating the correct date format and so on) and this can be done in the View. Is this a good idea?

2)

The example shows a "one-way data flow" but what's about the forms? I have a register() function in the UserController as well. It asks the UserView to read the form data (and check if it was sent at all). I've done this in the UserView because the UserController does not know about the concrete form. Where should I validate the data? Before HTML5 the input fields of a form are sent as simple strings without restrictions (expect the radio buttons and these of course).

I would put the validation in the Controller. If I do that in the Controller, I can check the incoming input with a pattern (eg. allow only letters and numbers for a username), check the length of the string (but this is determined by the model, isn't it?) and the most important aspect is that I can use the Model and Service directly to validate the data (eg. do not allow duplicated username).

Do you think it's a good idea to put the validation in the controller? My problem with the validation in the controller is that if the Model is (for some reason, don't ask why) modified directly without using the Controller's function then the data wouldn't be validated.

Yet another PHP MVC form validation

I'm working on my own "MVC" framework for learning purposes and for personal use. I use the "MVC" expression as a concept not a concrete design pattern. So I refer to MVC as a "separation concept" not a concrete thing since (mainly in the world of web) the implementation can be different in every framework.

The htaccess rewrite rule says that everything (which is not a file or a directory) is passed to the index.php (bootstrap). A Router instance is created and asked to process the URL and render the page.

The specific Controller instance is instantiated by and its method is called by the Router. The Router extract the controller and action name from the URL.

Example:

is "translated to":

I have a concrete View class which is pretty simple, I can add, remove and get custom data which is used when the page is rendered, something like this:

The Model layer is the data representation and storage layer. I split up the persistence and representation. So I have a concrete class (like User) which represents a single user. I also have a UserService class which is used to add/remove/modify/query user to/from the database or memory or anything (because it's an abstract class, different implementations can be created).

Because I want to write less boilerplate code I've created a simple "class inspector" class. It can be used to get (and parse) the doc comment of a class, its functions and fields. I use this inspector in the base Model class to "generate" automatic getters and setters. This place can be used to final-validate data. 

Code example:

The Router creates a new controller and call its action. The Controller creates the corresponding view instance and pass the model layer's data to it as a function parameter. The View instance then loads the required template file, set the data which is required by the template (strong coupling).

I also created a ServiceContainer class which is used by the controllers. Because the Controller is instantiated by the Router I cannot use direct dependency injection and this was my solution for this problem.

Here comes a simple example of a one-way data flow. The UserController creates the UserView and call its viewUser(id) function which loads the required template, sets the "presentable" data and done.

  1. As you can see I totally separated the Model from the View Template, so the actual rendering does not know about the data in the Model layer. This looks good for me because of the separation. I also sometimes want to preprocess the data before sending it to rendering (like creating the correct date format and so on) and this can be done in the View. Is this a good idea?

  2. The example shows a "one-way data flow" but what's about the forms? I have a register() function in the UserController as well. It asks the UserView to read the form data (and check if it was sent at all). I've done this in the UserView because the UserController does not know about the concrete form. Where should I validate the data? Before HTML5 the input fields of a form are sent as simple strings without restrictions (except the radio buttons and these of course).

I would put the validation in the Controller. If I do that in the Controller, I can check the incoming input with a pattern (eg. allow only letters and numbers for a username), check the length of the string (but this is determined by the model, isn't it?) and the most important aspect is that I can use the Model and Service directly to validate the data (eg. do not allow duplicated username).

Do you think it's a good idea to put the validation in the controller? My problem with the validation in the controller is that if the Model is (for some reason, so don't ask why) modified directly without using the Controller's function then the data wouldn't be validated.

Rollback to Revision 1
Source Link
Jamal
Jamal
  • 35.2k
  • 13
  • 134
  • 238

Edit (2016/10/10) - response to Mike Brant's answer

That's a lot of comments, thanks. :) I try to reply for your questions/suggestions in the same order.

First of all, thanks for your effor for reading my long post and writing your own! :)

I don't plan to make this framework public, so I'm actually free to make restrictions. And one of these restrictions is to use only the POST HTTP action. The url is read from the server's REQUEST_URI data. I think the controller/action/parameters extracting from the url fits better for the MVC architecture. It also looks better. :)

Sorry for not posting namespace declarations. The StringHelper is in a namespace as well, I've just wrote a "use" in that file where I've used the StringHelper. I'm actually okay with the namespace usage. Maybe it's confusing with this small amount of code (and without directory structure).

Actually the better parameter validation is in my TODO list. :) (eg. the setData/setFile functions in the View class you've mentioned), but thanks for the reminder!

The reason for the "hacky" template file inclusion is that I really want to throw that exception since it's catched at higher level. I could set error handling as you suggested, but this "hack" (which is actually not that bad imho :D) does the trick without (noticable) overhead.

The UserService is used to add/delete/etc. user to/from the data storage as well, so I felt that it's more than a provider since deleting something from somewhere is not "providing" something.

I planned to "split up" the index.php and use the config files to declare things like the custom routing patterns and services as you've suggested. Thanks for pointing it out. If I do it right I don't have to touch the index.php file - just in some exceptional cases. However the config files have to be read somewhere, so 1) I have to make restrictions to the config file names or 2) I have to edit the index.php when I want to load a different config.

Anyway, this line of code

$services = new \Framework\ServiceContainer($config);

also breaks the Law of Demeter. Direct dependency injection is always better. So instead of passing a config to the constructor of the ServiceContainer, it's better to set the services by yourself. It's an important thing for testability and flexibility.

Well, yea... passing the ServiceContainer to the Router is not the fanciest solution because it breaks the Law of Demeter. The Router itself does not use the ServiceContainer directly but it passes it to the instantiated Controller's constructor. And because the Controller instantiation is inside the dispatch() function of the Router class (again which is not the best design since it's "hard to test"), I have to provide the registered services to the controllers somehow.

The dispatchAndRender function is more like a "helper" function. I have a separate dispatch() function - which extracts the controller/action/parameters from the url and instantiates the right controller - which also returns the instantiated controller. If the Router is not able to find an appropriate controller/action/parameters trio it throws an exception (more specifically a RouterException). If this happens I have to render a "Resource not found" view. If everything is okay, I can ask the controller to render it's selected view. And these things are done in the dispatchAndRender function. Here it is:

public function dispatchAndRender($notFoundView)
{
    if (!is_object($notFoundView) || !($notFoundView instanceof View)) {
        throw new Exceptions\TypeException("Invalid \"notFoundView\" parameter: must be a View instance");
    }

    try {
        $controller = $this->dispatch();
    } catch (Exceptions\RouterException $ex) {
        echo $notFoundView->render();
        return;
    } catch (\Exception $ex) {
        throw $ex;
    }

    $controller->render();
}

I want to point it out that I'm not using the HTTP error codes properly. So instead of manually rendering a 404 page if the Router is failed to find the controller/action/parameters trio, I could "redirect" the page with a 404 error code. And eg. the htaccess could be used to use custom error code pages, because I don't like the ugly, default error pages. :)

The reason for "setting the view twice" in the controller is that the setView() function is a protected function in the base class which sets a private variable. And because I can't access it in the subclass, I have to store it for myself. This is just a possible implementation of a controller. There might be a controller which use different views for each actions.

The View is strongly coupled with the templates, so the view does know that the template can handle the null user. Actually that is the way to say "user not found". :)

Anyway, most of the things you wrote are in the Application. The application uses the framework, so these errors/suggestions are out of the framework's scope. It's the application's job to make decisions. Is it really necessary to check if the passed $user is a User instance? I don't think so. Why? Because it's passed by the controller which is implemented by myself and I know that I've instantiated a User instance or simply left as a null. Of course you can hit your own balls, but why would you want to do it? It's easy to do that with typeless languages anyway... So my point is that if I want to kill my own application, of course I can do that. But why would I want to do that? I think the important part of an application is to validate data which is acquired from the user (or from "outside"), anything else is not (as) important. You have to learn trust yourself. :)

Right now you have your controllers directly instantiating the views, which probably be instantiated through the model

I'm not sure what do you mean here. This is simply not true. The model layer is used by the controller. The controller ask the model to do things the view has zero knowledge about the model. The view is a brainless renderer which gets some input data and creates an output from it.

I'm not sure about the Model-Controller role switch. In my opinion the Controller is like a glue which has access to both the model and the view layer.

then your model does all the heavy lifting and injects data into the appropriate view

You say that the Model should know about the View which equals to breaking the whole concept, and sorry I don't agree with you. I like to follow this concept:

enter image description here

Edit (2016/10/10) - response to Mike Brant's answer

That's a lot of comments, thanks. :) I try to reply for your questions/suggestions in the same order.

First of all, thanks for your effor for reading my long post and writing your own! :)

I don't plan to make this framework public, so I'm actually free to make restrictions. And one of these restrictions is to use only the POST HTTP action. The url is read from the server's REQUEST_URI data. I think the controller/action/parameters extracting from the url fits better for the MVC architecture. It also looks better. :)

Sorry for not posting namespace declarations. The StringHelper is in a namespace as well, I've just wrote a "use" in that file where I've used the StringHelper. I'm actually okay with the namespace usage. Maybe it's confusing with this small amount of code (and without directory structure).

Actually the better parameter validation is in my TODO list. :) (eg. the setData/setFile functions in the View class you've mentioned), but thanks for the reminder!

The reason for the "hacky" template file inclusion is that I really want to throw that exception since it's catched at higher level. I could set error handling as you suggested, but this "hack" (which is actually not that bad imho :D) does the trick without (noticable) overhead.

The UserService is used to add/delete/etc. user to/from the data storage as well, so I felt that it's more than a provider since deleting something from somewhere is not "providing" something.

I planned to "split up" the index.php and use the config files to declare things like the custom routing patterns and services as you've suggested. Thanks for pointing it out. If I do it right I don't have to touch the index.php file - just in some exceptional cases. However the config files have to be read somewhere, so 1) I have to make restrictions to the config file names or 2) I have to edit the index.php when I want to load a different config.

Anyway, this line of code

$services = new \Framework\ServiceContainer($config);

also breaks the Law of Demeter. Direct dependency injection is always better. So instead of passing a config to the constructor of the ServiceContainer, it's better to set the services by yourself. It's an important thing for testability and flexibility.

Well, yea... passing the ServiceContainer to the Router is not the fanciest solution because it breaks the Law of Demeter. The Router itself does not use the ServiceContainer directly but it passes it to the instantiated Controller's constructor. And because the Controller instantiation is inside the dispatch() function of the Router class (again which is not the best design since it's "hard to test"), I have to provide the registered services to the controllers somehow.

The dispatchAndRender function is more like a "helper" function. I have a separate dispatch() function - which extracts the controller/action/parameters from the url and instantiates the right controller - which also returns the instantiated controller. If the Router is not able to find an appropriate controller/action/parameters trio it throws an exception (more specifically a RouterException). If this happens I have to render a "Resource not found" view. If everything is okay, I can ask the controller to render it's selected view. And these things are done in the dispatchAndRender function. Here it is:

public function dispatchAndRender($notFoundView)
{
    if (!is_object($notFoundView) || !($notFoundView instanceof View)) {
        throw new Exceptions\TypeException("Invalid \"notFoundView\" parameter: must be a View instance");
    }

    try {
        $controller = $this->dispatch();
    } catch (Exceptions\RouterException $ex) {
        echo $notFoundView->render();
        return;
    } catch (\Exception $ex) {
        throw $ex;
    }

    $controller->render();
}

I want to point it out that I'm not using the HTTP error codes properly. So instead of manually rendering a 404 page if the Router is failed to find the controller/action/parameters trio, I could "redirect" the page with a 404 error code. And eg. the htaccess could be used to use custom error code pages, because I don't like the ugly, default error pages. :)

The reason for "setting the view twice" in the controller is that the setView() function is a protected function in the base class which sets a private variable. And because I can't access it in the subclass, I have to store it for myself. This is just a possible implementation of a controller. There might be a controller which use different views for each actions.

The View is strongly coupled with the templates, so the view does know that the template can handle the null user. Actually that is the way to say "user not found". :)

Anyway, most of the things you wrote are in the Application. The application uses the framework, so these errors/suggestions are out of the framework's scope. It's the application's job to make decisions. Is it really necessary to check if the passed $user is a User instance? I don't think so. Why? Because it's passed by the controller which is implemented by myself and I know that I've instantiated a User instance or simply left as a null. Of course you can hit your own balls, but why would you want to do it? It's easy to do that with typeless languages anyway... So my point is that if I want to kill my own application, of course I can do that. But why would I want to do that? I think the important part of an application is to validate data which is acquired from the user (or from "outside"), anything else is not (as) important. You have to learn trust yourself. :)

Right now you have your controllers directly instantiating the views, which probably be instantiated through the model

I'm not sure what do you mean here. This is simply not true. The model layer is used by the controller. The controller ask the model to do things the view has zero knowledge about the model. The view is a brainless renderer which gets some input data and creates an output from it.

I'm not sure about the Model-Controller role switch. In my opinion the Controller is like a glue which has access to both the model and the view layer.

then your model does all the heavy lifting and injects data into the appropriate view

You say that the Model should know about the View which equals to breaking the whole concept, and sorry I don't agree with you. I like to follow this concept:

enter image description here

reply to the first answer
Source Link
csisy
csisy
  • 178
  • 1
  • 7

Edit (2016/10/10) - response to Mike Brant's answer

That's a lot of comments, thanks. :) I try to reply for your questions/suggestions in the same order.

First of all, thanks for your effor for reading my long post and writing your own! :)

I don't plan to make this framework public, so I'm actually free to make restrictions. And one of these restrictions is to use only the POST HTTP action. The url is read from the server's REQUEST_URI data. I think the controller/action/parameters extracting from the url fits better for the MVC architecture. It also looks better. :)

Sorry for not posting namespace declarations. The StringHelper is in a namespace as well, I've just wrote a "use" in that file where I've used the StringHelper. I'm actually okay with the namespace usage. Maybe it's confusing with this small amount of code (and without directory structure).

Actually the better parameter validation is in my TODO list. :) (eg. the setData/setFile functions in the View class you've mentioned), but thanks for the reminder!

The reason for the "hacky" template file inclusion is that I really want to throw that exception since it's catched at higher level. I could set error handling as you suggested, but this "hack" (which is actually not that bad imho :D) does the trick without (noticable) overhead.

The UserService is used to add/delete/etc. user to/from the data storage as well, so I felt that it's more than a provider since deleting something from somewhere is not "providing" something.

I planned to "split up" the index.php and use the config files to declare things like the custom routing patterns and services as you've suggested. Thanks for pointing it out. If I do it right I don't have to touch the index.php file - just in some exceptional cases. However the config files have to be read somewhere, so 1) I have to make restrictions to the config file names or 2) I have to edit the index.php when I want to load a different config.

Anyway, this line of code

$services = new \Framework\ServiceContainer($config);

also breaks the Law of Demeter. Direct dependency injection is always better. So instead of passing a config to the constructor of the ServiceContainer, it's better to set the services by yourself. It's an important thing for testability and flexibility.

Well, yea... passing the ServiceContainer to the Router is not the fanciest solution because it breaks the Law of Demeter. The Router itself does not use the ServiceContainer directly but it passes it to the instantiated Controller's constructor. And because the Controller instantiation is inside the dispatch() function of the Router class (again which is not the best design since it's "hard to test"), I have to provide the registered services to the controllers somehow.

The dispatchAndRender function is more like a "helper" function. I have a separate dispatch() function - which extracts the controller/action/parameters from the url and instantiates the right controller - which also returns the instantiated controller. If the Router is not able to find an appropriate controller/action/parameters trio it throws an exception (more specifically a RouterException). If this happens I have to render a "Resource not found" view. If everything is okay, I can ask the controller to render it's selected view. And these things are done in the dispatchAndRender function. Here it is:

public function dispatchAndRender($notFoundView)
{
    if (!is_object($notFoundView) || !($notFoundView instanceof View)) {
        throw new Exceptions\TypeException("Invalid \"notFoundView\" parameter: must be a View instance");
    }

    try {
        $controller = $this->dispatch();
    } catch (Exceptions\RouterException $ex) {
        echo $notFoundView->render();
        return;
    } catch (\Exception $ex) {
        throw $ex;
    }

    $controller->render();
}

I want to point it out that I'm not using the HTTP error codes properly. So instead of manually rendering a 404 page if the Router is failed to find the controller/action/parameters trio, I could "redirect" the page with a 404 error code. And eg. the htaccess could be used to use custom error code pages, because I don't like the ugly, default error pages. :)

The reason for "setting the view twice" in the controller is that the setView() function is a protected function in the base class which sets a private variable. And because I can't access it in the subclass, I have to store it for myself. This is just a possible implementation of a controller. There might be a controller which use different views for each actions.

The View is strongly coupled with the templates, so the view does know that the template can handle the null user. Actually that is the way to say "user not found". :)

Anyway, most of the things you wrote are in the Application. The application uses the framework, so these errors/suggestions are out of the framework's scope. It's the application's job to make decisions. Is it really necessary to check if the passed $user is a User instance? I don't think so. Why? Because it's passed by the controller which is implemented by myself and I know that I've instantiated a User instance or simply left as a null. Of course you can hit your own balls, but why would you want to do it? It's easy to do that with typeless languages anyway... So my point is that if I want to kill my own application, of course I can do that. But why would I want to do that? I think the important part of an application is to validate data which is acquired from the user (or from "outside"), anything else is not (as) important. You have to learn trust yourself. :)

Right now you have your controllers directly instantiating the views, which probably be instantiated through the model

I'm not sure what do you mean here. This is simply not true. The model layer is used by the controller. The controller ask the model to do things the view has zero knowledge about the model. The view is a brainless renderer which gets some input data and creates an output from it.

I'm not sure about the Model-Controller role switch. In my opinion the Controller is like a glue which has access to both the model and the view layer.

then your model does all the heavy lifting and injects data into the appropriate view

You say that the Model should know about the View which equals to breaking the whole concept, and sorry I don't agree with you. I like to follow this concept:

enter image description here

Edit (2016/10/10) - response to Mike Brant's answer

That's a lot of comments, thanks. :) I try to reply for your questions/suggestions in the same order.

First of all, thanks for your effor for reading my long post and writing your own! :)

I don't plan to make this framework public, so I'm actually free to make restrictions. And one of these restrictions is to use only the POST HTTP action. The url is read from the server's REQUEST_URI data. I think the controller/action/parameters extracting from the url fits better for the MVC architecture. It also looks better. :)

Sorry for not posting namespace declarations. The StringHelper is in a namespace as well, I've just wrote a "use" in that file where I've used the StringHelper. I'm actually okay with the namespace usage. Maybe it's confusing with this small amount of code (and without directory structure).

Actually the better parameter validation is in my TODO list. :) (eg. the setData/setFile functions in the View class you've mentioned), but thanks for the reminder!

The reason for the "hacky" template file inclusion is that I really want to throw that exception since it's catched at higher level. I could set error handling as you suggested, but this "hack" (which is actually not that bad imho :D) does the trick without (noticable) overhead.

The UserService is used to add/delete/etc. user to/from the data storage as well, so I felt that it's more than a provider since deleting something from somewhere is not "providing" something.

I planned to "split up" the index.php and use the config files to declare things like the custom routing patterns and services as you've suggested. Thanks for pointing it out. If I do it right I don't have to touch the index.php file - just in some exceptional cases. However the config files have to be read somewhere, so 1) I have to make restrictions to the config file names or 2) I have to edit the index.php when I want to load a different config.

Anyway, this line of code

$services = new \Framework\ServiceContainer($config);

also breaks the Law of Demeter. Direct dependency injection is always better. So instead of passing a config to the constructor of the ServiceContainer, it's better to set the services by yourself. It's an important thing for testability and flexibility.

Well, yea... passing the ServiceContainer to the Router is not the fanciest solution because it breaks the Law of Demeter. The Router itself does not use the ServiceContainer directly but it passes it to the instantiated Controller's constructor. And because the Controller instantiation is inside the dispatch() function of the Router class (again which is not the best design since it's "hard to test"), I have to provide the registered services to the controllers somehow.

The dispatchAndRender function is more like a "helper" function. I have a separate dispatch() function - which extracts the controller/action/parameters from the url and instantiates the right controller - which also returns the instantiated controller. If the Router is not able to find an appropriate controller/action/parameters trio it throws an exception (more specifically a RouterException). If this happens I have to render a "Resource not found" view. If everything is okay, I can ask the controller to render it's selected view. And these things are done in the dispatchAndRender function. Here it is:

public function dispatchAndRender($notFoundView)
{
    if (!is_object($notFoundView) || !($notFoundView instanceof View)) {
        throw new Exceptions\TypeException("Invalid \"notFoundView\" parameter: must be a View instance");
    }

    try {
        $controller = $this->dispatch();
    } catch (Exceptions\RouterException $ex) {
        echo $notFoundView->render();
        return;
    } catch (\Exception $ex) {
        throw $ex;
    }

    $controller->render();
}

I want to point it out that I'm not using the HTTP error codes properly. So instead of manually rendering a 404 page if the Router is failed to find the controller/action/parameters trio, I could "redirect" the page with a 404 error code. And eg. the htaccess could be used to use custom error code pages, because I don't like the ugly, default error pages. :)

The reason for "setting the view twice" in the controller is that the setView() function is a protected function in the base class which sets a private variable. And because I can't access it in the subclass, I have to store it for myself. This is just a possible implementation of a controller. There might be a controller which use different views for each actions.

The View is strongly coupled with the templates, so the view does know that the template can handle the null user. Actually that is the way to say "user not found". :)

Anyway, most of the things you wrote are in the Application. The application uses the framework, so these errors/suggestions are out of the framework's scope. It's the application's job to make decisions. Is it really necessary to check if the passed $user is a User instance? I don't think so. Why? Because it's passed by the controller which is implemented by myself and I know that I've instantiated a User instance or simply left as a null. Of course you can hit your own balls, but why would you want to do it? It's easy to do that with typeless languages anyway... So my point is that if I want to kill my own application, of course I can do that. But why would I want to do that? I think the important part of an application is to validate data which is acquired from the user (or from "outside"), anything else is not (as) important. You have to learn trust yourself. :)

Right now you have your controllers directly instantiating the views, which probably be instantiated through the model

I'm not sure what do you mean here. This is simply not true. The model layer is used by the controller. The controller ask the model to do things the view has zero knowledge about the model. The view is a brainless renderer which gets some input data and creates an output from it.

I'm not sure about the Model-Controller role switch. In my opinion the Controller is like a glue which has access to both the model and the view layer.

then your model does all the heavy lifting and injects data into the appropriate view

You say that the Model should know about the View which equals to breaking the whole concept, and sorry I don't agree with you. I like to follow this concept:

enter image description here

Source Link
csisy
csisy
  • 178
  • 1
  • 7