1
\$\begingroup\$

I have an HTML file that contains a submit form, which asks the users the fill in their personal info:

enter image description here

then it will post and store into the DB by method of PHP SQL:

 // Check input errors before inserting in database
if (empty($CName_err) && empty($Address_err) && empty($amount_err) && empty($Phone_err)) {
    // Prepare an insert statement
    $pdo = Database::connect();
    $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $sql = "INSERT INTO database (CName, Address, Phone, Amount ,Ticket, Purpose) VALUES (?, ?, ?, ? ,?, ?)";

    $q = $pdo->prepare($sql);
    $q->execute(array($CName, $Address, $Phone, $amount ,$Ticket ,$Purpose));
    Database::disconnect();

Hence, any risks of an SQL injection attack in this case?

What should I do to improve my coding?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you get great answers. \$\endgroup\$ – Phrancis Jul 4 '18 at 0:47
-1
\$\begingroup\$

What should I do to improve my coding?


Address_err, amount_err

Your variable names have inconsistent capitalization.

CName, Address, Phone, Amount ,Ticket, Purpose
$CName, $Address, $Phone, $amount ,$Ticket ,$Purpose

The commas are sometimes on the left, sometimes on the right.

$sql

The contents are not "structured query language". Maybe name it "$query".

$pdo = Database::connect();

You should check to make sure you are connected before continuing.

$q

What does "q" stand for?

if (empty($CName_err) && empty($Address_err) && empty($amount_err) && empty($Phone_err))

Might be better to stop processing on first error.

if (!empty($CName_err)) {
    ...
    return
}

if (!empty($Address_err)) {
    ...
    return
}

[normal processing here]

If you don't need to handle separate errors, simply use a boolean.

if (errorOccurred) {...}
\$\endgroup\$
  • \$\begingroup\$ Hence, any risks of an SQL injection attack in this case? \$\endgroup\$ – moon Jul 4 '18 at 4:07
  • \$\begingroup\$ What you are using is called "prepared statements". I do not have sufficient knowledge of PHP to tell if you are vulnerable. stackoverflow.com/q/134099 may be of interest. \$\endgroup\$ – FromTheStackAndBack Jul 4 '18 at 4:12
  • \$\begingroup\$ "You should check to make sure you are connected before continuing." - PHP would take care of that. "Might be better to stop processing on first error." - PHP would take care of that. \$\endgroup\$ – Your Common Sense Sep 2 '18 at 19:02

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.