Taking advantage of the falsey and ineffective "" empty string

Question

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();
?>

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.