Displaying MySQL data using PHP

Question

I have MySQL data that I am rendering in an HTML table using PHP. This is the full code of my table and PHP. It does output fine and looks how I would expect but I am wondering if there is a better way to write this code. Also, the 'Delete' button specifically, is there a better way to output the button itself as right now, I noticed especially on a mobile device, I will see a text cursor indicator when tapping the button as if I am trying to select the text on the button itself.

<div class="container" style="width: 95%;"> <table class="table table-bordered table-striped table-hover"> <thead> <tr> <th class="col-sm-6"> Description </th> <th class="col-sm-2"> Date Created </th> <th class="col-sm-1"> Created By </th> <th class="col-sm-2" style="text-align: right;"> Edit - Delete </th> </tr> <?php $noLists = "<div>You have no lists. Why dont you create one?</div>"; if ($db->connect_error) { die("Connection failed: " . $db->connect_error); } $sql = "SELECT DISTINCT lists.id, lists.listdescription, lists.createdon, lists.createdby, lists.sharedlist FROM lists INNER JOIN users ON users.fname = '$displayname' AND users.fname = lists.createdby"; $result = $db->query($sql); if ($result->num_rows > 0) { // output data of each row while($row = $result->fetch_assoc()) { $listID = $row["id"]; $listDescription = $row["listdescription"]; $createdOn = $row["createdon"]; $createdBy = $row["createdby"]; echo "<form>"; echo "<tr>"; echo "<td>$listDescription</td>"; echo "<td>$createdOn</td>"; echo "<td>$createdBy</td>"; echo "<td align=\"right\">"; echo "<form action='account.php' method='post'>"; echo "<input class=\"btn btn-success\" type=\"submit\" name=\"\" value=\"Edit\">"; echo "<a href='includes/deletelist.php?id=$row[id]'><input class=\"btn btn-danger\" value=Delete style=\"width: 85px;\"></a> <!--<a href=\"index.php?id=$id\">Delete</a>--> </td>"; } } else { echo "</thead>"; echo "<tbody>"; echo "</tbody>"; echo "</table>"; echo "<h4> $noLists</h4>"; } $db->close(); ?> 

Answer 1

Generally speaking, you should keep your markup and your PHP logic separate.

And the intermediate variables don't really have a purpose, just use the values from the array.

You've got two opening form tags and no closing form tags.

Your first condition (if there are records in the table) does not close the thead or the create a tbody.

Your delete button is no button at all, it's an input wrapped in an anchor tag (<a>). That's why it's not working as expected. Either remove the anchor and give the input a type attribute of "button", or remove the input and put some text in there instead.

Use a prepared statement. It's not cool to throw variables into your queries all willy-nilly. You will anger the code gods.

Here's a quick re-write (assumes you're using PDO - If you're using MySQLi, see the docs to convert the prepared statement to MySQLi)

I'm just going to go ahead and post this since I spent three or four minutes on it, but after looking at this code, there is no way this was working as you had it. This site is for reviewing working code, not fixing broken code. If your code doesn't work you need to ask on StackOverflow in the future.

<?php // Do all the PHP stuff before you start ouputting $noLists = "<div>You have no lists. Why dont you create one?</div>"; if ($db->connect_error) { die("Connection failed: " . $db->connect_error); } $sql = "SELECT DISTINCT lists.id, lists.listdescription, lists.createdon, lists.createdby, lists.sharedlist FROM lists INNER JOIN users ON users.fname = ? AND users.fname = lists.createdby"; $result = $db->prepare($sql); $result->execute(array($displayname)); $output = array(); if ($result->rowCount() > 0) { // output data of each row $output[] = "</thead>"; $output[] = "<tbody>"; while($row = $result->fetch(PDO::FETCH_ASSOC)) { $output[] = "<tr>"; $output[] = "<td>{$row["listdescription"]}</td>"; $output[] = "<td>{$row["createdon"]}</td>"; $output[] = "<td>{$createdBy}</td>"; $output[] = "<td align=\"right\">"; $output[] = "<form action='account.php' method='post'>"; $output[] = "<input class=\"btn btn-success\" type=\"submit\" name=\"\" value=\"Edit\">"; $output[] = "</form>"; $output[] = "<a href='includes/deletelist.php?id={$row["id"]}' class=\"btn btn-danger\" style=\"width: 85px;\">Delete</a></td>"; } $output[] = "</tbody>"; $output[] = "</table>"; } else { $output[] = "</thead>"; $output[] = "<tbody>"; $output[] = "</tbody>"; $output[] = "</table>"; $output[] = "<h4> $noLists</h4>"; } $db->close(); // Turn the output array back into a string $output = implode('', $output); ?><div class="container" style="width: 95%;"> <table class="table table-bordered table-striped table-hover"> <thead> <tr> <th class="col-sm-6"> Description </th> <th class="col-sm-2"> Date Created </th> <th class="col-sm-1"> Created By </th> <th class="col-sm-2" style="text-align: right;"> Edit - Delete </th> </tr> <?php echo $output; ?> 

Edit... you're not closing your table row in the loop either. I'm not gonna bother updating the code snippet.

Answer 2

1. You should be using prepared statements to feed values into the SQL as a matter of security/stability.
2. I have a hard time believing that DISTINCT is providing any benefit; especially considering the lists.createdon column is highly likely to create uniqueness among qualifying rows.
3. Don't fetch lists.sharedlist if you are going to use it.
4. Though your ON conditions provide the correct logic, I recommend that you only use ON to dictate the relationship between joined tables and use WHERE to dictate qualifying rows within the collection of data.
5. Don't bother creating HTML table tags unless you know you need a table.
6. Don't bother catching database connection errors, let the system catch those for you. You should not be displaying error messages and you should be logging error messages.
7. It is invalid HTML markup to wrap a tablerow inside of a <form> tag. A form tag must exist within a table cell.
8. Sending the user to a screen to edit a list item is a "reading" task, not a "writing" task (they haven't submitted any editted data yet). This is a task that should be triggered by a $_GET payload.
9. Allowing a user to delete something is a "writing" task, this should be something that a webcrawler can stumble upon and wipe clean your table data. This task should be triggered by a $_POST payload.
10. Avoid inline styling. This makes your HTML document easier to maintain and keeps all of your styling conveniently in a .css file.
11. Don't bother closing your database connection; it will be closed automatically after your script is finished executing.
12. Try to keep your code relatively narrow. Horizontal scrolling while reviewing/maintain code is annoying. Have a read of PSR-12 coding standards regarding line width limits and other guidelines that will help you to write professional looking code.

Suggested Untested Code:

$sql = <<<SQL SELECT lists.id, lists.listdescription, lists.createdon, lists.createdby, lists.sharedlist FROM lists JOIN users ON lists.createdby = users.fname WHERE users.fname = ? SQL; $stmt = $db->prepare($sql); $stmt->bind_param("s", $displayname); $stmt->execute(); $stmt->store_result(); if (!$stmt->num_rows) { exit("<div>You have no lists. Why don't you create one?</div>"); } $trTemplate = <<<HTML <tr> <td>%1$s</td> <td>%2$s</td>"; <td>%3$s</td>"; <td><a href="editlistitem.php?id=%4$d"> <td> <form action="account.php" method="post"> <input class="btn btn-success" type="submit" name="delete" value="%4$d"> </form> </td> </tr> HTML; ?> <div class="container" style="width: 95%;"> <table class="table table-bordered table-striped table-hover"> <thead> <tr> <th class="col-sm-6">Description</th> <th class="col-sm-2">Date Created</th> <th class="col-sm-1">Created By</th> <th class="col-sm-1">Edit</th> <th class="col-sm-1">Delete</th> </tr> </thead> <tbody> <?php foreach ($stmt->get_result() as $row) { printf( $trTemplate, $row["listdescription"], $row["createdon"], $row["createdby"], $row["id"] ); } ?> </tbody> </table> </div>