Basic PHP template loader

Question

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;
        }
    }
}

?> 

Answer 1

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).

Answer 2

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']);
}  

?>