Very small MVC login framework

Question

This is a very small MVC framework. I have tested this framework and it's working fine. I can do CRUD without any troubles. I am using Smarty to handle the view section. I still feel I can improve this framework and also I want to find out whether I'm doing something wrong here.

Could you please spend a moment to check this? Please tell me how I can improve this mini framework. Are there any errors or wrong implementations? Does anything violate OOP structure?

index.php

This is the 1^st file to run. It just do all the includes and initiate the bootstrap class.

<?php

session_start();
/*For debugging*/
error_reporting(E_ALL);
require_once('libs/core/Bootstrap.php');
require_once('libs/core/Model.php');
require_once('libs/core/View.php');
require_once('libs/core/Controller.php');
require_once('libs/core/Database.php');
require_once('libs/smarty/Smarty.class.php');
require_once('application/config/config.php');
/* Initiate Bootstrap */
**$bootstrap = new Bootstrap();**

?>

Bootstrap.php explodes the url('/') and gets the controller name and the method name. It loads the controller, loads the method, and passes the parameters (callControllerMethod()). Currently I can pass up to 3 parameters. If the controller/method is invalid, the index controller will run.

<?php

class Bootstrap {

    private $url = array();
    private $controller = null;
    private $controllerPath = 'application/controllers/'; // Always include trailing slash
    private $defaultFile = 'index.php';

    function __construct() {
        $this->url = $this->getUrl();
        if (empty($this->url[0])) {
            $this->loadDefaultController();
            return false;
        }
        $this->loadExistingController();
        $this->callControllerMethod();
    }

    private function getUrl() {
        $url = isset($_GET['url']) ? $_GET['url'] : NULL;
        $url = rtrim($url, '/');
        $url = filter_var($url, FILTER_SANITIZE_URL);
        $this->url = explode('/', $url);
        return $this->url;
    }

    private function loadDefaultController() {
        require_once($this->controllerPath . $this->defaultFile);
        $this->controller = new Index();
        $this->controller->index();
    }

    private function loadExistingController() {
        $file = $this->controllerPath . $this->url[0] . '.php';
        if (file_exists($file)) {
            require_once($file);
            $this->controller = new $this->url[0]();
        } else {
            die('404 Controller is missing!');
        }
    }

    private function callControllerMethod() {
        //Get array length
        $length = count($this->url);
        if($length > 1){
            if (!method_exists($this->controller, $this->url[1])) {
                die('404 Method is missing!');
            }
        }
        switch ($length) {
            case 5:
                //Controller->Method(Param1, Param2, Param3)
                $this->controller->{$this->url[1]}($this->url[2], $this->url[3], $this->url[4]);
                break;
            case 4:
                //Controller->Method(Param1, Param2)
                $this->controller->{$this->url[1]}($this->url[2], $this->url[3]);
                break;
            case 3:
                //Controller->Method(Param1)
                $this->controller->{$this->url[1]}($this->url[2]);
                break;
            case 2:
                //Controller->Method()
                $this->controller->{$this->url[1]}();
                break;
            default:
                $this->controller->index();
                break;
        }
    }
}
?>

Base View.php Initiate Smarty.

This sets the Smart dir paths, sets values to the template, and loads the content inside the base template. In the default_theme.tpl I'm calling {include file=$content} to load the custom content.

<?php
class View {

    protected $data;
    protected $smarty;

    function __construct() {
        $this->data = array();
        //View
        $this->smarty = new Smarty();
        $this->smarty->setTemplateDir('application/views/templates/');
        $this->smarty->setCompileDir('application/views/templates_c/');
        $this->smarty->setConfigDir('application/views/configs/');
        $this->smarty->setCacheDir('application/views/cache/');
        //$this->smarty->debugging = true;
    }
    public function set($key, $val) {
        $this->smarty->assign($key, $val);
    }
    public function render($view) {
        $this->smarty->assign('content', $view.'.tpl');
        $this->smarty->display('base/default_theme.tpl');
    }
}
?>

Base Controller.php

<?php 

class Controller {

    protected $view;
    protected $model;

    function __construct() {
        $this->view = new View();
        $this->model= new Model();
    }
}
?>

Base Model.php

I have my own Database.php (PDO class). I have included the model load function here (load). From the controller I can do $this->helpModel = $this->model->load('help_model');.

<?php
class Model{

    protected $db;

    function __construct() {
        $this->db = new Database(DB_TYPE, DB_HOST, DB_NAME, DB_USER, DB_PASS);
    }
    function load($name) {
        $path = 'application/models/'. $name .'.php';

        if (file_exists($path)) {
            require_once($path);
            return new $name();
        } else {
            die('Error: Model is not existing.');
        }
    }
}
?>

This is one of my sample controllers:

<?php 
class help extends Controller{

    private $helpModel;

    function __construct() {
        parent::__construct();
        $this->helpModel = $this->model->load('help_model');
    }
    function index(){
        //get data
        $data = $this->helpModel->selectAll();
        //set data
        $this->view->set('data',$data);
        $this->view->render('customer/cust');
    } 
    public function addUser(){
        if($_POST){
            extract($_POST); 
            //testing insert  
            $data2["name"] = 'asdasdsd';
            $data2["email"] = 'asdasdsd';
            $data2["pass"] = 'asdasdsd';
            $data2["added"] = date("Y-m-d");
            $this->helpModel->create($data2);
        }
        $this->index();
    }
    function other($val = false) {
        echo "inside other function<br/>";
        echo "value $val <br/>";
        //$this->index();
    }

    function another($val1,$val2) {
        echo "inside other function<br/>";
        echo "value $val1 and $val2 <br/>";
        //$this->index();
    }
}
?>

My sample Model:

<?php
class help_model extends Model {

    function __construct() {
        parent::__construct();
    }
    public function selectAll() {
        $results = $this->db->selectAll("users");
        return $results;
    }
    public function create($data) {
        $values = array(
            "admin_name"=>$data["name"],
            "admin_email"  =>$data["email"],
            "admin_pass" =>$data["pass"],
            "admin_added" =>$data["added"]
        );
        $results = $this->db->insert("admins",$values);
        return $results;
    }
}
?>

Sample View

<h3>Add Record</h3>
<form action="{$smarty.const.BASE}help/addUser" method="post">
    <table>
        <tr>
            <td><label for="name">Name:</label></td>
            <td><input type="text" name="name" /></td>
        </tr>
        <tr>
            <td><label for="email">Email:</label></td>
            <td> <input type="text" name="email" /></td>
        </tr>
        <tr>
            <td colspan="2">
                <input class="btn btn-success" type="submit" value="Submit" name="submit"/>
                <button class="btn">Cancel</button>
            </td>
        </tr>
    </table>
</form>
<br/>
<table class="table">
    <thead>
        <tr>
            <th>Stauts</th>
            <th>Name</th>
            <th>Email</th>
            <th>Token</th>
            <th>Option</th>
        </tr>
    </thead>
    <tbody>
        {foreach from=$data item=foo}
            <tr>
                <td>{$foo['user_status']}</td>
                <td>{$foo['user_name']}</td>
                <td>{$foo['user_email']}</td>
                <td>{$foo['user_token']|truncate:40}</td>
                <td><a href="">Edit</a> /<a href="">Delete</a></td>
            </tr>
        {/foreach}
    </tbody>
</table>

Database.php

<?php

class Database extends PDO {

    public function __construct($DB_TYPE, $DB_HOST, $DB_NAME, $DB_USER, $DB_PASS) {
        $options = array(
            PDO::ATTR_PERSISTENT => true,
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
        );
        try {
            parent::__construct($DB_TYPE.':host='.$DB_HOST.';dbname='.$DB_NAME, $DB_USER, $DB_PASS, $options);
        } catch (PDOException $e) {
            die($e->getMessage());
        }
    }
    public function selectAll($table, $fetchMode = PDO::FETCH_ASSOC) {
        $sql = "SELECT * FROM $table;";
        $sth = $this->prepare($sql);
        $sth->execute();
        return $sth->fetchAll($fetchMode);
    }
    public function select($sql, $array = array(), $fetchMode = PDO::FETCH_ASSOC) {
        $sth = $this->prepare($sql);
        foreach ($array as $key => $value) {
            $sth->bindValue("$key", $value);
        }
        $sth->execute();
        return $sth->fetchAll($fetchMode);
    }
    public function insert($table, $data) {
        ksort($data);
        $fieldNames = implode('`, `', array_keys($data));
        $fieldValues = ':' . implode(', :', array_keys($data));

        $sth = $this->prepare("INSERT INTO $table (`$fieldNames`) VALUES ($fieldValues)");
        print_r($fieldValues);
        foreach($data as $key => $value) {
            $sth->bindValue(":$key", $value);
        }
        $sth->execute();
    }
    public function update($table, $data, $where) {
        ksort($data);
        $fieldDetails = NULL;
        foreach ($data as $key => $value) {
            $fieldDetails .= "`$key`=:$key,";
        }
        $fieldDetails = rtrim($fieldDetails, ',');
        $sth = $this->prepare("UPDATE $table SET $fieldDetails WHERE $where");
        foreach ($data as $key => $value) {
            $sth->bindValue(":$key", $value);
        }
        $sth->execute();
    }
    public function delete($table, $where, $limit = 1) {
        return $this->exec("DELETE FROM $table WHERE $where LIMIT $limit");
    }
}

Can I just check isUserLoggedIn() in the Bootstrap.php class inside the __construct function? If logged in, run the custom controller. If not, run the default controller.

Answer 1

There are several areas that could be improved:

1. Stop calling it "MVC framework". There is no model, because model is a layer, not an abstraction for DB table. There is no controller, because controller is not responsible for passing data, rendering template and initializing DB abstractions.
2. Learn about autoloading in PHP. Look up spl_autoload_register() and how it is used.
3. Never perform any computation in the constructors. Those should be used only for assigning values, otherwise the code is difficult to test.
4. Learn to write unit tests.
5. Learn how to (and when) to use prepares statements. Also, please actually learn how to use PDO. The code does is using emulated prepared statements, which removes any protection against injections.
6. Apply S.O.L.I.D principles to the codebase, as there are numerous violations throughout.
7. Avoid global state in form of static structures or constants.
8. Most of the code is tightly coupled to to specific class names. Instead learn about dependency injection.
9. Controllers in MVC are not autoloaders for Model layer structures.

Answer 2

Well, I haven't read through all of the code yet, but I couldn't help noticing that you're using a lot of hard-coded paths, and your index.php is just requires the entire FW.
If I were to use your code, in combination with some components, taken from Symfony, or some other FW, I might encounter some problems.

The easy way to solve this is to register an autoloader, set a constant, containing a base path, and using namespaces for your FW's classes.
Consider storing your classes in libs/MyFW/, and adding all classes in the _Core_directory (ucfirst your dirs, too) this first line:

namespace MyFW\Core;

The files in Smarty, of course, start with:

namespace MyFW\Smarty;

Then use (or write your own) autoloader, I suggest using Symfony2's UniversalClassLoader, it's quite good.

Then change the index.php to:

<?php
use Symfony\Component\ClassLoader\UniversalClassLoader;
use MyFW\Core\Bootstrap;

defined('MY_FW_BASE') ||
    define('MY_FW_BASE', realpath(dirname(__FILE__).'../'));
//the MY_FW_BASE should be your document root
defined('LIBS_PATH') || 
    define('LIBS_PATH', realpath(MY_FW_BASE.'/libs/'));

 //this is the only require you'll ever need
require_once(LIBS_PATH.'/Symfony/Component/ClassLoader/UniversalClassLoader.php');
set_include_path(
    LIBS_PATH.PATH_SEPARATOR.
    get_include_path()
);
$loader = new UniversalClassLoader();
$loader->useIncludePath(true);
$loader->registerNamespaces(
    array(
        'Symfony'       =>  LIBS_PATH,
        'MyFW'          =>  LIBS_PATH
    )
);
//suppose you want to use ZendFW 1.x, store it in libs/Zend, and add this line:
$loader->registerPrefix('Zend_', LIBS_PATH);
$loader->register();//register the autoloader, job done

$bootstrap = new Bootstrap;

Note that using namespaces, if you want to access the core objects in some namespace, or access globaly defined constants, you'll have to use a \ to indicate the object or constant was defined in the global namespace:

namespace MyFW\Core;
$pdo = new \PDO(\GLOBAL_PDO_STRING);

Or

namespace MyFW\Core;
use \PDO,
    \PDOException;
$pdo = new PDO(\GLOBAL_PDO_STRING);

You can avoid name-conflicts this way, easily:

namespace MyFW\Core
use OtherNS\Database as OtherDB;
class Database extends OtherDB
{
}

We now have 2 Database classes, one extending the other, but in the namespace of the MyFQ\Core\Database, the other Database is called OtherDB, so there's no conflict.
Read up on Namespaces in PHP, when writing a framework, they're quite handy indeed.

You're also extending from PDO, which in itself is fine, but I tend to say that extending an object you don't control isn't really good practice, just create a wrapper object if needs must.

I'll update this answer along the way, if I think of something else whilst reviewing your code