Updating Database changes all entries

[Moorcam]

Hi all,

 

In a pickle again.

 

I am trying to update a database from a html table, which I will post below.

The issue is, if I have more than one entry in the table, clicking update will change all entries with the changes mate.

 

Here is the update code along with the HTML table:

                       <div class="panel-body">
                            <div class="table-responsive">
							<form role="form" action="" method="post">
<?php

 	if(isset($_POST['Submit'])){//if the submit button is clicked
	$id = mysqli_real_escape_string($mysqli, $_POST['id']);
	$fname = mysqli_real_escape_string($mysqli, $_POST['fname']);
	$lname = mysqli_real_escape_string($mysqli, $_POST['lname']);
	$email = mysqli_real_escape_string($mysqli, $_POST['email']);
	$phone = mysqli_real_escape_string($mysqli, $_POST['phone']);
	$sql="UPDATE clients SET fname='$fname', lname='$lname', email='$email', phone='$phone'";
	$mysqli->query($sql) or die(mysqli_error($mysqli));//update or error
	}

?>
                                <table class="table table-striped table-bordered table-hover" id="tab_logic">
                                    <thead>
                                        <tr>
                                            <th>Client ID</th>
                                            <th>First Name</th>
                                            <th>Last Name</th>
                                            <th>Email</th>
                                             <th>Phone</th>
                                        </tr>
                                    </thead>
<?php

if (isset($_POST['Delete'])){
    $checkbox = $_POST['checkbox'];
    $count = count($checkbox);

    for($i=0;$i<$count;$i++){

        if(!empty($checkbox[$i])){ /* CHECK IF CHECKBOX IS CLICKED OR NOT */
        $id = mysqli_real_escape_string($mysqli,$checkbox[$i]); /* ESCAPE STRINGS */
        mysqli_query($mysqli,"DELETE FROM clients WHERE id = '$id'"); /* EXECUTE QUERY AND USE ' ' (apostrophe) IN YOUR VARIABLE */

        } /* END OF IF NOT EMPTY CHECKBOX */

    } /* END OF FOR LOOP */

} /* END OF ISSET DELETE */

$sql = "SELECT id, fname, lname, email, phone FROM clients";
$result = $mysqli->query($sql);
if ($result->num_rows > 0) {
	while($row = $result->fetch_assoc()) {
	$id = mysqli_real_escape_string($mysqli, $row['id']);
?>
       				<tbody>
					<tr id='addr0'>
						<td>
						<input type="text" size="5" name='id'  placeholder='01' class="form-control" value="<?php echo $row['id']; ?>"/>
						</td>
						<td>
						<input type="text" name='fname'  placeholder='First Name' class="form-control" value="<?php echo $row['fname']; ?>"/>
						</td>
						<td>
						<input type="text" name='lname'  placeholder='Last Name' class="form-control" value="<?php echo $row['lname']; ?>"/>
						</td>
						<td>
						<input type="text" name='email' placeholder='Email' class="form-control" value="<?php echo $row['email']; ?>"/>
						</td>
						<td>
						<input type="text" name='phone' placeholder='Phone' class="form-control" value="<?php echo $row['phone']; ?>"/>
						</td>
						<td>
						<input name="checkbox" value="0" type="hidden">
						  <?php echo "<td><input type='checkbox' name='checkbox[]' value='$id'></td>"; ?>
						</td>
					</tr>
                    <tr id='addr1'></tr>
				</tbody>
									<?php
	}
}
$mysqli->Close();
?>
                                </table>
								<a href="new-client.php" type="submit" class="pull-left btn btn-success">Add New Client</a><button type="submit" name="Submit" class="btn btn-success">Save Changes</button>   <input type="submit" name="Delete" class="pull-center btn btn-success" value="Delete Selected" />
								</form>
                            </div>
                        </div>
                    </div>
                    </div>
                </div>
                </div>     

Please note that deleting works fine. Adding is done from a separate file.

Any help would be appreciated.

Cheers,

Dan

[Moorcam]

Silly Irish man.

I forgot to set a Unique key in the database.

Works fine now.

Sorry for the hassle and thanks again.

[Barand]

The unique key is the wrong answer. To limit the updates to the single row you need the the same WHERE clause that you have in the delete query.

 

Do not put user data directly into the queries - use prepared statements and pass the data as parameters, otherwise you are vulnerable to SQL injection attacks.

[NotionCommotion]

To reiterate what Barand said, use prepared statements.  I know it is just another thing to learn and that you already know the way you are doing it, however, they are easy and secure, and once you do start using them, you will wonder why you didn't start earlier.  There are multiple ways to implement them, but 99% of the time I just use associated arrays with :whatever placeholders (first example) or unassociated arrays with ? placeholders (second example).  If there are just a few pieces of data, I use ? placeholders, otherwise I use : placeholders.

<?php

//You should first validate your data before inserting it.  I feel it is sometimes okay to use the DB for some validation, however, others will disagree.

$data=['id'=>$_POST['id'],'fname'=>$_POST['fname'],'lname'=>$_POST['lname'],'email'=>$_POST['email'],'phone'=>$_POST['phone']];
$stmt=$db->prepare("UPDATE clients SET fname=:fname, lname=:lname, email=:email, phone=:phone WHERE id=:id");
$stmt->execute($data);

// or

$data=[$_POST['fname'],$_POST['lname'],$_POST['email'],$_POST['phone'],$_POST['id']];
$stmt=$db->prepare("UPDATE clients SET fname=?, lname=?, email=?, phone=?WHERE id=?");
$stmt->execute($data);

[Jacques1]

The OP is using mysqli, so it's not quite that easy:

$client_update_stmt = $mysqli->prepare('
    UPDATE clients
    SET
        fname = ?,
        lname = ?,
        email = ?,
        phone = ?
    WHERE id = ?
');
$client_update_stmt->bind_param('ssssi', $_POST['fname'], $_POST['lname'], $_POST['email'], $_POST['phone'], $_POST['id']);
$client_update_stmt->execute();

There doesn't seem to be any kind of CSRF protection, though. This means anybody can manipulate or delete the data by making the admin visit a website with a piece of JavaScript code or a button that triggers a POST request.

 

You need to fix that. Besides session-based authentication (which I hope you've already implemented), you also need to authenticate every single POST request with a secret token.

[NotionCommotion]

The OP is using mysqli, so it's not quite that easy:

 

Providing that one has the ability to modify the server, is there any reason to use mysqli anymore?

[Jacques1]

No. mysqli should be phased out entirely or at least turned into a “hidden” extension for special purposes. But as long as its marketed as an easy way out of the ext/mysql deprecation, it won't go away anytime soon.

[Moorcam]

Thanks for the feedback and input guys. I really appreciate it.

I haven't used php and mysql for a lifetime. Recently just started to get back into it. So a bit of a learning curve with a dash of hit and miss as I go. So I really appreciate the guidance.

 

People are raving about this PDO thingy. Will this work on MySQL servers? I have been told it is a lot more secure than MySQLi etc.

 

I really need to find the time (between work etc) to sit down and actually read up on all of these changes that were made since I did it around 2005 lol

[Jacques1]

People are raving about this PDO thingy. Will this work on MySQL servers? I have been told it is a lot more secure than MySQLi etc.

 

It's not. But it's a lot easier to use and works with all mainstream database systems, not just MySQL.

 

mysqli is a low-level interface, which means the programmer has to do a lot of work to get things done, and many steps aren't very intuitive. Executing a prepared statement and fetching the data requires no less than five(!) different functions. With PDO, you just need PDO::prepare(), PDOStatement::execute() and one of the fetch methods (you can even iterate over the result set with a foreach loop).

 

Even worse, mysqli creates a “vendor lock-in”. You can't just switch to a different database system, even if it can run all your SQL queries just fine. You'd have to go through your entire code, remove the mysql parts, learn a new interface and start all over again. With PDO, you just have to change the parameters of the initial connection and maybe update a few queries where you use MySQL-specific syntax.

[Moorcam]

It's not. But it's a lot easier to use and works with all mainstream database systems, not just MySQL.

 

mysqli is a low-level interface, which means the programmer has to do a lot of work to get things done, and many steps aren't very intuitive. Executing a prepared statement and fetching the data requires no less than five(!) different functions. With PDO, you just need PDO::prepare(), PDOStatement::execute() and one of the fetch methods (you can even iterate over the result set with a foreach loop).

 

Even worse, mysqli creates a “vendor lock-in”. You can't just switch to a different database system, even if it can run all your SQL queries just fine. You'd have to go through your entire code, remove the mysql parts, learn a new interface and start all over again. With PDO, you just have to change the parameters of the initial connection and maybe update a few queries where you use MySQL-specific syntax.

Thanks for that.

Well, it looks like PDO is the way to go then. It had been suggested before by bananamen but never got round to using it.

Will be away for a few days but will have the laptop so if I get a few hours on New Years Day will change to PDO.

Thanks again and Happy New Year.

[mac_gyver]

your code and queries are only as secure as you make them. if you use a prepared query incorrectly, it will won't be secure.

 

by using a true prepared query (the PDO extension has emulated prepared queries turned on by default for all database types, despite what the documentation states, and you need to turn them off when you make the database connection) to supply data values to the sql query statement, your sql query will be safe against sql special characters in the data from breaking the sql syntax or breaking out of, i.e. escaping from, the sql syntax in the case of intentionally injected sql.

 

after you choose the php database extension you are going to use, the posted code needs to do or not do a number of things -

 

1) you are mixing procedural and OOP style for the msyqli statements. you should be constant in your programming. using the PDO extension will fix this since it only has OOP statements.

 

2) using prepared queries will eliminate all the ...real_escape_string() calls. you are using one in the case of a value being output to the browser. the ...real_escape_string() statements are only used on string data being supplied to a non-prepared sql query statement, not on data being output to the browser.

 

3) you need to use exceptions to handle database statement errors. your code has error handling on only one of the query statements now. the PDO extension already uses exceptions for connection errors. you need to set the error mode to exceptions, for all the rest of the statements, when you make the connection.

 

4) all the post method form processing code should be grouped together and come near the top of your code. you should actually test if a post method form was submitted, then if you have multiple possible forms, detect a field name or field value that identifies if the Delete or Update form was submitted.

 

5) you should validate all the submitted data before using it, then only use it if it is valid. use an array to hold validation errors. you can then test if the array is empty or not to determine if there have been any validation errors. to display the errors, just output the contents of the array.

 

6) your delete checkbox logic only works for a single/last checkbox. in fact, all the form fields don't work when there is more than one set of data in the form. you need to use array names for all the form fields, with the $row['id'] value as the array index (this is needed to distinguish which row each form field value corresponds to) and you need to remove the hidden field with name='checkbox' (having this causes only the last checkbox in the form to work.) you would also need to add a loop in the Update form processing code to loop over all the submitted sets of data.

 

7) with prepared queries, when you are looping over data and executing the query inside the loop, you will prepare the query once before the start of the loop. the code inside the loop only gets the correct set of data and calls the ->execute() method.

 

when you remove the hidden form field with name = 'checkbox' (which was done to prevent php errors when no checkboxes are checked, but because it is being output repeatedly, only allows the last checkbox to work), the logic in the delete form processing code will need to be changed. if there are no checked checkboxes, $_POST['checkbox'] won't be set. you need to add an if(isset($_POST['checkbox'])) test.

 

9) the code to retrieve the data needed to display the page should be removed from the actual html document/template. this will make it easier to write and test your code, and if you need to change the code to use a different database extension, such as the PDO extension, consolidates the database specific code in one place and makes the html document/template general purpose. just fetch the data from the query into a php variable and use that variable in the html document/template.

 

10) the html document you are creating is reusing DOM id='...' attribute values. id's must be unique. if the client-side code isn't actually referencing the id's, don't even put them into the markup.

 

11) you are outputting an empty <tr></tr> after each actual <tr>....</tr> of output. makes no sense and is repeating an id attribute value which is also not valid. only output markup that you need.

 

12) you are outputting the $row['id'] value in a form field. this is not an editable data value. the $row['id'] specifies which row the data belongs to. you can display the $row['id'] value if you wan't, but it should not be in a visible form field. see item #6 in this list for how the $row['id'] value should be used to associate the submitted values with the $row['id'] value they belong to.

 

13) when your SELECT query doesn't match any data, your code is correctly skipping over the code to produce the output from that data, but your code is also not doing anything to notify the user that there was no data to display. you should set up and display a message that the reason for the blank section on the page is that there is no data to display.

 

14) all data values being output to the browser should be passed through htmlentities(), with an appropriate 2nd parameter value, to prevent any html entities in the data from breaking the html on the page and to help prevent cross site scripting.

 

15) you are repeating the <tbody>...</tbody> tags for each set of data being output. while this is valid markup, unless you are going to style each table body section separately, there's no point in doing this and it is just adding clutter to the result and would prevent a large table body section from being able to be scrolled on the page.

Edited December 30, 2016 by mac_gyver

[NotionCommotion]

3) you need to use exceptions to handle database statement errors. your code has error handling on only one of the query statements now. the PDO extension already uses exceptions for connection errors. you need to set the error mode to exceptions, for all the rest of the statements, when you make the connection.

Or let your normal error handler catch the uncaught exception.

 

14) all data values being output to the browser should be passed through htmlentities(), with an appropriate 2nd parameter value, to prevent any html entities in the data from breaking the html on the page and to help prevent cross site scripting.

Or use a template engine such as http://twig.sensiolabs.org/.

 

Most of mac_gyver's great comments were not specifically about PDO.  Remember PDO is easy, and remember to have fun!

 

Happy New Year to you and all others!

 

 

Edited December 30, 2016 by NotionCommotion