4
\$\begingroup\$

I'm a newbie to PHP and other programming for that matter. I'm doing this simply to learn. Figured the code could be used to search through a police database or something. The statements are Microsoft SQL Server. I'm running rocky Linux 9 with the unixODBC drivers in order to accomplish the task. I'd be happy to provide the information necessary to set it up if any one needs help with that. I ask the above, but would welcome any information on improvements regardless. I'd also like to know if the below is vulnerable to SQL injection. Thank you for your time. Added content... The code below is purely for enhancing my abilities, there is no contract. The code below takes both a string derived from a form as an identified number of words to search for and another string which simply consists of a case number. I figured something like that could be used for the police or lawyers, maybe health professionals to find similar cases. The code then splits the string into an array with singular words. Because the string is of indeterminate length the code has to create a query with a loop and bind values with a separate loop. In this code I use for loops. There was a need to create place holders as names for the Microsoft SQL Query which I foolishly denoted as $str but will change that when I have time. All had to be created somewhat autonomously. I also needed to bind the correct placeholder names and values to the statement, which I did so again in a similar fashion. When I know what I'm doing I will update this to include correct indentation and change the name of $str2 and $str to query1 and query2. Thank you again.

    <?php 
include "includes/header.php";
if ($administrator['admin'] == 1) {
$resultsPerPage = 6;
$currentPage = isset($_GET['page']) ? $_GET['page'] : 1;



$offset = ($currentPage - 1) * $resultsPerPage;


if (isset($_GET['str'])) {
    $caseNum = $_SESSION['caseNum'];
    $keywords = preg_split('/[\s]+/', $_SESSION['str']);
    $totalKeywords = count($keywords);
    
    
    
 
    
        
          // initial creation of statement
            $str2 = "SELECT count(*) AS count from evidence where";
            $str2 .= " (full_name LIKE :search00 OR evidence LIKE :search10) ";
            if ($totalKeywords > 1) {
         // loop to create remainder of statement via place holders and search bits   
            for ($i = 1 ; $i < $totalKeywords; $i++) {
                $search_bit = ":search0" . $i;
                $search_bit2 = ":search1" . $i;
                $str2 .= " AND (evidence like " . $search_bit2 . " OR full_name LIKE " . $search_bit . ") ";
                
            }}
            $str2 .= "AND case_number like :casenum ";
            $stmt = $cms->db->prepare($str2);
         // if statment prepared binding of arguments via for loop      
            if (!empty($stmt)) {
    
                for ($x = 0; $x<$totalKeywords; $x++) {
                    $keywords[$x] = "%" . $keywords[$x] . "%";  
                    $stmt->bindValue(':search0' . $x, $keywords[$x]);
                    $stmt->bindValue(':search1' . $x, $keywords[$x]);
                    }
                    $stmt->bindValue('casenum', $caseNum);
            }
        
        // execution of statemnt and fetching of row for row count
    $stmt->execute();
    $row = $stmt->fetch();
    $totalResults = $row['count'];
    $totalPages = ceil($totalResults / $resultsPerPage);
    
    
        // creation of second statement to gather via fetch all
    
    
    
    $sql_str = "SELECT * from evidence where";
    $sql_str .= " (full_name LIKE :search00 OR evidence LIKE :search10) ";
    if ($totalKeywords > 1) {
    for ($i = 1 ; $i < $totalKeywords; $i++) {
        $search_bit = ":search0" . $i;
        $search_bit2 = ":search1" . $i;
        $sql_str .= " AND (evidence like " . $search_bit2 . " OR full_name LIKE " . $search_bit . ") ";
        
    }}
    
    
    
    
    $sql_str .= "and case_number LIKE :casenum ORDER BY id ASC OFFSET $offset ROWS FETCH NEXT $resultsPerPage ROWS ONLY";
    
    $stmt = $cms->db->prepare($sql_str);
    // binding of values and execution of statment
    if (!empty($stmt)) {
    
        for ($x = 0; $x<$totalKeywords; $x++) {
            $keywords[$x] = "%" . $keywords[$x] . "%";  
            $stmt->bindValue(':search0' . $x, $keywords[$x]);
            $stmt->bindValue(':search1' . $x, $keywords[$x]);
            }
            $stmt->bindValue('casenum', $caseNum);
    }
    $stmt->execute();
    $array = $stmt->fetchAll();
}

if ($_SERVER['REQUEST_METHOD'] === 'POST') {
    $caseNum = $_POST['case_number'] ?? '';

    $str = $_POST['str'];
$_SESSION['caseNum'] = $caseNum;
$_SESSION['str'] = $str;
$keywords = preg_split('/[\s]+/', $str);
$totalKeywords = count($keywords);


    
    // creation of count statement for pagination
        $str2 = "SELECT count(*) AS count from evidence where";
        $str2 .= " (full_name LIKE :search00 OR evidence LIKE :search10) ";
        if ($totalKeywords > 1) {
        for ($i = 1 ; $i < $totalKeywords; $i++) {
            $search_bit = ":search0" . $i;
            $search_bit2 = ":search1" . $i;
            $str2 .= " AND (evidence like " . $search_bit2 . " OR full_name LIKE " . $search_bit . ") ";
            
        }}
        $str2 .= "AND case_number like :casenum ";
        $stmt = $cms->db->prepare($str2);
    // binding of values and execution to fetch count result
        if (!empty($stmt)) {

            for ($x = 0; $x<$totalKeywords; $x++) {
                $keywords[$x] = "%" . $keywords[$x] . "%";  
                $stmt->bindValue(':search0' . $x, $keywords[$x]);
                $stmt->bindValue(':search1' . $x, $keywords[$x]);
                }
                $stmt->bindValue('casenum', $caseNum);
        }
    
    
$stmt->execute();
$row = $stmt->fetch();
$totalResults = $row['count'];
$totalPages = ceil($totalResults / $resultsPerPage);



// creation of statment to gather information via fetchall
$sql_str = "SELECT * from evidence where";
$sql_str .= " (full_name LIKE :search00 OR evidence LIKE :search10) ";
if ($totalKeywords > 1) {
for ($i = 1 ; $i < $totalKeywords; $i++) {
    $search_bit = ":search0" . $i;
    $search_bit2 = ":search1" . $i;
    $sql_str .= " AND (evidence like " . $search_bit2 . " OR full_name LIKE " . $search_bit . ") ";
    
}}




$sql_str .= "and case_number LIKE :casenum ORDER BY id ASC OFFSET $offset ROWS FETCH NEXT $resultsPerPage ROWS ONLY";

$stmt = $cms->db->prepare($sql_str);


// binding of values


if (!empty($stmt)) {

    for ($x = 0; $x<$totalKeywords; $x++) {
        $keywords[$x] = "%" . $keywords[$x] . "%";  
        $stmt->bindValue(':search0' . $x, $keywords[$x]);
        $stmt->bindValue(':search1' . $x, $keywords[$x]);
        }
        $stmt->bindValue('casenum', $caseNum);
}
$stmt->execute();
$array = $stmt->fetchAll();
}



?>

<form action="administrator_search_for_evidence.php" method="POST">
<label for="str">Search for name or through evidence:</label>
<input type="text" name="str" size="50" ><br>
<label for="case_number">Case number:</label>
<input type="text" name="case_number" size="50">
<input type="submit" value="Search!">
<?php 

//  foreach loop and pagination to display information
if (isset($array)) {
foreach ($array as $evidence) {?>
<?php ?>
    <h4><?= "ID:" . $evidence['id'] ?><?php   ?>
<h1 id="<?= $evidence['id'] ?>"> <br>Full Name:<?= $evidence['full_name'] ?><br>

Email:<?= $evidence['email'] ?><br>Phone Number: 
<?= $evidence['phone_number'] ?></h1><br>
<h3> evidence:</h3> <br>
<h4><p><?= $evidence['evidence'] ?></h4>
</p><br>
<br>


<?php 

}
for ($i = 1; $i <= $totalPages; $i++) {
    echo '<a href="administrator_search_for_evidence.php?page=' . $i . '&str=yes">' . $i . '</a> - ';
} }?>
</body>
</html>
<?php
} else {
    header("Location: how_dare_you.php");
    exit();
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Here is the outline that I saw:

  1. // please disregard this part. Although functional it is for a lack of sleep.
  2. // initial creation of statement
  3. // loop to create remainder of statement via place holders and search bits
  4. // if statment prepared binding of arguments via for loop
  5. // execution of statemnt and fetching of row for row count
  6. // creation of second statement to gather via fetch all
  7. // binding of values and execution of statment
  8. // creation of count statement for pagination
  9. // binding of values and execution to fetch count result
  10. // creation of statment to gather information via fetchall
  11. // binding of values
  12. // foreach loop and pagination to display information

Php offers the function keyword. I recommend you break out helper functions. Not quite a dozen of them. But I bet half a dozen would leave us with better organized code.

The function names are suggested by those dozen remarks.

Show us the gazintas and the gozowtas. Knowing those plus the contract we will be able to tell if the code did what the spec asked of it. Right now, I see lots of code and very little specification.


coupling

The code as written is one giant monolith, with all variables interacting with all of the code. This is not a problem for the machine, but it makes it harder for humans to reason about what the code does and whether it does that correctly.

Prefer to write functions which accomplish a single goal. Do one thing and do it well. Leave the other things for other functions to worry about.


meaningful identifiers

This may very well be the second string:

            $str2 = "SELECT count(*) ...

But merely being true does not make for a good identifier. We need more than that. Perhaps this is a $query.

Here you're making a mockery of identifiers in your Public API:

    $str = $_POST['str'];

This is far from self documenting code, and I saw no documentation (beyond the ambiguous "creation of second statement"). A concept lurks within, something from the business domain. Decide what those encoded bits mean, and tell us loud and clear via an improved name.


random indents

Whitespace doesn't matter to the machine. But it will very much affect human cognition.

Format your code in a consistent way, in case you wish to share it with others.


It is not yet clear to me whether this code achieves its design goals.

I would not be willing to delegate or accept maintenance tasks on it.

\$\endgroup\$
2
\$\begingroup\$

While the other answer is mostly dwelling on the abstract topics, I will concentrate on the practical issues.

The idea is very good, and your code is safe from injections. The only issue which bothers me, is $resultsPerPage and $offset being put in the SQL directly. Although currently they are safe, being cast to numbers, but this code could be changed in the future, opening it for SQL injection. So I wouldn't leave it for a chance and would make them bound, just like all other variables.

So here is how this code can be improved:

  1. Exit early. When your else just exits, it makes sense to make it an early if and save an extra indent for the entire code.
  2. Split the header. Apparently, your header.php serves as both a bootstrap file (that indeed has to be included at the top) and the design top that has to be included way down below.
  3. Do not use sessions. They overcomplicate things. Why not to simply use the GET method for both the form and the pages? Instead of just str send the actual query as urlencode($_GET['str']) in the pagination links.
  4. Like it was said above, use placeholders for all variables in the query
  5. The most important part: a duplicated code is a mortal sin for a programmer. And the whole business is repeated three times in your case! Even with your POST/SESSION approach it could be (and must have been) done only once.
  6. The actual query building code could be simplified as well:
    • only one loop is needed, with a single condition
    • instead of binding all values individually, they can be sent into execute() at once (also, the if (!empty($stmt)) condition is distinctly useless)
    • positional placeholders (? marks) are wayyyyy simpler to handle

So in the end, the core functionality of "applying placeholders to an array in a loop for pdo" could be like this:

// First, create the common SQL part and collect all parameters into array     
$base_sql = '';
$params = [];
foreach ($keywords as $i => $word) {
    if ($base_sql) {
        $base_sql .= " AND ";
    $base_sql .= "(evidence like ? OR full_name LIKE ?)";
    $params[] = "%$word%";
    $params[] = "%$word%";
}
$base_sql = " evidence WHERE $base_sql AND case_number like ? ".
$params[] = $caseNum;

// then create actual queries and execute them
$sql = "SELECT count(*) FROM $base_sql";
$stmt = $cms->db->prepare($sql);
$stmt->execute($params);
$count = $stmt->fetchColumn();

$sql = "SELECT * FROM $base_sql ORDER BY id ASC ";
$sql.= "OFFSET ? ROWS FETCH NEXT ? ROWS ONLY";
$stmt = $cms->db->prepare($sql);
$params[] = $offset;
$params[] = $resultsPerPage;
$stmt->execute($params);
$data = $stmt->fetchAll();

though this code would work for mysql, I am not sure if it will make it for MS SQL. In case it won't we can fall back to bindValue:

// First, create the common SQL part and collect all parameters into array     
$base_sql = '';
foreach ($keywords as $word) {
    $base_sql .= $base_sql ? " AND " : ""; // using a ternary
    $base_sql .= "(evidence like ? OR full_name LIKE ?)";
}
$base_sql = " evidence WHERE $base_sql AND case_number like ? ".

// then create actual queries and execute them
$sql = "SELECT count(*) FROM $base_sql";
$stmt = $cms->db->prepare($sql);
$i = 1;
foreach ($keywords as $param) {
    $stmt->bindValue($i++, "%$param%");
    $stmt->bindValue($i++, "%$param%");
}
$stmt->bindValue($i++, "%$caseNum%");
$stmt->execute();
$count = $stmt->fetchColumn();

$sql = "SELECT * FROM $base_sql ORDER BY id ASC ";
$sql.= "OFFSET ? ROWS FETCH NEXT ? ROWS ONLY";
$stmt = $cms->db->prepare($sql);
$i = 1;
foreach ($keywords as $i => $param) {
    $stmt->bindValue($i++, "%$param%");
    $stmt->bindValue($i++, "%$param%");
}
$stmt->bindValue($i++, "%$caseNum%");
$stmt->bindValue($i++, $offset, PDO::PARAM_INT);
$stmt->bindValue($i++, $resultsPerPage, PDO::PARAM_INT);
$stmt->execute();
$data = $stmt->fetchAll();
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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