PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!
I think that I need some help with the way I am piecing together a SQL statement from URL parameters. I'm using this statement to sort products by colors, styles, sizes, etc. based on whatever a user clicks. Say a user clicks on the color "berry" and then a style option of "short sleeve" and "sleeveless" - I need a sql statement that searches for a match in the color "berry" and "short sleeve" or the color "berry" and "sleeveless". The following code is what I've written and I have a feeling that it's a bad way to go.
if (isset($_GET['color']))
$color_filter = $_GET['color'];
$color = explode(',', $color_filter);
if (isset($_GET['fit']))
$fit_filter = $_GET['fit'];
$fit = explode(',', $fit_filter);
if (isset($_GET['style']))
$style_filter = $_GET['style'];
$style = explode(',', $style_filter);
if (isset($_GET['fabric']))
$fabric_filter = $_GET['fabric'];
$fabric = explode(',', $fabric_filter);
if (isset($_GET['chamois']))
$chamois_filter = $_GET['chamois'];
$chamois = explode(',', $chamois_filter);
if (isset($_GET['ride']))
$ride_filter = $_GET['ride'];
$ride = explode(',', $ride_filter);
$conditions = '';
if (!empty($color_filter) || !empty($fit_filter) || !empty($style_filter) || !empty($fabric_filter) || !empty($chamois_filter) || !empty($ride_filter)) {
$i = 0;
foreach ($color as $color_value) {
foreach ($fit as $fit_value) {
foreach ($style as $style_value) {
foreach ($fabric as $fabric_value) {
foreach ($chamois as $chamois_value) {
foreach ($ride as $ride_value) {
if ($i == 0)
$conditions .= 'subcategory = "' . $sub . '"';
if ($i !== 0)
$conditions .= ' OR subcategory = "' . $sub . '"';
if (!empty($fabric_filter) && $fabric_value == 'solid' && $sub == 'tops')
$conditions .= ' AND name LIKE "%' . $fabric_value . '%" AND active = 1';
if (!empty($fabric_filter) && $fabric_value == 'print' && $sub == 'tops')
$conditions .= ' AND name NOT LIKE "%solid%" AND active = 1';
if (!empty($color_filter))
$conditions .= ' AND colors LIKE "%' . $color_value . '%" AND active = 1';
if (!empty($fit_filter))
$conditions .= ' AND fit = "' . $fit_value . '" AND active = 1';
if (!empty($style_filter))
$conditions .= ' AND style LIKE "%' . $style_value . '%" AND active = 1';
if (!empty($chamois_filter))
$conditions .= ' AND specs LIKE "%' . $chamois_value . '%" AND active = 1';
if (!empty($ride_filter))
$conditions .= ' AND ride LIKE "%' . $ride_value . '%" AND active = 1';
$i++;
}
}
}
}
}
}
$filter = $conditions;
}
This code does exactly what I need it to but it does throw out undefined variable notices. Does anyone know of a better way that I can achieve what I with this code or point me in the direction of starting new code that's more efficient? Are the nested foreach's a bad way to go?
if (isset($_GET['color'])) { //without brackets the explode is not within the if (this is not like in python)
$color_filter = $_GET['color'];
$color = explode(',', $color_filter);
foreach ($color as $i=>$_) $color[$i] = mysql_real_escape_string($color[$i]); //escape!
} else {
$color = Array();
}
Thank you for the help with that. Your solution didn't exactly work the way I needed it to though. I realized the problem lied within my select statement. It was far more complicated than it should have been. So I came up with this instead:
Much better. And even though it doesn't affect you in this instance, you should fix this in principle:
Escaping should be the last thing you do to a string before placing it in a query.
Also, double quotes make the code more readable:
I actually changed it to use LIKE instead of REGEXP because it was faster. REGEXP was taking about .0007 seconds and LIKE took about .0005 seconds. Should I use fulltext if my table has 39 rows?
$valid = array( 'color', 'fit', 'style', 'fabric', 'chamois', 'ride');
if ($_GET) {
foreach($valid as $column) {
if (isset($_GET[$column])) {
$esc = mysql_real_escape_string($_GET[$column]);
$explode = explode(',', $esc);
$new = array();
foreach ($explode as $value) {
$new[] .= $column . ' LIKE "%' . $value . '%"';
}
$implode = implode(' OR ', $new);
$filter .= ' AND (' . $implode . ')';
}
}
}
That code will ouput a statement that looks along the lines of this if a parameter is set: AND (color LIKE "%berry%" OR color LIKE "%amethyst%") AND (fabric LIKE "%print%"). Where should I put the mysql_real_escape_string and why should escaping be the last thing I do to a string? Should I escape in the second foreach loop? Thank you for your help. I really do appreciate it!
For 38 lines any string matching method is fine for all practical purposes.
As I said before, in this particular code it wouldn't matter, since what you do after the escaping cannot be used to break it.
That said, it should be like this:
if (isset($_GET[$column])) {
$explode = explode(',', ($_GET[$column]);
$new = array();
foreach ($explode as $value) {
$value = mysql_real_escape_string($value);
// In other cases, you might or might not also want to prevent the user to enter LIKE metacharacters like % and _ here. For %$value% - type of matches it doesn't really matter anymore.
$new[] .= $column . ' LIKE "%' . $value . '%"';
}
$implode = implode(' OR ', $new);
$filter .= ' AND (' . $implode . ')';
}