2
\$\begingroup\$

For a project, I need to write a basic template manager who will be responsible to load the requested HTML or PHP template when the user clicks on a menù link. It's like a page loader since that all the loaded templates are static HTML pages. I want to improve it if is it possible. Here is the code.

on the main index of the project I have a jQuery code like this:

<script>
$(document).ready(function(){

$('.contents').load('templates/home.php');

$(document).on('click','a',function(e){
    e.preventDefault();
    $('#nav-modal').modal('hide');
    var url = $(this).attr('href');
    if(url == 'booking/'){
        window.location.href= 'booking/';
    } else {

    $.ajax({
        type: 'GET',
        url: 'templates/TemplateController.php'+url,
        cache: false,
        dataType: 'html',
        success: function(response){
             $('.contents').empty()
             .html(response);
        }
        });
    }

    });    

});
</script>

on the PHP 'controller' side I have this code who will only call a class to obtain the needed files. The URL passed from AJAX to the controller is something like this: ?tpl=atemplate

<?php
require_once 'Autoloader.php';

if(isset($_GET['tpl'])){
    $tpl = new TemplateLoader;
    echo $tpl->render($_GET['tpl']);
}

?>

And then in the simple class, I have only a switch control that will select what is the requested template to load on the index

<?php

class TemplateLoader{

    public function __construct(){
        #$this->path = $path;
    }

    public function render($tpl){

        switch($tpl){
            case 'home':
                echo file_get_contents('home.php');
                break;
            case 'about':
                echo file_get_contents('about.php');     
                break;
            case 'services':
                echo file_get_contents('services.php');
                break;    
            case 'contacts':    
                echo file_get_contents('contacts.php');
                break;
            case 'prices':
                echo file_get_contents('prices.php');
                break;
        }
    }
}

?> 
\$\endgroup\$
  • \$\begingroup\$ Instead of using jQuery.ajax() for your requests, you should consider the benefits of the fetch API. Importantly, fetch works asynchronously and it also won't reject on HTTP error codes. If you do not need to worry about authentication or user sessions, then it may be helpful. \$\endgroup\$ – esote Jun 30 '18 at 4:37
3
\$\begingroup\$

Technical mistakes

echo void

echo $tpl->render($_GET['tpl']);

This echo will display empty string (null), because you've already output a string with echo within method and have void return type (implicit return null). Make render() method return a string, because you want to defer output as much as you can. You won't be able to do anything with it after that (including headers management). Eventually you might want to use psr-7 response object with stream and plug yourself into some framework - class with method that returns string can be left untouched (Open-closed Principle) and wrapped with an object that changes string into stream.

closing tag

?>

Simply don't use it at the end of file. It causes hard to debug problems with undesired output - more here.

Algorithm

Render method works on hardcoded strings, which rarely is a good idea. You can make it more generic, as the logical pattern is quite distinct. Append path & file extension to given name and get file contents if it exists. The only thing you need to be careful about is path traversal attack - if you allow only lowercase letters it won't be possible. Also some error handling will be required.

OOP

Api

First of all your class api is perfect. Giving only template name, without implying that file system is involved makes it encapsulated abstraction and that's what OOP is about - don't be fooled by magic tricks and utilities. It is really a hard part, that often goes into area of so called "unknown unknowns" if you get it wrong, and it is really easy to do so.

Naming

You can write a book about that, so I'll point a few things:

  • Avoid naming your classes with names ending with -or or -er. It fits interfaces pretty well - they're focused on delivering something specific (from client's perspective) in unspecified way.
  • On the other hand avoid mentioning IO gateway names in data focused interfaces. It fits class names - they're interface implementations delivering something by talking to concrete IO mechanism.
  • Your TemplateLoader is concrete kind of loader, but you should be able to switch it with implementation that delivers strings from database or memory (when testing the class that uses it). You won't see the point when global namespace is the client using it, but when additional layers wrap it up it won't require messing with code beside the place where object composition happens.
  • render() method usually implies that some data processing is involved - you are simply returning template contents.

Refactored

Since this class is small (as it should) I've written refactored version based on the things I've pointed out and type hints for php 7:

interface TemplateLoader
{
    /**
     * @param string $templateName
     *
     * @throws InvalidArgumentException|TemplateNotFoundException
     *
     * @return string
     */
    public function contents(string $templateName): string;
}

class TemplateFiles implements TemplateLoader
{
    private $templatesDirectory;

    public function __construct(string $templatesDirectory = '') {
        $this->templatesDirectory = $templatesDirectory;
    }

    public function contents(string $templateName): string
    {
        $filename = $this->templateFilename($templateName);
        return file_get_contents($this->existingFile($filename));
    }

    private function templateFilename(string $templateName): string
    {
        $file = $this->validName($templateName) . '.php';
        return rtrim($this->templatesDirectory, '/') . '/' . $file;
    }

    private function validName(string $template): string
    {
        if (preg_match('/[^a-z]/', $template)) {
            throw new InvalidArgumentException('...');
        }

        return $template;
    }

    private function existingFile(string $filename): string
    {
        if (!file_exists($filename)) {
            throw new TemplateNotFoundException('...');
        }

        return $filename;
    }
}

You would have to implement two exceptions and put method call into try-catch block. There are two of them, because one may come from potentially malicious request (you might want to log these), while the other may be dev error or bypassed UI by user (it's not control flow exception which should be avoided).

\$\endgroup\$
  • \$\begingroup\$ Thanks for your tips. I wrote this code for a small project and quickly, so, for some aspect, I didn't considered some things like path traversal attack or file verification using file_exsist(). \$\endgroup\$ – user9741470 Jun 30 '18 at 9:09
  • 1
    \$\begingroup\$ File exists check is required, but with hardcoded filenames path traversal was not possible, because your switch statement served as whitelist. As a general rule you need to be extra careful when parameters provided by user are used directly as string parameters (and escaped for output). BTW. The solution I suggested is convention-based. There is also configuration approach, and it would require constructor to take associative array mapping template names to files (also serves as whitelist) - more instantiation effort, but flexible and simple implementation. \$\endgroup\$ – shudder Jun 30 '18 at 10:03
  • \$\begingroup\$ Ok, i will consider to implement some parameters inside the constructor, but i also consider that the user will not pass any parameter to the class, so i will focus on the improving of this code by adding a check for the file existence while I'm acquiring more OOP experience to make a more complex and generic solution. \$\endgroup\$ – user9741470 Jun 30 '18 at 10:18
  • \$\begingroup\$ That unhandled user parameters I ment for low level functions (like include, file_get_contents, mail, or sql methods) - you can pass them to your methods without any problem. Also you don't have to go for configuration based solution - its instantiation will be teadious. \$\endgroup\$ – shudder Jun 30 '18 at 11:47
1
\$\begingroup\$

To avoid another topic, here is a reworked code of the same class. I've made it more generic, but needs to be improved, I don't think it's perfect for now. (I want to implement namespace but it'will not work and I can't figure why?)

The class is inside a folder named lib where all other classes are hosted. The controller is inside a folder named controllers both folders are inside a subfolder of the root project,this folder is named app. All the template files are instead inside a folder named templates who is in the same folder of controllers and lib folders.

<?php
// if namespace are set, the autoloader will not load the class
//namespace lib;

class TemplateView{

    private $tpl;
    private $fileName;
    private $templatePath;

    public function __construct($templatePath){
        $this->templatePath = $templatePath;
    }

    public function renderTemplate(string $tpl){
        return $this->loadTemplate($tpl);
    }

    private function loadTemplate(string $tpl){
        $this->fileName = basename($tpl.'.php');
        if(file_exists($this->templatePath.'/'.$this->fileName)){
            return file_get_contents($this->templatePath.'/'.$this->fileName);
        } else {
            throw new Exception('Template not found.');
        }
    }
}

?> 

Controller code

<?php
require_once '../lib/Autoloader.php';
// use lib\TemplateView as TemplateView;    

if(isset($_GET['tpl'])){
    $tpl = new TemplateView('../templates');
    echo $tpl->renderTemplate($_GET['tpl']);
}  

?>
\$\endgroup\$

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.