WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#33501 closed defect (bug) (fixed)

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

Reported by: RSkoon Owned by: pento
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.1.2
Component: Database Keywords: needs-patch needs-unit-tests
Focuses: Cc:

Description

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!

Attachments (2)

patch.diff (599 bytes) - added by justingreerbbi 3 years ago.
Using empty()
33501.patch (657 bytes) - added by justingreerbbi 3 years ago.

Download all attachments as: .zip

Change History (25)

#1 @RSkoon
3 years ago

  • Focuses administration added

#2 @RSkoon
3 years ago

  • Focuses administration removed

#3 @RSkoon
3 years ago

  • Focuses administration added

#4 @jorbin
3 years ago

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.

#5 @miqrogroove
3 years ago

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.

#6 follow-up: @pento
3 years ago

  • Focuses administration removed
  • Milestone Awaiting Review deleted
  • Version changed from 4.3 to 4.1.2

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. :-)

#7 @RSkoon
3 years ago

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

#8 in reply to: ↑ 6 ; follow-up: @justingreerbbi
3 years ago

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

Using empty()

#9 in reply to: ↑ 8 ; follow-up: @RSkoon
3 years ago

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;
}

#10 @miqrogroove
3 years ago

var_dump( empty( false ) );

bool(true)

#11 in reply to: ↑ 9 @justingreerbbi
3 years ago

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

#12 @MikeHansenMe
3 years ago

  • Keywords has-patch added

#13 @jorbin
3 years ago

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.

#14 @dd32
3 years ago

  • Milestone set to Awaiting Review

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.

#15 @pento
3 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to 4.4

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

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

#16 @wonderboymusic
3 years ago

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

#17 @jorbin
3 years ago

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

#18 @RSkoon
3 years ago

  • Version changed from 4.1.2 to trunk

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

Last edited 3 years ago by RSkoon (previous) (diff)

#19 @netweb
3 years ago

  • Version changed from trunk to 4.1.2

#20 @pento
3 years ago

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.

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

#22 @SergeyBiryukov
3 years ago

  • Keywords needs-unit-tests added

#23 @pento
3 years ago

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

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.

Note: See TracTickets for help on using tickets.