2
\$\begingroup\$

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();
?>
\$\endgroup\$
1
\$\begingroup\$

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.
\$\endgroup\$
  • \$\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 May 14 '15 at 17:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.