4
\$\begingroup\$

I have working code that gets product details and config values from a MySQL database, but it's combined with HTML. Is there a better way to do this, maybe a class that I can use?

<!DOCTYPE html>
<!--[if lt IE 7]>      <html class="no-js lt-ie9 lt-ie8 lt-ie7"> <![endif]-->
<!--[if IE 7]>         <html class="no-js lt-ie9 lt-ie8"> <![endif]-->
<!--[if IE 8]>         <html class="no-js lt-ie9"> <![endif]-->
<!--[if gt IE 8]><!--> <html class="no-js"> <!--<![endif]-->
<?php
require __DIR__ . '/vault/MySQL.php';

$id = ( isset( $_GET['pid'] ) && is_numeric( $_GET['pid'] ) ) ? intval( $_GET['pid'] ) : 0;
if ( $id != 0 ) {
try {

// select all data
// $query = "SELECT product_id, product_title, product_description, product_category, product_image FROM products WHERE product_id = ? LIMIT 1";
$query = "SELECT p.product_id AS product_id, p.product_title AS product_title, p.product_description AS product_description, categories.category_name AS category_name, p.product_category AS product_category, p.product_image AS product_image, p.product_image_thumb AS product_image_thumb FROM products p LEFT JOIN categories ON p.product_category=categories.category_id WHERE p.product_id = ? LIMIT 1";
$stmt = $con->prepare( $query );
$stmt->bindParam(1, $_REQUEST['pid'], PDO::PARAM_INT);
$stmt->execute();

// store retrieved row to a variable
$row = $stmt->fetch(PDO::FETCH_ASSOC);

// values to fill up our form
extract ($row);

} catch (PDOException $exception) { // to handle error
    echo "Error: " . $exception->getMessage();
}
    }
else        {
    echo "something went wrong";
    }
// select all data
$iQ = "SELECT config_name, config_value FROM config";
$stmt = $con->prepare( $iQ );
$stmt->execute();

while ($row = $stmt->fetch(PDO::FETCH_ASSOC) )
{
$config[$row['config_name']] = $row['config_value'];
}
?>
<head>
   <meta charset="utf-8">
   <!--[if IE]><meta http-equiv='X-UA-Compatible' content='IE=edge,chrome=1'><![endif]-->
   <title><?php echo $config['frontend_title'] . " - " . $product_title;?></title>
   <meta name="description" content="<?php echo "{$product_description}";?>">
\$\endgroup\$
1
\$\begingroup\$

Several things...

Indentation

This:

if ( $id != 0 ) {
try {
// etc.
} catch (PDOException $exception) { // to handle error
    echo "Error: " . $exception->getMessage();
}
    }

Would read easier as such:

if ( $id != 0 ) {
    try {
        // etc.
    } catch (PDOException $exception) { // to handle error
        echo "Error: " . $exception->getMessage();
    }
}

Comments

Many of your comments are of very little use, when the code speaks for itself. For instance:

    } catch (PDOException $exception) { // to handle error

Or:

// select all data

Or even:

// store retrieved row to a variable

SQL

You will save yourself and Mr. or Ms. Maintainer a good deal of grief if you format your SQL in a way that SQL programmers will be familiar with. For instance this:

$query = "SELECT p.product_id AS product_id, p.product_title AS product_title, p.product_description AS product_description, categories.category_name AS category_name, p.product_category AS product_category, p.product_image AS product_image, p.product_image_thumb AS product_image_thumb FROM products p LEFT JOIN categories ON p.product_category=categories.category_id WHERE p.product_id = ? LIMIT 1";

Could instead be:

$query = "SELECT 
            p.product_id AS product_id, 
            p.product_title AS product_title, 
            p.product_description AS product_description, 
            categories.category_name AS category_name, 
            p.product_category AS product_category, 
            p.product_image AS product_image, 
            p.product_image_thumb AS product_image_thumb 
        FROM products p 
        LEFT JOIN categories ON p.product_category=categories.category_id 
        WHERE p.product_id = ? LIMIT 1";

I will say that it is good that you are using PDO. More experienced PHP programmers will hopefully have more to say about PHP code, I'm mostly a SQL guy.

\$\endgroup\$
1
\$\begingroup\$

Error Messages

Going by the names, this seems to be an online shop or similar software with actual customers. In that case, error messages should be factual and non-insulting. You cannot know if this was an attacker, or someone who might just have made a mistake.

Handling Errors

Contrary to your comment, catching an exception, printing the message, and then continuing the script as if nothing happened is not handling an error. If you cannot recover from it, redirect to a custom error page or similar.

Style

Use correct indentation. This is really important and improves readability of your code a lot.

Request Types

Be consistent with your request types. Right now, you are checking GET, but retrieving REQUEST. So an attacker could post the pid value and get around your checks (which isn't that bad because you are using prepared statements, but still).

Return Early

You can reduce if nesting by returning early:

if ( $id == 0 ) {
    echo "Only numeric values allowed";
    return;
}

Loading Config Data

You are loading all config entries into an array, but you are only using frontend_title. It would be more efficient to only load what you actually need.

XSS

You are not sanitizing your data with htmlspecialchars, so anyone creating frontend titles, product titles, or product descriptions will be able to execute arbitrary Javascript on the clients machines, and can thus steal cookies, log passwords, deface the website, etc.

\$\endgroup\$
  • \$\begingroup\$ i thought to myself, you can't click a product with id=0 if it came from mysql ... so the person who did try to access a product with id 0 would be a "hacker" ... right ? \$\endgroup\$ – Alexander Br. Oct 25 '14 at 11:09
  • \$\begingroup\$ maybe. or maybe they followed a broken link, or maybe they changed the value in the URL because they had a product id memorized, but mistyped it. You never know, so you should assume good faith and reply with a friendly message. \$\endgroup\$ – tim Oct 25 '14 at 11:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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