Pages always defaulting to error page

[billyboy]

I've got the folllowing function which works... almost. With the else part added, no matter what link is selected the error page is returned. Without it the function works as expected with the appropriate file included and variables echoed, depending on the link selected.

<?php
function createContent($arr) {
$id = $_GET['id'];
  for ($i = 0; $i < count($arr); $i++) {
    if (!isset($id)) { // default page
      $inc = "includes/{$arr[0][0]}.php";
      $x = $arr[0][1];
      $y = $arr[0][2];
      $z = $arr[0][3];
    }
    elseif ($id == $arr[$i][0]) { // link is selected
      $inc =  "includes/{$arr[$i][0]}.php"; 
      $x = $arr[$i][1];
      $y = $arr[$i][2];
      $z = $arr[$i][3];
    }
    else { // invalid URL
      $inc = 'includes/error.php';
      $x = 'Error';
      $y = '';
      $z = '';
      header('HTTP/1.0 404 Not Found');
    }
    $pg = basename($_SERVER['PHP_SELF']); // create links
    $link = "  <li><a href=\"{$pg}?id={$arr[$i][0]}\">{$arr[$i][1]}</a></li>\n";
    if ($id == $arr[$i][0] || !isset($id) && strstr($link, $arr[0][0])) {
      $link = str_replace($link, "  <li id=\"current\">{$arr[$i][1]}</li>\n", $link);
    }
  $links[] = $link;
  }
  $vars = array($inc,$x,$y,$z,$links);
  return $vars;
}
?>

The values are stored in a 2 dimensional array.

$arr = array(array('home', 'Home Page', 'a aa aaa.', 'z, zz, zzz,'), 
             array( 'about', 'About Us' etc...

I suspect the problem may lie in the condition for the elseif part but don't know how to fix this.

[mwinter]

$id = $_GET['id'];
  for ($i = 0; $i < count($arr); $i++) {
    if (!isset($id)) { // default page

To avoid generating notices, test the superglobal, first:

if (!isset($_GET['id'])) { /* Default page */
    /* ... */
}
for (... 

It's also more efficient as this test only needs to be performed once.

The error-testing code also needs to be removed from within the loop; you should be checking if valid no link was selected once the list has been examined.

Consider:

function createContent($arr) {
    /* Assume the worst and prepare for an incorrect id parameter. */
    $id = null;
    $validId = false;
    if (isset($_GET['id'])) {
        /* Consider validating the type of value passed. For instance,
         * if id should be a number, check that it actually is!
         */
        $id = $_GET['id'];
    } else {
        /* No id parameter was passed. Whilst not a valid id,
         * it's not an error, either.
         */
        $validId = true;
        $inc = "includes/{$arr[0][0]}.php";
        $x = $arr[0][1];
        $y = $arr[0][2];
        $z = $arr[0][3];
    }
    for ($i = 0, $n = count($arr); $i < $n; ++$i) {
        /* Generate a special list item for the current element, or
         * for the initial item if the default page is served.
         */
        if (($id == $arr[$i][0]) || (($i == 0) && !$id)) {
            /* I strongly recommend that you use a class
             * attribute here, not an id attribute.
             */
            $link = "  <li id=\"current\">{$arr[$i][1]}</li>\n";
            if ($id) {
                $validId = true;
                $inc = "includes/{$arr[$i][0]}.php";
                $x = $arr[$i][1];
                $y = $arr[$i][2];
                $z = $arr[$i][3];
            }
        } else {
            $path = basename($_SERVER['PHP_SELF']);
            $link = "  <li><a href=\"{$path}?id={$arr[$i][0]}\">{$arr[$i][1]}</a></li>\n";
        }
        $links[] = $link;
    }
    if (!$validId) {
        $inc = "includes/error.php";
        $x = 'Error';
        $y = '';
        $z = '';
        header('HTTP/1.1 404 Not Found');
    }
    return array($inc, $x, $y, $z, $links);
} 

Be sure to test this thoroughly; I haven't at all.

Mike

[billyboy]

Thanks Mike.

Boy I wish I understood PHP enough to write a piece of code like that and post it without testing! It works great!

Wondering if you would mind explaining what you mean by checking for the type of value passed and how that might be done, And I really don't understand what you did there with the $validId variable and whats happening.

Also, when I got sick of trying not to always get the error page, I made a few attempts at getting rid of the query string in the URL using .htaccess with the same amount of (non) success. Wondering if you could point me in direction of a good article/tutorial that a noob could understand.

Thanks again,
Bill

[mwinter]

Boy I wish I understood PHP enough to write a piece of code like that and post it without testing! It works great!

Things don't always go so smoothly when I don't test. Quite often, but not always.

Wondering if you would mind explaining what you mean by checking for the type of value passed and how that might be done

Upon reflection, it doesn't seem to be really necessary - not for the posted code, at least. The value is only used for comparison, not for accessing data: indexing an array, identifying a file or path, etc.

What I was suggesting was that you ensured that the data received from the user is actually what it was supposed to be. Consider what might happen if the code did use the id query component parameter to index a numerically-keyed array. If the value was a number and it was a number within range of the array bounds, there wouldn't be a problem. But what if the value was alphanumeric (or out-of-bounds)? The attempt would fail because the index is undefined, but the program would continue with bad data.

The most cautious test would be to pass the value through a regular expression, then check bounds. For instance,

function isNaturalNumber($string) {
    return (bool) preg_match('/^[1-9][0-9]*$/', $string);
}

if (!isNaturalNumber($id) || ($id >= count($data)) {
    /* Not a natural number (an integer in the unbounded
     * set {0, 1, 2, ...}) or exceeds the upper bound of $data.
     */
}
$id = (int) $id; /* Cast the string to a integer */ 

If the test fails, the code could resort to default behaviour, such as it does if the id parameter isn't passed at all.

And I really don't understand what you did there with the $validId variable and whats happening.

The error condition from your posted code seemed to revolve around whether the value of $id matched a value in the $arr array. Initially, my code assumes no ($validId is false). If no id parameter was passed, there will never be a match, but this acceptable so $validId is set to true. After this point, the value will only change within the loop, and only if a match is found. Once the loop exits, an exhaustive search of the array will have been completed, so if the value of $validId is still false, $id contains an invalid identifier and a 404 HTTP response is generated.

Within the loop, the link generation code is more-or-less the same, but rather than edit the first attempt at creating the list item if the current link is for the current document, the code path branches earlier.

On that note, it might be better if the two if statements within the loop are modified somewhat so that each begins:

if (($id == $arr[$i][0]) || (($i == 0) && ($id === null))) { 

and

if ($id !== null) { 

respectively. If zero (0), or some other non-null value that type-converts to false is legal is valid, the code will misbehave.

Also, when I got sick of trying not to always get the error page, I made a few attempts at getting rid of the query string in the URL using .htaccess with the same amount of (non) success. Wondering if you could point me in direction of a good article/tutorial that a noob could understand.

The Apache server documentation contains lots of examples for the mod_rewrite module. However, most of the tricks require access to the server configuration files so you may not be able to make the changes yourself. It depends on what sort of scheme you want to implement.

Mike