3
\$\begingroup\$

I am slowly learning PHP and have been using prepared statements.

I have a simple category.php page, which takes a category tag to display posts from that category, such as localhost/php/category.php?tag=sysadmin. I want to select the Category Title from the "j_oracle_cat" table based on the value of the incoming tag in the querystring.

My code seems to be extremely cumbersome and long-winded (I'm using MySQLi instead of PDO):

<?php
/* connection string up here */

/* get category tag */
$foo = $_GET['tag'];

/* if it contains an apostrophe go to homepage */
$foo2 = strpos($foo, '\'');
if ($foo2 > 0) { Header("Location: default.php"); }

/* sql */
$cat_title_sql = "SELECT fld_title FROM j_oracle_cat WHERE fld_tag = ? LIMIT 1";

/* category variable */
$foo_cat = $foo;

/* initialise and prepare */
$stmt = $conn->stmt_init();
$result_cat_title = $stmt->prepare($cat_title_sql);
if(!$result_cat_title) { trigger_error("Error preparing statement: $stmt->error<br/>SQL query: $cat_title_sql", E_USER_ERROR); }

/* bind */
$result = $stmt->bind_param('s',$foo_cat);
if(!$result) { trigger_error("Error binding parameter: $stmt->error<br/>", E_USER_ERROR); }

/* execute */
$stmt->execute();

/* store */
$stmt->store_result();
$count_cat = $stmt->num_rows;

/* if no results go to homepage */
if ($count_cat == 0) {
    Header("Location: ../default.php");
    } elseif ($result) {
        $stmt->bind_result($fld_title);
        while ($stmt->fetch()) {
            $cat_ttl = $fld_title;
        }
}

/* ... header include down here */
?>
<title><?php echo $cat_ttl?></title>

I just wondered if I am making a meal of this, and if the code is over the top, or if I am on the right track (both with the prepared statement and handling the incoming querystring variable).

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$
$foo2 = strpos($foo, '\'');
if ($foo2 > 0) { Header("Location: default.php"); }
  1. Creating another variable for this, especially with a generic name like "foo2", is superfluous and should be avoided. if (strpos(..) > 0) will do just fine.
  2. Just redirecting with a header does not stop the script execution. Your script will continue to run, making this rather pointless. You need to exit explicitly.
  3. This check will fail if the quote is the first character, since strpos returns 0 in that case.
  4. Why are you worrying about quotes in particular at all? There's really no point to this.

What you may want to check for instead is whether the value is defined at all.

$foo_cat = $foo;

There's no point in reassigning this variable yet again.

$result_cat_title = $stmt->prepare($cat_title_sql);
if(!$result_cat_title) ...

Using yet another rather mis-named variable is pretty superfluous, again.

trigger_error(..)

If you're using a (poorly) user-defined error handler (or somebody who may be using this code in the future adds one), your script may not actually exit when you trigger an error. You should either still explicitly exit after this, or perhaps switch to exceptions.

$count_cat = $stmt->num_rows;

Again a rather superfluous variable assignment.

Header("Location: ../default.php");

Again a header which does not terminate script execution.
You should also consider indicating a 404 Not Found error instead of a silent redirect.

else if ($result)

Why are you checking $result here? That was a variable used in completely different context. Also, since the previous header statement was supposed to terminate the script, you can skip this check entirely.

while ($stmt->fetch())

You're only expecting a single result, no need for a loop.

$cat_ttl = $fld_title;

Yet another superfluous variable assignment.

<title><?php echo $cat_ttl?></title>

You should take care of HTML injection/syntax issues and HTML-encode your output appropriately.

Beyond this indentation and variables names are often sub-optimal.

In summary:

<?php

if (!isset($_GET['tag'])) {
    header('HTTP/1.0 400 Bad Request');
    exit;
}

/* connection string up here */

$stmt = $conn->stmt_init();

$query = 'SELECT ld_title FROM j_oracle_cat WHERE ld_tag = ? LIMIT 1';
if (!$stmt->prepare($query)) {
    throw new Exception("Error preparing statement: $stmt->error, SQL query: $query");
}

if (!$stmt->bind_param('s', $_GET['tag'])) {
    throw new Exception("Error binding parameter: $stmt->error");
}

$stmt->execute();
$stmt->store_result();

if ($stmt->num_rows == 0) {
    header('HTTP/1.0 404 Not Found');
    exit;
}

$stmt->bind_result($title);
$stmt->fetch();

?>
<title><?php echo htmlspecialchars($title); ?></title>
\$\endgroup\$
2
  • \$\begingroup\$ Thanks @deceze, very helpful and informative that was too! Re. taking care of SQL injection issues, I thought that if I used a prepared statement, that I don't need to worry too much about things like that? That was the reason I was trying to trap a single speech mark / apostrophe? \$\endgroup\$
    – 4532066
    Commented Feb 16, 2016 at 5:52
  • \$\begingroup\$ You don't need to worry about SQL injection with prepared statements and hence there's no point in worrying about single quotes either. You still need to worry about other sorts of injection in other contexts, like HTML. See kunststube.net/escapism. \$\endgroup\$
    – deceze
    Commented Feb 16, 2016 at 6:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.