6
\$\begingroup\$

I recently decided to see if I could make a functioning templating system. It works, but I feel like it is not all that great. What I'm looking for here is just general critique or ideas to improve. I'm new to this stackexchange, so if this is not the right place to do this, please let me know.

As requested below, I'll outline why I feel this templating system is not up to par. I drew inspiration from the Wordpress templating system. However, without having all the WP backend, its does not have nearly the same functionality (though it should be noted, I'm not trying to recreate WP, I just used it templating system as my inspiration). I supposed the most glaring piece missing is the lack of GET compatibility. You know how you can take www.site.com/?user=37 and make it www.site.com/user/37. Basically stripping out the GET requests and making it look nice. That's something that is missing. However, I feel like already my code in ungainly and a mess, and adding in exceptions for words or numbers that might be GET requests or might be filenames will only exacerbate the problem.

A problem I've run into is that, for some reason, sometimes it likes to grab my GET calls for the CSS and JS files and pipe it through the templating system, which obviously breaks the CSS and JS. Near as I can tell, it grabs the path/to/file.js and the templating system looks for templates/pages/path.to.file.js.php. I'm not 100% sure why it does this to the link and script includes of CSS and JS, but it is annoying. And one of the reasons that this system is sub-par.

To protect your sanity, I'll only include the important files. The Template class (shown) the request chained through public/index.php->public/bootstrap.php->utilities/main.php-> utilities/Template.php (thispage/requests->this/one)

/public
  index.php
  bootstrap.php
/templates
  /layouts
    header.php
  home.php
  404.php
  page.php
  multilevel.page.php
/utilities
  main.php
  Template.php

public/index.php

<?php require_once 'bootstrap.php'; ?>
<!-- if page is called with AJAX -->
<?php if(!empty($_SERVER['HTTP_X_REQUESTED_WITH']) && strtolower($_SERVER['HTTP_X_REQUESTED_WITH']) == 'xmlhttprequest') : ?>
  <?= Template::find(); ?>
<?php endif; ?>
<!-- otherwise, display -->
<!DOCTYPE html>
<html>
  <head>
    <?= Template::show('components.head'); ?>
  </head>
  <body class="<?= Template::classes(); ?>">
    <header>
      <?= Template::show('layouts.header'); ?>
    </header>
    <main>
      <div class="container">
        <?= Alert::display() ?>
      </div>
      <?= Template::find(); ?> <!-- main content -->
    </main>
    <footer>
      <?= Template::show('layouts.footer'); ?>
    </footer>
    <?= Template::show('components.scripts'); ?>
  </body>
</html>

utiltiies/Template.php

<?php

class Template
{
  public static function show($file, $page = false)
  {
    if (!$page) {
      $filepath = str_replace('.', '/', $file);
    } else {
      $filepath = 'pages/'.$file;
    }
    $template = ROOTPATH.'/templates/'.$filepath.'.php';
    require_once $template;
  }

  public static function find()
  {
    $fileName = Template::getFileName();
    if (Template::pageExists()) {
      Template::show($fileName, true);
    } else {
      Template::show('404', true);
    }
  }

  public static function getTitle()
  {
    if (Template::pageExists()) {
      $fileName = Template::getFileName();
      $file = ROOTPATH.'/templates/pages/'.$fileName.'.php';
      $docComments = token_get_all(file_get_contents($file))[0][1];
      if (strpos($docComments, "Title")) {
        $woFront = str_replace('<!-- Title: ', '', $docComments);
        $title = explode(' -->', $woFront)[0];
      } else {
        $title = ucwords(str_replace('.', ' ', $fileName));
      }
    } else {
        $title = "Page not found";
    }
    return $title;
  }

  public static function classes($custom = false)
  {
    if ($custom) {
      $custom = ' '.$custom;
    }
    $classes = Template::pageClass() . $custom;
    return $classes;
  }

  private static function getFileName()
  {
    $URI = $_SERVER['REQUEST_URI'];
    if ($URI === '/') {
      $fileName = 'home';
    } else {
      $fileName = str_replace('/', '.', trim($URI, '/'));
    }
    return $fileName;
  }

  private static function pageClass()
  {
    $URI = $_SERVER['REQUEST_URI'];
    if ($URI === '/') {
      return 'home';
    } elseif (Template::pageExists()) {
      $className = str_replace('/', '-', trim($URI, '/'));
      return $className;
    } else {
      return '404';
    }

    $fileName = Template::getFileName();
    if (Template::pageExists()) {
      $className = str_replace('/', '-', trim($fileName, '/'));
      return $className;
    } else {
      return '404';
    }
  }
  private static function pageExists($page = false)
  {
    if (!$page) {
      $fileName = Template::getFileName();
    } else {
      $fileName = $page;
    }
    $file = ROOTPATH.'/templates/pages/'.$fileName.'.php';
    if (file_exists($file)) {
      return true;
    } else {
      return false;
    }
  }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ can you add more info like any reasons why you feel it maybe limited in effect or versatility, any known or predicted limitations, any required functionality you predict being a possibility in the future from this? Also, a nod towards any direct inspiration sources would be nice, if you have them, as it might help us answer. \$\endgroup\$ – Daniel Brose Mar 17 '16 at 4:26
  • \$\begingroup\$ @DanielBrose I have added the information requested above as the second paragraph down. Thanks \$\endgroup\$ – amflare Mar 18 '16 at 15:55
  • \$\begingroup\$ I added an edit with more information pertaining to why I am not satisfied with my system. \$\endgroup\$ – amflare Mar 23 '16 at 15:57
  • \$\begingroup\$ Clean URLs can be achieved with url-routing, done using PHP or .htaccess (if using Apache HTTPD). Those mechanisms should be independent of the templating system. \$\endgroup\$ – 200_success Mar 23 '16 at 16:09
  • \$\begingroup\$ @200_success That would require a fundamental change to several of my Template functions, yes? Or are you saying there is a way to merge that in? Not that I am opposed to the fundamental change, I just want to be sure I'm not going about it the hard way. \$\endgroup\$ – amflare Mar 23 '16 at 16:13
2
\$\begingroup\$

The biggest problem is that you aren't building a templating system. You are building an entire PHP framework. Your code actually does very little templating. Instead it mainly does routing and asset loading. My general suggestion would be very simple: take some time to really understand MVC frameworks in PHP, because that is what you are really trying to build. The simplest way to do that is to learn one. I would start with something simple. Go download CodeIgniter, use, learn it, understand it and the many aspects of it, and then come back and try again.

I suggest this because your code shows that you have a general idea of what you are trying to do, but there are a very large number of important specifics that you are going to have to familiarize yourself with. What you have right now is a great way to get started with exploring these principles yourself, but it is a long way from being something that can be actually used in practice. So in short: keep working on it! Your next step is to work towards really understanding MVC frameworks in PHP. That is what you are actually trying to do (or at least, a good target to aim for), whether you realize it or not.

As for some specifics about your code:

1. It isn't really OOP

You are using classes, but you have static functions exclusively. This means that you could drop the class and just have functions and everything would work exactly the same. If you only ever use static functions you aren't really using OOP. Some better understanding of OOP principles and how the work/apply will be very helpful.

2. You need a real routing setup

Most common way to handle routing in PHP is with the front-controller pattern. You can find lots of articles about it. Wordpress uses it, as do just about all MVC frameworks in PHP. Yes, you will have to refactor everything.

3. File inclusion vulnerability

Your current code is vulnerable to a remote-file inclusion vulnerability. I can use this code to read any file on your computer that PHP has read access to. To get something like this production ready, you need to be an expert on defense-in-depth and web vulnerabilities in PHP.

4. No master classes!

You've basically got one class that just does everything. That runs very much contrary to the basic principles of OOP that make it so helpful in terms of long-term code management. Spend some time reading up on SOLID, its benefits, and how to implement it.

Those are just some highlights. Again, to emphasize, the best way to master concepts like this is to do exactly what you are doing: give it a try and look for feedback. So you're headed in the right direction! Next you want to learn about the guiding principles of good OOP (i.e. SOLID), principles/practical details of the MVC pattern, routing, and security.

\$\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.