Calling $wpdb->query() when no connection exists causes mysql_error() to throw an error

[mbijon]

In the query() method of wp-db.php, the mysql_error() expects that if a parameter is passed to it then it should be a valid link identifier.

*May need feedback or added testing* ...this error may be specific to transactions, which is why it hasn't been noticed before. Unit tests do use transations and I only get these errors when ROLLBACK is called during specific unit tests that don't modify the DB state. A simple transaction opened & rolled back in the Debug Bar Console is not triggering this for me.

Unit test failure examples:

Test: test_is_image_positive

mysql_error() expects parameter 1 to be resource, integer given
Execution time: 0.02507209777832s
Stack Trace:
Array
(
    [0] => Array
        (
            [file] => C:\xampp\htdocs\wpnew\wp-includes\wp-db.php
            [line] => 1202
            [function] => mysql_error
            [args] => Array
                (
                    [0] => 0
                )

        )

    [1] => Array
        (
            [file] => C:\xampp\htdocs\wp_test_svn\includes\testcase.php
            [line] => 25
            [function] => query
            [class] => wpdb
            [type] => ->
            [args] => Array
                (
                    [0] => ROLLBACK
                )

        )

)

Test: test_is_image_negative

mysql_error() expects parameter 1 to be resource, integer given
Execution time: 0.02237606048584s
Stack Trace:
Array
(
    [0] => Array
        (
            [file] => C:\xampp\htdocs\wpnew\wp-includes\wp-db.php
            [line] => 1202
            [function] => mysql_error
            [args] => Array
                (
                    [0] => 0
                )

        )

    [1] => Array
        (
            [file] => C:\xampp\htdocs\wp_test_svn\includes\testcase.php
            [line] => 25
            [function] => query
            [class] => wpdb
            [type] => ->
            [args] => Array
                (
                    [0] => ROLLBACK
                )

        )

)

I don't think we should attempt if ( $this->last_error = mysql_error() ). That is because when mysql_error() is called without a link identifier then the most recent connection to MySQL is referenced. On a busy server this may not be the same connection as our page or transaction originally used. See #3544 for history/details.

[kurtpayne]

I'm worried how you're hitting this error condition. Is this a stock setup?

I can reproduce this by setting $wpdb->dbh = 0 in a test case.

If MySQL went away, I would suspect you'd get a different error, like not a valid MySQL-Link resource.

As for the fix, I would suggest using is_resource otherwise you're back to where you started. If $wpdb->dbh is set to 1, your first condition would pass, and you'd still get the above error.

[mbijon]

The setup I tested on was stock. And I'm not able to reproduce those errors on anything but the old laptop that I was using over the holiday.

Then I updated the XAMPP install on that laptop and the problem went away. It's likely specific to that version of PHP or MySQL. Here's the before & after phpinfo's showing what versions were runing: ​https://gist.github.com/4575935

I've never seen a MySQL resource id less than 1, but you're right about is_resource(). New patch using that is coming. We may want to include it just in case there's a bug in a specific version of PHP or mysql_connect.

[mbijon]

patch update with is_resource()

[nacin]

If dbh isn't a resource, isn't mysql_error() causing an error the least concern?

[mbijon]

Yes, especially if it's during WP page delivery.

This is during test though, so there may be something other than a missing dbh value. Possibly a late initialization or something to do with order of operations. If dbh were really missing, why do the test only show it's invalid in this one place?

Based on how many times $this->dbh and $dbh are set and tested for in the wpdb class the order of operations could be a problem for the setup/teardown process in the tests. If so, then our test coverage could just be missing something else. Should $wpdb be checking for errors in a try...catch instead of a conditional block?

[chriscct7]

Patch needs to be refreshed and updated to do the same for mysqli (which is 2 lines above it now in trunk).

[Craig Ralston]

refresh of 23085.2.diff

[Craig Ralston]

23085.3.patch refreshes 23085.2.diff

As far as I can tell, there doesn't need to be anything special for mysqli as there is already a check before $this->last_error is set.

[Craig Ralston]

refresh + same check for mysqli

[pento]

In 37548:

Database: Don't generate unnecessary warnings in wpdb::query().

In the event that the database has gone away for some reason, calls to mysqli_errno() and mysqli_error() (and their ext/mysql equivalents, of course), will generate PHP warnings, which are unsightly, and not how we do things in these parts.

Props mbijon, craig-ralston for the original patch.

Fixes #23085.