(passing data around) Is this good practice for my personal

[hazel1919]

Hi all,

I have just put together a tentative proof of concept MVC framework proposal.

After many weeks of slogging through an array of PHP MVC tutorials and courses (some of which I paid for) I have decided to strip back all the frills, bells and whistles to try and put together a core proof of concept that uses best practices.

In this simplified example I am trying to test the viability of the framework itself... so there is no routing code or query string management and the controller and action are hard coded.

Here is the repository: https://github.com/JethroHazelhurst/psr-4-mvc

I like this framework because it:
 

1. Has clear seperation of concerns
2. Is not cluttered with static functions
3. Is clearly namespaced - thanks to the PSR-4 autoloading
4. Has a clear way of passing data to and from the model - e.g. $this->_view->foo = $this->_model->getBar();

Below is the directory structure:

 

 

and here is a print_r of my Object structure:

 

 

Questions

I am interested in hearing any feedback on the idea... I can't see any glaring issues at them moment. Some questions in the back of my mind are:

1. Are there any dangers in heavily depending on the parent::__construct() function to call parent classes?
2. In the controller, is passing data from Model to View using $this->_view->foo = $this->_model->getBar(); good practice?
3. Are there any dangers in using Namespaces?

I am also interested in reading up on MVC framework best practices so if anyone can recommend some resources I would be very grateful.

It seems like there are a million ways to write an MVC framework, I am confused as to which way is best practice.

EDIT: Hmm, can't seem to get a list to display here...

[Jacques1]

Right now, your project is really too trivial for proper feedback. You have models, controllers and views. Yeah, that's the basic structure of MVC – but nothing more.

 

All I can tell you is that you have several security vulnerabilities other issues in your code:

• The views and models are loaded from arbitrary paths without any kind of validation. This will quickly lead to local file inclusion vulnerabilities. Dynamic file paths must be validated and ideally whitelisted (compared with a fixed set of acceptable paths). Do not load random scripts into your application.
• (Ab)using PHP as a template engine is generally questionable. PHP templates are inherently insecure (because they may do absolutely anything), they often lead to spaghetti code, and they are incredibly ugly and limited. PHP is a template engine of the 90s. You should be using a modern library like Twig or Smarty.
• In your database class, you're using the default emulated prepared statements, which is a security problem and can lead to SQL injection vulnerabilities. It's also impossible to set the character encoding of the database connection (unless one accesses the underlying PDO instance). Disable emulation by default and add an encoding parameter.
• You shouldn't extend internal PHP classes like PDO. You may accidentally override a feature and screw up the inner workings. It also makes the framework very inflexible. What if I don't have PDO, only mysqli?
• You catch exceptions, print the error message on your site and then continue the script. That's a very bad idea. Not only will you leak internal information and irritate legitimate users with cryptic messages. You also put your program into an erroneous state. If the database connection just crashed, how is the application supposed to continue? Leave exceptions alone so that PHP can send the message to the correct(!) destination and abort the script in a controlled manner. Don't catch exceptions unless you know exactly why you're doing it.
• Your classes are currently hard-coded everywhere, making it impossible to really extend or test anything. Once you understand the basics of MVC, you should look into dependency injection.

 

Are there any dangers in heavily depending on the parent::__construct() function to call parent classes?

 

No.

 

 

 

In the controller, is passing data from Model to View using $this->_view->foo = $this->_model->getBar(); good practice?

 

See above.

 

 

 

Are there any dangers in using Namespaces?

 

No.

Edited July 4, 2016 by Jacques1

[hazel1919]

Hi Jaques, I want to really thank you for taking out the time to look over the code... with regard to security concerns... I have a routing system that works using predefined regular expressions to match routes, it works... what I am concerned about here is the overall architecture of my framework so to make it as clear as possible I have stripped out all of the components that I know work, such as routing. I should have made that clearer from the beginning.

 

Regarding your points.

 

1 - I have strip out everything except the minimal amount of code to display something, so controllers, methods, they are all hard coded for simplicity.

2 - I have actually implemented twig into the framework before, for now I am just learning the pure PHP method but it's definitely something I will do in the future, thanks!

3 - Good point, thanks.

4 - Great advice, will put into practice.

5 - I stripped out the error handling code for this example, but it basically writes errors to a log file and displays a generic 404 to the user.

6 - This is an interesting point...

 

Re: point six... dependency injection: I am confused as to how I should go about this. The framework works as is, but I would like to expand upon it down the road and don't want my code quality to degrade with each new feature. Your comment prompted me to delve into dependency injection most of yesterday.

 

Here Here is a flowchart of my simplified framework without dependency injection...

 

 

 

As I understand it, the main dependencies are...

Core\Router depends on Foo\Controller
FooController depends on Core\Controller (via the parent::__construct method)
Core\Controller depends on Core\View
Foo_Model depends on Core\model which depends on Core\Database

So I am a bit confused as to how I should use dependency injection here... is depending on parent::__construct like this making the framework too tightly coupled?

Thanks for the time you have spent on helping me already.

[hazel1919]

On further analysis it seems the only viable places to use dependency injection is in the Core\Controller (new View) and Core\Model (new Database) as the dependent classes are not related to the instantiated classes?

[Jacques1]

The dependencies which DI is supposed to avoid are hard-coded names of associated classes. This includes the ones you've mentioned, but there's also the Database class which has hard-coded references to constants of some Config class, making it impossible to use any other configuration mechanism. Class constants are particularly nasty, because they cannot be set dynamically at all. I cannot have an external configuration file, I cannot temporarily switch to different settings for unit tests. My only choice is to manually edit the constant values in your class.

 

With DI, you let the caller choose a class; they can even implement their own. You only create an interface to specify the expected methods of the class. In this particular case, however, you can just pass the connection parameters as arguments to the constructor. No need for any classes.

 

Besides that: I understand you currently want to keep your framework as simple as possible, but if you plan to use this for any real website in the future, I strongly recommend you write secure and correct code from the beginning. It's trivial to add features like DI at a later stage (which is why it was my last suggestion), but it's very hard to analyze an entire project for subtle vulnerabilities and try to fix them with ad-hoc solutions. Chances are you'll miss some of them and end up with actual security issues.

 

Whenever you encounter a critical problem like the local file inclusion vulnerability, fix it immediately. It's far more important than optimizing the framework architecture.