MVC Structured CMS

Question

I'm creating a MVC structured content management system as both a means to learn better OOP and redesign my tacky website to me more modular and simplistic. My aim is to approach everything with regard for minimalism so I want all my code to be precise and to the point. As this is my first go at both MVC and true OOP php I'd like to get some feedback on my progress thus far. Below is a sample of my websites blog code, this is been my main area of focus and is where I try out new code before implementing them elsewhere. If other snippets of code are required I'll append them at the bottom.

Controller - controllers/Blog.php

<?php

class Blogs extends AdminController {

    function __construct(Request $request, $config) {
        parent::__construct($request, $config);
    }

    function show() {
        $blog = new Blog();

        $post['title'] = "TITLE";
        $post['id'] = "IDENT";

        $blog->newPost(1, $post);

        $content['blogs'] = $blog->get_where();
        $al = new Post();
        $content['posts'] = $al->get_where();
        $this->view->load('content', 'blogs/list', $content);
        $this->view->render("theme_admin");
    }

    function edit() {
        $id = $this->request->segment(2);
        $al = new Post($id);
        $content['post'] = $al;
        $this->view->load('content', 'blogs/edit', $content);
        $this->view->render("theme_admin");
    }

    function save() {
        $al = new Post();
        $_POST['title'] = htmlspecialchars($_POST['title'], ENT_QUOTES);
        $_POST['content'] = htmlspecialchars($_POST['content'], ENT_QUOTES);
        $_POST['likes'] = htmlspecialchars($_POST['likes'], ENT_QUOTES);
        $al->save($_POST);

    //  redirect('admin/blogs');
    }

    function delete() {
        $id = $this->request->segment(2);
        $al = new Post($id);
        $al->delete();
        last_page();
    }

}

View - views/blog/list.php

<div class="box">
    <div class="box-header">
        <h1>Typography</h1>
    </div>
    <div class="box-content">
    <table>
    <thead>
        <tr>
            <th>ID</th>
            <th>Blog</th>
            <th>Title</th>
            <th>Content</th>
            <th>Likes</th>
            <th>Actions</th>
        </tr>
    </thead>
    <tbody>
<? if ($posts): ?>

<? foreach ($posts as $post) : ?>
    <tr>
        <td><?= $post->id ?></td>
        <td><?= $post->blogID ?></td>
        <td><?= $post->title ?></td>
        <td><?= $post->content ?></td>
        <td><?= $post->likes ?></td>
        <td>
            <a href="<?= url('admin/blogs/edit/' . $post->id) ?>" class="button plain">Edit</a>
            <a href="<?= url('admin/blogs/delete/' . $post->id) ?>" class="button plain">Delete</a>
            <a href="<?= url('blogs/' . $post->id) ?>" class="button plain">View</a>
        </td>
    </tr>  
<? endforeach; ?>

<? endif; ?>
    </tbody>
</table>

<div class="action_bar">
    <a href="<?= url('admin/blogs/edit') ?>" class="button blue">Add New Post</a>
</div>
</div>


<? if ($blogs): ?>

<? foreach ($blogs as $blog) : ?>
    <tr>
        <td><?= $blog->id ?></td>
        <td><?= $blog->title ?></td>
    </tr>  
<? endforeach; ?>

<? endif; ?>



<form enctype="multipart/form-data" action="<?= url('admin/blogs/upload') ?>" method="post">
<input type="hidden" name="MAX_FILE_SIZE" value="2000000" />
<p>File to upload <input name="userfile" type="file" />
<input type="submit" name="send" value="Upload File" /></p>
</form>

Models models/Blog.php

<?php

    class Blog extends Model {
}

Right now there's no actual unique code for the blog, everythings simply loaded through the main model class below. This is something that I've probably messed up so I'd love to get feedback here

Models lib/Model.php

<?php

require LIB . 'Database.php';

class Model {

    public $model;
    public $table;
    public $fields;
    public $db;

    function __construct($id = null) {
        $this->db = new Database();
        $this->model = get_class($this);
        $this->table = strtolower($this->model) . 's';
        $this->init_fields($id);
    }

    function init_fields($id = null) {
        $this->fields = $this->fields();
        if (!empty($id)) {
            $this->get($id);
        } else {
            foreach ($this->fields as $field) {
                $this->{$field} = null;
            }
        }
    }

    function fields() {

        $res = $this->db->query('SHOW COLUMNS FROM ' . $this->table);
        $fields = array();
        while ($row = $res->fetch()) {
//          var_dump($row);
            $fields[] = $row->Field;
        }
        return $fields;
    }

    function get($id) {
        $res = $this->db->query('SELECT * FROM ' . $this->table . ' WHERE id=?', array($id));
        $row = $res->fetch();
        if ($row) {
            foreach ($row as $key => $val) {
                $this->{$key} = $val;
            }
        }

        return $this;
    }

    function exists() {
        if (!empty($this->id)) {
            $res = $this->db->query('SELECT * FROM ' . $this->table . ' WHERE id=?', array($this->id));
            return $res->num_rows() ? true : false;
        }
        return false;
    }

    function get_where() {
        $res = $this->db->query('SELECT * FROM ' . $this->table);
        $arr = array();
        while ($row = $res->fetch()) {
            $arr[] = $row;
        }
        return $arr;
    }


    function save($arr = null) {

        //append values from assoc array
        if ($arr) {
            foreach ($this->fields as $key) {
                if (array_key_exists($key, $arr)) {
                    $this->{$key} = $arr[$key];
                }
            }
        }

        $update_arr = array();
        $update_str = array();
        foreach ($this->fields as $key) {
            if (!empty($this->{$key})) {
                $update_str[] = "$key = ?";
                $update_arr[] = $this->{$key};
            }
        }

        $sql = ' SET ' . implode(',', $update_str);

        if ($this->exists()) {
            $sql = "UPDATE " . $this->table . $sql . " WHERE id=?";
            $update_arr[] = $this->id;
        } else {
            $sql = "INSERT INTO " . $this->table . $sql;
        }

        $res = $this->db->query($sql, $update_arr);
        if (empty($this->id)) {
            $this->id = $res->insert_id();
        }

        return $res->affected_rows();
    }

    function delete() {

        if( isset($this->id) ) {
        $res = $this->db->query('DELETE FROM ' . $this->table . ' WHERE id=?', $this->id);
        return $res->affected_rows();
        }
        return false;
    }

}

Answer 1

Woah boy, sorry about the wall.

View

I'll start with the easiest section first to get it out of the way. Short tags should be avoided, even in views. Not all servers support them and its just easier to write it out the first time rather than worry about something being unsupported in the future. There are ways of changing this setting via a file, but again, not all servers allow that file to be modified. If you can guarantee that your server will always support these, then I guess it wont be an issue, but this is typically avoided and many developers will flag you for it (though many wont).

If you can guarantee that $posts and $blogs are always set, which it appears that you can since you aren't explicitly doing an isset() check, then you can just call the foreach loop without needing the if statement. You'll need to ensure that they are always at least an empty array. Foreach can't loop over an empty array so it will just skip it. Some people would also complain that you are using separate PHP tags to escape the if and foreach blocks. I'm not going to say one way or the other as I agree with your method of separating them, just figured I'd mention it.

Controller

When defining a class, you should always specify the access level of your properties and methods (public, private, protected). By default it sets everything to public, which isn't always desirable. But that isn't the big issue here, well, not the only big issue. Eventually PHP is going to deprecate support for the "default" property/method declarations and it will be mandatory (as it should have been from the beginning). Plan for the future and make sure your code is compatible now. Besides, its just good coding practice.

When extending a class, it is not necessary to redefine an inherited method unless you are going to do something extra with it. As it stands this is just redundant. I show an example of how to properly override the contructor a little later.

Your method names are a little misleading, not just here, but in the other files too. If we follow the Single Responsibility Principle, as we should with OOP, then we know that a method which says they are going to "show" us something should only be concerned with "showing" us something. You have it creating a new blog, new post, and setting up the view variables. All of this is necessary to render the final view, but its not necessary for the show() method to be concerned with how each part is done. It just needs it done. So you should delegate these tasks to the right methods.

There's another issue in your show() method. The $post array came out of no where. It was never defined, it just started getting indices defined to it. I hope its just that you didn't know any better rather than that you are using globals. If the later, stop immediately and ask for help on that subject specifically before proceeding, you should never use globals. If the former, keep reading. The reason this works is because PHP is so lenient and allows you to create things on the fly. Sometimes this is good, sometimes bad. Its bad in this situation. I believe you may even be receiving silent warnings in the background. But what would happen if you had mistakenly defined the $post variable as a string earlier? Or this was implemented in an environment with a global $post (which I just said were bad, this being one of the reasons)? I'm honestly not sure, but I would imagine chaos would ensue. I know with numerical indices you can read and write to strings using the array syntax, but I don't know how associative array syntax would work. With the globals you would have similar issues. Always define your arrays before using them, this isn't as important with variables, but even those should be defined before attempting to modify them. The proper way to initialize an associative array with predefined data is like so:

$post = array(
    'title' => 'TITLE',
    'id' => 'IDENT'
);

Now, if you were to delegate these tasks like I mentioned earlier, this would become even easier. Imagine the above code was in one of those new methods. You wouldn't always want to use the same title and id, so you would pass those as parameters. And an easy way to get parameters or variables into an associative array using their names as keys, is to use compact().

public function createPost( $title, $id ) {
    $this->blog->newPost( 1, compact( 'title', 'id' ) );
}

Now, I introduced something new above, the $this->blog property. Another key principle of OOP is the "Don't Repeat Yourself" (DRY) Principle. The name is pretty self explanatory. It usually refers to duplicate code being refactored to functions/methods. This is similar. This principle goes hand-in-hand with the first one I mentioned. If your methods follow the first principle, they are ready to accept DRY code. So, all these methods where you are creating and recreating the same objects can more easily be done by implementing them as class properties. For instance, if we define a new Post() in our constructor and set it as a class property, then we can reuse that property anywhere throughout the rest of the class simply by referring to it by $this->. It doesn't have to be in the constructor, this just ensures that it has been initialized before we try using it.

private
    $blog,
    $al
;//define your properties

public function __construct( Request $request, $config ) {
    parent::__construct( $request, $config );

    $this->blog = new Blog();
    $this->al = new Post();
}

function show() {
    //$blog = new Blog();//unnecessary now
    //your code
    $this->blog->newPost( 1, $post );
    //$al = new Post();//unnecessary now
    $content[ 'posts' ] = $this->al->get_where();
    //your code
}

I would suggest not directly modifying the POST data. If you want to clean and sanitize, by all means do, but make a copy of it and change that. I'm not entirely sure, but I believe some browsers allow you to access this information. So POST information, which is normally private, would be available with any changes we made to it. While this may not be too important with sanitizing, it is important if you do something like, hash and salt a password. There are other reasons, such as data integrity. Perhaps a more efficient way to go about this would be to use the filter_input_array() function PHP provides (assuming your PHP version >= 5.2). I've played with the filter_input() function, but haven't used the other yet, so I don't want to demonstrate something I'm not 100% on. See the documentation and google if you are curious.

Model

A parent class should declare only the base functionality necessary to do something. A child class should extend the parent by adding more specific functionality. For instance, a database class would only have functionality available to all databases (add, delete, find, etc...). A SQL database would specifically define how the parent functionality would work for SQL and would add any additional functionality it would need. How does this relate to your code? Well, if all of your models use a SQL database, the SQL specific functionality is reasonable, but, depending on the scope of this project, you might consider defining a new SQL class that extends the database class as I described above, then have your models extend the SQL class. That way, if you ever decide to change the type of database you are using, you can do so with very little refactoring. Of course, you'll probably want to rename the SQL database to something more generic, that way you won't have to change all the "extends".

There are so many minor things here that would just help with code flow. For instance, don't create something you aren't going to use. In your init_fields() method you define the $fields property, but then only use it if the $id is empty. Move that into the else statement where it belongs. And you shouldn't be checking if $id is empty but if it isset(). Unless its an array, but then you should have the default be an empty array not null. And you shouldn't use the "not" ! operator unless its necessary. Here you have an else clause, just switch the conditions around and lose the "not". Although isset() will probably do this for you. Just remember it for future code.

Variable variables are mostly considered bad. If you are confused as to what a variable variable is, see below. Sometimes, such as in controllers, it is acceptable. But in other cases you want to avoid it. I didn't see you doing it elsewhere, but I just wanted to make sure you understood that. A more common way is to use extract() in your render method just before you include the view. This is the companion method to the compact() function I explained earlier, it dumps all the array keys/values into the local scope as variables with the keys for names and the values as the new values for those variables.

$this->{$field} = null;
//Could also be rewritten like this
$this->$field = null;
//More common way
extract( $this->fields );
include $templateFile;

Method get() is oddly named, it isn't getting anything. It is setting quite a bit. But get and set should be avoided as method names altogether. They could be confused with the magic __get() and __set() methods. Again, the if statement is unnecessary, I believe the fetch method returns an empty array if no results were found (may be wrong). Returning $this is only useful for chaining methods together or returning the entire object so that it can be cloned. You are doing neither here so the return is one unnecessary and two inefficient.

Conclusion

Now, I stopped right about here because I've already written quite a bit and I was starting to feel lost. I'm not really sure about your framework because none of it looks exactly right. I tried covering specific problems in each section, but there are some overall problems I did not address.

You've got your controller, which is ALMOST a controller, but relies on other classes to render the page for it. It appears that you have a "master" controller for a set of "subcontrollers" that each focuses on a different view. This is backwards from how it should be. Traditionally, you would cut out the "master" controller and just use the "subcontrollers" directly. Any code you find yourself needing in multiple controllers can be added to a parent controller for that group of pages, and any code you need in ALL controllers would be in the "base" controller class. As it is, each page can potentially render any other page just by calling a specific method. This is bad for two reasons. First because this adds unnecessary overhead to each view using this controller. Second, because your controllers should all be set up the same. Controllers typically have two constant methods. A "main" or "init" method that initiates the specific session needed before calling the second constant method, the render method, which extracts the variables for the view before including it. And then any helper methods you may need to help interface your controller with your model.

You've got your model, which COULD be a model, but it is too specific to the application. For one, it is doing things that the controller should actually be doing such as setting the variables for the view. Think of a model as the interface to your database. It should accept commands from the controller with the parameters necessary to complete that command. The commands should be common tasks associated with databases, such as add(), delete(), update(), etc... In other words, the model is only concerned with reading and writing data, not with how it is displayed or where it is coming from (though to a lesser extent).

Only your views look right. Though, as I said, the preferred method is to extract the variables so that the view is not concerned with the class being instantiated.

When starting off on such a large task you should start small and expand gradually. Instead you started large and now you are having to go back to the smaller things. I don't know about you, but that a very daunting task. When I make MVC frameworks, I start off with the views because they are the easiest and provide immediate results. I create the page as I want it to look, and define all the variables I think I'm going to need. I say think, because I always find I either don't need as many as I think I do, or because I need more. In other words, be prepared for things to change. Then I create a skeleton Controller. The first thing I do in the new controller is create the init() method. Inside this method I hard code all the variables my view will need into a $data array property. Then I create the render() method to extract() my $data array into the local scope and include the view. After that I work on getting my model fleshed out. Once the model is completely done I go back to the controller and remove all those hard coded variables, one at a time, and replace them with the proper sequences necessary to retrieve them from the Model. This way, at the conclusion of each step, I have a fully functional test I can touch and tweak as the code progresses.

Again, sorry for the long wall of text. I got a bit carried away. Hope it helps!