The following code snippets are academic. I was required to make a PHP generated HTML interface for a MySQL database of a movie rental store, and the requirement was filtering the displayed movies by category.
The approach for having the filter persist across all form actions (without using a session) was to have the hidden form input inserted into all the other forms on the page. And my first approach to this was testing if $_POST['category']
is set, and if so echoing the HTML tag for the hidden input to carry the category variable to the next page.
This was still the method, but, I tinkered with it and decided that instead of testing if a category was selected at every form I could test once, and if not leave the string variable for the HTML tag empty and echo it regardless.
This worked nicely but I'm curious if if in a larger/co-operative project if the possible confusion for the outputting of a variable that is sometimes empty (i.e. ineffectual) and sometimes not will be more confusing than the lines of code and processing time saved.
<?php
$categorySelected = '';
$hiddenCategory = '';
////....................
//conect to databes
//create mysqli object
if( isset($_POST['category']) && $_POST['category'] != $Allcats ){
//<check that catagory still has entries>
//if there are entries for category filter, set local variable, and html tag for form input
//hidden input tag is inserted to all forms to carry category selection to next page reload
if(<videos of that category exist>){
$categorySelected = $_POST['category'];
$hiddenCategory = "<input type='hidden' name='category' value='$categorySelected'>";
} //else //<display message that there are no videos of that category>
}
//database manipulations completed
//html header
//in all forms in html $hiddenCategory is echod
//to add category k/v pair to post
//or '' is echoed with no consequence if category not set
?>
<form id='addvideo' action = <?php echo $SELF; ?> method ='post'>
<?php echo $hiddenCategory; ?>
<input type='hidden' name='ACTION' value='addvideo'>
<input type="submit" value="ADD VIDEO">
Name:<input type="text" name="name">
<!-- more fields.... -->
</form>
<?php
//prepare statement for all entries of category filtered
if(!$categorySelected){
$vidStmt = $mysqli->prepare("SELECT * FROM records");
}else{
$vidStmt = $mysqli->prepare("SELECT * FROM records WHERE category=?");
$vidStmt->bind_param("s",$categorySelected);
}
//iterate over all videos in database query, creating <tr> for each
while($vidStmt->fetch()){
echo "<tr>
<td> $name <td> $category <td> $length <td>
<form class='rent' action = $SELF method ='post'>
$hiddenCategory
<input type='hidden' name='ACTION' value='rent'>
.... more fields ..
</form>
";
}
$vidStmt->close();
?>
1 Answer 1
I wouldn't do it this way, because it does seem confusing. And not just because you are using truthiness (which I really wouldn't do, because it's hard to understand and just screams for bugs). It takes a while to see what happens when there is no POST value, what happens if a categories doesn't exist, etc.
You don't need multiple checks either though, just move all the code that belongs to category-doesnt-exists and category-exists into one block. If you also extract some code to functions, your code might look something like this:
if(!isset($_POST['category']) || $_POST['category'] == $Allcats ){
displayAddForm("", $SELF);
displayRentForm("", getAll());
} else if (<videos of that category doesnt exist>) {
// message that video doesn't exist
displayAddForm("", $SELF);
displayRentForm("", getAll());
} else {
$categorySelected = $_POST['category'];
$hiddenCategory = "<input type='hidden' name='category' value='htmlspecialchars($categorySelected, ENT_QUOTES, 'UTF-8') '>";
displayAddForm($hiddenCategory, $SELF);
displayRentForm($hiddenCategory, getByCategoryId($categorySelected));
}
function getAll() {
$mysqli->prepare("SELECT * FROM records");
// return array of results
}
function getByCategoryId($id) {
$vidStmt = $mysqli->prepare("SELECT * FROM records WHERE category=?");
$vidStmt->bind_param("s",$categorySelected);
// return array of results
}
function displayRentForm($hiddenCategory, $categories) {
foreach($categories as $category){
echo "<tr>
<td> htmlspecialchars($name, ENT_QUOTES, 'UTF-8') $name <td> htmlspecialchars($category, ENT_QUOTES, 'UTF-8') <td> htmlspecialchars($length, ENT_QUOTES, 'UTF-8') <td>
<form class='rent' action = htmlspecialchars($SELF, ENT_QUOTES, 'UTF-8') method ='post'>
$hiddenCategory
<input type='hidden' name='ACTION' value='rent'>
.... more fields ..
</form>
";
}
}
function displayAddForm($hiddenCategory, $SELF) {
echo "<form id='addvideo' action = <?php echo htmlspecialchars($SELF, ENT_QUOTES, 'UTF-8') ; ?> method ='post'>" .
$hiddenCategory .
"<input type='hidden' name='ACTION' value='addvideo'>
<input type="submit" value="ADD VIDEO">
Name:<input type="text" name="name">
<!-- more fields.... -->
</form>";
}
It's not perfect, but I you get the idea.
XSS
You shouldn't write vulnerable code, even for academic work. It's bad practice, and it normalizes unsecure code; you get used to writing it, and other people get used to reading it.
Always protect against XSS when echoing variable data, eg:
$hiddenCategory = "<input type='hidden' name='category' value='" . htmlspecialchars($categorySelected, ENT_QUOTES, 'UTF-8') . "'>";
And
<form id='addvideo' action = '<?php echo htmlspecialchars($SELF, ENT_QUOTES, 'UTF-8'); ?>' method ='post'>
Misc
- Don't mix
"
and'
so much. Something like<input type='hidden' name='ACTION' value='addvideo'><input type="submit" value="ADD VIDEO">
is hard to read, and harder to change than uniformly quoted code (you can see this in my code example above, where I didn't bother to correct it). - Your indentation is off, making your code harder to read.
-
\$\begingroup\$ Thank you so much for the feedback! and indeed I want to protect against XSS, especially while its still academic, all of the SQL interactions (which aren't seen here) were done with prepared statements, but I had forgot to sanitize my post variables, Thanks to you, I will try not to make that same mistake in the future. \$\endgroup\$Brandon Swanson– Brandon Swanson2015年05月14日 17:33:52 +00:00Commented May 14, 2015 at 17:33