Skip to main content
edited body
Source Link
200_success
  • 145.6k
  • 22
  • 191
  • 481

If you are using the mysqli functions, then do not call mysql_real_escape_string(), which is part of the deprecated mysql suite of functions. At the leaseleast, you should change it to call mysqli_real_escape_string() so that you are using the same suite of functions.

That said, using sprintf() to compose the query and escaping the string parameter yourself is not the best practice. A more foolproof way would be to use prepared statements, such that the library takes care of any necessary escaping for you.

$select_query = $conn->prepare("SELECT * FROM … WHERE email = ?");
$select_query->bind_param('s', $email);
if (! $select_query->execute()) {
    die("Database error");
}
if ($select_query->num_rows > 0) {
    die("Already subscribed");
}

I wouldn't use a SELECT query at all to detect whether an e-mail address is already subscribed. Rather, I would place a UNIQUE constraint on the email column, and catch any attempt to insert a duplicate.

Reasons for doing it that way are:

  • It's generally good practice to enforce such uniqueness constraints in the database schema. It makes the schema more foolproof and self-documenting. It also automatically creates an index on the email column, which could improve search performance.
  • If you enforce uniqueness in the PHP code rather than in the database, then you are vulnerable to a race condition. If two requests come in at the same time, it's possible that one of them could manage to sneakily perform an insert in between the other request's select and insert.
  • Eliminating a SELECT query would make it more efficient. You should be checking the result of the INSERT anyway — which you don't. (Your code ignores $result2.)

If you are using the mysqli functions, then do not call mysql_real_escape_string(), which is part of the deprecated mysql suite of functions. At the lease, you should change it to call mysqli_real_escape_string() so that you are using the same suite of functions.

That said, using sprintf() to compose the query and escaping the string parameter yourself is not the best practice. A more foolproof way would be to use prepared statements, such that the library takes care of any necessary escaping for you.

$select_query = $conn->prepare("SELECT * FROM … WHERE email = ?");
$select_query->bind_param('s', $email);
if (! $select_query->execute()) {
    die("Database error");
}
if ($select_query->num_rows > 0) {
    die("Already subscribed");
}

I wouldn't use a SELECT query at all to detect whether an e-mail address is already subscribed. Rather, I would place a UNIQUE constraint on the email column, and catch any attempt to insert a duplicate.

Reasons for doing it that way are:

  • It's generally good practice to enforce such uniqueness constraints in the database schema. It makes the schema more foolproof and self-documenting. It also automatically creates an index on the email column, which could improve search performance.
  • If you enforce uniqueness in the PHP code rather than in the database, then you are vulnerable to a race condition. If two requests come in at the same time, it's possible that one of them could manage to sneakily perform an insert in between the other request's select and insert.
  • Eliminating a SELECT query would make it more efficient. You should be checking the result of the INSERT anyway — which you don't. (Your code ignores $result2.)

If you are using the mysqli functions, then do not call mysql_real_escape_string(), which is part of the deprecated mysql suite of functions. At the least, you should change it to call mysqli_real_escape_string() so that you are using the same suite of functions.

That said, using sprintf() to compose the query and escaping the string parameter yourself is not the best practice. A more foolproof way would be to use prepared statements, such that the library takes care of any necessary escaping for you.

$select_query = $conn->prepare("SELECT * FROM … WHERE email = ?");
$select_query->bind_param('s', $email);
if (! $select_query->execute()) {
    die("Database error");
}
if ($select_query->num_rows > 0) {
    die("Already subscribed");
}

I wouldn't use a SELECT query at all to detect whether an e-mail address is already subscribed. Rather, I would place a UNIQUE constraint on the email column, and catch any attempt to insert a duplicate.

Reasons for doing it that way are:

  • It's generally good practice to enforce such uniqueness constraints in the database schema. It makes the schema more foolproof and self-documenting. It also automatically creates an index on the email column, which could improve search performance.
  • If you enforce uniqueness in the PHP code rather than in the database, then you are vulnerable to a race condition. If two requests come in at the same time, it's possible that one of them could manage to sneakily perform an insert in between the other request's select and insert.
  • Eliminating a SELECT query would make it more efficient. You should be checking the result of the INSERT anyway — which you don't. (Your code ignores $result2.)
Source Link
200_success
  • 145.6k
  • 22
  • 191
  • 481

If you are using the mysqli functions, then do not call mysql_real_escape_string(), which is part of the deprecated mysql suite of functions. At the lease, you should change it to call mysqli_real_escape_string() so that you are using the same suite of functions.

That said, using sprintf() to compose the query and escaping the string parameter yourself is not the best practice. A more foolproof way would be to use prepared statements, such that the library takes care of any necessary escaping for you.

$select_query = $conn->prepare("SELECT * FROM … WHERE email = ?");
$select_query->bind_param('s', $email);
if (! $select_query->execute()) {
    die("Database error");
}
if ($select_query->num_rows > 0) {
    die("Already subscribed");
}

I wouldn't use a SELECT query at all to detect whether an e-mail address is already subscribed. Rather, I would place a UNIQUE constraint on the email column, and catch any attempt to insert a duplicate.

Reasons for doing it that way are:

  • It's generally good practice to enforce such uniqueness constraints in the database schema. It makes the schema more foolproof and self-documenting. It also automatically creates an index on the email column, which could improve search performance.
  • If you enforce uniqueness in the PHP code rather than in the database, then you are vulnerable to a race condition. If two requests come in at the same time, it's possible that one of them could manage to sneakily perform an insert in between the other request's select and insert.
  • Eliminating a SELECT query would make it more efficient. You should be checking the result of the INSERT anyway — which you don't. (Your code ignores $result2.)