WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#18176 closed defect (bug) (fixed)

$wpdb should store the database engine and request this of drop-ins

Reported by: nacin Owned by: nacin
Milestone: 3.3 Priority: normal
Severity: normal Version:
Component: Database Keywords: has-patch
Focuses: Cc:

Description

If you have a db.php drop-in, MySQL version checks will be ignored. An update will be allowed to go through.

However, once the update is complete, the drop-in's check_database_version() will be run. If the drop-in uses MySQL (good example would be HyperDB or more commonly a database caching layer, such as W3TC), this is definitely not ideal.

We should do $wpdb->engine = 'mysql'; Then, we'd only skip MySQL version checks if db.php exists AND if engine != 'mysql'.

Only thing this would prevent would be a database drop-in that is designed to support MySQL 4. (And even then, it's just prevent auto-updates.) I don't care to support that.

Attachments (3)

18176.diff (3.5 KB) - added by nacin 4 years ago.
18176.2.diff (3.7 KB) - added by nacin 4 years ago.
18176.notice.patch (892 bytes) - added by ocean90 4 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 @simonwheatley4 years ago

  • Cc simon@… added

@nacin4 years ago

comment:2 @nacin4 years ago

  • Keywords has-patch added; 2nd-opinion removed

Untested, but this is what I had in mind.

comment:3 @nacin4 years ago

Wondering if $is_mysql = true is better than $engine = 'mysql', otherwise we won't have much of a standard for other engines that may wish to implement this.

One problem though, is anyone currently inheriting the wpdb class, will end up with $engine, even if they don't implement MySQL. While I can't imagine why someone would even bother to inherit it at that point, I could see that being a problem.

Thoughts on how to solve that problem? I thought about a class constant or private variable but that would prevent it from being overridden at all. We'd need to implement some methods in order to allow them to properly communicate to the base class what engine they are.

comment:4 follow-up: @Otto424 years ago

Nacin: don't set the $engine variable there, set it in the wpdb class's constructor. If there's not one, make one. Then subclasses only get it if they call the parent constructor.

comment:5 in reply to: ↑ 4 @nacin4 years ago

Replying to Otto42:

Nacin: don't set the $engine variable there, set it in the wpdb class's constructor. If there's not one, make one. Then subclasses only get it if they call the parent constructor.

Thanks for the idea. In that case, we should set it in db_connect(). That method is the first one which calls mysql_connect(). Anyone not using MySQL will *need* to override that (the constructor is a bit agnostic).

@nacin4 years ago

comment:6 @nacin4 years ago

New patch up.

comment:7 @nacin4 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [19060]:

Introduce ->mysql to allow drop-ins to declare themselves as MySQL and therefore allow minimum version checks to still apply. fixes #18176.

@ocean904 years ago

comment:8 @ocean904 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

After [19060] I get a warning:

NOTICE: wp-admin/update-core.php:51 - Undefined variable: required_mysql_version.

Seems like a copy+paste error. 18176.notice.patch does fix it.

comment:9 @ryan4 years ago

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

In [19267]:

Use correct var. Props ocean90. fixes #18176

Note: See TracTickets for help on using tickets.