php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #81084 PDOStatement::queryString is uninitialized
Submitted: 2021-05-26 23:24 UTC Modified: 2021-06-01 12:57 UTC
Votes:1
Avg. Score:3.0 ± 0.0
Reproduced:0 of 0 (0.0%)
From: corey dot taylor dot fl at gmail dot com Assigned:
Status: Open Package: PDO Core
PHP Version: master-Git-2021-05-26 (Git) OS:
Private report: No CVE-ID: None
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If this is not your bug, you can add a comment by following this link.
If this is your bug, but you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: corey dot taylor dot fl at gmail dot com
New email:
PHP Version: OS:

 

 [2021-05-26 23:24 UTC] corey dot taylor dot fl at gmail dot com
Description:
------------
At some point in php 8.1 development, PDOStatement::queryString became a typed property which throws an error when uninitialized.

https://www.php.net/manual/en/class.pdostatement.php

We were checking whether $queryString was truthy before using it, but now that access throws an error.

https://3v4l.org/gHlc0/rfc#git.master

The property is documented as "readonly string" which implies that it is always non-null.

Can we make it nullable or initialize it when PDOStatement is created?

The work around is using "isset($statement->queryString)" directly, but doesn't seem like it should be uninitialized.


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2021-05-27 08:09 UTC] nikic@php.net
I'm generally open to making this nullable, but would first like to understand how you ended up in this situation.

The reason why we decided that it's okay to make it non-nullable, is that the only way to end up with an uninitialized $queryString is if you don't create the PDOStatement through the PDO API and instead do something like "new PDOStatement". This is an illegal operation and the resulting object will throw "Error: PDO object is uninitialized" for all method calls. Now it will also throw an Error when accessing its only property, so that seems sensible and consistent. (Ideally we'd throw when you do "new PDOStatement" in the first place, but unfortunately this interferes with the ability to extend PDOStatement, thus we get the current situation where the object can be created but throws on all operations.)

Is my assumption about how you can end up with an uninitialized $queryString wrong? Is there some other pathway besides "new PDOStatement()"? If no, why are you creating / interacting with this kind of object?
 [2021-05-27 09:31 UTC] corey dot taylor dot fl at gmail dot com
We are not manually instantiating the PDOStatement. That was clearly just a simple snippet that would run in that environment.

The statement is generated by a call to "PDO::prepare()".

The use of $queryString in this instance is determining if certain drivers need a post-execute query such as SQLite for a row count.

Since we didn't know how to tell if the query string was available, we were using something like "if ($statement->queryString)". However, that now throws an error since it's uninitialized.
 [2021-05-27 09:46 UTC] nikic@php.net
Would it be possible to share a snippet (not necessarily runnable on 3v4l) where this occurs when going through the PDO::prepare() API?

Based on my reading of the code we will always initialize $queryString when PDO creates the PDOStatement (https://github.com/php/php-src/blob/ae6c1b0c4ff3468cbc14ffaaaee4e5dc6e480427/ext/pdo/pdo_dbh.c#L457-L460) and I wasn't able to create an uninitialized $queryString via PDO::prepare() in some simple experiments. I'm probably missing something really basic here.
 [2021-05-27 15:40 UTC] corey dot taylor dot fl at gmail dot com
So, the actual issue code generating the issue is not what I thought. We do create PDOStatement through prepare(), but it seems these failures are from a mock object.

We assumed it was generating it for all queries. Your initial assessment is probably close to correct.

        $statement = $this->getMockBuilder('\PDOStatement')
            ->onlyMethods(['execute', 'rowCount', 'closeCursor', 'fetchAll'])
            ->getMock();
        $driver->getConnection()->expects($this->once())
            ->method('prepare')
            ->with('SELECT 1 FROM sqlite_master WHERE name = "sqlite_sequence"')
            ->will($this->returnValue($statement));
        $statement->expects($this->once())
            ->method('fetchAll')
            ->will($this->returnValue(['1']));
        $statement->method('execute')->will($this->returnValue(true));

        $table = new TableSchema('articles');
        $result = $table->truncateSql($connection);
        $this->assertCount(2, $result);
        $this->assertSame('DELETE FROM sqlite_sequence WHERE name="articles"', $result[0]);
        $this->assertSame('DELETE FROM "articles"', $result[1]);


It would be nice if we could somehow generate PDOStatement's in a mock scenario, but if that requires "isset()" because PDOStatement cannot initialize the property, then we can handle that.
 [2021-05-28 09:09 UTC] nikic@php.net
Thanks for the explanation -- this coming up in a mock scenario makes sense to me.

Could it be that your original reason for the `if ($statement->queryString)` check was this mock scenerio as well? Because I don't think it would ever fail in other cases.

I think that the best solution here would be to construct the mock with the property initialized:

        $queryString = 'SELECT 1 FROM sqlite_master WHERE name = "sqlite_sequence"';
        $statement = $this->getMockBuilder('\PDOStatement')
            ->onlyMethods(['execute', 'rowCount', 'closeCursor', 'fetchAll'])
            ->getMock();
        $statement->queryString = $queryString;
        $driver->getConnection()->expects($this->once())
            ->method('prepare')
            ->with($queryString)
            ->will($this->returnValue($statement));

Because then, the code would see the same value it would see in a non-mock scenario.

Unfortunately, this will currently result in:

> Fatal error: Uncaught Error: Property queryString is read only

However, I think we can relax this to allow an assignment as long as the property is still uninitialized. Nowadays our understanding of "read-only" is really "init-once", so that would be in line.

Would this work for you?
 [2021-05-28 16:17 UTC] corey dot taylor dot fl at gmail dot com
The cake database wrapper is too large to show here, but the reason the queryString is checked in a mock scenario is the SQLite driver checks the query post-execute. There's just too much code to completely split up every line in a mock/test scenario so this kind of stuff still runs.

The mock scenario makes it hard to determine if $queryString "should" already be initialized in the driver. We tried tracking things like "ensure execute was called", but the mock scenario created an invalid PDOStatement so that made no difference.

I think the argument that properties can only be initialized by other code is contrary to other argument seen in php dev that all properties should be initialized by the constructor.

That said, allowing us to initialize the queryString as needed in a one time, "readonly" style  would definitely work. That would actually help us test code triggering this error in a mock up properly.
 [2021-05-31 12:55 UTC] nikic@php.net
I've allowed initializing assignments to PDOStatement::$queryString in https://github.com/php/php-src/commit/91eb201fd881f7f897d0647113e4a99d4d4f59e3.
 [2021-05-31 23:15 UTC] corey dot taylor dot fl at gmail dot com
Looks like this should work for us in the future, thanks!
 [2021-06-01 12:57 UTC] cmb@php.net
Can this ticket be closed now?
 [2021-06-01 22:51 UTC] corey dot taylor dot fl at gmail dot com
Ok with me. Maybe we can add an 8.1 specific note the PDOStatement docs.
 
PHP Copyright © 2001-2021 The PHP Group
All rights reserved.
Last updated: Thu Sep 16 20:03:36 2021 UTC