It's not a recommendation, it's a must. Otherwise you are vulnerable to a race condition, where two simultaeneous inserts happen before the first one has the time to retrieve MAX(id), which will then be retrieving the id of the second insert.
NEVER use MAX(id) when you want to retrieve last insert id.
Derokorian;10986707 wrote:
I see nothing in there that would cause the mail function to be looping.
Neither do I, but since the email address is missing the 2 second times, you can at least prevent the unnecessary emails and inserts by checking that required input data is actually present, which should be done regardless of this issue.
Daadian;10986704 wrote:
I have narrowed my issue down to the select queires re triggering the insert query. I just can't figure out how to resolve the issue.**
Not quite. Even if there were triggers for select queries (there are only triggers for INSERT, UPDATE, DELETE, REPLACE), you'd "only" have data inserted several times, not several emails being sent out.
You do have some other possible things that could cause this behaviour.
- multiple includes/requires, in different places, of this script file
- multiple includes/requires of this file due to an include/require being inside a loop
- redirects to this file, either through header('location: ...') or javascript
Each of these cases can be found by doing a search in all files for the presence of this file's filename. Just make sure you don't just look for only multiple occurances but also consider loop iterations. It's entirely possible that I've overlooked other possibilities, so don't assume these are the only ones. But more on that later.
Daadian;10986704 wrote:
Then almost immediately, the supervisor will get two more emails, with the same information except missing the orignal users email
Which means you have a way of preventing these emails from being sent, by checking that the input data is actually present. And that should always be done anyway, regardless of if you have issues such as this or not. Also, and (perhaps) even more important: SANITIZE all external data so that it becomes safe to use. Else you will be very sorry some day.
Daadian;10986704 wrote:
Here is my code:
Please use php tags instead of code tags for php code. Also indent your code properly so it's possible to follow the program flow.
$link = mysqli_connect("$dbServer1", "$dbUser1", "$dbPass1") or die("Could not connect post");
mysqli_select_db($link, "$dbName1") or die("Could not select database your_db_name1");
/* NEVER use input data without properly sanitizing it
* cast itegers and floats, escape strings with mysqli_escape_string()
*/
/* Also, you should be checking that the needed information is really present and
* not just assume it is.
*/
$job = isset($_GET['jobnumber']) ? (int) $_GET['jobnumber'] : false;
$vend = isset($_GET['vendor']) ? mysqli_escape_string($link, $_GET['vendor']) : false;
$cat = isset($_GET['category']) ? (int) $_GET['category'] : false;
$amt = isset($_GET['amount']) ? (float) $_GET['amount'] : false;
$note = isset($_GET['note']) ? mysqli_escape_string($link, $_GET['note']) : false;
$email = isset($_SERVER['HTTP_RIM_DEVICE_EMAIL']) ?
mysqli_escape_string($link, $_SERVER['HTTP_RIM_DEVICE_EMAIL']) :
false;
/* Should any of these not be required, simply remove them
* Since you claimed $email was empty when things failed, that one should
* definitely be checked though
*/
if ($job && $vend && $cat && $amt && $note && $email)
{
$sql = mysqli_query($link, "insert po (job,vend,cat,amount,user,notes,date) values ('$job','$vend','$cat','$amt','$email','$note',NOW())");
if (!$sql)
{
print("FAILED");
}
else
{
print("SUCCESS");
/* VERY IMPORTANT: use mysql_insert_id()
* NEVER EVER use MAX(id) - Not for this purpose anyway
*/
$po = mysqli_insert_id($link);
# Restructuring of code to be able to handle failures of $query2
$subject = "Purchase Order Request";
$supsubject = "FYI Purchase Order Completion";
$body = "Your purchase order request has been completed. \n \n Your PO number is: $po \n \n Job Number:$job \n Vendor:$vend \n Category:$cat \n Amount:$amt \n Items: \n $note \n \n A copy of this email has been sent to your supervisor.";
$supbody = "A purchase order request has been completed. \n \n PO number $po was issued to $email for the following: \n \n Job Number:$job \n Vendor:$vend \n Category:$cat \n Amount:$amt \n Items: \n $note \n \n Please address this if this request is in error.";
mail($email, $subject, $body);
$query2 = "SELECT supervisoremail FROM job WHERE jobnum = $job";
$result2 = mysqli_query($link, $query2) Or die(mysqli_error($link));
/* Also, ALWAYS check what is returned by mysqli_query (and any other query
* which uses return values to indicate success or failure)
*/
if (!$result2)
{
# error_log() / trigger_error()
}
else
{
/* As pointed out before, don't foreach over a reulst set containing 1 row.
* The effect will be identical, but you confuse the hell out of anyone reading
* the code, since they will automatically assume you are dealing with several
* rows and thus make not only errnoneous assumptions about the program flow
* but also the database.
*/
if($row2 = mysqli_fetch_assoc($result2))
{
mail($row2['supervisoremail'], $supsubject, $supbody);
}
}
}
}
# And otherwise gather what information you can to see if you can find the cause for the problem
# instead of just getting rid of the symptom by checking the presence of $email
else
{
$vars = array('job', 'vend', 'cat', 'amt', 'note', 'email');
$missing = array();
foreach ($vars as $var)
{
/* $var will be one of job, vend etc,
* so $$var will be one of $job, $vend etc,
* thus checking the original input data variables
*/
if (!$$var)
{
$missing[] = $var;
}
}
$s = sprintf('Missing or empty vars: %s%s', implode(', ', $missing), PHP_EOL);
$s = sprintf('HTTP_REFERER: %s%s', $_SERVER['HTTP_REFERER'], PHP_EOL);
$s = sprintf('%sBacktrace%s', PHP_EOL, PHP_EOL);
$d = debug_backtrace();
for ($i = 0; $i < 6 && $i < count($d); ++$i)
{
$s += sprintf('%s (%s): %s%s', $d[$i]['file'], $d[$i]['line'], $d[$i]['function'], PHP_EOL);
}
}
