Opened 9 years ago
Closed 9 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)
Change History (25)
#5
@
9 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:
↓ 8
@
9 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
@
9 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:
↓ 9
@
9 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;
topublic $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.
#9
in reply to:
↑ 8
;
follow-up:
↓ 11
@
9 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;
topublic $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; }
#11
in reply to:
↑ 9
@
9 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;
topublic $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
#13
@
9 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
@
9 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
@
9 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.
#18
@
9 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
#20
@
9 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.
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.