1

I have 2 variables to define a price range for a query. The problem I'm trying to solve is when these are not set in which case I want to show all rows (from 1, if the lower boundary is null, and to max(price) if the upper boundary is null).

I've tried with ifnull, but without success.

$priceFrom = $_POST['priceFrom'];
$priceTo = $_POST['priceTo'];

if(is_null($priceFrom) || is_null($priceTo)){
    $priceFrom = 0;
    $priceTo = 0;
}
$mass = array();
foreach($data as $current){
$sql = "SELECT  p.price,
    p.type,
    p.area,
    p.floor,
    p.construction,
    p.id as propertyID,
    CONCAT(u.name, ' ',u.family) as bname, 
    p.type as ptype, 
    n.name as neighborhoodName,
    CONCAT(o.name,' ',o.surname,' ',o.family) as fullName 
    FROM `property` p 
    LEFT JOIN `neighbour` n  ON p.neighbour = n.id 
    RIGHT JOIN `owners` o ON p.owner = o.id
    LEFT JOIN users u ON p.broker = u.id
    WHERE `neighbour`= '$current'
    AND `price` BETWEEN ifnull('$priceFrom', '1') AND ifnull('$priceTo','2000000')
    ";}
ppeterka
  • 20,583
  • 6
  • 63
  • 78
andrags
  • 17
  • 7

1 Answers1

1

SQL INJECTION

^ Please Google that! Your code is seriously vulnerable! Your data can be stolen or deleted...

You have to sanitize your inputs at least with mysqli_real_escape_string()

Even better would be to take proper countermeasures to SQL injection and use prepared statements and parametrized queries! (as shown in the code below)


I think the best approach would be to handle the logic by altering the query based on the values of the variables:

$sql = "SELECT  p.price,
    p.type,
    p.area,
    p.floor,
    p.construction,
    p.id as propertyID,
    CONCAT(u.name, ' ',u.family) as bname, 
    p.type as ptype, 
    n.name as neighborhoodName,
    CONCAT(o.name,' ',o.surname,' ',o.family) as fullName 
    FROM `property` p 
    LEFT JOIN `neighbour` n  ON p.neighbour = n.id 
    RIGHT JOIN `owners` o ON p.owner = o.id
    LEFT JOIN users u ON p.broker = u.id
    WHERE `neighbour`= :current "; //note: ending white space is recommended

    //lower boundary clause -- if variable null - no restriction
    if(!is_null($priceFrom){
        sql = sql . " AND `price` >= :priceFrom "; // note: whitespace at end and beginning recommended
    }

    //upper boundary -- better than to set it to an arbitrary "high" value
    if(!is_null($priceTo)){
        sql = sql . " AND `price` <= :priceTo "; // note: whitespace at end and beginning recommended
    }

This approach allows for any upper value: if there is a serious inflation, a different currency, or suddenly the code will be used to sell housese and there will be products with prices > 200000, you don't need to go out and change a lot of code to make it show...

The parameters need to be bound when executing the query of course:

$stmt = $dbConnection->prepare(sql);
$stmt->bind_param('current', $current);
if(!is_null($priceFrom)){
    $stmt->bind_param('priceFrom', $priceFrom);
}
if(!is_null($priceTo)){
    $stmt->bind_param('priceTo', $priceTo);
}
//execute and process in same way
$stmt->execute();

Also note: from your code it seems you are issuing queries in a loop. That is bad practice. If the data on which you loop comes

  • from the DB --> use a JOIN
  • from an array or other place of the code --> better use an IN clause for the elements

to fetch all data with one query. This helps a lot both in organizing and maintaining the code and results generally in better performance for the most cases.

Community
  • 1
  • 1
ppeterka
  • 20,583
  • 6
  • 63
  • 78