Update Multiple Rows using $_POST

[bradgrafelman]

Your checkbox contains no information about the corresponding DB row that was used to generate it.

Rather than using a useless value like "delete" for all checkboxes, use the ID value to identify the correct tuple in the DB.

EDIT: Also note that this PHP code:

<?php $delete=isset($_POST['delete'])?true:false; ?>

does absolutely nothing and might as well be removed. (At least, nothing that makes any sense.)

[sneakyimp]

Unless I'm missing something, you don't have an input with name="id[]" which is going to cause problems with this statement:

$item_id = $_POST['id'][$i]; 

You're referring to a $_POST array that has not been defined. You should be careful testing this, you might end up deleting a lot of records in your db.

You might try this instead:

$delete_id = $_POST['delete'][$i]; 

[bradgrafelman]

sneakyimp;10993671 wrote:

Unless I'm missing something, you don't have an input with name="id[]" which is going to cause problems with this statement:

$item_id = $_POST['id'][$i]; 

Agreed. However, note that even if you added a hidden input element for each row, checkboxes are only included in the form data if they are checked, so there would be no way to correlate the id with the checkbox elements included in the form data.

sneakyimp;10993671 wrote:

You might try this instead:

$delete_id = $_POST['delete'][$i]; 

At present, that's just going to get you the string "delete" no matter which checkbox you check, hence why I suggested replacing that useless string with something more useful (such as the ID of the row to be deleted).

[veeps]

I'm still fairly new to this, and the references I've got don't explain very well how checkboxes work outside of email forms. I only know that they don't $_POST values automatically, as a text field might. So, this morning I did what I could with:

Rather than using a useless value like "delete" for all checkboxes, use the ID value to identify the correct tuple in the DB.

I got the id for each by changing adding <input ... value="<?php echo $row['image_id']; ?>" /> into the checkbox input. Then extracted values like this (it's not pretty, but it works):

if(isset($_POST['delete'][$i])) {
//get $_POST var
	$delete_id = $_POST['delete'][$i];
//find matching id in db and assign filename to var
	$sql = "SELECT filename FROM table_x WHERE col_name = 'deez_gals' && row_id = '$delete_id'";
	$result = mysql_query($sql) or die(mysql_error());
	$row = mysql_fetch_row($result);
	$file_name = $row[0];
//now delete from db
	$dsql = "DELETE FROM table_x WHERE col_name = 'deez_gals' && row_id = '$delete_id'";
	$deleted = mysql_query($dsql) or die(mysql_error());
//then, rm files from directory
	unlink(FILE_DIR.$file_name);
	unlink(FILE_DIR.'thumbs/'.$file_name);
}

If this is just plain bad, please let me know how I can fix. Otherwise, I hope this thread helps somebody.

[bradgrafelman]

Where is $i defined? Is this in some sort of loop?

You could get rid of the loop (and corresponding counter variable) altogether by simply doing a [man]foreach/man loop over the $_POST['delete'] array (assuming you've already verified that it is indeed an array rather than a nonexistent array element, such as in the case when no checkboxes were checked).

EDIT: Also, don't forget that user-supplied data should never be placed directly into a SQL query, else your code will be vulnerable to SQL injection attacks and/or just plain SQL errors.

Also note that numeric data should not be delimited with quotes. (Just throwing this out there, since we don't know what data type this 'col_id' column is.)

EDIT: Just revisited your code above... if the checkbox is checked, are you both DELETE'ing a row from one table while still UPDATE'ing data in another?

[sneakyimp]

It's been awhile since I did any forms like this, so I don't recall exactly how arrays of checkboxes work. I seem to recall that checking some checkboxes and not others may result in a non-contiguous array (meaning that array elements 1, 3, and 6 might be defined but 2, 4, and 5 would be undefined).

I would highly recommend taking the time to play with arrays of checkboxes outside the context of your script to get used to them. I would also recommend trying to use explicit array keys (e.g., [$i] rather than just []) so that all of the checkboxes that correspond to one item all share the same array key.

[bradgrafelman]

sneakyimp;10993691 wrote:

I seem to recall that checking some checkboxes and not others may result in a non-contiguous array (meaning that array elements 1, 3, and 6 might be defined but 2, 4, and 5 would be undefined).

It's quite the opposite, actually. PHP numbers whatever checkbox elements it receives at the time it processes form data, so you're always going to have a contiguous range of array indexes starting at 0.

And therein lies the problem; $_POST['delete'][0] could exist but that does not tell you which checkbox was checked. Instead, it only tells you what the value of the first checked checkbox contains (whichever checkbox that might've been).

The browser is simply sending PHP form data like:

delete[]=1&delete[]=3&delete[]=999

which PHP is nice enough to transform into an array for you by internally doing something like:

$_POST['delete'] = array();
$_POST['delete'][] = 1;
$_POST['delete'][] = 3;
$_POST['delete'][] = 999;

[veeps]

bradgrafelman;10993690 wrote:

Where is $i defined? Is this in some sort of loop?...

first, just edited the explanation of the working code. second, i used the for(loop) you recommended.

and, yes, this is protected and escaped text. Thanks, all!

[bradgrafelman]

veeps;10993695 wrote:

first, just edited the explanation of the working code. second, i used the for(loop) you recommended.

Right, but just to make sure... if a checkbox is checked, you're wanting to both DELETE a row from one table, and also UPDATE a row based on the form data? Seems to me that if you're wanting to DELETE the data presented, there's no UPDATE'ing to be done with whatever form data was submitted for that item. Instead, your code above is doing both.

veeps;10993695 wrote:

and, yes, this is protected and escaped text.

Not according to the code you posted... this:

veeps;10993689 wrote:

$delete_id = $_POST['delete'][$i]; 

is neither "protected" nor "escaped" before it is placed into two SQL queries.

[sneakyimp]

bradgrafelman;10993693 wrote:

It's quite the opposite, actually. PHP numbers whatever checkbox elements it receives at the time it processes form data, so you're always going to have a contiguous range of array indexes starting at 0.

Right! Sorry, had it backwards. This is why one needs to explicitly specify indexes (using [$i] rather than just []). Either that or put some value in the checkbox that lets you know which record/object/item it applies to rather than just having true or false.

veeps wrote:

and, yes, this is protected and escaped text.

The code you've posted looks pretty suspect to me. What's going to stop me from making my own form and specifying any delete ids I want? Or whatever edits I want?

[veeps]

The code works to update at least one of the (LIMIT) five possible results per page while allowing the possibility of deleting at least one of the others. That is/was the goal. So, the admin can delete one image and/or update another.

This particular page is protected by a login $SESSION, and the user is restricted to the bare minimum of db changes. And, I didn't think the delete checkbox requires any escaping, as nothing here is being INSERTED, only deleted (the value thereof being strictly INT). All other values are escaped (so mysql_real_escape_string($POST['keywords'][$i]) and such. )

Is it necessary to escape even the checkbox $_POST['delete'] value?

If I have time, I'll try to post most of the code here.

[bradgrafelman]

veeps;10993708 wrote:

The code works to update at least one of the (LIMIT) five possible results per page while allowing the possibility of deleting at least one of the others. That is/was the goal. So, the admin can delete one image and/or update another.

Right, but as it is right now your code is going to process every group of form fields and execute an UPDATE query, regardless of whether or not it also has deleted (or will delete in a future iteration) that same row in the DB.

veeps;10993708 wrote:

And, I didn't think the delete checkbox requires any escaping, as nothing here is being INSERTED, only deleted

The type of query is irrelevant. It could be SELECT, INSERT, UPDATE, DELETE, or WATERMELONS_ARE_GROSS and the fact remains that user-supplied data should not be inserted anywhere into the SQL query string without first being sanitized.

veeps;10993708 wrote:

(the value thereof being strictly INT)

How do you know? Who says the user-supplied data you're receiving is going to be in the same format that you assume it will be?

veeps;10993708 wrote:

Is it necessary to escape even the checkbox $_POST['delete'] value?

Is it user-supplied data? If so, yes. If not, no. (Hint: the answer is yes!)

It doesn't matter if it's a checkbox, text field, button, hidden input, HTTP header, or anything else - all user-supplied data should be considered suspect until it has been properly sanitized for use in a SQL query.

[sneakyimp]

It might help to read up on how SQL injection works. Consider this bit of php

$sql = "DELETE FROM my_table WHERE id='" . $_POST['user_submitted_id'] . "'";

What's to stop a user from concocting their own form which submits to your page (or a cURL script which POSTs to your page) wherein the user_submitted_id contains this:

1234' OR id > 0 OR id='0

As you can see, this would result in this query being run:

DELETE FROM my_table WHERE id='1234' OR id > 0 OR id='0'

This would have the result of deleting any record in my_table with id of zero or greater.

[veeps]

I like watermelons.

Okay. Got it.