Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have been working on a php driven template engine. This is fairly light weight, and fast from all of my testing, but I was hoping to get some feed back on it. First I would like to show you an example usages before I show the actual library. For full documentation and more examples: http://plater.phpsnips.com/

This first part is a example users homepage

templates/user.tpl

<!DOCTYPE html>
<html>
    <head>
        <title>User Home Page</title>
    </head>
    <body>
        <h2>Welcome, $session.first.ucfirst();</h2>
        <p>
            Here is where you will find the last 5 images that you have uploaded.
        </p>
        $each("myimages"):
            <div style="border: solid 1px activeborder;margin-bottom: 5px;">
                <p>
                    <img src="images/$image;" />
                </p>
                <p>
                    $description;
                </p>
            </div>
        $endeach;
    </body>
</html>

This next section is the php part for the above template.

user.php

<?php
session_start();
require_once "Plater.php";
require_once "db.php";
$tpl = new Plater();

// Query a database
$user_id = (int)$_SESSION["user_id"];
$sql = mysql_query("select * from images where user_id = $user_id order by date desc limit 5");
$images = [];
// put into the array
while($row = mysql_fetch_assoc($sql)){
    $images[] = ["image" => $row["filename"], "description" => $row["descr"]];
}

// replacemet
$tpl->assign("myimages", $images);

// show
$tpl->show("templates/user.tpl");

That was just a little taste of the template system. Here are some of the current features that it has so far:

  • Import templates within the template
  • Run PHP functions
  • Run Custom functions
  • Tidy
  • Import css (decreases http requests)
  • Loops
  • globals
    • $get
    • $post
    • $session
    • $cookie
    • $server
  • Template comments (Won't display in html output)
    • Multi line: /$ Multiline comment $/
    • Single line: $$ Single line comment
  • Remove empty tags after all replacements are done

And finally, here is the main library:

<?php

/**
 * @author php Snips <hi@phpsnips.com>
 * @copyright (c) 2012, php Snips
 * @version 0.0.1
 * @see http://plater.phpsnips.com/docs/
 */
class Plater{

    protected
            $template     = "",
            $replacements = array(),
            $cssFiles = array(),
            $disableTidy = false;

    public function __construct(){

    }

    public function show($filename){
        try{
            $this->template = $this->import($filename);
            $this->format();
            echo $this->template;
        }catch(Exception $e){
            echo $e->getMessage();
        }
    }

    public function attachCSS($filename = null){
        $tmp = $this->template;
        if(empty($filename)){
            $matches = array();
            preg_match_all("/\\\$attachCSS\((\"|')(.+)(\"|')\);/U", $tmp, $matches);
            $tmp = preg_replace("/\\\$attachCSS\((\"|')(.+)(\"|')\);/U", "", $tmp);
            foreach($matches[2] as $filename){
                $this->cssFiles[] = $this->import($filename);
            }
            $this->template   = $tmp;
        }else{
            $this->cssFiles[] = $this->import($filename);
        }
    }

    public function import($filename){
        if(!is_file($filename)){
            throw new Exception("Could not find \"<b>$filename</b>\" it does not exist.");
        }
        return file_get_contents($filename);
    }

    public function assign($key, $value){
        $this->replacements[$key] = $value;
    }

    public function disableTidy($boolean){
        $this->disableTidy = (bool)$boolean;
    }

    private function format(){
        $this->loadIncludes();
        $this->get();
        $this->post();
        $this->session();
        $this->server();
        $this->cookie();
        $this->template = $this->removeComments();
        $this->runWhileLoops();
        $this->template = $this->replaceTags();
        $this->loadIncludes();
        $this->template = $this->replaceTags();
        $this->template = $this->removeEmptyTags();
        $this->attachCSS();
        $this->template = $this->replaceCSS();
        if(!$this->disableTidy){
            $this->template = $this->tidy();
        }
    }

    private function tidy(){
        if(class_exists("tidy")){
            $tmp    = $this->template;
            $tidy   = new \tidy();
            $config = array(
                "indent"        => true,
                "indent-spaces" => 4,
                "clean"         => true,
                "wrap"          => 200,
                "doctype"       => "html5"
            );
            $tidy->parseString($tmp, $config, 'utf8');
            $tidy->cleanRepair();
            $string         = $tidy;
        }
        return $string;
    }

    private function get(){
        foreach($_GET as $k => $v){
            $this->replacements["get." . $k] = $v;
        }
    }

    private function post(){
        foreach($_POST as $k => $v){
            $this->replacements["post." . $k] = $v;
        }
    }

    private function server(){
        foreach($_SERVER as $k => $v){
            $this->replacements["server." . $k] = $v;
        }
    }

    private function session(){
        if(isset($_SESSION)){
            foreach($_SESSION as $k => $v){
                $this->replacements["session." . $k] = $v;
            }
        }
    }

    private function cookie(){
        foreach($_COOKIE as $k => $v){
            $this->replacements["cookie." . $k] = $v;
        }
    }

    private function loadIncludes(){
        $tmp     = $this->template;
        $matches = array();
        preg_match_all('/(\\$import\(("|\')(.+?)("|\')\).*;)/i', $tmp, $matches);
        //print_r($matches);
        $files   = $matches[3];
        $replace = 0;
        foreach($files as $key => $file){
            $command        = preg_replace("/\\\$import\((\"|').+?(\"|')\)/", "", $matches[0][$key]);
            $string         = $this->import($file);
            $string         = $this->runFunctions($string, "blah" . $command);
            $f              = preg_quote($file, "/");
            $tmp            = preg_replace('/\\$import\(("|\')' . $f . '("|\')\).*;/i', $string, $tmp);
            $replace++;
        }
        $this->template = $tmp;
        if($replace > 0){
            $this->loadIncludes();
        }
    }

    private function runWhileLoops(){
        $tmp     = $this->template;
        $matches = array();
        preg_match_all("/\\\$each\((\"|')(.+)(\"|')\):(.+)\\\$endeach;/isU", $tmp, $matches);
        if(isset($matches[4]) && !empty($matches[4])){
            foreach($matches[4] as $id => $match){
                $new   = "";
                $match = "";
                $name  = $matches[2][$id];
                $ntmp  = $matches[4][$id];
                if(isset($this->replacements[$name])){
                    foreach($this->replacements[$name] as $val){
                        $new .= $this->replaceTags($val, $ntmp);
                    }
                }
                $name           = preg_quote($name);
                $tmp            = preg_replace("/\\\$each\((\"|')$name(\"|')\):(.+)\\\$endeach;/isU", $new, $tmp);
            }
        }
        $this->template = $tmp;
    }

    private function replaceCSS(){
        $tmp = $this->template;
        $css = "<style>\n";
        foreach($this->cssFiles as $cssStr){
            $css .= "$cssStr\n";
        }
        $css .= "</style>\n";
        if(preg_match("/<\/head>/i", $tmp)){
            $tmp = preg_replace("/<\/head>/i", "$css</head>", $tmp, 1);
        }else{
            $tmp .= $css;
        }
        return $tmp;
    }

    private function replaceTags($keys = null, $tmp = null){
        if(empty($tmp)){
            $tmp = $this->template;
        }
        if(!empty($keys)){
            $replacements = $keys;
        }else{
            $replacements = $this->replacements;
        }
        foreach($replacements as $key => $value){
            if(is_array($value)){
                continue;
            }
            $matches = array();
            preg_match_all('/\\$' . $key . '\..+?;/', $tmp, $matches);
            if(!empty($matches[0])){
                foreach($matches[0] as $match){
                    $result = $this->runFunctions($value, $match);
                    $m      = preg_quote($match);
                    $tmp    = preg_replace('/' . $m . '/', "$result", $tmp);
                }
            }
            if(!is_array($value)){
                $tmp = str_replace('$' . $key . ';', $value, $tmp);
            }
        }
        return $tmp;
    }

    private function runFunctions($value, $functions){
        $functions = explode(".", $functions);
        array_shift($functions);
        foreach($functions as $func){
            $func = trim($func, "$();");
            if(function_exists($func)){
                $value = $func($value);
                /* if(empty($value)){
                  throw new Exception("Invalid parameter for <b>$func</b> received \"<b>$v</b>\" within template.");
                  } */
            }
        }
        return $value;
    }

    private function removeEmptyTags(){
        $tmp = $this->template;
        $tmp = preg_replace("/\\$[^\"' ]+?;/", "", $tmp);
        return $tmp;
    }

    private function removeComments(){
        $tmp = $this->template;
        $tmp = preg_replace("/\/\\$.*\\$\//isU", "", $tmp);
        $tmp = preg_replace("/.*\\$\\$.+(\n|$)/iU", "", $tmp);
        return $tmp;
    }

}

So, after review, what are your thoughts on this library?

Thanks!

share|improve this question
    
Have you compared your template engine with Smarty (which is licensed under the LGPL)? smarty.net –  Barbarrosa Jan 9 '13 at 1:38
    
I have. Mine doesn't all the features of smarty, but some of smarty's features seem a little unnecessary any way. I have added features that I felt were needed, and I am willing to add more. –  Ryan Naddy Jan 9 '13 at 3:26
    
The PHP it self is a template engine. I see no reason to create an overhead. Creating some helper functions/methods are okay but $each("myimages"); --- $endeach; syntax is horrible. –  Peter Kiss Feb 9 '13 at 6:56

2 Answers 2

Please, don't use mysql_* functions in new code. They are no longer maintained and are officially deprecated. See the red box? Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.

PHP itself already is a templating language, so my first thought would be - this is completely pointless. And no, "designers do not know PHP" is not an argument for making such templates, since those same developer would have to learn how to use your miracle instead.

About the code ..

  • There is only one way to render you template:

    public function show($filename){
        try{
            $this->template = $this->import($filename);
            $this->format();
            echo $this->template;
        }catch(Exception $e){
            echo $e->getMessage();
        }
    }
    

    The problem is that this approach severely limits the ability to cache the rendered templates (you could use ob_*, that would be extremely hack'ish).

  • The public function attachCSS($filename = null) should be refactored. It's basically two different methods.

  • You have replaces the PHP built in templating functionality with preg_*, which seems quite silly.

  • These kind of templates will most likely conflict with JS libraries

share|improve this answer
2  
Having a template engine that works four times slower than normal PHP in order to have {var} instead of <?=$var?> is not worth it. –  Sofffia Feb 10 '13 at 2:12

Disclaimer:

If you are just doing this for experience that's fine, but there are already many popular templating engines out there. Trying to compete with them isn't going to be easy or even probable. Sorry to be so blunt about it, but its the truth. Don't expect this to "make it big" or anything. That being said, have you profiled your engine and compared it to others on the market? If not, you should consider it. That will let you know if you really want to continue development, or if you need to go back to the drawing board.

Some General Considerations

Your class lacks an API. There is no interface for others to extend from, making it hard for anyone to know how to use it. There is also no documentation/doccomments, which makes it harder to use. At the very least you need some doccomments for your public scoped methods to make integration easier.

You are continuously scraping your HTML with regex. If you could find some way to do it once, then you would probably notice some immediate improvements in performance. Since, most, if not all, of your templating language begins with a dollar sign $, you could probably scrape for that and get everything into an array. Then you could loop over said array to determine what methods you will need to run.

Regex is sometimes slower than other options, especially with HTML. Have you considered loading your HTML into DOM to manipulate it? I'd give it a shot and profile it to check for any differences. I'd be willing to bet you'd see some improvements, and not just in speed, but in legibility as well. Regex is really bad on the eyes.

If you aren't going to do anything with your constructor, there is no need to have one. You can remove it. That being said, you can probably find something to do with it. Rarely ever, unless using a base class to be extended from (usually an interface or abstract class), will you not need a constructor. I've already given a couple of suggestions.

About Your Code

You should generally avoid outputting text directly. This makes your code less reusable. Let the application using your class determine what it wants to do with its data. Change those echos to returns.

    return $this->template;
}catch(Exception $e){
    return $e->getMessage();

A NULL value is essentially the same as not being set. This means that a default value of NULL isn't really necessary, an isset() check will determine this for us. Empty, on the other hand, is sometimes useful for callbacks when checking FALSE values (FALSE, NULL, etc..), but is more commonly used for determining if an array is empty. So, to check if a string is NULL we could use isset() or is_null, or we could just query the variable directly without needing to use a function at all. The benefit of not using a function is that the code is a little more efficient (slightly faster, though negligible), and is a little cleaner (less parenthesis to clutter the statement).

public function attachCSS( $filename ) {//default NULL is unnecessary

if( ! $filename ) {

I really don't know the proper procedure here. There are only a few functions in PHP that use references to automatically populate variables provided to them via their arguments, but I don't really use that many of them. Typically, when I see these functions being used, this referenced variable is not declared beforehand. I don't think there is anything wrong with it really, it just confused me at first. So, I'm not saying this needs to be changed, just letting you know its unnecessary to declare the $matches array beforehand.

$matches = array();//unnecessary
preg_match_all("/\\\$attachCSS\((\"|')(.+)(\"|')\);/U", $tmp, $matches);

Accessing an array's elements "magically" ($matches[ 2 ]) is really confusing. I for one have no idea how you came to the decision to use "2" here, nor why you chose to use "4" later. This might be a familiar formula for the regex functions, I don't really know because I never use them, but to the regular user, they probably aren't going to know what's going on. There are a few ways you can fix this to make it easier. One is to use list() to label each element of your array. Another is to use more general array functions, such as array_shift() and array_pop() to abstract the desired elements. And the final way, assuming the array is an associative one, you can use extract() to dump the array into the local variable scope so that each of its keys corresponds to a new variable. The latter obviously wont work here, but either of the first two might.

//list
array_pad( $matches, 2, array() );//padding to ensure length
list( $label1, $match );
list( , $match );//also works

//array functions
$match = array_pop( $matches );//assuming its the last element

//then you can loop
foreach( $match AS $filename ) {

Inline CSS/JS is slow. By using file_get_contents() to dump these files directly into your HTML you are forcing your browser to load all those styles and scripts every time the page loads. This is why its strongly urged for people to use external scripts and styles, so that the browser can cache them. Instead of reading the file, you should just verify that it exists, and then use the external script and style tags to include them.

<script src="path/to/script.js"></script>
<link rel="stylesheet" href="path/to/style.css" />

If you aren't going to use the value portion of the foreach loop, you can just loop over the array keys. There's no need to NULL out the value. Additionally, what is $ntmp? Don't use acronyms or obscure references for your variables/functions unless they are extremely common (PHP, HTML, etc...). Doing so will make your code harder to read. Strive for self-documenting code. This sort of goes along with that advice I gave about magically accessing arrays too.

$match = array_keys( $matches[ 4 ] );
foreach( $match AS $id ) {

My biggest problem with your code, asides from the regex, is the inconsistencies in how the $template property is set. In the following snippet that property is changed three times, yet, it only looks like it happens twice. Either way is fine, but it should be consistent. I don't think there is a right or wrong here, except in maybe that the first way is more reusable, sort of like the situation with echo and return earlier.

$this->template = $this->removeComments();
$this->runWhileLoops();
$this->template = $this->replaceTags();

Your tidy() method has an error. If the tidy class doesn't exist, then you still attempt to return $string, which hasn't been set. An appropriate move here is to either return early with the default string, or to throw an error.

Hope this helps!

share|improve this answer
    
public function attachCSS( $filename ) {//default NULL is unnecessary is at best misleading and at worst blatantly wrong. function g($x = null) {} function f($x) {}. g() is a valid call. f() is not. The default value is necessary if he wants to call it without a parameter (at least without issuing a warning). –  Corbin Jan 10 '13 at 2:26
    
@Corbin: I'll have to take your word for that until I can test it. I don't ever remember having such an error, and I work with error reporting all the way on. –  mseancole Jan 10 '13 at 14:04
    
do you work with strict php on? you would get a warning somewhat like this: Warning: Missing argument 1 for func_name()... for a function that looks like this: func_name($thing){return $thing;} –  Ryan Naddy Jan 10 '13 at 15:49
    
Some comments: 1) Defining variables before use is good practice. 2) I don't want to use a return for show I could make another function though such as render which could return instead. 3) Why pad to ensure length, when I could just check to see if it exists and then go off that. 4) Inline css isn't as slow as making multiple http requests. Thanks for the input, I did like a few things, which I did change (or I'm in the process of changing). –  Ryan Naddy Jan 10 '13 at 15:57

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.