DB security and efficiency in WordPress plugins

I recently wrote my first WordPress plugin, which was also my first PHP project. After I was done, I didn’t care for the code at all, so I have spent the past couple of days refactoring it. I’m trying to take a more OOP approach to improve readability and make it easier to optimize. Prior to this, my only real exposure to OOP was in a C# class a few years ago, so this is pretty new to me, as well.

One of the problems with my original design (if you can even call it design) was that I was using get_option(), add_option() and update_option() all over the place willy-nilly. I decided I should get all of my database handling stuff into one place and make an effort to minimize the number of calls to the database.

So, I made this DBLiaison class. The constructor is the only place where data is pulled from the database. There are also two methods that put data in the database, one that updates existing data and one that deletes data. Both of these methods are private, with public methods that hook into them (or whatever the appropriate terminology is?).

All of my plugin’s data is in a multidimensional array on a single row in the DB, which I understand to be the best practice when working with WordPress plugins. The structure of that is:

plugin_options = ({
    'template_id1' => ({ 'key' => 'value' }),
    'template_id2' => ({ 'key' => 'value' })
})

Obviously, with more descriptive names and appropriate PHP syntax. There are 17 key/value pairs for each template.

So, this is my DBLiaison class. I would appreciate any feedback on efficiency, design and security anyone has to offer.

<?php

class scbsDBLiaison {

    public $templates = array();

    public function __construct() {
        if ( !current_user_can( 'manage_options' ) )
            wp_die( __( 'You do not have sufficient permissions to access this page.' ) );

        if (get_option( ECBS_TEMPLATES ) )
            $this->templates = get_option( ECBS_TEMPLATES );

        else
            wp_die(__("No templates in DB"));
    }

    public function get_all_templates() {

        if ( isset( $this->templates ) )
            return $this->templates;
        else
            return false;
    }

    public function get_this_template( $template ) {
        $template_id = $this->get_template_id( $template );
        return $this->templates[$template_id];
    }

    public function template_exists_in_options( $template ) {
        $template_id = $this->get_template_id( $template );
        return array_key_exists( $template_id, $this->templates );
    }

    public function get_template_id( $template ) {
        if ( is_array( $template ) )
            $template_id = $template[ECBS_TEMPLATE_ID];
        else
            $template_id = $template;

        return $template_id;
    }

    public function add_template_to_options( &$template ) {

        if ( !is_object( $template ) )
            wp_die( __( 'That is not a template!' ) );

        return $this->do_option_add( $template );
    }

    public function delete_template_from_options( $template_id ) {
        //$template_id = $this->get_template_id( $template );
        if (!isset($template_id)) wp_die(__("No template ID given."));

        return $this->do_option_delete( $template_id );
    }

    private function do_option_add( &$template ) {
        $template_id = $template->get_template_id();


        if ( $this->template_exists_in_options( $template_id ) )
            $this->do_option_delete( $template_id );

        unset( $this->templates[$template_id] );
        $this->templates[$template_id] = $template->get_all_values();
        ksort( $this->templates );

        return update_option( ECBS_TEMPLATES, $this->templates );
    }

    private function do_option_delete( $template_id ) {

        unset( $this->templates[$template_id] );

        if ( array_key_exists( $template_id, $this->templates) )
            wp_die(__('Unset failed'));

        ksort( $this->templates );

        return update_option( ECBS_TEMPLATES, $this->templates );
    }

}

---

I’m noticing right now that my do_option_add() and do_option_delete() methods look almost identical. So I should probably consolidate those into a single method.

---

Also, if it is helpful to know this, there is a template class and a templateManager class. The template just holds the template data with setters and getters for accessing and changing data inside the template object, but doesn’t interact with the DB. The templateManager is what actually pulls the template data from the DB and updates the template data or deletes the template in the DB.

I’m not sure if that is good design or not. I went back and forth on whether all of that should be part of the template class or broken up as I have it now.

---

Oops, one more edit. I looked closer at do_option_add() and do_option_delete() and realized that I was calling do_option_delete() from do_option_add(), which meant I was adding data to the database twice, which is not what I intended. I added a new method to handle the overlapping code between those two called unset_template().

Here is what those three methods look like now:

private function do_option_add( &$template ) {
    $template_id = $template->get_template_id();      

    if ( $this->template_exists_in_options( $template_id ) )
        $this->unset_template( $template_id );

    $this->templates[$template_id] = $template->get_all_values();

    return update_option( ECBS_TEMPLATES, $this->templates );
}

private function do_option_delete( $template_id ) {

    $this->unset_template( $template_id );

    return update_option( ECBS_TEMPLATES, $this->templates );
}

private function unset_template( $template_id ) {

    unset( $this->templates[$template_id] );

    if ( array_key_exists( $template_id, $this->templates) )
            wp_die(__('Unset failed'));

    ksort( $this->templates );
}

A little better, hopefully.

I’m just going to write some comments from the top down.

First of all I would like to point you to PSR, and especially PSR-2, since following a well known coding style will help other developers read your code. But please note that PSR-2 is nowhere near WordPress’s coding standards (For a more entertaining read I also recommend the Linux kernel coding style, but that’s for C-programming). Whichever you chose is up to you, it all boils down to a matter of preference.

Now on to the interesting part.

<?php

class scbsDBLiaison {

I’ll start with your class name. I guess scbs is your vendor prefix. If you would like your code to be easily autoloaded I recommend using an underscore after the prefix, according to PSR-0.

I’d also rethink the name of the class. DBLiaison is not a very common name for what you’re doing here. The pattern I think you are trying to use is called a Data mapper pattern (Edit: Actually it’s more like the adapter pattern or some other wrapper. But let’s ignore that for now.) and therefore I’d recommend you calling the class something like Scbs_TemplateOptionsMapper. That would make it easier for other developers to understand the purpose of your class.

public $templates = array();

If no other class is supposed to use this array I’d recommend making it protected instead of public.

public function __construct() {
    if ( !current_user_can( 'manage_options' ) )
        wp_die( __( 'You do not have sufficient permissions to access this page.' ) );

    if (get_option( ECBS_TEMPLATES ) )
        $this->templates = get_option( ECBS_TEMPLATES );

    else
        wp_die(__("No templates in DB"));
}

Here you are actually violating the Single Responsibility Principle when you are checking the permissions of the user. I’d recommend moving that part to some piece of code responsible for authorization.

You are also using a globally defined constant ECBS_TEMPLATES. Normally you would want to make that a constant of a class, and in this case that would probably be this class. A class constant doesn’t pollute the global namespace which means less risk of collision with other plugins. I’m also wondering why you’re using ECBS_ as a prefix here, when you used scbs for the class? I’d stay with one vendor prefix.

public function get_all_templates() {

    if ( isset( $this->templates ) )
        return $this->templates;
    else
        return false;
}

This method will never hit the else-block, since you’re setting $this->templates = array(); where you define your attribute (unless you unset the entire array in some other class, which of course is possible as long as it’s public).

public function get_this_template( $template ) {
    $template_id = $this->get_template_id( $template );
    return $this->templates[$template_id];
}

Since a template is a class you could use type hinting here. If you use type hinting you could just get the template id directly from the class through some public getter method there.

public function template_exists_in_options( $template ) {
    $template_id = $this->get_template_id( $template );
    return array_key_exists( $template_id, $this->templates );
}

Here’s another place where type hinting would be useful. (I also personally prefer the shorter isset to array_key_exists but beware of the differences.)

public function get_template_id( $template ) {
    if ( is_array( $template ) )
        $template_id = $template[ECBS_TEMPLATE_ID];
    else
        $template_id = $template;

    return $template_id;
}

This method should be a getter method in a template class.

public function add_template_to_options( &$template ) {

    if ( !is_object( $template ) )
        wp_die( __( 'That is not a template!' ) );

    return $this->do_option_add( $template );
}

Type hinting would remove two out of three lines here.

Also, you don’t need to specify that you want a reference of the parameter, since objects are always passed by reference (not really, but something like that).

public function delete_template_from_options( $template_id ) {
    //$template_id = $this->get_template_id( $template );
    if (!isset($template_id)) wp_die(__("No template ID given."));

    return $this->do_option_delete( $template_id );
}

I can forgive you for not removing the commented line here, but I’d do that before committing your changes 🙂

Also, $template_id will be set, so what you’re checking is whether somebody called this method with null. That shouldn’t happen if the rest of your code does what it’s supposed to.

private function do_option_add( &$template ) {
    $template_id = $template->get_template_id();      

    if ( $this->template_exists_in_options( $template_id ) )
        $this->unset_template( $template_id );

    $this->templates[$template_id] = $template->get_all_values();

    return update_option( ECBS_TEMPLATES, $this->templates );
}

This method could be written like this

private function do_option_add( Scbs_Template $template ) {
    $this->templates[$template->get_id()] = $template->get_all_values();
    return update_option( ECBS_TEMPLATES, $this->templates );
}

The result would be the same.

private function do_option_delete( $template_id ) {

    $this->unset_template( $template_id );

    return update_option( ECBS_TEMPLATES, $this->templates );
}

I have nothing to add here.

private function unset_template( $template_id ) {

    unset( $this->templates[$template_id] );

    if ( array_key_exists( $template_id, $this->templates) )
            wp_die(__('Unset failed'));

    ksort( $this->templates );
}

Checking if unset() failed is not necessary. It really shouldn’t fail.

This is also not a very good place to sort your array. If you have a sorted list like [a, b, c] and remove b, would it not still be sorted?

I hope this helps you in your endeavours.