Starting w/ WP4.2, upgrade scenario may fail for non-MySQL installs

[RSkoon]

CONFIGURATION:
Current Wordpress
Azure SQL DB w/ Brandoo DB Abstraction Plugin

FORUM REPORTS:
​https://wordpress.org/support/topic/update-to-421-on-azure-with-ms-sql-server

REPRO:
Attempt to upgrade from < v4.2 to anything current

RESULT:
Endless loop for "database upgrade required" and / or HTTP 500 error

EXPECTED:
Upgrade to work properly

DEBUG INFO:
In wp-include\wp-db.php there are some checks for the table's charset. At the beginning of each of these functions there is a quick check to determine whether or not it is a MySQL database. The issue is that not all abstraction plugins are setting the is_mysql variable, thus causing these functions to return data that causes the upgrade to fail.

There is a quick fix (tested on my production site up until 4.3) that appears to work, but it would likely be good for folks more familiar with the code to chime in on.

In get_col_charset() and get_col_length() there is the following code:

Skip this entirely if this isn't a MySQL database.
if ( false === $this->is_mysql ) {
	return false;
}

Updating each instance (~lines 2404 and 2453) to the following solves the problem:

Skip this entirely if this isn't a MySQL database.
if ( false === $this->is_mysql || empty( $this->is_mysql ) ) {
	return false;
}

This is my first bug report, so not sure how it all works, but I can provide my copy of wp-db.php with the fix if that would be beneficial.

Thanks for your attention to this!

[jorbin]

Thanks for the report RSKoon and Welcome.

To me, this seems like a bug that is better fixed in the database dropin/plugin, not in WordPress core. There are only two ​requirements listed for running WordPress, and MySQL is one of them. Code that overrides this requirement should be responsible for staying updated version to version.

[miqrogroove]

not all abstraction plugins are setting the is_mysql variable

Are you reporting a problem in the plugin or a problem in wp-db.php? When plugins fail to do things, it's usually not a core issue.

[pento]

Given that the is_mysql flag was created specifically for compatibility with non-MySQL db dropins, I'm okay with making some changes to how it works in WPDB.

I think the correct fix would be to change public $is_mysql = null; to public $is_mysql = false;, but I haven't tested that. :-)

[RSkoon]

Thanks all for looking into this and responding...

My thoughts were along the same line as @pento in that a fix was needed to wp-db.php because this was something that appeared to be added to the WP Core to support a non-MySQL scenario. Starting with 4.2.x upgrades in this code path started to cause failures mentioned above.

w.r.t. the fix that I chose, I tried to limit the scope to where the problem(s) were specifically happening. There are other places in the code that are specifically looking, perhaps expecting, is_mysql to be null, so I thought just adding the extra check here was safer.

Thanks for the discussion...
Rob

[justingreerbbi]

Replying to pento:

Given that the is_mysql flag was created specifically for compatibility with non-MySQL db dropins, I'm okay with making some changes to how it works in WPDB.

I think the correct fix would be to change public $is_mysql = null; to public $is_mysql = false;, but I haven't tested that. :-)

If we follow suit with is_mysql instances in WP, then simply using

if ( empty( $this->is_mysql ) ) {
	return false;
}

would work just right and would also make the checks uniform throughout the entire code base. Just for giggles, I am adding a patch.

[justingreerbbi]

Using empty()

[RSkoon]

Replying to justingreerbbi:

Replying to pento:

Given that the is_mysql flag was created specifically for compatibility with non-MySQL db dropins, I'm okay with making some changes to how it works in WPDB.

I think the correct fix would be to change public $is_mysql = null; to public $is_mysql = false;, but I haven't tested that. :-)

If we follow suit with is_mysql instances in WP, then simply using

if ( empty( $this->is_mysql ) ) {
	return false;
}

would work just right and would also make the checks uniform throughout the entire code base. Just for giggles, I am adding a patch.

I think you need both parts of the test...

Consider the scenario where a DB Drop-in does explicitly set is_mysql = false... Then you'll not properly return out of the function because you are only testing for empty().

In that edge case, you would still want this:

if ( false === $this->is_mysql || empty( $this->is_mysql ) ) {
	return false;
}

[miqrogroove]

var_dump( empty( false ) );

bool(true)

[justingreerbbi]

Replying to RSkoon:

Replying to justingreerbbi:

Replying to pento:

Given that the is_mysql flag was created specifically for compatibility with non-MySQL db dropins, I'm okay with making some changes to how it works in WPDB.

I think the correct fix would be to change public $is_mysql = null; to public $is_mysql = false;, but I haven't tested that. :-)

If we follow suit with is_mysql instances in WP, then simply using

if ( empty( $this->is_mysql ) ) {
	return false;
}

would work just right and would also make the checks uniform throughout the entire code base. Just for giggles, I am adding a patch.

I think you need both parts of the test...

Consider the scenario where a DB Drop-in does explicitly set is_mysql = false... Then you'll not properly return out of the function because you are only testing for empty().

In that edge case, you would still want this:

if ( false === $this->is_mysql || empty( $this->is_mysql ) ) {
	return false;
}

By gosh that is true! 0_o

[jorbin]

I think this patch will cause DB dropins that are for MySQL to have failures unless they have been updated to set $this->is_mysql to a truthy value or extended the existing wpdb class.

[dd32]

See #18176 for original implementation.

Based on that, it looks like DB dropins were supposed to declare themselves as mysql, with wpdb::db_connect() doing it for them (for back-compat).
That would suggest that a if ( empty( $wpdb->is_mysql ) ) would be the right check IMHO.

[pento]

I agree with @dd32 - empty( $wpdb->is_mysql ) is the correct test.

This patch will also need to be backported to old branches.

[jorbin]

I reverse my objection and now support this. Please carry on.

[RSkoon]

Thanks all... I'm not sure how the QA process here works, but I am happy to try out an official patch prior to 4.4 if that would help.

Appreciate it

EDIT: I somehow unintentionally changed the version with this comment and it won't give me an option to undo / revert it back. I apologize, and if someone could change back

[pento]

Hey @RSkoon, you're totally welcome to write a patch!

For the wp-db.php part of the patch, just change the false === $this->is_mysql tests to empty( $wpdb->is_mysql ).

I think it'd be good to have some unit tests for this, too. If you're up for writing some, check out tests/phpunit/tests/db.php as a place to add them, I'd go with faking the DB drop-in behaviour by changing the value of $is_mysql in the test.

[pento]

In 34655:

WPDB: Make sure we don't run sanity checks on DB dropins.

Previously, we'd run the sanity checks if is_mysql was not set to false. This caused problems for DB drop-ins that didn't define is_mysql at all. Instead, we can just check if is_mysql is empty().

Also fix some unit tests that accidently ran correctly because of the strict false === comparison.

Fixes #33501.