Running the SimpleTest Unit Tests in HHVM-nightly caused problems with regards to warnings complaining about "Too many arguments being passed to PDOStatement::execute". I initially thought that it was a bug with the way HHVM implements object subclassing and when a function with the same name but different signature was defined.
I debugged further, and found that the reason why Zend was succeeding was instead because there was an earlier exception raised that prevented SimpleTest from seeing a clean shutdown. To reproduce this: in includes/database/mysql/database.inc, go to function nextIdDelete(), in the catch block, add var_dump($e).
The problem seems to affect the MySQL driver mainly. During shutdown, DatabaseConnection_mysql::__destruct is called, which calls DatabaseConnection_mysql::nextIdDelete, but only after DatabaseConnection::destroy is called. This means two things:
PDOStatement; new prepared statements would therefore be of type PDOStatementDatabaseStatementInterface methods (fetchField et. al)I note that DatabaseConnection_mysql::nextIdDelete uses fetchField (breaking condition #2 above), and the $this->query line would produce a PDOStatement (following condition #1 above). The code thus does not behave as expected.
Workarounds for the masses are, I believe, not currently needed. This is coming purely from a correctness standpoint, specifically when running unit tests (which only developers would do). In my case, it is also to improve HHVM compatibility with D7.
My current proposed solution does a few things:
PDO::ATTR_STATEMENT_CLASS back to PDOStatement, expects the attribute value to be array('PDOStatement') since its class constructor takes no arguments. This resolves the code path problem which silently made the test pass.DatabaseConnection::query will check that the query it is executing is a DatabaseStatementInterface before using the execute variant with the options argument. If it is a PDOStatement (as can be during shutdown) it will use the PDOStatement variant (without the $options argument)DatabaseConnection_mysql::nextIdDelete function will only use PDO-variants of the functions, because it is called in the destructor.| Comment | File | Size | Author |
|---|---|---|---|
| #6 | drupal-check-database-connections-objects-2216899-6-D8.patch | 1.97 KB | lowjoel |
| #1 | drupal-check-database-connection-objects-2216899-1-D7.patch | 1.83 KB | lowjoel |
Comments
Comment #1
lowjoel commentedComment #2
lowjoel commentedComment #3
lowjoel commentedBerdir has suggested that this might apply to D8 too; let me know what you think and if it does I'll work to forward-port this.
Comment #4
sunI vaguely remember an issue that changed the invocation order of
DatabaseConnection::destroy()#1791602: Database::closeConnection() does not actually close connection
#843114: DatabaseConnection::__construct() and DatabaseConnection_mysql::__construct() leaks $this (Too many connections)
Comment #5
berdirYes I'm pretty sure that this happens in 8.x too, I've seen segfaults the last time I've run run-tests.sh in 8.x. Let's verify, port the patch and fix it there first.
Comment #6
lowjoel commentedComment #7
berdirComment #9
lowjoel commented6: drupal-check-database-connections-objects-2216899-6-D8.patch queued for re-testing.
Comment #11
sunSomehow, that doesn't look right?
The query retrieves a single result set, and
fetchField()just returns the first col of the first result row.Perhaps more generally, the issue summary talks about a faulty invocation order of methods upon shutting down / destroying a connection — that doesn't appear to be attacked by this patch? Shouldn't we rather attack the root cause...?
Comment #12
lowjoel commentedNo, fetchColumn is correct: see the PHP docs
As it stands though, I don't think there's any way around this design issue without a larger, more involved rewrite. The problem here is that there are database queries during the database connection destruction sequence. Either we fix it in this manner, or somehow remove the need for database queries during destruction.
Comment #13
jhedstromThe
elseshould be on a new line.Also, the code pointed out in #11 works either way, and isn't really needed for the fix?
Comment #14
mgiffordComment #25
quietone commentedThis is about "Running the SimpleTest Unit Tests in HHVM-nightly" which is not supported.
Closing as outdated per #2165377-25: [meta] HHVM compatibility.
If that is wrong, reopen the issue, by setting the status to 'Active', and add a comment.
Thanks