Trying to pass a variable to a function and use it within a PDO select statement

Question

I am currently trying to get a PDO SELECT statement to work with a variable passed in through the parameter of the function it is contained within. I am using this function to return values to the class constructor.

Here is the function for which i am encountering a "Fatal error: Call to a member function prepare() on a non-object"

    public function select($field) {
    global $pdo;

    $query = $pdo->prepare("SELECT `".$field."` FROM `iwp_incident` WHERE `incident_id=`". $this->oid ."`");
    $query->execute();

    $result = $query->fetch();

    return $result[0];

Relying on globals is the first of your problems here. Why not inject the `PDO` instance into the class constructor and store it as a property. Then you can use `$this->pdo->prepare(...)`, safe in the knowledge that it is not `null` — Phil, Apr 09 '14 at 06:31
I found the problem. As most of you said, the $pdo was null. I initially could not figure out why as the variable is persisted within the session. It turned out that silly me forgo to insert the include statement for the proper file that contained the $pdo. However, thank you for introducing me to alternative ways of looking at the way in which i pass the $pdo variable. — weberwe3, Apr 09 '14 at 12:56
Glad you found it! I didn't see this comment for a while so I wondered if you got it sorted. Nothwithstanding the curmudgeonly downvotes *ahem*, there are a couple of answers here that pointed out the issue so perhaps accept one of 'em? — RobP, Apr 09 '14 at 14:48

score 0 · Accepted Answer · answered Apr 09 '14 at 06:42

Your main problem is that the assumed PDO instance is not what you think it is (my guess is null).

To alleviate this problem, you should remove the reliance on globals and instead use dependency injection. For example

namespace Some\Namespace;

use \PDO;

class YourClass {
    /**
     * @var PDO
     */
    private $pdo;

    private $oid;

    public function __construct(PDO $pdo, $oid) {
        $this->pdo = $pdo;
        $this->oid = $oid;

        $someField = $this->select('some_field');
    }

    public function select($field) {
        $stmt = $this->pdo->prepare(sprintf(
            'SELECT `%s` FROM `iwp_incident` WHERE `incident_id = ? LIMIT 1',
            $field));
        $stmt->bindParam(1, $this->oid);
        $stmt->execute();
        return $stmt->fetchColumn();
    }
}

To use this, you simply pass in your PDO instance when creating the class, eg

$pdo = new PDO('mysql:...', 'username', 'password', [
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
]);

$obj = new \Some\Namespace\YourClass($pdo, 123);

RobP · Answer 2 · 2014-04-09T07:27:57.047

-1

The error message is quite helpful. $pdo is a non-object, as stated. presumably your initial connection failed. It might be good practice to check that $pdo is valid, though the initial connection (wherever that is in your code) should throw an exception if the connection fails. (Did you catch that exception and swallow it without reporting anything...?)

As an aside, may I suggest that instead of string concatenation you do something like this which handles quoting and escaping to help prevent mistakes that allow injection attacks:

$query = $pdo->prepare("SELECT `".$field."` FROM `iwp_incident` WHERE `incident_id`= :oid");
$params = array(":oid"=>$this->oid); 
$success = $query->execute($params);
if($success) {
    $results = $query->fetchAll(PDO::FETCH_ASSOC);
    // process results
}

edited Apr 09 '14 at 07:27

answered Apr 09 '14 at 06:03

RobP

9,144
3
20
33

May I ask, do you have any experience with PDO? – Your Common Sense Apr 09 '14 at 06:12
yes, quite a lot, though there might be a couple months' rust... what are you driving at? – RobP Apr 09 '14 at 06:14
1

Well, I am sure then, you can run such a query yourself and wonder what would be the result – Your Common Sense Apr 09 '14 at 06:22
fixed the code to reflect that parameter substitution is good practice for WHERE clause, not SELECT clause. Seems a bit rude to downvote without giving reasons, but anyway I wrote and tested the code to refresh my memory. – RobP Apr 09 '14 at 07:09
it is still prone to SQL injection, which makes whole mess with prepared statements useless – Your Common Sense Apr 09 '14 at 07:14
to a degree, yes it's vulnerable, though it depends where the inputs *come from*... if the calling function just iterates over a constant array of field names to provide $field that's quite different from it is taken from $_GET[]. – RobP Apr 09 '14 at 07:16
There are NO "degrees" in vulnerabilities. It's a boolean value: either it's vulnerable or not. Your solution is. – Your Common Sense Apr 09 '14 at 07:46
as someone who worked in Internet security at one of the largest software firms in the world, I respectfully disagree with your blanket statement. Nothing is invulnerable. It's *all* a question of degrees, and protection commensurate with the exposure to risk. – RobP Apr 09 '14 at 07:55
but I am curious, are you saying it is in some way MORE vulnerable than a string concatenation solution? if so, how? – RobP Apr 09 '14 at 07:57
Just working somewhere doesn't make one an expert himself. So, it is not a proof for anything. I prefer to talk of codes, not titles. Your solution is openly vulnerable due to usage of string concatenation and even lack of proper formatting. So, this code is intentionally vulnerable. Thus, there is no talk of "degrees". – Your Common Sense Apr 09 '14 at 08:34
I am not saying it is more vulnerable. I am still within bonds of my boolean algebra. So, there is no "more" or "less" categories. It is AS vulnerable as "string concatenation" (implied "for data literals"). Because it is still using the same string concatenation (but for identifier literals). – Your Common Sense Apr 09 '14 at 08:55
I do think it would be good practice to have a static array of valid column names and return error if $field does not match against that array, or pass a constant which is used as a lookup key to retrieve the column name. That prevents vulnerability and also prevents the SQL statement failing due to invalid field name. We agree on that, though perhaps it was beyond scope of OP's question. – RobP Apr 09 '14 at 14:00

Trying to pass a variable to a function and use it within a PDO select statement

2 Answers2