3
\$\begingroup\$

I have a custom class for Smarty that was partially borrowed. This is how the only example reflects the basic idea of using it across my current project:

class Template {

    function Template() {
        global $Smarty;
        if (!isset($Smarty)) {
            $Smarty = new Smarty;
        }
    }

    public static function display($filename) {
        global $Smarty;
        if (!isset($Smarty)) {
            Template::create();
        }
        $Smarty->display($filename);
    }

Then in the PHP, I use the following to display templates based on the above example:

Template::display('head.tpl');
Template::display('category.tpl');
Template::display('footer.tpl');

I made the following example of code (see below) work across universally, so I wouldn't repeat the above lines (see 3 previous lines) all the time in each PHP file.

I would just like to set, e.g.:

Template::defauls();

that would load:

Template::display('head.tpl');
Template::display('template_name_that_would_correspond_with_php_file_name.tpl');
Template::display('footer.tpl');

As you can see Template::display('category.tpl'); will always be changing based on the PHP file, which name is corresponded with the template name, meaning, if for example, PHP file is named stackoverflow.php then the template for it would be stackoverflow.tpl.

I've tried my solution that have worked fine but I don't like it the way it looks (the way it's structured).

What I did was:

  1. Assigned in config a var and called it $current_page_name (that derives the current PHP page name, like this: basename($_SERVER['PHP_SELF'], ".php"); ), which returned, for e.g.: category.
  2. In my PHP file I used Template::defaults($current_page_name);.
  3. In my custom Smarty class I added the following:

    public static function defaults($template) {
        global $Smarty;
    
        global $msg;
        global $note;
        global $attention;
        global $err;
    
        if (!isset($Smarty)) {
            Templates::create();
        }
    
        Templates::assign('msg', $msg);
        Templates::assign('note', $note);
        Templates::assign('attention', $attention);
        Templates::assign('err', $err);
    
        Templates::display('head.tpl');
        Templates::display($template . '.tpl');
        Templates::display('footer.tpl');
    }
    

Is there a way to make it more concise and well structured?

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

Well, there's a few things wrong here, but let's go over it together and I'll try to steer you in the right direction. Let's start with globals. Globals are extremely bad and should be avoided at all costs. If you have a variable that needs to be available in multiple parts of your script, then you should pass that variable as a parameter to whichever function needs it, and return the modified version if applicable. Why? Well globals are an old feature that have been proven to be full of all kinds of security issues. Not to mention they are just plain impossible to track. Sure, I know where $Smarty came from because I saw you define it in your constructor, but lets say I'm pages into your code and see that for the first time. How am I to know where this came from? Hopefully PHP will deprecate this soon, but until then, just take my word, and that of the entire community's, and don't use globals.

Lets take a look at your constructor. First you should know that the access parameter (public, private, or protected) should be used on every function, more accurately known as a method. This includes the constructor. By default it uses the public status, but I've heard rumors of this getting deprecated soon and it is better to go ahead and assume the default won't always be there for you. Besides, its always a good idea to explicitly define what you expect a method or property's access type to be. Second, the proper way to make a constructor is no longer to create a method with the same name as the class. This used to be the case in PHP 4 I think, but it is now deprecated. It still exists for backwards compatibility, but there's no saying how long that will last. Use the magic method __construct() instead. Magic meaning reserved.

So, let's review. Globals are bad, constructors are built with __construct(). How then do we create a class where we have a "global" variable created in the class contructor? Well like this:

private $smarty;

public function __construct() {
    $this->smarty = new Smarty();
}

You'll notice there's a couple of new things going on here. Hopefully you'll also notice how much cleaner this is. So, what's private $smarty? Its pretty much the same as global $smarty, except the variable isn't available globally, only within the class scope ($this->smarty). If we set the access parameter to public, it would be a little more like a global in that we could access it outside of the class, but in order to do so we would have to use an instance of the class like so: $template->smarty. That's because the variable, or, more accurately property, is associated with that class. The reason we are using a private access type her is because $smarty defines an interior "property" that should only be accessed from within the scope of the class. Sorry I can't explain that any better. Look at it like this. If I created a user class, I wouldn't want the password database to be "public" and available outside of the class. It is sort of the same here, except without sensitive data in the mix.

Let's continue. Next is a static display() method. Static methods exist for no other purpose than to defy OOP. Some might argue that I'm wrong here, but let's break down that acronym. Object Oriented Programming. About the only thing static still has in common with OOP is that it is vaguely related to an object. The inner principles of OOP (encapsulation, inheritance, and polymorphism) are simply ignored by static methods. In a lot of cases you'd be better off with just a normal helper function. Assume you won't ever need static methods for the time being. It is an advanced feature of classes and isn't likely to be necessary until you have a better grasp on the whole concept. Instead, what you should do is drop the static keyword and use the class property like it was meant to be.

public function display( $filename ) {
    $this->smarty->display( $filename );
}

Now, you may have started noticing a pattern here. Each of these new methods I have showed you are simply wrappers for the Smarty class. And that is because you are not doing anything yet that requires "extending" the Smarty class. From what I can see, you would be better of just using the smarty class directly and dropping classes altogether. But hopefully the above helped explain some of the basic concepts for you so that when you do decide to start using OOP, you have a better grasp of it.

EDIT

Oh, and about your problem. The above should help you with the structure, but until you start getting into the more advanced stuff (frameworks and autoloading), this is the best solution. At least as far as I know. The only improvement I would make is to use an absolute path name instead of relative, and you can do that using the magic constant __FILE__.

\$\endgroup\$
6
  • \$\begingroup\$ Thanks you, mseancole! ;) Here is the full example of my current code. codepad.org/nTVDHMVP. So far I have no luck applying to what you have said. If I remove static metod PHP 5.4.7 throws a warning: Strict Standards: Non-static method Template::create() should not be called statically. If I try to do what you have mentioned (I'm not sure that I'm doing everything correctly), I get fatal errors. I will keep trying but if you have chance please take a look at my small but complete custom Smarty class and tweak few lines for me so it would be your great explanation + working example \$\endgroup\$ – Ilia Rostovtsev Sep 16 '12 at 9:01
  • \$\begingroup\$ This is what I just tried: codepad.org/u98EBoSF \$\endgroup\$ – Ilia Rostovtsev Sep 16 '12 at 9:25
  • \$\begingroup\$ Please also take a look at my today's question about global's: stackoverflow.com/questions/12445972/stop-using-global-in-php/… \$\endgroup\$ – Ilia Rostovtsev Sep 16 '12 at 11:19
  • \$\begingroup\$ @IliaRostovtsev: Well, as the error states, you are still trying to call those methods statically with the class name and scope resolution operator ::. You should create an instance of your template class and assign it to a variable like so: $template = new Template();. Then you can call methods using that instance, just like you would with $this, for example $template->create();. And, as codepad as said, the required inclusion failed, and that's because $config is undefined. \$\endgroup\$ – mseancole Sep 16 '12 at 15:25
  • \$\begingroup\$ That second attempt should be asked as a second question, its completely different and could use some attention itself. Such as those properties being undefined and the continued use of globals, but I wont go into that here. As to your final question, I think that was adequately addressed in both the answers you got personally and those provided via links. You are lucky they didn't downvote that question into oblivion. As pointed out, it has been asked many times, but I believe your show of effort and knowledge helped there :) \$\endgroup\$ – mseancole Sep 16 '12 at 15:35

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.