Bug #81084 [Com]: PDOStatement::queryString is uninitialized

Edit report at https://bugs.php.net/bug.php?id=81084&edit=1 ID: 81084 Comment by: corey dot taylor dot fl at gmail dot com Reported by: corey dot taylor dot fl at gmail dot com Summary: PDOStatement::queryString is uninitialized Status: Open Type: Bug Package: PDO Core PHP Version: master-Git-2021-05-26 (Git) Block user comment: N Private report: N New Comment: Looks like this should work for us in the future, thanks! Previous Comments: ------------------------------------------------------------------------ [2021-05-31 12:55:21] nikic@php.net I've allowed initializing assignments to PDOStatement::$queryString in https://github.com/php/php-src/commit/91eb201fd881f7f897d0647113e4a99d4d4f59e3. ------------------------------------------------------------------------ [2021-05-28 16:17:27] 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-28 09:09:26] 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-27 15:40:57] 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-27 09:46:19] 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. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=81084 -- Edit this bug report at https://bugs.php.net/bug.php?id=81084&edit=1