Template engine

I've just completed coding my template engine. I'm willing to have a review on how to increase the quality, efficiency, elegance and performance of the code, perhaps by shortening the code, etc. I'm also open for suggestions on other ways of doing this. It would be really appreciated if you point out every tiny bit of things, as I am willing to modify and improve the code accordingly.

Model class:

site_url = $global['site_url'];
        $this->theme    = $global['theme'];
    }
    public function createParam($key, $value) {
        $this->params['{' . $key . '}'] = $value;
    }
    public function replaceParams($content) {
        foreach($params as $key => $value) {
            str_replace($key, $value, $content);
        }
        return $content;
    }
}

View class:

model      = $model;
        $this->controller = $controller;
    }
    public function handlePageLoad($page_name) {
        $this->path = $model->site_url . "/app/tpl/skins/" . $model->theme . "/" . $page_name . ".php";
        if($controller->pageExists($this->path) === true) {
            $this->content = $controller->createPage($this->path);
            return $this->content;
        } else {
            $this->newPath = $model->site_url . "/app/tpl/skins/" . $model->theme . "/404.php";
            $this->content = $controller->createPage($this->path);
            header('Location: ' . $model->site_url . "/index.php?page=404");
            return $this->content;
        }
    }
}

Controller class:

model = $model;
    }
    public function pageExists($path) {
        if(file_exists($path)) {
            return true;
        } else {
            return false;
        }
    }

    public function createPage($path) {
        $this->content = $model->replaceParams(file_get_contents($path));
        return $this->content;
    }
}

Global.PHP

createParam('site_title', 'HassanTech');

There are at least two places in your code where it looks like you wanted to use an object's attributes, but you say $attribute rather than $this->attribute. PHP requires $this-> before property accesses. If it's not there, PHP assumes there's a local variable. Setting stuff that way will have no effect, and trying to read it will trigger a notice about undefined variables. In fact, i'd be rather surprised if the code works at all.

But with the brokenness set aside, it looks like your three classes are quite confused about their jobs. Let's review what each part of an MVC triad is supposed to do:

• The "view" basically handles all the rendering.

• The "model" represents the business objects, and should be the meat of the application.

• The "controller" coordinates stuff, routing stuff to the right place, etc.

Now, let's take a look at your classes.

• View tries to directly manage requests, doing the routing that the controller (or a router script) should be doing.

• Model is basically filling in a template. That's part of the rendering, so that's the view's job.

• Controller barely does anything. It just tells the model to replace params, and determines whether a template exists. It shouldn't be doing either one; that's the view's job.

As for why all that is bad: What happens if you want to use a template library like Twig or Smarty? If your concerns were properly separated (ie: if everything were in the right place), you'd only have to change the View class. Right now, though -- since rendering stuff is spread throughout the app -- you have to change everything. This is why MVC was invented: to keep each part separate, so you can change one part without breaking the others.

Oh...and as i'm looking at it...it doesn't look like these scripts actually do anything. $view->handlePageLoad never gets called.

Other possible issues:

-
You're redirecting to a 404 page...and that page is probably not returning a 404 status code to the browser. If Google ever finds a broken link to your site, it'd think that was the content...and your error page would end up indexed.

Rather than redirecting, you could simply say header('HTTP/1.1 404 Not Found');, and print out the contents of your 404 page.

-
__autoload is discouraged and may be removed in future versions of PHP. Instead, use spl_autoload_register to register an autoloader (which can be named whatever you like). Then you can have more than one autoloader. That will be very important later, when your site grows and you want to include other people's libraries, like Composer packages, which have their own autoloaders.

StackExchange Code Review Q#57461, answer score: 6