PHP - Best price?

[djr33]

echo various checks along the way, like echo $rs; after it's set.
Just see if you're looking for the right thing.
Maybe there's something we're not seeing without running it.

[nikomou]

i tried this..

get no error this way, but only the "handset" name appears, all other info is blank..

$sql = "SELECT handset, MIN(total) FROM xxxxx WHERE make='$getmake' GROUP BY handset"; 

[mwinter]

$sql = "SELECT handset, MIN(price) WHERE make='$getmake' GROUP BY handset"; 

It would have helped somewhat if you altered the statement to reflect your table structure: you don't have a "price" column and the table name needed to be added using a FROM clause. However, a GROUP BY clause isn't particularly appropriate with the amount of information you seem to want. I did mention that this might have been a possibility.

No simple, one-query solution comes to mind. Instead, obtain all handsets (adding or ORDER BY clause to sort them, if you like):

  SELECT DISTINCT handset FROM ... WHERE make='$make'

then, for each handset (loop through the results), order the options by cost and take the first one:

  SELECT * FROM ... WHERE make='$make' AND handset='$handset' ORDER BY `Total Cost` LIMIT 1

There's a possibility that there are two handsets with the same cost. I don't know what you'd want to do in that case.

Remember this time to edit the SQL statements to use the correct column/table/variable names. Neither of them should be used verbatim.

$getmake = $_GET['make'];
...
$sql = "SELECT handset, MIN(price) WHERE make='$getmake' GROUP BY handset";

I really hope that you omitted the call to mysql_real_escape_string (or mysqli_real_escape_string), otherwise you're opening yourself up to SQL injection.

Mike

[nikomou]

Hey, thanks Mike...

the correct collumn was used in the script - price is used for the cost of the handset, and total is the total cost of a contract...

How would i go about to do what your saying? i would still like the results to show in 4 collumns, could i still do that?

as for "SQL injection" - what is it? and what should i do to prevent it?!

Cheers.

[d-one]

A lillte helpfull debug tip:

//Try and run this after your query:
if(mysql_errno() > 0){
    echo mysql_error();
    exit;
}

[GITs]

as for "SQL injection" - what is it? and what should i do to prevent it?!

SQL Injection is a way that hackers retrieve data/delete data etc in a database, this can usually be prevented by making sure that your query fits a certain format before you execute it, or that you make sure the input from the form generating the query is encoded to prevent it from making a code execute... Just need to be careful about what you let the user try to search for...

[mwinter]

the correct collumn was used in the script - price is used for the cost of the handset, and total is the total cost of a contract...

Oh, OK. The database "sample" you posted used different names than what I'd posted beforehand (which was just a guess). Still, an appropriate FROM clause needed to be added. I assume that it wasn't, which was why the error occurred.

How would i go about to do what your saying?

A bare-bones example show the sort of thing I was envisioning. Obviously it isn't tested (I don't have your database to play with), but it should be a start.

If you look at the getCheapestHandset function, the SQL statement only lists a small number of column names. These were the ones I could find in your previous post. If you need others, add them. One that I wasn't sure about was the $network variable that you use when generating one of the image URIs; it wasn't defined anywhere in the code you posted.

How you handle database errors is up to you. You could use exception handling, raising appropriate exceptions where "/* Handle error */" appears in the code. An alternative would be to return null or false. Either approach will require some minor restructuring of the code around the loop.

The make query parameter isn't checked in the code below, so you should write code to make sure that it exists and is valid before using it.

function getCheapestHandset($make, $handset, $dbConnection) {
    $statement = 'SELECT handsetid, handset, gift, tariff, xnet, offpeak, texts FROM ...'
        . " WHERE make='{$make}' AND handset='{$handset}' ORDER BY total LIMIT 1";
    $resultSet = mysql_query($statement, $dbConnection);
    if (!$resultSet) { /* Handle error */ }

    $result = mysql_fetch_assoc($resultSet);
    mysql_free_result($resultSet);
    return $result;
}

function getHandsets($make, $dbConnection) {
    $result = array();
    $statement = "SELECT DISTINCT handset FROM ... WHERE make='{$make}'"
    $resultSet = mysql_query($statement, $dbConnection);
    if (!$resultSet) { /* Handle error */ }

    while (($row = mysql_fetch_row($resultSet))) $result[] = $row[0];
    mysql_free_result($resultSet);
    return $result;
}

$dbConnection = mysql_connect( /* ... */ );
$make = mysql_real_escape_string($_GET['make'], $dbConnection);
foreach (getHandsets($make, $dbConnection) as $handset) {
    $cheapestHandset = getCheapestHandset($make, mysql_real_escape_string($handset),
        $dbConnection);
    /* Generate output from the elements of $cheapestHandset */
} 

i would still like the results to show in 4 collumns, could i still do that?

You could. The foreach statement is analogous to your while loop in the code you posted. However, such a rigid layout might not be a good idea.

Constructing the table you would generate seems to produce a very wide element that takes up the entire width. It might be better to create floated "cells". These would reflow based on the available width, forming a single column in a small viewport, and perhaps as many as five or six in a larger one. You could always limit the maximum width used, if necessary.

The second issue is that your markup is very bloated and invalid. In one cell, there were as many as three nested font elements. There shouldn't a single occurrence of that element anywhere in modern markup.

as for "SQL injection" - what is it? and what should i do to prevent it?!

SQL injection involves inserting values into an SQL statement that changes how that query operates. Consider the simple query:

"SELECT `First name`, `Last name` FROM users WHERE `Last name` = '{$_GET['name']}'"

With a name query parameter value like "Jones", the resulting statement would be:

"SELECT `First name`, `Last name` FROM user WHERE `Last name` = 'Jones'"

Harmless. However, consider a value like "Jones'; UPDATE user SET password='foo' WHERE username='admin". This would produce:

"SELECT `First name`, `Last name` FROM user WHERE `Last name` = 'Jones';
UPDATE user SET password='foo' WHERE username='admin'"

It ends the first statement cleanly, then adds a completely new one. Disastrous.

Validating input is crucial, as is escaping strings that are to added to a query. Never trust anything that comes from the user, including hidden fields and the like. You can find more information on-line.

Mike

[nikomou]

Thanks mwinter,

Thats way to complex for me! I'm having trouble putting it all together, where would i put the connection details, and all of these:

$id = $row[handsetid];
$handset = $row[handset];
$make = $row[make];

Also, what would i have to do to "Generate output from the elements of $cheapestHandset"

Could you give me an example? cheers

[mwinter]

... where would i put the connection details,

The variable, $dbConnection, is defined after the function definitions in the code I posted. Initialisation can go there.

and all of these:

$id = $row[handsetid];
$handset = $row[handset];
$make = $row[make];

As I wrote previously, the foreach loop is analogous to the while loop in the code you posted. Much of what you do in that can be included in what I posted.

The return value from the getCheapestHandset function (assigned to $cheapestHandset) is the result from the database. It is an associative array that contains the column names as listed in the query. Therefore, the id, for example, can be obtained from $cheapestHandset['handsetid']. Remember that not all of data has been returned from the database; you seemed to be using only a small subset and there's no point in returning twenty values if you're only using five. Modify the query as necessary to add more values to the returned array.

As a point of syntax, don't write things like $arrayName[elementName]. For instance, in the code you posted, you had expression statements such as:

  $handset = $row[handset];

where handset is the literal name of the key. This is currently tolerated, but it's wrong. PHP will initially thing that handset is a constant, and by constant I mean something like PHP_VERSION or E_NOTICE. If it can't find a constant by that name, it assumes that you really meant to use:

  $handset = $row['handset'];

That is, a string with the value 'handset'. This tolerance may disappear in future versions of PHP, so get out of the habit of using it. This sort of thing currently generates E_NOTICE messages in the error log. The PHP manual describes this in more detail.

Also, what would i have to do to "Generate output from the elements of $cheapestHandset"

Again, you could just do what you posted in your own code: output the contents of a table cell. You'd have to output the table start- and end-tags outside of the loop, and generate rows as you did previously. As an alternative, you could write code to generate "cells", as I described them previously. It's up to you.

Mike

[nikomou]

<?php
$getmake = $_GET['make'];

$address = "localhost";
$username = "xxxx";
$password = "xxxx";
$db = "xxxx";
$table = "xxxx";
$dbConnection = mysql_connect($address, $username, $password);

function getCheapestHandset($make, $handset, $dbConnection) { 
    $statement = 'SELECT handsetid, handset, gift, tariff, xnet, offpeak, texts FROM $table' 
        . " WHERE make='{$getmake}' AND handset='{$handset}' ORDER BY total LIMIT 1"; 
    $resultSet = mysql_query($statement, $dbConnection); 
    if (!$resultSet) { /* Handle error */ } 

    $result = mysql_fetch_assoc($resultSet); 
    mysql_free_result($resultSet); 
    return $result; 
} 

function getHandsets($make, $dbConnection) { 
    $result = array(); 
    $statement = "SELECT DISTINCT handset FROM $table WHERE make='{$getmake}'" 
    $resultSet = mysql_query($statement, $dbConnection); 
    if (!$resultSet) { /* Handle error */ } 

    while (($row = mysql_fetch_row($resultSet))) $result[] = $row[0]; 
    mysql_free_result($resultSet); 
    return $result; 
} 

$dbConnection = mysql_connect($address, $username, $password); 
$make = mysql_real_escape_string($_GET['make'], $dbConnection); 
foreach (getHandsets($make, $dbConnection) as $handset) { 
    $cheapestHandset = getCheapestHandset($make, mysql_real_escape_string($handset), 
        $dbConnection); 
    echo("$cheapestHandset['handsetid']")
}