4
\$\begingroup\$

I'm not doing too well in school, so to pass my final year, and get my degree I need to do an off-school project. The teacher chose to give me the assignment of an A grade CMS, since I've always had a thing with PHP.

Now I've made a new template class for in this particular CMS, but I'm not completely sure about it, so I came here for your help. Please review my code.

  • Have I used things in a correct way?
  • Have I used things in a efficient way?
  • Is there a better way to achieve the same result?

class template
{

    function __construct( $db, $functions )
    {
        $this->db = $db;
        $this->functions = $functions;
    }

    public function pageExist( $id )
    {
        $query = $this->db->connection->prepare( 'SELECT id FROM ' . $this->functions->prefix( 'pages' ) . ' where id = ?' );
        $query->bind_param( 'i', $id );
        $query->execute();
        $query->store_result();
        if ( $query->num_rows > 0 )
        {
            return true;
        } 
            else
        {
            return false;
        }
        $query->close();
    }

    public function fetchPage( $id )
    {
        $query = $this->db->connection->prepare( 'SELECT * FROM ' . $this->functions->prefix( 'pages' ) . ' WHERE id = ?' );
        $query->bind_param( 'i', $id );
        $query->execute();
        $result = $query->get_result();
        while ( $row = $result->fetch_assoc() )
        {
            $this->setTemplateData( 'title', $num_rows[ 'title' ] );

        }
        $query->close();
    }

    public function getTheme()
    {
        $theme = 'theme';
        $query = $this->db->connection->prepare( 'SELECT value FROM ' . $this->functions->prefix( 'settings' ) . ' WHERE name = ?' );
        $query->bind_param( 's', $theme );
        $query->execute();
        if ( ! $query )
        {
            throw new Exception( $query->error(), 1 );
        }
        $query->bind_result( $result );
        while ( $query->fetch() )
        {
            return $result;
        }

    }

    public function loadPage()
    {
        $path = './_templates/' . $this->getTheme() . '/';
            $this->content = file_get_contents( $path . 'header.tpl');
            $this->content .= file_get_contents( $path . 'middle.tpl');
            $this->content .= file_get_contents( $path . 'footer.tpl');
    }

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

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

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

    public function parseParameters()
    {
            foreach ($this->parameter as $key => $value)
            {
                $this->content = str_replace( '{' . $key . '}', $value, $this->content);
            }

            foreach ($this->langParameters as $key => $value) 
            {
                $this->content = str_replace( '{' . $key . '}', $value, $this->content);
            }

            foreach ($this->templateData as $key => $value) 
            {
                $this->content = str_replace( '{' . $key . '}', $value, $this->content);
            }


        echo $this->content;
    }

    public function handlePageLoad( $id )
    {
        if ( $this->pageExist( $id ) )
        {
            $this->fetchPage( $id );
            $this->loadPage();
            $this->parseParameters();
            return true;
        }
        else
        {
            $this->fetchPage( 404 );
            $this->loadPage();
            $this->parseParameters();
            //header('Location: index.php?page=404');
            echo '! <br /> He\'s a true genius.';
        }
    }


}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ pageExist() returns before closing. \$\endgroup\$
    – user45891
    Aug 15, 2014 at 11:24

2 Answers 2

2
\$\begingroup\$

The class name, why template? Template is much better and follows the naming convention in all sane languages (oook, i know it's PHP :()

Where are $db and $functions? You missed to declare them in the class

class template
{
   private $db;
   private $functions = [];

Your

$query = $this->db->connection->prepare( 'SELECT id FROM ' . $this->functions->prefix( 'pages' ) . ' where id = ?' );

You use prepared statement for where id = ? but now for $this->functions->prefix( 'pages' ) it makes the prepared statement useless.

The same for fetchPage, getTheme.

As user45891 said, your $query->close() will never be executed due to the return. You can improve this with:

$query->close();
return $query->numRows > 0;

This block

if ( ! $query )
{
    throw new Exception( $query->error(), 1 );
}

Great, i love Exceptions but what about a more specific exception? When i write PHP code i check this list to know which exceptions php have. What about UnexpectedValueException or LogicException. LogicException should be used when the error in your code, if $query is false it means you DB is OFF or your question is wrong so the problem should be in your code.

Anyway, your check should go before you use $query! because it will fails with

 $query->bind_param( 's', $theme );

while ( $query->fetch() )
{
   return $result;
}

You need only the first value? Well, what about

$query->fetch();
return $result;

?

Anyway, you don't close $query in every function.

Your handlePageLoad don't return everytime, only if it joins $this->pageExist( $id ) path. Maybe you want to add return false;?

You could improve the this code with this:

public function handlePageLoad( $id )
{
    if ( $this->pageExist( $id ) )
    {
        $this->fetchPage( $id );
        $this->loadPage();
        $this->parseParameters();
        return true;
    }

    $this->fetchPage( 404 );
    $this->loadPage();
    $this->parseParameters();
    //header('Location: index.php?page=404');
    echo '! <br /> He\'s a true genius.';
    return false;
}

The indentation of parseParameters is not coerent with the other code (and ugly :()

As i said above, you missed to declare fields in your class. Do it! You will need to remember what fields you use and what fields your class have (and your IDE will help you more!)

How you handle the case where someone uses the same name for lang and template data?

Your parseParameters feels to be very slow with many arguments... what about RegEx?

Why you use three different arrays parameter, langParameters and templateData (except this which is filled by another method) ? They do the same thing? What about only one array?

You need it separate?


I don't use PHP everyday (only when i need to) so i'm not sure, so don't take it serious:

In getTheme you used $theme = 'theme'; because bind_param wants references ok, but what about &"theme"? I'm not sure, but you could try

Anyway, good luck!

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

Templating gone wrong

The templating approach you are doing at the moment is:

  1. read the .tpl file using file_get_contents and store it in memory
  2. cycle over all parameters and perform a str_replace on the in-memory template.

The bigger your .tpl files are, the more memory your application will use and the more power is neede for the str_replace calls. Replacing a string in a 1kb file is way faster then a 10kb file.

Allready, we have some bugs in this aproach:

  • what happens to {my-unset-param} ? you only replace the parameters that are filled in and leave the other just as-is.
  • What about loops? (i.e. showing multiple news-items)
  • what about some simple templating functions, i.e. date, currency, ...

Because writing your own templating language and an interpreter written in PHP isn't easy. Why not use one of the really great ones out there?

But let's for the sake of your question ignore all those great things and focus on solving the templating-engine problem.

The problem(s)

The first question that always comes to my mind when looking at template engines is: why not use plain php?

PHP started of as (kind of) a templating engine. Writing

<h1><?php print $title; ?></h1>

or

<h1>{title}</h1>

Doesn't look that different. The first approach however works out of the box where the second one needs an extra processor:

str_replace('{title}',$title);

If however move into some more complex structures:

<?php foreach( $newsItems as $newsItem ) : ?>
<article class="news">
    <h1><?php print $news->title; ?></h1>
    <p><?php print $news->content; ?></p>
</article>
<?php endforeach; ?>

Or

<?php if ( $news ): ?>
<article class="news">
    <h1><?php print $news->title; ?></h1>
    <p><?php print $news->content; ?></p>
</article>
<?php endif; ?>

You can quickly see the strength of PHP over you str_replace.

The one advantage that your method has is encapsulation. We have full control over what the template can see, and what it cannot see.

OB_* to the rescue

A common approach is the use of output buffers (OB). Instead of parsing the a string/tpl file we open an output buffer with ob_start(), include the tpl file (with php code) and let the template include all the right data. Then we get the content of the buffer using ob_get_contents:

public function renderTemplate($templateFile)
{
    ob_start();
    include $templateFile;
    return  ob_get_clean();#retrieve the buffer and then clean it
}

and your template file would look something like this:

<h1><?php print $this->parameter['title']; ?></h1>
<p><?php print $this->parameter['content']; ?></p>
<small><?php print date("l", strtotime($this->parameter['submitted_date'])); ?></small>

The only problem we have to solve now is encapsulation. Our template still needs to know what the exact names of the parameters are. Also if you change the variable name of your parameters, or key, or... you will have to change the template. double the work, double the trouble.

Lucky for us, every problem has it's solution: extract

Instead of relying on our template to know how to use the object ($this->parameter[..] ) we smiply extract all the parameters before including the template:

public function renderTemplate($templateFile)
{
    ob_start();
    extract($this->parameter);
    include $templateFile;
    return  ob_get_clean();#retrieve the buffer and then clean it
}

and then clean up our template file:

<h1><?php print $title; ?></h1>
<p><?php print $content; ?></p>
<small><?php print date("l", strtotime($submitted_date)); ?></small>

Smart readers will have noticed that the $this->parameter access will still work (sadly enough). Simply because we are still inside the object. This however (again) can be solved.

We could go for an anonymous function approach (php 5.3+) or for a differnt Class that only handles rendering a template.

A TemplateParser class would then look something like this:

class TemplateParser
{
    private $data;
    
    private $templateFile;
    
    public function __construct($templateFile, $data=array())
    {
        $this->data = $data;
        $this->templateFile = $templateFile;
    }

    public function with($data)
    {
        $this->data = array_merge($this->data,$data);
        return $this;
    }
    
    public function getContent()
    {
        ob_start();
        extract($this->data);
        include $templateFile;
        return ob_get_clean();#retrieve the buffer and then clean it
    }
}

And you would use it as:

public function renderTemplate($templateFile)
{
    $tpl = new TemplateParser('path/tp/template.tpl', $this->parameter);
    $tpl->with($this->langParameter);
    return $tpl->getContent();
}

All code provided is for explanation purposes only. I'm sure that this code isn't ready for production. It should be tested first, then fixed and tested again. And the creatin of the TemplateParser could be done a lot better. bla bla bla

Some overall remarks

Try to be SOLID. Or atleast try to be DRY and have Single responsibilities. One class should only be concerned about one responsibility. Your Template class however fetches data from the database, selects the template and renders them. Wow, the GOD object does exist. And looking at how GOD fucked php up (this is a really good read) we shouldn't trust him with so much ;)

Naming could be better to. Your class tells me it's a Template, so it should handle stuff like loadFile(), getContents(), render(). You know, template stuff. The template of course needs data. And that data is stored in a Repository that handles all the selecting and updating of data. You then have some kind of controller or ServiceProvider that calls the repository $rep->find($id); and passes it to the template $tpl->with($data);.

Oh, and don't just thinkg that you should start using MVC.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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