WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 3 years ago

#23085 closed defect (bug) (fixed)

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

Reported by: mbijon Owned by: pento
Milestone: 4.6 Priority: normal
Severity: normal Version: 3.5
Component: Database Keywords:
Focuses: Cc:
PR Number:

Description

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.

Attachments (4)

23085.1.diff (521 bytes) - added by mbijon 7 years ago.
23085.2.diff (536 bytes) - added by mbijon 7 years ago.
patch update with is_resource()
23085.3.patch (517 bytes) - added by Craig Ralston 4 years ago.
refresh of 23085.2.diff
23085.4.patch (729 bytes) - added by Craig Ralston 4 years ago.
refresh + same check for mysqli

Download all attachments as: .zip

Change History (13)

@mbijon
7 years ago

#1 @kurtpayne
7 years ago

  • Cc kurtpayne added

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.

#2 @mbijon
7 years ago

  • Severity changed from normal to minor

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
7 years ago

patch update with is_resource()

#3 @nacin
7 years ago

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

#4 @mbijon
7 years ago

  • Keywords 2nd-opinion added

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?

#5 @chriscct7
4 years ago

  • Keywords good-first-bug needs-refresh added; has-patch 2nd-opinion removed
  • Severity changed from minor to normal

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

#6 @obenland
4 years ago

  • Owner set to mbijon
  • Status changed from new to assigned

@Craig Ralston
4 years ago

refresh of 23085.2.diff

#7 @Craig Ralston
4 years ago

  • Keywords has-patch added; needs-refresh removed

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
4 years ago

refresh + same check for mysqli

#8 @pento
3 years ago

  • Keywords dev-feedback good-first-bug has-patch removed
  • Milestone changed from Awaiting Review to 4.6
  • Owner changed from mbijon to pento

#9 @pento
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Note: See TracTickets for help on using tickets.