SQL Parameters not being replaced

[rossc]

Hi All

 

this is my first post here I have a problem with some code I am writing and have been trying to find the problem for a while any help would be very much appreciated,

 

I have a class for company and this method is supposed to return an associative array but as far as I can work out from dumping values etc I don't think the ? marks are being replaced by the parameters.

    function readLike($page, $from_record_num, $records_per_page, $searchval){

        $query = "SELECT
        id, company_name, company_reg_number, vat_number, website
        FROM
        " . $this->table_name . "
        where company_name like ?
        ORDER BY
        company_name ASC
        LIMIT ?, ?;";


        $stmt = $this->conn->prepare( $query );
        $stmt->bindParam(1, $searchval, PDO::PARAM_STR);
        $stmt->bindParam(2, $from_record_num, PDO::PARAM_STR);
        $stmt->bindParam(3, $records_per_page, PDO::PARAM_STR);
        $stmt->execute(array($searchval, $from_record_num,$records_per_page);
        $result = $stmt->fetch(PDO::FETCH_ASSOC);
        return $result;
    }

Thanks

 

Ross

 

 

[mac_gyver]

your query statements are definitely producing an error, due to the LIMIT clause being supplied string values. do you have PDO's error mode set to exception (you have to specifically do this after you make the database connection) so that any of the errors from the PDO statements will throw an exception (that you can catch in one place to handle db errors) to let you know they failed?

 

at a minimum, the two bound parameters for the LIMIT ?, ? must use PDO::PARAM_INT for the types and you cannot pass these in the ->execute() method (it treats all values as strings) and since you are binding the parameters, you shouldn't be passing anything in the ->execute() method. also, if the two values for the LIMIT x,y are entirely produced within your code, there's no need to bind them into the query, just put them directly in the query statement.

[rossc]

Thanks for the quick response, sorry yes I have been going backwards and forwards with the same code I meant to remove the parameters in the execute method.  That makes sense thank you.  I will have another go at it now.

[rossc]

Please could you help me specify the error level here is where a declare the database connection

$database = new Database();
$db = $database->getConnection();
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

I just added the third line, sorry I'm new to php and so far the server logs have helped me to find my errors.  Being able to show errors for the sql part will be really helpful.

 

Thanks

[rossc]

this is my new revised function but it still doesn't seem to replace the parameter.  I know I'm probably missing something really obvious

    function readLike( $searchval ){

        $myquery = "SELECT
        id, company_name, company_reg_number, vat_number, website
        FROM
        " . $this->table_name . "
        where company_name like ?
        ORDER BY
        company_name ASC";

        $searchval = "'%".$searchval."%'";

        $mystmt = $this->conn->prepare( $myquery );
        $mystmt->bindParam(1, $searchval, PDO::PARAM_STR);
        $mystmt->execute();

        $myresult = $mystmt->fetch(PDO::FETCH_ASSOC);
        return $myresult;
    }

[hansford]

    function readLike( $searchval ){


        $myquery = "SELECT

        id, company_name, company_reg_number, vat_number, website

        FROM $this->table_name where company_name like ? ORDER BY company_name ASC";


        $searchval = "%".$searchval."%";


        $mystmt = $this->conn->prepare( $myquery );

        $mystmt->bindParam(1, $searchval, PDO::PARAM_STR);

        $mystmt->execute();


        $myresult = $mystmt->fetch(PDO::FETCH_ASSOC);

        return $myresult;

    }

[mac_gyver]

@hansford, it's usually better to state what's wrong, rather than to post 'fixed' code. by stating what the problem is, some learning will take place, rather than mimicking. the OP may not even realize everything you changed in the code.

Edited September 24, 2015 by mac_gyver

[hansford]

Apologies. I've added comments to the code.

    function readLike( $searchval ){

        // altered the SELECT statement to embed $this->table_name directly into the double quoted string
        // It is not necessary to concatenate variables and strings when using double quotes.
        $myquery = "SELECT
        id, company_name, company_reg_number, vat_number, website
        FROM $this->table_name where company_name like ? ORDER BY company_name ASC";

        $searchval = "%".$searchval."%";  // removed single quotes from string as they are being included in the search term

        $mystmt = $this->conn->prepare( $myquery );
        $mystmt->bindParam(1, $searchval, PDO::PARAM_STR);
        $mystmt->execute();

        $myresult = $mystmt->fetch(PDO::FETCH_ASSOC);
        return $myresult;
    }

[rossc]

Many thanks to both of you for your help, I still cannot get this to work all the other similar methods n this class work fine but I can't find the problem with this one.  What is the best way to check if the parameter is being replaced in the SQL string?

 

below is my updated code

 

Thanks

 

Ross

function readLike($myvalue){

        //echo $myvalue;
        //die;

        $myquery = "SELECT
        id, company_name, company_reg_number, vat_number, website
        FROM $this->table_name 
        where company_name like ? ORDER BY
        company_name ASC";

        $myvalue = "%".$myvalue."%";
        $mystmt = $this->conn->prepare( $myquery );
        $mystmt->bindParam(1, $myvalue);

        try{
            $mystmt->execute();
            }catch(PDOException $exception){
            echo "error: " . $exception->getMessage();
        }
        

        //var_dump($mystmt);

        $myresult = $mystmt->fetch(PDO::FETCH_ASSOC);
        return $myresult;
    }

[Jacques1]

So what is the problem? Are you getting an error message? An unexpected result? “Doesn't work” doesn't really tell us anything.

 

You need to get rid of the try statement, though. It's completely useless, it will suppress very important debug information (line number, stack trace etc.), and if you forget to remove it in production, your users will be greeted with weird error messages. Just leave the exception alone so that PHP uses the standard error handler.

Edited September 25, 2015 by Jacques1

[benanamen]

You need to get rid of the try statement, though. It's completely useless, it will suppress very important debug information (line number, stack trace etc.),

 

That is completely wrong. The catch part of the "try" will give you all that information if you do it right. You cannot have the catch without the try. The code below is included in my catch. It will tell you all you want to know and more. (filename, line number, stack trace, error message)

 

There is also additional code in the include to optionally log and or email the exception data. I get notified automatically if something breaks, whether error details are displayed to user or not.

 

try{
//some bad action here
}
catch (PDOException $e)
    {
    include_once('./config/pdo_catch_error.php');
    }

 

pdo_catch_error.php

$error_msg = $e->getMessage() . ' in ' . $e->getFile() . ' on line ' . $e->getLine();

if (DEBUG == 1)
    {
    echo '<div class="error_custom">Error Message:<br>'.$error_msg.'</div>';

    echo '<div class="error_custom">SQL Statement:<br>';
    echo ''.$stmt->debugDumpParams().'';
    echo '</div>';

    echo '<div class="error_custom">Stack Trace:<br>';
        foreach ($e->getTrace() as $trace) {
       echo $trace['file'] . ' Line #' . $trace['line'] . '<br>';
       }
   echo '</div>';
    } //DEBUG == 1

//Email Error - suppressed error message to user with @ if no mail server.
if (EMAIL_ERROR == 1)
    {
    @error_log("ERROR: $error_msg\n", 1, "$email_admin", "From: $email_to");
    } //EMAIL_ERROR == 1


// Write error to log
if (LOG_ERROR == 1)
    {
    error_log("$mysql_datetime|$error_msg\r\n", 3, "$errorlog_path");
    } //LOG_ERROR == 1


die('<div class="error_custom">Fatal Error! Admin has been notified</div>');

Edited September 25, 2015 by benanamen

[Jacques1]

Congratulations, you've just reinvented the standard error handler, except that your implementation sucks.

 

The standard error handler requires zero extra code, it aborts the script in a controlled manner, and it can log the errors in production so that you have a chance to fix remaining bugs. Your implementation forces you to clutter the code with try statements, it fails to stop the script so that it will crash uncontrolled, and you don't log anything. So it's a lot of code for half as much features and twice as much problems. That doesn't sound like a good idea.

 

Besides that, why are you so obsessed with PDO exceptions in particular? That's just a small subset of all possible errors. According to your logic, you'd have to put pretty much every single statement into a try block:

<?php

try
{
    do_this();
}
catch (SomeException $e)
{
    include_once './config/catch_error.php';
}
try
{
    do_that();
}
catch (SomeException $e)
{
    include_once './config/catch_error.php';
}
try
{
    another_action();
}
catch (SomeException $e)
{
    include_once './config/catch_error.php';
}
try
{
    yet_another_action();
}
catch (SomeException $e)
{
    include_once './config/catch_error.php';
}

And that helps us how exactly?

 

I repeat: Just let the standard error handler do its job. It's much better than that, and it's already built into PHP. If you do need a special error feature (which you appearently don't), then install your own handler rather than drowning the code in try statements.

[benanamen]

Your example is ridiculous! The multiple try catch's you show is not needed and you know full well that is not how it is used. You only need one. As far as query errors, It will tell you exactly what the problem is and where it is.  And what part of your solution will notify you as soon as there is an error when you are not the user? The exact purpose of a try catch block is to catch the errors and handle it as you see fit. Since I am not the user of the app, I want to know as soon as it breaks and not hope a user is going to be able to contact me and tell me the details I need.

 

The OP's example uses try/catch executing a query which is exactly where I use it. Not with this random action or that random action which is why the "Obsession" with PDO exceptions.  For a database driven application query exceptions are everything.

 

As an include, the code is hardly cluttered in the file it is used in. Write once, use everywhere. My code is an even more controlled manner. It is not going to display error messages to the user except "Fatal Error! Admin has been notified".

Edited September 25, 2015 by benanamen

[Jacques1]

Just like the standard error handler! In other words, you don't need any of this. That's the whole point.

 

Why is this so hard to understand? PHP already knows how to handle errors, because that's one of the most basic features of a programming language. It knows how to print stack traces, display the message, figure out the location of the error etc. There's absolutely no need for us to do it manually.

 

Maybe your PHP environment is just horribly misconfigured. What happens when you throw an error without catching it? If you only get a blank screen, it's a problem of your configuration: Turn error_reporting all the way up, turn on display_errors, and now you should see a full error report with all relevant information. No extra work required.

[benanamen]

 

 

Turn error_reporting all the way up, turn ondisplay_error

Turn error_reporting all the way up, turn ondisplay_errors, and now you should see a full error report with all relevant information.

 

 

 

 

 

In production you never do this as I am sure you know. In dev, of course I have everything turned full on.

 

 As I said, I am not the user, so I would never see the displayed error messages and I dont want the user to see it either but I do need to know about it as soon as it happens so it can be fixed immediately. The only errors I need to know about right away are query errors. Anything else I read the last X lines of the particular apps error log. Since there a couple large apps on the same server I send the errors to a custom error log just for that app so I dont have to sort through what errors came from what app.

 

I am assuming you are knowledgeable as an Advanced member, so please share what you would do for the following requirements.

 

1. Admin needs to be notified immediately of query problem. (Downtime for my apps cost big bucks)

2. Admin needs to readily have available the details to fix problem

3. User should not see server error message but needs to know there was a problem

4. App is to have its own error log.

 

By all means, if you have a better way, I will adopt it in a heartbeat.

Edited September 25, 2015 by benanamen

[hansford]

You need to break this problem down into simple steps.

Get rid of that try/catch block. Work on a single file outside of your class/function and attempt to make a connection and query data from your database.

Once you get that working, then move the parts over to your class and methods.

<?php
error_reporting(E_ALL);
ini_set('display_errors',1);

$tablename = 'table_name';
$myvalue = 'what_to_search_for';

$conn = new PDO("mysql:host=localhost;dbname=$dbname",$user,$pass);
$conn->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION);

$myquery = "SELECT id, company_name, company_reg_number, vat_number, website FROM $tablename where company_name like ? ORDER BY company_name ASC";

$myvalue = "%". $myvalue ."%";
$mystmt = $this->conn->prepare( $myquery );
$mystmt->bindParam(1, $myvalue, PDO::PARAM_STR);
$mystmt->execute();


$myresult = $mystmt->fetch(PDO::FETCH_ASSOC);

print_r($myresult);

[Jacques1]

By all means, if you have a better way, I will adopt it in a heartbeat.

 

Like I already said: If you have specific requirements, set up a custom exception handler on top of the standard error mechanism, preferrably in a prepended script.

 

In that handler, you can do anything you like: Send e-mails, write the data to a special log etc. There are also premade solutions.

Edited September 25, 2015 by Jacques1

[benanamen]

In theory that is great, and is exactly what I do but I should have prefaced, that the app that code is from is in CONSTANT flux on a daily basis as well as the DB structure due to the client coming up with stuff as they use it so query errors are quite common place in this instance. Many, many things are tightly integrated with conditions galore, per the clients business rules and a simple change or addition in one place can easily break something somewhere else. (I personally wouldn't do what they have me do and it is truly the most complex app I have done in my 17 plus yrs coding). The DB schema is an Enterprise Party Model structure due to their business requirements. 

[Jacques1]

I understand, but still none of this is a good reason for duplicating the same try statement over and over again as opposed to using a single exception handler.

 

What you describe is clearly a global error handling strategy, so it should be installed globally and not for every single query in your code. This also gives you much more flexibility. Maybe one day the errors will happen less in the database and more in some other area (networking, file handling, whatever). In your model, you'd have to rewrite the entire application, add new try statements everywhere and possibly remove the PDO-specific statements. With a global handler, you just have to change a few lines in a function.

[benanamen]

Looking into it. I am all about using the least yet correct amount of code to get the job done while also considering maintenance. For now my main concern on this app is fatal errors.

 

* Thanks for the Sentry link.

[benanamen]

Okay @Jacques1,

 

We have a winner!

 

After a little research and testing, set_exception_handler is clearly the better way to my global error handling and does eliminate the need for all the try/catch blocks and will still allow me to handle my errors the custom way I need to. Since I already have a config file that is required everywhere, I dont really need to prepend anything. I can just call set_exception_handler from there

 

In all honesty, that is a function I was unaware of. I will be updating my code accordingly.

 

I know a lot but I dont know everything. Always willing to learn.

   

Edited September 25, 2015 by benanamen

[rossc]

Hi thanks for the input everyone.  I started from scratch and now everything works as expected below is my working example. I'm still not quite sure what was wrong.

   function readLike($page, $from_record_num, $records_per_page, $searchval){

        $query = "SELECT
        id, company_name, company_reg_number, vat_number
        FROM
         $this->table_name WHERE company_name LIKE ?
        ORDER BY
        company_name ASC
        LIMIT
        {$from_record_num}, {$records_per_page}";

        $searchval = "%".$searchval."%";

        $stmt = $this->conn->prepare( $query );
        $stmt->bindParam(1, $searchval, PDO::PARAM_STR);
        $stmt->execute();

        return $stmt;
    }

I am going to read through your posts to understand more about the debugging process.  Out of curiosity does my code prevent against sql injection ?

 

Once again thanks for all the help 

[Jacques1]

No, this is not secure.

 

You can and should use parameters for the LIMIT clause, because that's the only way to make sure you'll never get an SQL injection. Don't just rely on $from_record_num and $records_per_page being "safe".

 

If you get an error for the LIMIT clause, that's because haven't activated prepared statements (PDO::ATTR_EMULATE_PREPARES is true). You need to fix this:

/*
 * Set up the database connection
 * - the character encoding *must* be defined in the DSN string, not with a SET NAMES query
 * - be sure to turn off "emulated prepared statements" to prevent SQL injection attacks and enable parameters in the LIMIT clause
 * - turn on exceptions so that you don't have to manually check for errors
 */
$dSN = 'mysql:host='.DB_HOST.';dbname='.DB_NAME.';charset='.DB_CHARSET;

$databaseConnection = new PDO($dSN, DB_USER, DB_PASSWORD, [
    PDO::ATTR_EMULATE_PREPARES => false,                 // activate prepared statements
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,         // activate exceptions
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,    // fetch associative arrays by default (this isn't critical)
]);

Now you can use parameters:

// Example: create a prepared statement with two parameters
$exampleQuery = $databaseConnection->prepare('
    SELECT * FROM test LIMIT :offset, :limit
');

// Execute statement and pass values to the parameters
$exampleQuery->execute([
    'offset' => 0,
    'limit' => 1
]);

Edited September 25, 2015 by Jacques1