15
\$\begingroup\$

Is there a more efficient way to write this? This script will be used heavily and I want to make sure I do not have any memory leaks or speed issues.

This script gets an input from a form and searches the database for that record.

<?php

date_default_timezone_set('America/Los_Angeles');
$hostname='xxx';
$username='xxx';
$password='xxx';
$dbname='xxx';

$db = mysql_connect($hostname,$username, $password) OR DIE ('Unable to connect to       database! Please try again later.');
mysql_select_db($dbname);

$input_id = $_POST['id'];  

$sql = "SELECT chart_id, CONCAT(first_name,' ',last_name) as full_name, expiration_date as exp, NOW() as now, DATE_FORMAT(date_of_birth,'%m-%d-%Y') as date_of_birth, DATE_FORMAT(expiration_date,'%M %d, %Y') as expiration_date, DATE_FORMAT(recommendation_date,'%M %d, %Y') as recommendation_date, notes
    FROM chart
    WHERE chart_id = '" . mysql_real_escape_string($input_id) . "'
   ";

$today = date("F j, Y");

$result = mysql_query($sql) or die(mysql_error());


if (mysql_num_rows($result) > 0) {
    $row = mysql_fetch_assoc($result);

if ($row['exp'] < $row['now']) {
    $row['status'] = 0;
}
else {
    $row['status'] = 1;
}

mysql_free_result($result);    
}
else {
    $row['status'] = 'none';
}

mysql_close($db);

?>
\$\endgroup\$
  • \$\begingroup\$ Deleted previous comment. SQL injection is not so much a concern here, however I feel that hard-coding auto-commit SQL into a script such as PHP that is often visible to the end-user is just asking to be messed with. I'll write you an answer on that a bit later tonight. \$\endgroup\$ – Phrancis May 28 '14 at 21:55
  • \$\begingroup\$ Not really worth an answer, but you could reduce that if/else block in the final results to $row['status'] = (int) ($row['exp'] >= $row['now']); \$\endgroup\$ – MrLore May 29 '14 at 0:26
8
\$\begingroup\$

Efficiency

Nothing to worry about. It's only a linear script, there are no loops anywhere. You don't need to worry about efficiency.

Security and best-practices horrible practices

I should really have you write down this sentence 100 times on a piece of paper:

DO NOT USE THE MYSQL_* FUNCTIONS, THEY ARE DEPRECATED
DO NOT USE THE MYSQL_* FUNCTIONS, THEY ARE DEPRECATED
DO NOT USE THE MYSQL_* FUNCTIONS, THEY ARE DEPRECATED
DO NOT USE THE MYSQL_* FUNCTIONS, THEY ARE DEPRECATED
(...)

Although your SQL is technically safe, I think you can write down this sentence 2 times, and read up on what it is about:

GIVE ME PARAMETRIZED SQL OR GIVE ME DEATH
GIVE ME PARAMETRIZED SQL OR GIVE ME DEATH

There are other positive things with prepared queries / parametrized SQL than just security issues. I strongly recommend switching to prepared queries and not looking back. It's easy to miss using the mysql_real_escape_string function (and in my opinion it has an overly long function name). Switching to prepared queries helps with all these things, plus a couple of others.

There is also the fact that the mysql_* methods will be entirely removed in future versions of PHP. You are not the only one still using mysql_* methods unfortunately, and it is not the first time I'm saying this.

Start converting your code to use mysqli or PDO NOW. Nothing Else Matters! (Feel free to listen to that song while you convert your code).

\$\endgroup\$
  • 1
    \$\begingroup\$ I find that the tone of this answer is too alarmist. While it's true that mysql_*() functions are deprecated and parameterized queries are superior to ad hoc escaping, it should also be acknowledged that the posted code does not actually contain a security risk. \$\endgroup\$ – 200_success May 28 '14 at 21:37
  • \$\begingroup\$ @200_success When it comes to mysql_* methods, I go into alarm-mode. But technically, you seem to be right. Although a recent chat message implies that there are some very very rare edge cases where it can be a security risk. \$\endgroup\$ – Simon Forsberg May 28 '14 at 21:46
  • \$\begingroup\$ @200_success I edited the answer to reduce the alarm level. Thanks for your comment. \$\endgroup\$ – Simon Forsberg May 28 '14 at 22:08
  • 2
    \$\begingroup\$ Can I do for(var i = 0; i < 100; i++) { Console.WriteLine("DO NOT USE THE MYSQL_* FUNCTIONS, THEY ARE DEPRECATED"); }? \$\endgroup\$ – Mathieu Guindon May 29 '14 at 4:03
8
\$\begingroup\$

Simon André Forsberg's answer is great for the PHP sections. I would like to briefly address your SQL section. I have a problem with hard-coding auto-commit SQL into a script in another programming dialect. You can achieve same or better result by using stored procedures in SQL with the following advantages:

  1. [removed]

  2. You can call the same procedure(s) from multiple PHP scripts without having to rewrite any SQL script.

  3. If/when you need to modify the SQL portion for any reason, instead of having to change every PHP script to fit, all you have to do is change the procedure in SQL, which takes minutes and is easy to debug if need be.

So, in your context, here is my suggestion. We can take your whole SQL statement and build a procedure from it in your MySQL client (or I guess you could execute it from PHP as a one-time SQL query, but easier in MySQL client). Please note, you only need to do this one time (per procedure).

DROP PROCEDURE IF EXISTS sp_FindChart;
DELIMITER //
CREATE PROCEDURE sp_FindChart(IN p_chart_id INT)
BEGIN
-- your original query
SELECT 
    chart_id, 
    CONCAT(first_name,' ',last_name) as full_name, 
    expiration_date as exp, 
    NOW() as now, 
    /* Unless the %m-%d-%Y format is crucial, 
    consider perhaps using CAST(date_of_birth AS date) etc. instead. 
    Format YYYY-MM-DD possibly faster to cast than concatenate */
    DATE_FORMAT(date_of_birth,'%m-%d-%Y') as date_of_birth, 
    DATE_FORMAT(expiration_date,'%M %d, %Y') as expiration_date, 
    DATE_FORMAT(recommendation_date,'%M %d, %Y') as recommendation_date, 
    notes
FROM chart
-- link it to the parameter from the procedure, this will be what PHP passes to MySQL
WHERE chart_id = p_chart_id;
END//
DELIMITER ;

Then all you have to do in PHP, instead of this:

$sql = "SELECT chart_id, CONCAT(first_name,' ',last_name) as full_name, expiration_date as exp, NOW() as now, DATE_FORMAT(date_of_birth,'%m-%d-%Y') as date_of_birth, DATE_FORMAT(expiration_date,'%M %d, %Y') as expiration_date, DATE_FORMAT(recommendation_date,'%M %d, %Y') as recommendation_date, notes
    FROM chart
    WHERE chart_id = '" . mysql_real_escape_string($input_id) . "'

Is this:

$sql = "CALL sp_FindChart('" . mysql_real_escape_string($input_id) . "' ")"

I think my PHP syntax is close anyways, I'm sure you get the idea. I would still recommend starting with Simon's suggestions first, this is secondary.

\$\endgroup\$
  • 1
    \$\begingroup\$ "I have a problem with hard-coding auto-commit SQL into a script in another programming dialect, particularly one which is visible to the end user, such as PHP." PHP is a server-sided language. The SQL calls being executed is not visible to the end-user, only to the programmer who wrote the PHP code. (As long as the programmer doesn't echo the SQL to the user of course) \$\endgroup\$ – Simon Forsberg May 29 '14 at 11:04
  • \$\begingroup\$ I've seen PHP coded right into HTML web pages, I guess this may not be the case. Sorry that my knowledge of PHP is somewhat limited. \$\endgroup\$ – Phrancis May 29 '14 at 14:52
  • 1
    \$\begingroup\$ -1 This post contains factually incorrect information. PHP is not exposed to the user. Also putting SQL into stored procedures is generally not recommended. \$\endgroup\$ – pgraham May 30 '14 at 20:27
  • \$\begingroup\$ I think your -1 is not warranted. PHP can and is used in user-visible ways see: php.about.com/od/learnphp/qt/php_with_html.htm Since the OP did not say either way, there is no way to know. Also on stored procedures, that post on SO is largely that person's opinion. I've been told by several experienced SQL developers that Procs are much better, faster and safer. techrepublic.com/article/… \$\endgroup\$ – Phrancis May 30 '14 at 20:40
  • \$\begingroup\$ I disagree, I think that SPROCs are best used for situations where you would otherwise write the query in your code for creating the page. keep the DB and the UI separate. the SPROC can be changed without needing to change code on the site itself (or rebuild the site). PHP is run on the server and is not sent to the client, the user will not see PHP code. it is just like any other server side scripting language. \$\endgroup\$ – Malachi May 30 '14 at 20:49
4
\$\begingroup\$

In addition to what others already said, it's not good to edit the $row variable that was returned by a query, I mean this one:

if ($row['exp'] < $row['now']) {
    $row['status'] = 0;
}
else {
    $row['status'] = 1;
}

It would have been better to write that condition in the query itself, like this:

expiration_date > NOW() as status

This way you will have this value directly in $row['status'] without having to calculate it in PHP code.

And here's an example running your query using mysqli and prepared statements:

$mysqli = new mysqli($hostname, $username, $password, $dbname);
if ($mysqli->connect_errno) {
    echo "Failed to connect to MySQL: (" . $mysqli->connect_errno . ") " . $mysqli->connect_error;
}

$sql = "SELECT chart_id, CONCAT(first_name, ' ', last_name) as full_name,
        expiration_date > NOW() as status,
        date_of_birth, expiration_date, recommendation_date, notes
    FROM chart
    WHERE chart_id = ?";

if (!($stmt = $mysqli->prepare($sql))) {
    echo "Prepare failed: (" . $mysqli->errno . ") " . $mysqli->error;
}

if (!$stmt->bind_param("i", $input_id)) {
    echo "Binding parameters failed: (" . $stmt->errno . ") " . $stmt->error;
}

if (!$stmt->execute()) {
    echo "Execute failed: (" . $stmt->errno . ") " . $stmt->error;
}

if (!($res = $stmt->get_result())) {
    echo "Getting result set failed: (" . $stmt->errno . ") " . $stmt->error;
}

$row = $res->fetch_assoc();

I don't know how you use $row['date_of_birth'] and the other dates in the rest of your code. If you ever convert them to real dates, then it's better to drop the DATE_FORMAT from the query, and only convert to strings later in the code when you really need it.

\$\endgroup\$

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.