0

I think it's quite a simple question. Is this my best bet or is there a 'proper' way of doing this?

<?php  
    $correctOrder = array("name", "address", "phone", "starttime", "endtime", 
                    "status", "details");  

    $sql->sql1 = "SELECT * FROM `pickups` WHERE";  

    if (isset($_GET["name"])){  
        $sql->sql2 = "`name` LIKE '%" . $_GET['name'] . "%'";  
        }  
    if (isset($_GET["address"])){  
        if (!isset($_GET['name'])){
            $q = "`address` LIKE '%" . $_GET['address'] . "%'";  
        } else {  
            $q = "AND `address` LIKE '%" . $_GET['address'] . "%'";  
            }
        $sql->sql3 = $q;
        }
    ...
    ...

    echo implode(" ", (array) $sql);  
?>  

So, right now:

?name=Jari%20Martikainen&address=some%20random%20street

and

?name=Jari%20Martikainen&address=some%20random%20street&blah=har

and

?address=some%20random%20street&blah=har&name=Jari%20Martikainen

all return the same result which is desired, but it just doesn't seem like a very efficient way of doing things.

4
  • and if there are no get parameters - or none in the correctOrder array what happens? It does not look the most efficient way of doing things - aloop through the correctOrder might be better Commented Jan 13, 2018 at 8:36
  • 1
    Its wide open to SQL injection, and it will break if no conditions are true. Commented Jan 13, 2018 at 8:44
  • 1
    For code review there's another stack-site. Commented Jan 13, 2018 at 8:49
  • correctOrder controls the correct order for the keys so it will always be populated. Lawrence Cherone, I guess I should have added 20 more lines of code there. u_mulder, that was helpful. Commented Jan 13, 2018 at 8:54

3 Answers 3

1

Build an array ($ands) of AND clauses, but without the "AND".

$ands = array();
if (...)
    $ands[] = "name LIKE ...";
if (...)
    $ands[] = "address LIKE ...";
...

Then build the query:

$query = "SELECT ... WHERE " . implode(' AND ', $ands);

I find this pattern to be simple, clean, and avoids kludges like 1=1 or removing the extra AND.

Sign up to request clarification or add additional context in comments.

Comments

1

This code uses an array to work out what the parameters are that it's interested in and builds up an array of where clauses.

The reason why I use "name" => "name" in this array ($correctOrder) is that it allows the parameter name and the column name to be different. You should add any parameters you need in here.

Also this code uses bind variables, not sure what flavour of database access your using, but you can pass the $data array to the execute to bind them.

$correctOrder = array("name" => "name", "address" => "address");

$_GET = [ 'name' => 'name1', 'address' => 'add'];

$where = [];
$data = [];
foreach ( $_GET as $paramName => $param ) {
    if ( isset($correctOrder[$paramName]) ) {
        $where[] = "`{$correctOrder[$paramName]}` like ? ";
        $data[] = '%'.$param.'%';
    }
}

$sql = "SELECT * FROM `pickups`";
if ( count($where) > 0 ){
    $sql .= " WHERE ".implode( " and ", $where);
}

echo $sql.PHP_EOL;
print_r($data);

The count($where) > 0 part will only add the where clause if there is something to add, and the implode adds the appropriate and bits as the glue.

Which with the test data gives...

SELECT * FROM `pickups` WHERE `name` like ?  and `address` like ? 
Array
(
    [0] => %name1%
    [1] => %add%
)

This only really works with strings, but you could add specific code for other data fields and add the clause into the $where array before getting to the final part which builds the statement.

Comments

0

Here's a bit more of an overhaul than you asked for.

First, start your WHERE clause with 1=1, then cycle through your array of search terms, and extend the where clause if any of them are included in the $_GET array.

Second, I'd use a PDO object to connect to your database, and then BIND the $_GET values. That will help protect you from SQL injection/hacking stuff.

Below, I cycle through your search terms, saving them to an array called $sqlParams if there's something in the $_GET array. Then I cycle through $sqlParams and bind the values.

// connect to database via PDO
try {
    $db = new PDO($dsn, $username, $password, $options);    
} catch (PDOException $e) {
    echo 'Connection failed: ' . $e->getMessage();
}

$correctOrder = array("name", "address", "phone", "starttime", "endtime", 
                "status", "details");  

$sql = "SELECT * FROM `pickups` WHERE 1=1 ";  

// check the $_GET array, build SQL, remember values in $sqlParams
$sqlParams = [];
foreach ($correctOrder as $key) {
    if (isset($_GET[$key])) {
        $sql .= "and `$key` LIKE :$key ";
        $sqlParams[":$key"] = "%{$_GET[$key]}%";
    }
}


// bind values from $sqlParams into SQL statement
$stmt = $db->prepare($sql);
foreach ($sqlParams as $key => &$value) {
    $stmt->bindParam($key, $value);
}

// execute SQL statement, catching exception
try {
    $stmt->execute();
} catch (PDOException $e) {
    echo 'Connection failed: ' . $e->getMessage();
}

// cycle through results and do stuff
while ($row = $stmt->fetch()) {
    print_r($row);
}

1 Comment

I realize now the below two answers are each better than this one in that they avoid the 1=1 trick, and NigelRen’s also prepares the data to be BINDed more efficiently.

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.