3
\$\begingroup\$

I like the way Zend Framework works it's views and I make extensive use of partials but every partial include results in it's own file system hit. As I'm building my own framework I thought I could do better so this is what I came up with. What I'm looking for is any potential problems that I should watch out for and just general feedback.

First off lets take a simple template partial - we will call this template.phtml :

<tr><td><?= $name ?></td><td><?= $address ?></td><td><?= $phone_number ?></td></tr>

Next the actual class (proof of concept at this point so don't be hatin' on me too much)

class TemplatePartial {
    protected $file;
    protected $func;

    public function __construct($file){
        if (!file_exists($file)){
            throw new Exception("Can't get template at [".$file."]");
        }
        $this->file = $file;
    }

    public function render($data){
        if (!is_callable($this->func)){
            $code = 'foreach($data as $k=>$v){${$k} = $v;} ob_start(); ?>';
            $code.= file_get_contents($this->file).'<?php return ob_get_clean();';
            $this->func = create_function('$data', $code);
        }
        $f = $this->func;
        $f($data);
    }        
}

and to use it the code would look like

$a = new TemplatePartial("template.phtml");
echo $a->render(array(
                  "name"=>"John Doe",
                  "address"=>"1234 Anystreet Anytown MN 12345-1234", 
                  "phone_number"=>"555-555-5555"));

So far on simple tests I'm showing about 8x speed increase with almost half the peak memory usage. I'm wondering where the down side is.

UPDATE

This was too interesting to not do so here is the revised code also on github

class Templar {

    protected  $templateCache;

    protected static $instance;

    protected $templatePaths;

    protected function __construct(){
        $this->templateCache = array();
        $this->templatePaths = array();

    }

    /**
     * Return singlton instance
     *
     * @return Templar The template Engine.
     **/
    public static function getInstance(){
        if (!isset(self::$instance)) {
            self::$instance = new self();
        }
        return self::$instance;
    }

    /**
     * Add a directory to the path cache
     *
     * @param string $path The path to the template
     **/
    public function addTemplatePath($path){
        // Make sure the directory ends in exactly one DIRECTORY_SEPARATOR

        $path = rtrim($path,DIRECTORY_SEPARATOR).DIRECTORY_SEPARATOR;

        // if the path doesn't exist the fail silently
        if (file_exists($path)){
            $this->templatePaths[] = $path;
        }
    }

    /**
     * Creates a template function
     *
     * @param string $path The path to the template
     * @return the compiled function
     * @throws Templar_Exception if the template can't be found or parsed
     **/
    protected function createTemplate($path){
        // Make sure template exist in one of the template directories -- no ../ paths
        if (strpos("..", $path) !== false) {
            throw new Templar_Exception("Templates must exist in one of the template directories");
        }
        $path = ltrim($path, DIRECTORY_SEPARATOR);
        if (!file_exists($path)) {
            foreach($this->templatePaths as $testPath){
                if (file_exists($testPath . $path)){
                    $targetPath = $testPath . $path;
                    break; // break out of if and foreach;
                }
            }
            // if I make it here and I don't have a valid template there a problem
            if (!$targetPath){
                throw new Templar_Exception("Could not load template [" . $path . "]");    
            }

        }
        // updated to use export vs {foreach($data as $k=>$v){${$k} = $v;}} - thanks to this answer http://codereview.stackexchange.com/a/26297/23307
        // slight performance dip but better memory usage and it hurts my eyes less  
        $code = 'export($__data); unset($__data); ?>' . file_get_contents($targetPath) . '<?php '; 

        $func = create_function('$__data = array()', $code);


        if (!$func){
            throw new Templar_Exception("Could not parse template [" . $targetPath . "]");
        }
        // add to the templateCache
        $this->templateCache[$path] = $func;
    }

    /**
     * Returns an instance of a template function
     *
     * @param string $path The Path to the Template
     * @return Callable The template function
     **/
    public function getTemplateFunction($path){
        if (!array_key_exists($path, $this->templateCache)){
            $this->createTemplate($path);
        }
        return $this->templateCache[$path];
    }

    /**
     *  Returns a rendered Template
     *
     *  @param string $template Path to the template
     *  @param array $data
     *  @return string The renderer Template
     **/
    public static function render($template, $data = array()){
        ob_start();
        self::display($template, $data);
        return ob_get_clean();
    }

    /**
     *  Outputs a rendered Template
     *
     *  @param string $template Path to the template
     *  @param array $data
     *  @return string The renderer Template
     **/
    public static function display($template, $data = array()){
        $tmplFunction = self::getInstance()->getTemplateFunction($template);
        $tmplFunction($data);
    }
}

Again any feedback you can supply would be helpful.

UPDATE

Thinking about retooling some of the internals and changing the create_function call to use runkit_method_add. Big advantage that this would give is that template functions could be given a name (I was thinking a bastardization of the path to the template file) that would make any runtime template errors more meaningful. Currently create_function just reports "Error in runtime created function". Disadvantage is it is a PECL extension that doesn't have any windows binaries.

Anyone have any thoughts on that? Monkey patching the Templar object vs just creating anonymous functions is going to determine how view helpers will be implemented.

\$\endgroup\$
1
\$\begingroup\$

I like your approach, but I see a problem:

public function render($data){
    if (!is_callable($this->func)){
        $code = 'foreach($data as $k=>$v){${$k} = $v;} ob_start(); ?>';
        $code.= file_get_contents($this->file).'<?php return ob_get_clean();';

If the file contains PHP code without closing ?> (which is recommended), you'll get a syntax error. trim() the code and remove ?> at the end.

        $this->func = create_function('$data', $code);
    }
    $f = $this->func;
    $f($data);
}        
\$\endgroup\$
  • \$\begingroup\$ good catch - it would have taken me a while to track that one down. \$\endgroup\$ – Orangepill May 17 '13 at 16:06
0
\$\begingroup\$

You might have noticed that this is not very clean code:

$code = 'if (is_array($data)) {foreach($data as $k=>$v){${$k} = $v;}}  ?>' . file_get_contents($targetPath) . '<?php '; 
$func = create_function('$data = array()', $code);

You don't need to use create_function or eval. You can do the same using include. And instead of using foreach($data as $k=>$v){${$k} = $v;}. You could use the extract function, which does the same for you. Note I put this in a seperate function to avoid problems with the scope.

function parse($data) {
    ob_start();
    extract($data);
    include $this->template_file;
    return ob_get_clean();
}

I also see no reason to use a Singleton for this class.

\$\endgroup\$
  • \$\begingroup\$ I agree that the create_function call is ugly but I don't believe the language offers anything more eloquent to address the issue that this template processor is trying to solve, which is to avoid file system hits on iterative includes. The referenced snippet allows me to include a template once and wrap it in a function, and call it as many times as needed without any subsequent file system hits. I haven't gotten to deep in it yet, but I am exploring using ClassKit as a means to avoid the create_function call but the jury is still out. Thank you for your input and suggestions. \$\endgroup\$ – Orangepill May 17 '13 at 14:44
  • \$\begingroup\$ Thank you for the info on extract. I thought about using that but I thought it dumped the vars to global scope... confirmed it didn't. I will evaluate making all of the protected properties static vs singlton as well. \$\endgroup\$ – Orangepill May 17 '13 at 15:00
  • \$\begingroup\$ extract worked beautifully. Performance wise there was a slight uptick in how long the test ran vs the loop but peak_memory_usage dropped and it's not as ugly so its going to stay. \$\endgroup\$ – Orangepill May 17 '13 at 15:49
  • \$\begingroup\$ -1, because the main goal in question is exactly to reduce filesystem access. \$\endgroup\$ – nibra May 17 '13 at 15:56
  • \$\begingroup\$ @nibra extract doesn't touch the filesystem, it imports the the values of an array as variables named for their keys, it replaces the goofy foreach($data as $k=>$v){${$k}=$v;}. To quantify the run time performance it was ~0.07s longer over a run of 100,000 template renders. Peak memory went down about ~200 bytes. \$\endgroup\$ – Orangepill May 17 '13 at 17:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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