CodeIgniter nav/menu insecurities

Question

I more or less asked the same questions on stackoverflow.com, but maybe this is more for this forum. I have a hard time getting to grips with how to implement a top navigation (common for the whole site) and a page specific navigation (common for the current controller). I use Colin Williams' Template library.

I have written my own MY_Controller which all page controllers should extend, but I do not know if this is the correct way. The page specific navigation is implemented in the code below.

MY_Controller.php

abstract class MY_Controller extends CI_Controller {
    private $_nav;

    public function __construct() {
        parent::__construct();
    }

    abstract protected function _get_nav();

    protected function _load_nav(array $nav = null) {
        if ($nav === null) {
            $this->_nav = $this->_get_nav();
        } else {
            $this->_nav = $nav;
        }
    }

    protected function _render() {
        if (func_num_args() < 3) {
            return false;
        }

        $args = func_get_args();
        $this->template->write('title', array_shift($args));
        $data = array_pop($args);

        for ($i = 0; $i < count($args); $i++) {
            $this->template->write_view('content', array_shift($args), $data);
        }

        if ($this->_nav === null) {
            $this->_load_nav();
        }

        $this->template->write_view('nav', 'templates/sidebar_nav', array('nav' => $this->_nav));
        $this->template->render();
        return true;
    }
}

collection.php

class Collection extends MY_Controller {
    public function __construct() {
        parent::__construct();
        $this->load->model('collection_model');
    }

    protected function _get_nav() {
        return $this->config->item('collection_nav');
    }

    public function index() {
        $this->view();
    }

    public function view() {
        $data = array();
        $data['list'] = $this->collection_model->get_list();
        $this->_render('View Collection', 'collection/view', $data);
    }
}

sidebar_nav.php

<ul class="nav nav-list bs-docs-sidenav">
    <?php foreach ($nav as $item) : ?>
    <?php $class = isset($item['class']) ? ' class="' . $item['class'] . '"' : ''; ?>
    <li<?php echo $class; ?>>
        <a href="<?php echo $item['href']; ?>"><?php echo $item['value']; ?></a>
    </li>
    <?php endforeach; ?>
</ul>

What do you think? Is this the proper way to produce a navigation system?

Answer 1

I'm not familiar with CI, but from what I can tell the concept seems alright. At least, what I am able to glean from your code seems to be. There are some improvements you could make however.

The only reason to abstract the setting of the $_nav property with the _load_nav() method would be so that you could inject different navigation schemes to allow for further expansion. However, you do not inject your navigation into it and instead hardcode it, albeit with another method. If you are always going to use the _get_nav() method to "inject" your navigation then there is no need for the _load_nav() method as it just adds unnecessary abstraction.

//injection
protected function _load_nav( $nav ) {
    $this->_nav = $nav;
}

Another abstraction issue is with the $_nav property itself. Your current implementation creates an associative array container for the navigation array. This is redundant as well as unnecessarily abstract. The only reason to abstract the menu in this way is if you were abstracting submenus and injecting their contents into a main menu container. This is pretty similar to what you are currently doing with your _get_nav() method, though you have combined them all already.

protected function _load_nav( $header, array $nav ) {
    $this->_nav[ $header ] = $nav;
}

In your current implementation of your _render() method you are getting all of the function arguments into an array then, essentially, removing the first element after setting it to the TITLE_REGION. If you use array_shift() to physically remove that first element, then your array will no longer contain that element and you can loop over it more naturally. This also has the added benefit of removing these "magic" numbers from your code.

$title = array_shift( $args );
$this->template->write( self::TITLE_REGION, $title );

$n = count( $args );
for( $i = 0; $i < $n; $i++ ) {

There is no need to explicitly check the $active variable. If it is NULL, then it will not, or rather should not, be set in the $_nav array. Alternatively, if a variable is NULL then it is considered to be not set, so you can use isset() on both variables to simulate an AND comparison, though again, this is unnecessary in this context.

if( isset( $this->_nav[ 'nav' ] [ $active ] ) ) {
//Unnecessary Alternative
if( isset( $active, $this->_nav[ 'nav' ] [ $active ] ) ) {

You might want to think about saving your navigation to a separate file so that if you ever need to change it you wont have to change the code directly to do so. This follows the Separation of Concerns, which helps to make your code more extensible. Another benefit is that it will allow you to implement that abstract method in MY_Controller without losing functionality. One method would be to use JSON. There are many other methods you can use, but JSON is the one I'll show you. The method you choose is up to you.

//JSON file
{ "collection/view" : [
    {
        "href"  : "collection/view",
        "value" : "View collection",
        "class" : "icon-film"
    },
    //etc...
] }

//Importing the JSON
protected function _get_nav( $file ) {
    $json = file_get_contents( $file );
    return json_decode( $json, TRUE );
}

Your index() method is redundant with your view() method. Seeing as how they are both public, having them both is unnecessary. If one were public and the other private then one could be considered the public wrapper, but unless that wrapper was doing something else then it would still be considered redundant and better if you just changed the scope of the other method to public.

You should define your arrays before trying to append to them. Otherwise you could be editing a global. If you meant to edit said global, then you should know that globals are evil and you should never do such a thing. Defining the array before trying to use it, assuming you aren't using globals, ensures that you aren't accidentally manipulating another array defined earlier or accessing a string through the array syntax. The former may not be as noticeable, but the latter will have vastly different results. Another reason not to do this is to avoid PHP issuing warnings, which it should be doing. If you are not seeing these warnings then you should turn up your error reporting so that you can.

$data = array();
$data['list'] = $this->collection_model->get_list();

Instead of rendering your HTML with PHP, you might want to consider using the templating syntax and just rendering the PHP values you need into your HTML. This makes for cleaner, and thus easier read and edit, HTML. For example:

<ul class="nav nav-list bs-docs-sidenav">
    <?php foreach( $nav AS $item ) : ?>

    <?php $class = isset( $item[ 'class' ] ) ? $item[ 'class' ] : ''; ?>
    <li class="<?php echo $class; ?>">
        <a href="<?php echo $item['href'];?>">

            <?php echo $item[ 'value' ]; ?>

        </a>
    </li>

    <?php endforeach; ?>
</ul>