Checkbox Group Function and Usage

Question

In CodeIgniter, I have created function (helper) to populate, store and retrieve checkbox group. I just wonder if I have written code is in proper way and optimized or needs some more finishing?

Function

if ( ! function_exists('checkbox_group') )
{
    function checkbox_group($checkboxes = array(), $name)
    {

    // setting hidden field for null, if no checkbox selected
    echo form_hidden($name.'[]', 'null');

    // start checkbox loop
    foreach($checkboxes as $check => $label_text):

        $checked = FALSE;                   
        $selects = get_config_row($name);
        if(in_array($check, $selects))
        $checked = TRUE;

        echo '<div class="checkbox">';
        echo form_label(form_checkbox($name.'[]', $check, $checked, 'id="'.$check.'" class="checkbox"') . $label_text, $check);
        echo '</div>';
    endforeach;
    }
}

Usage

// checkbox items
$checkboxes = array(
    'noindex'=>'No Index',
    'nofollow'=>'No Follow',
    'noarchive'=>'No Archive',
    'nosnippet'=>'No Snippet'
);

// render checkboxes useing function
checkbox_group($checkboxes, 'indexes');

Answer 1

I haven't worked with CodeIgniter before, so if I say something that argues their standards, please excuse me.

It's such a basic piece of code that there isn't too much to be said. One things I notice right off the bat, your checkbox_group() function has a first parameter with a default value. I'm surprised you haven't done anything about this because it should be throwing you a warning.

Note that when using default arguments, any defaults should be on the right side of any non-default arguments; otherwise, things will not work as expected.

As per the PHP Functions page

To better separate the function's business and the view, consider creating an output string and returning it to be echoed. You'd then have:

$output = '';
// setting hidden field for null, if no checkbox selected
$output .= form_hidden($name . '[]', 'null');

// start checkbox loop
foreach ($checkboxes as $check => $label_text) :

    $checked = FALSE;
    $selects = get_config_row($name);
    if (in_array($check, $selects))
        $checked = TRUE;

    $output .= '<div class="checkbox">';
    $output .= form_label(form_checkbox($name . '[]', $check, $checked, 'id="' . $check . '" class="checkbox"') . $label_text, $check);
    $output .= '</div>';
endforeach;
return $output;

Now you call:

// render checkboxes useing function
echo checkbox_group($checkboxes, 'indexes');

Answer 2

Splitting the assignment like this is clumsy:

$checked = FALSE;
…
if (in_array($check, $selects))
    $checked = TRUE;

in_array() returns a bool. Why not just say:

$checked = in_array($check, get_config_row($name));

Answer 3

The HTML output would look like this (pretty-printed):

<input type="hidden" name="indexes[]" value="null" />
<div class="checkbox">
  <label for="noindex">
    <input type="checkbox" name="indexes[]" value="noindex" id="noindex" class="checkbox" />No Index
  </label>
</div>
<div class="checkbox">
  <label for="nofollow">
    <input type="checkbox" name="indexes[]" value="nofollow" checked="checked" id="nofollow" class="checkbox" />No Follow
  </label>
</div>
…

I think that is problematic in several ways:

• It will always submit a indexes[]=null value, even when some checkboxes are selected. What good does indexes[]=null do? Just omit that hidden field.

• <div class="checkbox"> is an unnecessary level of nesting. Just <label class="checkbox"> would suffice. Then, in CSS, you can optionally style it with

  label.checkbox {
      display: block;
  }
  

  if you want it to be displayed as a block.

• Putting class="checkbox" on an <input type="checkbox"> is redundant. You could just use input[type="checkbox"] as a CSS selector.

• Every id attribute in an HTML document must be unique. (If any other element also has id="nofollow", then the document is invalid.) Therefore, you should make a greater effort to avoid conflicting element IDs. For example, id="indexes-nofollow" would be reasonable.

In addition, your code seems to be vulnerable to HTML injection. Since $label_text should be considered to contain arbitrary text, not just sane identifiers, it needs to be escaped.

Recommended rewrite:

echo form_label(
    form_checkbox($name.'[]', $check, $checked, "id=\"$name-$check\"") .
        htmlspecialchars($label_text),
    '',
    array('class' => 'checkbox')
);

… which would produce output (pretty-printed):

<label class="checkbox">
  <input type="checkbox" name="indexes[]" value="noindex" id="indexes-noindex" />No Index
</label>
<label class="checkbox">
  <input type="checkbox" name="indexes[]" value="nofollow" checked="checked" id="indexes-nofollow" />No Follow
</label>
…