Page 1 of 1

delete statement does not work

Posted: Sun Aug 04, 2013 4:12 am
by viajero_yhz
Hello,
I previously posted the topic "Passing multiple variables with url php?" and have the solution requested.

meals_correction.php code snippet:

Code: Select all

<?php
			for ($i=0; $i<$num_result; $i++)
			{
				$row = mysqli_fetch_row($result);

				$url='meals_delete.php?id='.$row[0].'&ordertime='.$row[11];

			?>
			<tr>
			<td><span class="style1"><?php echo $row[2];?></span></td> <td><span class="style1"><?php echo $row[4];?></span></td> <td><span class="style1"><?php echo $row[5];?></span></td> <td><center>
			  <span class="style1"><?php echo '<a href='.$url.'>'. $row[0].'</a>';?></span></td>
meals_delete.php code:

Code: Select all

<?php

$id=$_GET['id'];
$ordertime=$_GET['ordertime'];
  
echo $ordertime;

  @ $db = new mysqli('…..');

  if (mysqli_connect_errno())
  {
     echo 'Error: Could not connect to database.  Please try again later.';
     exit;
  }

  $result = $db->query("DELETE FROM meals_entry WHERE ordertime = $ordertime ");

   $db->close();
 header("location:meals_entry.php");
  ?>
The problem is that the delete statement does not work. Yes, I have made certain that the field name in the table is correct, and I have retyped the delete statement just to make certain that there are no extraneous characters.

If I change the query WHERE to id = $id the code works, but from the previous post, the id is member-assigned, and a member could order more than one meal. Using id in the delete statement deletes all rows with the id, and LIMIT gets the first row encountered, not necessarily the meal to be deleted.

The echo $ordertime; statement was followed with exit; just to make certain that the ordertime variable was indeed passed, and it was.

The table meals_entry field ordertime is the last in the table and was set as time, but I have since set it to varchar just to see if that made a difference, but it does not.

Any ideas as to what is going on and how to resole the problem?

Thanks again for your help.

Cheers.

Re: delete statement does not work

Posted: Sun Aug 04, 2013 5:19 am
by requinix
1. Can you do a favor for me? Step away from the computer for an hour or so (if you've been sitting in front of it for a while), make a sandwich, watch some TV, maybe go out for a bit. Then come back and tell me right off the top of your head: in that code you posted, what does $row[5] represent? Do you know? Did you have to look at your code to find out? That's a bad sign.
Avoid mysqli_fetch_row() because all it gives you is those numbers, like 0 and 11. Instead try mysqli_fetch_assoc and use

Code: Select all

for ($i=0; $i<$num_result; $i++)
{
    $row = mysqli_fetch_row($result);

    $url='meals_delete.php?id='.$row['id'].'&ordertime='.$row['ordertime'];
2a. What is the "ordertime"? An actual TIME type? Sounds like you're saying it is. Your DELETE query is not only syntactically wrong but very unsafe: never ever put stuff from $_GET into a query without being absolutely paranoid about the exact value it has.
You're using mysqli. That's good. It offers prepared statements which let you bypass the whole "absolutely paranoid" step.

Code: Select all

$query = mysqli_prepare("DELETE FROM meals_entry WHERE ordertime = ?");
$query->bind_param("s", $ordertime); // or i or d, see the documentation for this function to know which to use
$query->execute();
2b. Additionally, is it guaranteed to be unique? If not, which if it's a time it is not, do you have anything that is? Because that's what you should be doing your DELETE on, not on some kind of date or time - a date or time that could match many more rows than you're expecting.

3.

Code: Select all

<?php echo '<a href='.$url.'>'. $row[0].'</a>';?>
That results in HTML like

Code: Select all

<a href=meals_delete.php?id=123&ordertime=something>123</a>
What's wrong with it is that you aren't using quotes around the href. Just like how your class="style1" in those <span>s has quotes, the href should also have quotes. So including the mysqli_fetch_assoc() change from #1,

Code: Select all

<?php echo '<a href="'.$url.'">'. $row['id'].'</a>';?>

Re: delete statement does not work

Posted: Mon Aug 05, 2013 6:36 am
by viajero_yhz
Hi requinix,

Thanks for your comments.

1. Yes, I do know what row[5] returns without looking at the code.

2. ordertime (yes it is the time the order was placed) is almost certain to be unique, that is why I chose to use it in the DELETE statement. id, meal, quantity and other information about the meals is not unique, but your point is well taken. Combining date and time in the DELETE statement would make an order absolutely unique.

meals_delete.php

Code: Select all

<?php

$id=$_GET['id'];
$ordertime=$_GET['ordertime'];
$todate=$_GET['todate'];

  @ $db = new mysqli('spencerhouse.ca', 'shonline', 'Ujmk8201db', 'onlinedb');

  if (mysqli_connect_errno())
  {
     echo 'Error: Could not connect to database.  Please try again later.';
     exit;
  }

  $query = mysqli_query("DELETE FROM meals_entry WHERE ordertime = $ordertime");
  
  $db->close();
Your DELETE query is not only syntactically wrong ...
I do not know what wrong with the syntax when this statement works:

$query = mysqli_query("DELETE FROM meals_entry WHERE id= $id");

but this one does not:

$query = mysqli_query("DELETE FROM meals_entry WHERE ordertime = $ordertime");

but I have resolved the problem.

The issue is with order time.. 09:32:07 for example. The DELETE statement could not find a match on contents of a column if there are colons or spaces, so I changed the column to order number with an auto_increment and the delete now works no problem.

Cheers!

Re: delete statement does not work

Posted: Mon Aug 05, 2013 6:53 am
by Celauran
viajero_yhz wrote:1. Yes, I do know what row[5] returns without looking at the code.
Will anybody else? Will you be the only person who has to deal with this code forever and always? I would not be amused if I inherited code that looked like that. Readability matters.
viajero_yhz wrote: ordertime (yes it is the time the order was placed) is almost certain to be unique, that is why I chose to use it in the DELETE statement. id, meal, quantity and other information about the meals is not unique
This just sounds like poor database design. Almost certain is not good enough. Every row needs a unique identifier. Typically this is simply called id, hence the confusion.
viajero_yhz wrote:
Your DELETE query is not only syntactically wrong ...
I do not know what wrong with the syntax when this statement works:

$query = mysqli_query("DELETE FROM meals_entry WHERE id= $id");

but this one does not:

$query = mysqli_query("DELETE FROM meals_entry WHERE ordertime = $ordertime");

but I have resolved the problem.
You need quotes around string values.

The point you seem to have glossed over here is by far the most important: sanitize your inputs! You're taking whatever the user gives you and dropping it right into a query.

Code: Select all

meals_delete.php?id=5; DROP TABLE users;
Fun stuff, right?

You can escape each field manually, but prepared statements really are the way to go. MySQLIi and PDO both support them. I strongly encourage you to read up on them.

Re: delete statement does not work

Posted: Mon Aug 05, 2013 7:12 am
by viajero_yhz
Thanks for the comment.
The point you seem to have glossed over here is by far the most important: sanitize your inputs! You're taking whatever the user gives you and dropping it right into a query.
The fields in the meals_entry table are populated by a form with selections from drop-down menus, thus eliminating a potential problem.

Cheers!