Page 1 of 1

Template - Page rendering class - Updated

Posted: Mon Jan 28, 2008 9:51 am
by dayyanb
Is this a proper way to do it?

Code: Select all

<?php
    class Page {
        private $pageVars = array();        
        private $serverroot;
        
        private $siteInfoLocation;
        private $templateLocation;
        
        public function __construct($serverroot) {
            $this->serverroot = $serverroot;
            
            $this->siteInfoLocation = $this->serverroot . 'siteinfo/';
            $this->templateLocation = $this->serverroot . 'templates/';
            
            $this->pageVars['body'] = '';
            $this->pageVars['name'] = '';
            $this->pageVars['title'] = '';
            $this->pageVars['template'] = 'default';
            $this->pageVars['navbar'] = array();
            $this->pageVars['keywords'] = $this->loadfile('keywords');
            $this->pageVars['description'] = $this->loadfile('description');
            $this->pageVars['footer'] = $this->loadfile('footer');
        }
        
        public function __set($var, $value) {
            //Return false if they are trying to set a nonexistant variable.
            if(!isset($this->pageVars[$var])) {
                return false;
            }
            //Return false if they are trying to set a variable to the wrong type.
            if((is_string($this->pageVars[$var]) != is_string($value)) && 
            (is_array($this->pageVars[$var]) != is_array($value))) {
                return false;
            }
            $this->pageVars[$var] = $value;
        }
        
        private function removeSlashes($var) {
            $var = preg_replace('/\\//', ':', $var);
            return $var;
        }
        
        private function loadFile($file) {
            $file = $this->removeSlashes($file);
            $filepath = $this->siteInfoLocation . $file . '.txt';
            $filesize = filesize($filepath);
            if($filesize > 0) {
                $data = file_get_contents($filepath);
            } else {
                $data = '';
            }
            return $data;
        }
        
        public function renderPage() {
            $pageVars = $this->pageVars;
            $pageVars['template'] = $this->removeSlashes($pageVars['template']);
            $templatefile = $this->templateLocation . $pageVars['template'] . '.php';
            require_once($templatefile);
        }
    }
?>
EDIT: updated code again.

Re: Template - Page rendering class

Posted: Mon Jan 28, 2008 10:37 am
by pickle
'Proper' is very subjective - there's an infinite number of ways to do anything in code ;) There are a few things I would do to improve what you've got there:
  • Don't typecast in your __construct() method. There's no need to.
  • Use file_get_contents() in your loadFile() function.
  • If you're using PHP5, don't create a new function setVar(), overwrite the magic method __set() (which essentially does the same thing, but invoking it is cleaner).
  • In your fixVars() function, you probably want to check $this->pageVars['title'] rather than $this->title.
  • You should set the directory you use in $filepath in loadFile() and $templatefile in requireTemplate(), in the constructor() - save yourself some duplication.
  • You should probably play it safe and put some checks in requireTemplate() and loadFile() to make sure the supposed filename isn't trying to navigate through your directory structure.

Re: Template - Page rendering class

Posted: Mon Jan 28, 2008 10:53 am
by dayyanb
pickle wrote:'Proper' is very subjective - there's an infinite number of ways to do anything in code ;)
a proper.
In your fixVars() function, you probably want to check $this->pageVars['title'] rather than $this->title.
:oops:


I'll fix those things and repost a new version, thanks a lot for the advice.

Re: Template - Page rendering class

Posted: Mon Jan 28, 2008 12:42 pm
by Christopher
Proper would be to pass the path to the constructor and not have a global. And I am no sure what fixVar() is for ... it seems like a kludge to me. Plus, it causes requireTemplate() to be separate from renderPage().

Re: Template - Page rendering class

Posted: Mon Jan 28, 2008 12:56 pm
by Mordred
The PHP Manual wrote:Never use gettype() to test for a certain type, since the returned string may be subject to change in a future version. In addition, it is slow too, as it involves string comparison.

Instead, use the is_* functions.

Re: Template - Page rendering class

Posted: Mon Jan 28, 2008 5:12 pm
by dayyanb
arborint wrote:Proper would be to pass the path to the constructor and not have a global. And I am no sure what fixVar() is for ... it seems like a kludge to me. Plus, it causes requireTemplate() to be separate from renderPage().
I can pass the path instead. I want to make the title equal to the name if the title is not set, but I guess this really belongs in the template itself.

Thanks everyone.

Re: Template - Page rendering class

Posted: Mon Jan 28, 2008 5:18 pm
by dayyanb
Mordred wrote:
The PHP Manual wrote:Never use gettype() to test for a certain type, since the returned string may be subject to change in a future version. In addition, it is slow too, as it involves string comparison.

Instead, use the is_* functions.
I was comparing a gettype to another gettype so it wasn't really applicable under these circumstances (unless you count the "NULL"). However I know the variables would only be an array or string so I will just check for those.

Re: Template - Page rendering class - Updated

Posted: Thu Jan 31, 2008 3:18 am
by Scrumpy.Gums
dayyanb wrote:

Code: Select all

private function removeSlashes($var) {
$var = preg_replace('/\\//', ':', $var);
return $var;
}
Why not use str_replace here instead?

Re: Template - Page rendering class - Updated

Posted: Sat Feb 02, 2008 4:13 pm
by dayyanb
Scrumpy.Gums wrote:
dayyanb wrote:

Code: Select all

private function removeSlashes($var) {
$var = preg_replace('/\\//', ':', $var);
return $var;
}
Why not use str_replace here instead?
Because I should have. Thanks.