This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.
Security
You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id']
and $row['to']
. Otherwise, you might be vulnerable to second order SQL injection. See also here.
if (cond) return true; else return false;
if ($mailer->send()) {
return true;
} else {
return false;
}
can be rewritten as:
return $mailer->send();
Duplicate Code
$sql = 'UPDATE mail_queue
SET mail_status_id = [id]
WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
$sth5 = $dbh->prepare($sql);
$sth5->execute();
You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)
), or just save the error code in a local variable and execute this code at the end.
Confusing Check
You have this check:
if (count($queue) > 0) {
// prepare smtp server
}
// do stuff with the smtp server
For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach
is an implicit check itself). I would rewrite it like this:
if (count($queue) > 0) {
return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server
If you at some point extend the code, your code could easier lead to bugs.
Length of first Code Block
Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)
).
This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.
Security
You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id']
and $row['to']
. Otherwise, you might be vulnerable to second order SQL injection. See also here.
if (cond) return true; else return false;
if ($mailer->send()) {
return true;
} else {
return false;
}
can be rewritten as:
return $mailer->send();
Duplicate Code
$sql = 'UPDATE mail_queue
SET mail_status_id = [id]
WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
$sth5 = $dbh->prepare($sql);
$sth5->execute();
You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)
), or just save the error code in a local variable and execute this code at the end.
Confusing Check
You have this check:
if (count($queue) > 0) {
// prepare smtp server
}
// do stuff with the smtp server
For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach
is an implicit check itself). I would rewrite it like this:
if (count($queue) > 0) {
return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server
If you at some point extend the code, your code could easier lead to bugs.
Length of first Code Block
Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)
).
This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.
Security
You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id']
and $row['to']
. Otherwise, you might be vulnerable to second order SQL injection. See also here.
if (cond) return true; else return false;
if ($mailer->send()) {
return true;
} else {
return false;
}
can be rewritten as:
return $mailer->send();
Duplicate Code
$sql = 'UPDATE mail_queue
SET mail_status_id = [id]
WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
$sth5 = $dbh->prepare($sql);
$sth5->execute();
You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)
), or just save the error code in a local variable and execute this code at the end.
Confusing Check
You have this check:
if (count($queue) > 0) {
// prepare smtp server
}
// do stuff with the smtp server
For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach
is an implicit check itself). I would rewrite it like this:
if (count($queue) > 0) {
return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server
If you at some point extend the code, your code could easier lead to bugs.
Length of first Code Block
Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)
).
This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.
Security
You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id']
and $row['to']
. Otherwise, you might be vulnerable to second order SQL injection. See also here here.
if (cond) return true; else return false;
if ($mailer->send()) {
return true;
} else {
return false;
}
can be rewritten as:
return $mailer->send();
Duplicate Code
$sql = 'UPDATE mail_queue
SET mail_status_id = [id]
WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
$sth5 = $dbh->prepare($sql);
$sth5->execute();
You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)
), or just save the error code in a local variable and execute this code at the end.
Confusing Check
You have this check:
if (count($queue) > 0) {
// prepare smtp server
}
// do stuff with the smtp server
For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach
is an implicit check itself). I would rewrite it like this:
if (count($queue) > 0) {
return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server
If you at some point extend the code, your code could easier lead to bugs.
Length of first Code Block
Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)
).
This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.
Security
You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id']
and $row['to']
. Otherwise, you might be vulnerable to second order SQL injection. See also here.
if (cond) return true; else return false;
if ($mailer->send()) {
return true;
} else {
return false;
}
can be rewritten as:
return $mailer->send();
Duplicate Code
$sql = 'UPDATE mail_queue
SET mail_status_id = [id]
WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
$sth5 = $dbh->prepare($sql);
$sth5->execute();
You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)
), or just save the error code in a local variable and execute this code at the end.
Confusing Check
You have this check:
if (count($queue) > 0) {
// prepare smtp server
}
// do stuff with the smtp server
For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach
is an implicit check itself). I would rewrite it like this:
if (count($queue) > 0) {
return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server
If you at some point extend the code, your code could easier lead to bugs.
Length of first Code Block
Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)
).
This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.
Security
You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id']
and $row['to']
. Otherwise, you might be vulnerable to second order SQL injection. See also here.
if (cond) return true; else return false;
if ($mailer->send()) {
return true;
} else {
return false;
}
can be rewritten as:
return $mailer->send();
Duplicate Code
$sql = 'UPDATE mail_queue
SET mail_status_id = [id]
WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
$sth5 = $dbh->prepare($sql);
$sth5->execute();
You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)
), or just save the error code in a local variable and execute this code at the end.
Confusing Check
You have this check:
if (count($queue) > 0) {
// prepare smtp server
}
// do stuff with the smtp server
For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach
is an implicit check itself). I would rewrite it like this:
if (count($queue) > 0) {
return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server
If you at some point extend the code, your code could easier lead to bugs.
Length of first Code Block
Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)
).
This is not a complete review, just a couple of points I noticed. I didn't see any bugs, but I didn't test the code in-depth.
Security
You should use prepared statements even if the data comes from the database (like with $row['mail_queue_id']
and $row['to']
. Otherwise, you might be vulnerable to second order SQL injection. See also here.
if (cond) return true; else return false;
if ($mailer->send()) {
return true;
} else {
return false;
}
can be rewritten as:
return $mailer->send();
Duplicate Code
$sql = 'UPDATE mail_queue
SET mail_status_id = [id]
WHERE mail_queue_id = ' . $row['mail_queue_id'] . ';';
$sth5 = $dbh->prepare($sql);
$sth5->execute();
You have this code 3 times, I would either extract it to it's own function (updateMailQeue(errorCode)
), or just save the error code in a local variable and execute this code at the end.
Confusing Check
You have this check:
if (count($queue) > 0) {
// prepare smtp server
}
// do stuff with the smtp server
For a second, it confused be why only part of the code is inside this check (the answer is obviously that the foreach
is an implicit check itself). I would rewrite it like this:
if (count($queue) > 0) {
return; // nothing to send
}
// prepare smtp server
// do stuff with the smtp server
If you at some point extend the code, your code could easier lead to bugs.
Length of first Code Block
Your first try block is too long and too deeply nested. You could extract the code for each individual row in a new function (for example sendMail(mail_id)
).