WordPress.org

Make WordPress Core

Opened 20 months ago

Closed 11 days ago

#21663 closed task (blessed) (fixed)

Use mysqli for MySQL queries when available

Reported by: scottconnerly Owned by:
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.5
Component: Database Keywords: needs-testing
Focuses: Cc:

Description

the mysql_* functions are officially deprecated for PHP 5.4 and will begin throwing E_DEPRECATED errors in the next version of PHP.
http://marc.info/?l=php-internals&m=131031747409271&w=2

Wordpress should use PDO by default, but fall back to mysql_* when PDO is not present.

See also: #11622 for last year's discussion.

Attachments (27)

21663.patch (21.8 KB) - added by kurtpayne 20 months ago.
Proof of concept - use a database driver system, leave wp-db mostly intact
21663.2.patch (21.4 KB) - added by kurtpayne 20 months ago.
Updated for changeset 21420
21663.3.patch (21.5 KB) - added by kurtpayne 20 months ago.
Fixed a bug with mysql_num_rows
21663.4.patch (21.5 KB) - added by kurtpayne 16 months ago.
Bugfixes, 3.6 compat
21663.5.patch (21.5 KB) - added by kurtpayne 15 months ago.
Address PDO::errorInfo notice
21663.6.patch (19.9 KB) - added by scribu 15 months ago.
get_driver() -> set_driver() + cleanup
21663.7.patch (81.6 KB) - added by gohanman 13 months ago.
Remove mysql extension requirement
ssl.patch (4.0 KB) - added by hypertextranch 9 months ago.
Path for wp-db-driver to add SSL support to PDO and mysqli
21663.diff (22.3 KB) - added by wonderboymusic 5 months ago.
21663-PDO-merge.diff (10.1 KB) - added by wonderboymusic 5 months ago.
21663.2.diff (6.0 KB) - added by aaroncampbell 5 months ago.
21663.3.diff (6.9 KB) - added by aaroncampbell 5 months ago.
21663.4.diff (7.0 KB) - added by aaroncampbell 5 months ago.
21663.5.diff (8.3 KB) - added by aaroncampbell 5 months ago.
21663.6.diff (7.8 KB) - added by aaroncampbell 5 months ago.
21663.7.diff (726 bytes) - added by wonderboymusic 5 months ago.
21663.8.diff (737 bytes) - added by wonderboymusic 5 months ago.
21663.9.diff (807 bytes) - added by wonderboymusic 5 months ago.
21663.10.diff (8.4 KB) - added by pento 3 months ago.
21663.11.diff (10.7 KB) - added by pento 8 weeks ago.
21663.12.diff (11.4 KB) - added by pento 8 weeks ago.
21663.13.diff (977 bytes) - added by nacin 7 weeks ago.
21663.14.diff (3.1 KB) - added by nacin 7 weeks ago.
21663.15.diff (1.9 KB) - added by pento 7 weeks ago.
21663.16.diff (472 bytes) - added by arnee 5 weeks ago.
21663.17.diff (1.1 KB) - added by pento 2 weeks ago.
21663.18.diff (1.4 KB) - added by pento 2 weeks ago.

Download all attachments as: .zip

Change History (256)

comment:1 follow-ups: kurtpayne20 months ago

  • Cc kpayne@… added

Would it make sense to create translation functions for mysql_* if they're not available?

I can see this being a saving grace for plugins / themes that do not use $wpdb. Example plugin

Incomplete non-functioning concept code:

<?php

if (!function_exists('mysql_query')) {
        $pdo_conn = null;
        function mysql_connect($host = 'localhost', $user = '', $pass = '' ) {
                global $pdo_conn = new PDO('mysql:;host=' . $host, $user, $pass);
        }
        function mysql_query($sql) {
                global $pdo_conn;
                return $pdo_conn->query($sql);
        }
        function mysql_fetch_assoc($rs) {
                return $rs->fetch(PDO::FETCH_ASSOC);
        }
        // ... etc.
}
Last edited 20 months ago by kurtpayne (previous) (diff)

comment:2 in reply to: ↑ 1 ; follow-up: scribu20 months ago

I don't think we should waste effort on making plugins/themes that still don't use $wpdb work in PHP 5.4.

The cleanest way to start using PDO would be something like this:

if ( PDO is available ) {
  require 'wp-includes/wp-db.php';
  $wpdb = new wpdb( ... );
} else {
  require 'wp-includes/wp-db-legacy.php';
  $wpdb = new wpdb_legacy( ... );
}

comment:4 sirzooro20 months ago

  • Cc sirzooro added

kurtpayne20 months ago

Proof of concept - use a database driver system, leave wp-db mostly intact

comment:5 kurtpayne20 months ago

  • Keywords dev-feedback added

Related, #21055


Patch attached. Should be fully backwards compatible. All unit tests pass, and no code changes required anywhere outside of wpdb.

This leaves wpdb alone for the most part, and just swaps in a different database driver to handle any msyql_* specific calls. No new strings, even! It includes drivers for mysql, mysqli, and pdo_mysql.

This doesn't overreach (no transaction support, and no rewriting of prepare ). It's just designed to address main concern of the ticket: the mysql extension may not be available (due to deprecation?) at some point, and wpdb should support other options.

As we discussed in IRC, this is probably far in the future as deprecation notices won't be turned on until after php 5.4 and other drivers (mysqli, pdo) have their quirks, too.

comment:6 wonderboymusic20 months ago

  • Keywords has-patch added; needs-patch removed

+2 - I love this, transaction support in PDO would make me giddy.

kurtpayne20 months ago

Updated for changeset 21420

comment:7 kurtpayne20 months ago

Patch updated for [21420]. Currently, the mysql driver is the only one that works with the new config options. The mysql, mysqli, and PDO drivers all have different ways to specify their options, so a single constant in wp-config.php won't work going forward with this patch.

Also, I'm not sure if there's a new_link flag for PDO/mysqli. Isn't this default unless specify persistent connections? Perhaps this is addressable in the drivers ...

kurtpayne20 months ago

Fixed a bug with mysql_num_rows

comment:8 kurtpayne20 months ago

Latest patch fixes a bug where mysql_num_rows was called on a result set that may not actually be valid (i.e. bad query).

comment:9 ericlewis19 months ago

  • Cc eric.andrew.lewis@… added

comment:10 convissor17 months ago

  • Cc danielc@… added

PHP is looking to deprecate the mysql soon:
https://wiki.php.net/rfc/mysql_deprecation

To aid this process, Rasmus is urging applications such as WP to provide mysqli support:
http://news.php.net/php.internals/63825

comment:11 rmccue17 months ago

+1 on this, especially in light of the MySQL deprecation. The question is whether to use mysqli or PDO with the MySQL driver. I think we should stick to a single one in core rather than having both. mysqli has a larger install base than PDO as far as I remember.

comment:12 scribu17 months ago

Advantages of PDO:

  • has client-side prepared statements
  • lightweight ORM (WP_Post anyone?)
  • supports multiple database types

comment:13 maxmaeteling17 months ago

  • Cc maxmaeteling added

comment:14 rmccue17 months ago

nacin has expressed a preference for PDO, so that's looking likely at the moment. It'd be nice to see statistics of mysqli vs PDO + PDO_MySQL.

comment:15 scribu17 months ago

As much as I'd like for us to just switch to PDO and be done with it, I think scottconnerly's approach is the safest:

WordPress should use PDO by default, but fall back to mysql_* when PDO is not present.

Later on, if needed, we can switch the fallback from mysql_* to mysqli_*.

comment:16 follow-up: rmccue17 months ago

The voting for the RFC is currently underway, for those interested. (It needs a 50% + 1 extra vote to succeed, as it's not a syntax change.)

comment:17 knutsp17 months ago

  • Cc knut@… added

comment:18 magnus7817 months ago

  • Cc magnus.melin@… added

comment:19 in reply to: ↑ 16 Viper007Bond17 months ago

Replying to rmccue:

The voting for the RFC is currently underway, for those interested. (It needs a 50% + 1 extra vote to succeed, as it's not a syntax change.)

Looks like it's going to pass.

comment:20 rmccue17 months ago

I think we should try and tackle this in 3.6-early so that it gets the maximum testing time through the cycle.

comment:21 rmccue17 months ago

The vote succeeded, 25-12. Post from PHP internals:

I ended up leaving the vote open for a couple of extra days (been a
busy week), but I've now closed the ext/mysql deprecation vote. The
full results are at https://wiki.php.net/rfc/mysql_deprecation — the
short version is that the final vote was 25-12 in favour of
deprecation in PHP 5.5.

This made the second question moot, but for the record, the result
there was 26-12 in favour of option (a) (deprecation in PHP 5.6 if not
in PHP 5.5).

[...]

I intend to commit the patch along with the relevant test updates in
the next few days. Admittedly, I'm bad with timeframes, so please
don't hang me if it slips a little. It'll be in before beta 1, I
promise.

comment:22 deltafactory16 months ago

  • Cc jeff@… added

comment:23 brokentone16 months ago

  • Cc kenton.jacobsen@… added

comment:24 rmccue16 months ago

We're now in 3.6-early, so I personally think we should try and land this as soon as possible. Will this need a refresh?

kurtpayne16 months ago

Bugfixes, 3.6 compat

comment:25 kurtpayne16 months ago

21663.4.patch has been tested with 3.6, unit tests, and installer code. Seems to work in PDO and classic mysql modes. This new version addresses a few bugs and allows for the user to define a driver via a WPDB_DRIVER constant in wp-config.php or db.php.

I'm open to input. There was some disagreement over whether interfaces was the right way to go in #6821.

This model should give plugin authors the ability to define a new db driver in db.php, too.

comment:26 hakre16 months ago

If you go the driver route, you should reduce duplicate code between those classes, e.g. to have that in a (abstract?) base class.

Also I'd say there is pretty much an overhead through the level of indirection by having "drivers". I mean PHP offers this Drivers Thingy with PDO, no need to re-invent the wheel technically - just to integrate PDO which is already driver based.

Just while I've quickly seen it in the patch: For the PDO Driver, keep in mind that the DSN can be an alias. You can not just convert host / database / username / password here into a DSN like in the patch. Also you've hardencoded the actual PDO driver. That's limiting the use of a PDO based database class for no reason. Just saying, what jumps into my sight, it's maybe too early for concrete feedback at all.

To reduce the levels of indirection, it is probably more straight forward to offer a set of default database classes that Wordpress can load (by configuration). If you work with Interfaces, it is not important any longer if that included database class then is having a full blown driver implementation loading diverse drivers or is just a straight-forward database class as we have it since years (and as we love it). The legacy database class then can become the blueprint for the interface.

Next generation classes could implement an interface that extends from the legacy interface (to deal with the problems the original implementation has, or at least to offer some way to deal with it).

Also I suggest you leave warnings all over the place that the prepare function is misnamed. I've seen on the Wordpress dev blog that users are being told this is the way to "prepare" just not so long ago, but the prepare function actually just verifies and sanitizes the first string parameter (the query) and then does some dumb stuff on the passed array that should represent values. This is the exact opposite of what prepared and parametrized queries are.

If you want to improve things, start there offering an additional function that follows common parametric queries patterns so that it's not such a burden to go there with a Mysqli / PDO based class. The escape route is not used by a driver based abstraction, like PDO demonstrates.

Otherwise, this all might just not be worth the work because even as of today, you can simply replace the database class with the taste you like most.

Just my 2 cents.

Last edited 16 months ago by hakre (previous) (diff)

comment:27 alex-ye15 months ago

  • Cc nashwan.doaqan@… added
  • Version set to trunk

I think we should do this in WordPress 3.6 Cycle to have a better performance and easy API :) , Also PHP 5.5.0 fully deprecate mysql_* functions now .

Last edited 15 months ago by alex-ye (previous) (diff)

comment:28 alex-ye15 months ago

  • Keywords needs-refresh added

comment:29 SergeyBiryukov15 months ago

  • Keywords needs-refresh removed
  • Version trunk deleted

21663.4.patch still applies cleanly.

comment:30 follow-up: SergeyBiryukov15 months ago

Seeing a notice with 21663.4.patch:

Notice: Undefined offset: 2 in wp-includes/class.wp-db-driver-pdo_mysql.php on line 60

comment:31 ryan15 months ago

escape() should probably stay in wpdb since it is a weak escape, not a real escape. If we introduce a public real escape method then that can go in the drivers.

comment:32 aaroncampbell15 months ago

  • Cc aaroncampbell added

comment:33 in reply to: ↑ 30 ; follow-up: kurtpayne15 months ago

Replying to SergeyBiryukov:

Seeing a notice with 21663.4.patch:

Notice: Undefined offset: 2 in wp-includes/class.wp-db-driver-pdo_mysql.php on line 60

How did you encounter this? PDO::errorInfo should always return a 3 element array.

comment:34 in reply to: ↑ 33 SergeyBiryukov15 months ago

Replying to kurtpayne:

How did you encounter this? PDO::errorInfo should always return a 3 element array.

Just applied the patch and saw a lot of these notices in the admin and on the front-end.

PHP 5.2.14 on Windows. This is what var_dump( $error ) shows:

array(1) { [0]=> string(5) "00000" } 

comment:35 rmccue15 months ago

Related: http://stackoverflow.com/questions/3999850/pdo-error-message

It seems like somewhere internally in PDO it runs an array_filter() and removes the second and third elements since they're null. Yay PHP.

kurtpayne15 months ago

Address PDO::errorInfo notice

comment:36 kurtpayne15 months ago

The PDO::errorInfo() notice should be addressed now. If there's no third element, this method will return an empty string.

comment:37 ryan15 months ago

  • Milestone changed from Awaiting Review to 3.6
  • Type changed from enhancement to task (blessed)
  • Version set to 3.5

comment:38 sc0ttkclark15 months ago

  • Cc lol@… added

comment:39 sbrajesh15 months ago

  • Cc sbrajesh added

comment:40 bpetty15 months ago

  • Cc bpetty added

comment:41 ocean9015 months ago

  • Cc ocean90 added

comment:42 follow-up: adegans15 months ago

In support of earlier comments -
I don't see any point in wasting time, as one put it, to keep plugins using mysql_* compatible.

If anything, such plugins should be removed from the repository and treated as a risk or as non-compatible to WordPress as a whole.
In a "get-with-the-program-or-stay-away" manner.

comment:43 toscho15 months ago

  • Cc info@… added

comment:44 rachelbaker15 months ago

  • Cc rachelbaker added

comment:45 in reply to: ↑ 42 alex-ye15 months ago

  • Keywords needs-testing added

Replying to adegans:

If anything, such plugins should be removed from the repository and treated as a risk or as non-compatible to WordPress as a whole.
In a "get-with-the-program-or-stay-away" manner.

Why the support of mysql_* functions is still exists , and if you see the code above there is a constant WPDB_DRIVER that can select the needed database driver .

Notice that the plugin that used mysql_* functions was should using $wpdb class insted of :)

comment:46 follow-up: scribu15 months ago

21663.6.patch:

  • renamed get_driver() to set_driver(), mark as private and update phpdoc
  • renamed new files to better match the other files in wp-includes
  • updated phpdoc for driver classes

What do we do with wpdb::set_charset()? It's calling mysql_set_charset().

scribu15 months ago

get_driver() -> set_driver() + cleanup

comment:47 adamsilverstein15 months ago

  • Cc ADAMSILVERSTEIN@… added

comment:48 in reply to: ↑ 46 kurtpayne15 months ago

Replying to scribu:

What do we do with wpdb::set_charset()? It's calling mysql_set_charset().

The new patch issues a query for SET NAMES %s and COLLATE %s which get delegated to the specific driver.

comment:49 kurtpayne15 months ago

From IRC today, the prevailing thought is to dump mysqli support and probably the driver system, too.

My original thoughts when writing this: Get off of the old mysql extension. If PDO is available, great. If not, jump onto mysqli. The original cause for the ticket was "mysql is going to be deprecated." I don't know of a firm method or date for that deprecation, though.

mysqli doesn't buy us anything over PDO.

What are our options?

  1. Keep the new system, it's fine
  2. Drop the mysqli driver
  3. Drop the new system and rewrite wp-db to just have PDO
  4. ???

It doesn't look like WordPress will easily support other database engines (e.g. sqlite) at this point without rewriting queries or abstracting out the database layer further.

Considerations: How will this impact hyperdb? What can we do to add value while we're under the hood?

comment:50 johndoe12345615 months ago

one thing pdo doesnt offer, but mysqli does is a set_charset() function. If you think its a good idea to emulate it by issuing a set names sql query, realize it's not fully equivalent.

read

http://www.php.net/manual/en/mysqlinfo.concepts.charset.php

http://stackoverflow.com/questions/1650591/whether-to-use-set-names/14132028#14132028

I don't use wordpress, but I have a feeling you and your users will still do manual string escaping for many years. And, they may change the charset at runtime after initial connection.

Not only could there be minor bugs, but maybe even rare sql injection opportunities reminiscent of the conditions talked about here
http://shiflett.org/blog/2006/jan/addslashes-versus-mysql-real-escape-string (the point is the escaping function was naive of the character set, and therefore couldn't properly escape the string).

although - this is nothing new for any current wordpress code that uses mysql ext and uses a set names query instead of calling mysql_set_charset(). its exactly the same.

food for thought. good luck.

comment:51 in reply to: ↑ 2 ; follow-ups: bpetty15 months ago

Replying to scribu:

I don't think we should waste effort on making plugins/themes that still don't use $wpdb work in PHP 5.4.

By my count, I've found 1,722 plugins in the WP.org plugin repository that make raw calls to mysql_*() functions. Some might have nothing to to with the WP main connection or not actually be calls to core PHP methods, but most of them likely are.

That's 7.4% of all plugins.

What those plugins are doing is wrong, however, it works, and I don't think we should intentionally break them before we absolutely need to. We can at least give them a fighting chance by only using the PDO (or mysqli) extension when either the mysql extension is no longer available, or just on PHP 5.5+ (to avoid dealing with deprecation notices during development even though the mysql extension is available). There's not any compelling reasons that I see for making the jump any earlier than PHP 5.5; we can continue using mysql through PHP 5.4.

comment:52 in reply to: ↑ 51 kurtpayne15 months ago

Replying to bpetty:

By my count, I've found 1,722 plugins in the WP.org plugin repository that make raw calls to mysql_*() functions. Some might have nothing to to with the WP main connection or not actually be calls to core PHP methods, but most of them likely are.

That's 7.4% of all plugins.

You know how many of those are > 2 years old?

comment:53 sc0ttkclark15 months ago

Does this include old versions too (from older tags)? I'm sure that could also slant things maybe.

comment:54 scribu15 months ago

Some might have nothing to to with the WP main connection or not actually be calls to core PHP methods, but most of them likely are.

That's a very important assumption. It could mean that 7.39% of all plugins would be affected or only 0.01%.

comment:55 bpetty15 months ago

It's pretty awesome how much everyone here raves about how WordPress maintains full compatibility and never removes old API, but when it comes down to things like this, everyone is like "screw them".

What are we gaining by using PDO or mysqli in PHP 5.3 or 5.4 as opposed to using mysql? The answer is absolutely nothing. In fact, if this new code isn't extensively tested before release, we're actually just risking potential bugs (including possible SQL injection issues) with new changes that provide no additional benefits in PHP 5.4 and below. So why is it even being debated? We have the option of slowly rolling this out, and we should take advantage of that.

comment:56 deltafactory15 months ago

Supposedly, there are performance gains by using *any* module but the original mysql. PHP recommends against using it in their own documentation, and it will be deprecated before long.

I agree that it's important to maintain compatibility, but I'm looking forward to performance gains as well.

I am curious to know how you queried the repository to look for mysql_* calls. I have a feeling that scribu is right that this number is inflated by older code or older tags.

Question: What sort of performance impact would be seen by creating legacy connection in addition to the PDO/mysqli connection. Wouldn't that be enough to allow older code to continue undisturbed?

comment:57 swissspidy15 months ago

  • Cc hello@… added

comment:58 brokentone15 months ago

@bpetty In fairness, the ticket has been open for 5 months, there has been time to raise concerns.

@deltafactory I've applied the patch to a copy of our production DB/code and benchmarked actual page generation. Thus far it's working fine in PDO-mode at performance numbers within a margin of error. No significant loss/gain.

I'm also interested to hear how this would impact HyperDB. I'm personally interested in moving the idea of DB roles into core (for comparison, Drupal has been there for some time).

comment:59 follow-up: bpetty15 months ago

Apparently everyone seems to have gotten the impression that I'm suggesting we don't migrate to PDO/mysqli at all, so I just want to clarify that this is *not* the case at all. I like this patch, and I do know the mysql extension is deprecated in PHP 5.5, so this has to happen.

To be perfectly clear, what I'm saying is that the patch needs to not default to PDO first if it's available *unless* the version of PHP being used is 5.5 or above, and in cases of PHP 5.4 and below, it should default to using the existing mysql extension it's using right now. In doing so, we give plugin authors who did incorrectly use raw mysql calls a chance of fixing their plugin before it breaks on every WordPress installation the second everyone upgrades to 3.6, and it also gives users of those plugins the time to perform their plugin updates appropriately as well.

comment:60 knutsp15 months ago

As long as we can disable mysql and enable PDO for any PHP version, and then run or test WordPress through PDO, we are fine. For PHP < 5.5, both drivers enabled, mysql takes priority. For PHP >= 5.5, both drivers enabled, PDO takes priority to avoid using deprecated functionality. For any PHP version, when only one driver is avaialable, WordPress will use that.

comment:61 in reply to: ↑ 59 Otto4215 months ago

Replying to bpetty:

To be perfectly clear, what I'm saying is that the patch needs to not default to PDO first if it's available *unless* the version of PHP being used is 5.5 or above, and in cases of PHP 5.4 and below, it should default to using the existing mysql extension it's using right now. In doing so, we give plugin authors who did incorrectly use raw mysql calls a chance of fixing their plugin before it breaks on every WordPress installation the second everyone upgrades to 3.6, and it also gives users of those plugins the time to perform their plugin updates appropriately as well.

I agree with the idea, but disagree with the approach.

Plugins using mysql_* functions basically are relying on the mysql_connect and mysql_select_db functions having been called to set up that initial connection for them to use. It would be possible to call both of those in PHP < 5.5 installs, just to set up the connection for plugins, and then to continue on using the newer methods, if available, for the speed improvements.

For the people who argue that making a connection which is then unused is rather stupid, I agree. But it would both maintain backward compatibility and give the performance boost. And, we could wrap this extra connection in a define or some such to allow power-users to turn it off manually via their wp-config.

If you want to take this idea further and somehow detect when mysql_* functions are being used and do the connection on the fly somehow, then feel free to think about that. But for a first draft, just adding a couple of extra old mysql calls is enough for backward compat, I believe.

comment:62 aaroncampbell15 months ago

I'm really against putting all this effort into PDO and then only enabling it by default on sites with PHP 5.5+. It seems like a waste.

I'm not in love with Otto's suggestion, but it may be a good way around these poorly coded plugins (and they are poorly coded, we've had wpdb::query() for this since BEFORE v1.0).

comment:63 sc0ttkclark15 months ago

Has anyone put together an actual list of these plugins that are doing it wrong? Would be good to call these plugins out in case anyone can get w/ these authors and help them migrate over.

I'd also like to see pdo/mysqli/mysql be the order priority of usage. Backwards compatibility connection to mysql_ if it's available and pre PHP 5.5.

Version 0, edited 15 months ago by sc0ttkclark (next)

comment:64 wonderboymusic15 months ago

I am very against bending over backwards to support bad old plugins. I like to think I do things the right way, and with every release, I've had to recode something to prepare. Exempting the lowest common denominator from such "pain" is just ignoring the problem. There was an admin action in 3.5 which was removed without warning that broke 3 of the plugins I wrote for our photo editor. Do we not expect plugin authors to be the ones responsible for keeping up with the times?

comment:65 follow-up: deltafactory15 months ago

@sc0ttclark, I asked a similar question. Not sure how to efficiently search the code of the repository, short of downloading the entire trunk.

@wonderboymusic, it's not bending over backward to *keep* the current functionality (on some level) while trying to move the platform to its most current. I'm surprised you are so gung-ho about leaving people behind considering the work you had to do so recently.

The lead devs are conscious of the "brand" impact this has. WP doesn't want a reputation of breaking during updates for a number of reasons that I don't need to cover here. The $wpdb->prepare() situation from 3.5 was another recent instance where bad plugin code penalized "regular" users who had otherwise had a safe experience upgrading WP. I don't think I'm alone in my desire to prevent bad plugin code + hasty decisions in core from hurting users who aren't web developers.

I would like to see a phased approach based on what I and others have suggested:

  1. Provide a legacy connection with mysql_connect() etc, no matter what $wpdb uses.
  2. Gather stats about offending plugins in the repository and notify authors. (How?)
  3. Announce the intention to deprecate, knowing it probably won't reach the offenders.
  4. Possibly scan for mysql_*() functions and warn (on the plugin admin page, only?)
  5. Remove legacy connection, with option to re-enable by config constant, starting in ~3.8

Dropping mysql isn't an option. Its removal from PHP is close enough that now is as good a time as any to start "ripping off that band-aid" as far as the WP community is concerned.

comment:66 in reply to: ↑ 65 scribu15 months ago

Replying to deltafactory:

Not sure how to efficiently search the code of the repository, short of downloading the entire trunk.

See https://github.com/markjaquith/WordPress-Plugin-Directory-Slurper

comment:67 brokentone15 months ago

In general I think @deltafactory's suggestions are reasonable, with one caveat and one question.

The legacy connection needs to be able to be turned on and off by a constant as soon as it's introduced. I'd prefer off by default, but I could go either way so long as it exists. Opening both a PDO and mysql connection will double the open mysql connections. While PDO/mysql seem negligible in my testing, doubling the connections will be a huge performance issue.

Just think of all the WP sites we've ever seen go down for a DB error, now imagine that suddenly happens at half the traffic.

The question is whether WP has a precedent of version detection. I've seen plenty of feature detection and that makes sense, but I don't know if I get the value of version detection at an app level in the PHP world.

comment:68 bpetty15 months ago

In regards to performance, this is the driver we're talking about, not the query itself. It's almost never the bottleneck compared with the query, your indexes, or your PHP code working on the results (which should already be cached anyway if it's been optimized properly).

Of course this doesn't stop people from benchmarking them anyway. Many of them don't find more than a 5% difference in performance with equivalent operations. Just as many find wildly different results. One benchmark even claims (though without publishing test details/results) that the original MySQL extension is faster in all tests they ran.

Regardless, don't believe most of the benchmarks out there in regards to this. Most of them don't configure their sandbox properly for benchmarking (tuning various optimization settings, clearing caches, etc), and don't measure the appropriate aspects, often mixing performance measurements of tasks unrelated to the driver itself such as tests that spend 90% of the time running the SQL query itself, or code that extracts data from the results - assuming that these will perform consistently enough that they won't affect measurements, and assuming that this is mostly how the driver is being used across all installations of the application.

Of course, we should be profiling these changes anyway, just in case some part of it is inefficiently written (outside of use of the driver anyway) and does make a significant difference for a default WP installation. However, focus on compatibility is much more important than database driver performance (and staying on what everyone is already using right now until PHP is upgraded isn't hurting anyone's current performance). I haven't seen any guides or presentations on WordPress performance ever suggest possibly switching this out to speed up WordPress (and it's ridiculous how many WP performance presentations I've seen, there's at least one per WordCamp). There's significantly more important aspects to consider when it comes to performance.

The scope of this ticket was originally to address the issue of compatibility, not performance. If performance was really a concern here, we would have seen a patch doing exactly the same thing here well before the MySQL extension was deprecated.

comment:69 ryanduff15 months ago

  • Cc ryan@… added

comment:70 dave101015 months ago

  • Cc dave1010@… added

comment:71 nacin14 months ago

  • Milestone changed from 3.6 to Future Release

Ryan and I discussed this ticket about a month ago in IRC and decided then it should be punted. I am sorry this did not make it back to the ticket. For a few reasons.

PHP 5.5 isn't out yet, and PHP 5.4 (despite being out for a year) as a whopping 1% market penetration. Because this only results in E_DEPRECATED (which we suppress anyway), we're at least a few months to a year out before we even start to look bad here, and we'll work just fine for a while. I would love to revisit this and get it done in either 3.7 or 3.8 (as in, this year). It just isn't a priority for 3.6.

Emphasis this cycle on slashing, caching, and our nascent terms roadmap needed to take precedence, and we didn't have as much bandwidth as expected to cover everything.

Once PHP 5.5 is out, I imagine there will be increased interest in this ticket. That is good. Maybe at that point there can be a better consensus on the approach, as well as a clearer picture of the impact.

Even something as simple as deciding which engine will be used if both are available (pre-5.5) is an important decision. Our mysql engine *does work*. We don't want a bug in the PDO engine to cause us problems on servers and PHP versions that are just fine. Also, PDO has quite a bit of bugs in earlier versions of PHP (5.2, 5.3) which make up the most of our target audience. We should research said issues.

comment:72 eddiemoya14 months ago

  • Cc eddie.moya+wptrac@… added

comment:73 elyobo13 months ago

  • Cc wp@… added

I was reading through this with growing excitement and repeated thoughts of "well, finally, about time", so I was pretty sad to hit nacin's comment at the end and have my hopes dashed for the next few years (most likely, PHP releases seem to roll around very slowly).

An approach which maintains backwards compatibility in the meantime (as advocated by some above), but allows PDO as a non-default option, would allow time for the code to stabilise, to get some practical research into any lingering PDO bugs (the only problems I ran into were pre-5.2) and to get feedback from other developers.

While we certainly can wait around, it's not something that we need to do and we're only delaying the inevitable and (most likely) making the eventual pain worse by choosing to do things at the last possible moment rather than allowing a smooth transition by getting it underway now.

gohanman13 months ago

Remove mysql extension requirement

comment:74 gohanman13 months ago

I know it's an old discussion. Change is in wp_check_php_mysql_versions() so that disabling the mysql extension in PHP doesn't throw an error provided an alternate extension is available. Diff is against latest master branch on github; apologies if I screwed something up int the git => svn patch translation.

comment:75 follow-up: bpetty13 months ago

That was a nice catch on the extra extension check that was missing from the previous patches in wp-includes/load.php.

You're right though, your patch is unusable aside from that. I'm not really sure what you did wrong to end up with what looks like 3 copies of the every addition besides your small change.

comment:76 ericlewis13 months ago

  • Cc eric.andrew.lewis@… removed

comment:77 in reply to: ↑ 75 ; follow-up: gohanman13 months ago

Replying to bpetty:

You're right though, your patch is unusable aside from that. I'm not really sure what you did wrong to end up with what looks like 3 copies of the every addition besides your small change.

Sorry. I can't find any good information on generating a patch-compatible patch from diff-ing two git branches. "Patch compatible patch" doesn't get particularly good search results.

comment:78 in reply to: ↑ 77 dd3213 months ago

Replying to gohanman:

Sorry. I can't find any good information on generating a patch-compatible patch from diff-ing two git branches. "Patch compatible patch" doesn't get particularly good search results.

http://scribu.net/wordpress/svn-patches-from-git.html should get you on your way

comment:79 ozh12 months ago

  • Cc ozh@… added

comment:80 Denis-de-Bernardy12 months ago

  • Cc ddebernardy@… added

comment:81 m_uysl12 months ago

  • Cc m_uysl@… added

comment:82 in reply to: ↑ description vibhavsinha12 months ago

  • Cc vibhavsinha added

comment:83 exz11 months ago

  • Cc rickard@… added

comment:84 dave101010 months ago

Update: PHP 5.5 is now out and now throws E_DEPRECATED errors when calling mysql_* functions. PHP 5.4 usage is still low (probably especially in shared hosting), but is increasing.

What needs to be done next?

comment:85 mbijon10 months ago

  • Cc mike@… added

comment:86 lkraav10 months ago

  • Cc leho@… added

comment:87 follow-up: markoheijnen10 months ago

I just released a plugin from the latest changes: http://wordpress.org/plugins/wp-db-driver/. Hopefully this helps out with testing.

@gohanman: Your repo has the same driver multiple times.

  • class-wp-db-driver-mysql.php
  • class.wp-db-driver-mysql.php
  • wp-db-driver-mysql.php

comment:88 in reply to: ↑ 87 knutsp10 months ago

Replying to markoheijnen:

I just released a plugin from the latest changes: http://wordpress.org/plugins/wp-db-driver/. Hopefully this helps out with testing.

I have done some testing of this plugin. It seems it fails when there is no pdo_mysql exension loaded. See plugin support thread http://wordpress.org/support/topic/testing-results-1?replies=1#post-4362085

comment:89 markoheijnen10 months ago

The MySQLi driver is broken. The part I don't understand is the return value from wpdb_driver::query( $query ). That doesn't seems needed.

Fix for the warning is adding is_object() around the while loop:

if( is_object( $this->result ) ) {
	while ( $row = $this->result->fetch_object() ) {
		$ret[] = $row;
	}
}

comment:90 knutsp10 months ago

The plugin now works also without PDO loaded. Nice!

I suggest that PDO is the first priority only for PHP >= 5.4. For PHP < 5.4 I suggest mysql has priority, then PDO as second. Don't change a winning team, and don't fix what isn't broken or not even undesired.

For PHP 5.2 the stability of PDO should be examined. May be prefer mysqli over PDO in case there is no mysql extension.

I any case, this task should enable the future WordPress to run on a PHP without the mysql extension, regardless of PHP version.

comment:91 TJNowell9 months ago

  • Cc contact@… added

comment:92 jdgrimes9 months ago

  • Cc jdg@… added

comment:93 hypertextranch9 months ago

I opened a new ticket for adding secure DB connection support to the db drivers in this ticket (#24816). Not sure if that's the right way to go but just wanted to get some feedback.

comment:94 SergeyBiryukov9 months ago

#24816 was marked as a duplicate.

comment:95 markmont9 months ago

  • Cc markmont added

comment:96 hypertextranch9 months ago

@markoheijnen, I added a patch for your plugin to get SSL support. I have chosen to add a options array to the interface for connect() but I'm not sure if this is the right way to go about it. We can also just have the classes themselves read directly from defined constants instead of getting them passed in. (The legacy mysql client does this for feature flags.)

hypertextranch9 months ago

Path for wp-db-driver to add SSL support to PDO and mysqli

comment:97 markoheijnen9 months ago

Do we need something like: mysqli_options ($db, MYSQLI_OPT_SSL_VERIFY_SERVER_CERT, true); ?

comment:98 markoheijnen9 months ago

Also I just released a new update of the plugin that allows SSL and fixed notices due the changes in 3.6. See http://wordpress.org/plugins/wp-db-driver/

comment:99 hypertextranch9 months ago

Looks like PHP const MYSQLI_OPT_SSL_VERIFY_SERVER_CERT for mysqli will set the MySQL option MYSQL_OPT_SSL_VERIFY_SERVER_CERT (http://docs.oracle.com/cd/E17952_01/refman-5.5-en/mysql-options.html) which turns on SSL cert verification. However it appears that the PHP constant is only defined if mysqli is compiled with mysql client 5.1.10+ or with mysqlnd (https://github.com/php/php-src/blob/master/ext/mysqli/mysqli.c#L699).

I guess the question is do we turn SSL cert verification on if available across the board or do we make it configurable? (If we make it configurable I'm guessing we should make it on by default?) In either case we should wrap that set options in a if defined block so mysqli compiled without verify support does not throw PHP notices.

EDIT: Not sure how this factors into what we do but it appears the MYSQLI_OPT_SSL_VERIFY_SERVER_CERT option is undocumented at the moment. http://www.php.net/manual/en/mysqli.options.php

Last edited 9 months ago by hypertextranch (previous) (diff)

comment:100 markmont9 months ago

There is a bug in the latest versions of the patch (including 21663.6 and 21663.7) that results in dbDelta() not creating new database tables when it should.

Details and a fix are available at
https://wordpress.org/support/topic/dbdelta-fails-to-create-new-tables?replies=1#post-4492564

comment:101 RicaNeaga8 months ago

PHP 5.5.3 is now available. Please consider pushing this to WP 3.7 / 3.8. Thanks. :)

comment:102 markoheijnen8 months ago

The plugin is quite stable but till now it didn't got a lot of feedback. For WP 3.7 it's to early but I guess 3.8 can make sense.

Last edited 8 months ago by markoheijnen (previous) (diff)

comment:103 markmont8 months ago

There is a bug that results in the wrong number of rows being returned for some queries. Details and a fix are available at
http://wordpress.org/support/topic/some-queries-return-wrong-number-of-rows

comment:104 iandunn8 months ago

  • Cc ian.dunn@… added

comment:105 markoheijnen8 months ago

#25211 was marked as a duplicate.

comment:106 brody1828 months ago

you need mysqli_connect() in wp-db.php

comment:107 tollmanz7 months ago

  • Cc tollmanz@… added

comment:108 jbutkus7 months ago

  • Cc butkus.justas@… added

comment:109 designsimply7 months ago

  • Cc sheri@… added

comment:110 pothi7 months ago

  • Cc pothi added

comment:111 ShinichiN7 months ago

  • Cc shinichi.nishikawa@… added

comment:112 RicaNeaga7 months ago

Noticed that #5932 it's going to be, quote, tackled in 3.8, probably with mysqli at the same time. I feel the need to comment on this:

1. I fully trust the decision of the wordpress team, to go either with PDO, either with mysqli.

2. I hope the (potential) discussion mysqli vs. PDO will be very brief, and a conclusion will be reached before wp 3.8 beta 1 lands, so full support for php 5.5 will be added in wp 3.8. Early adopters can react to PDO / mysqli (probable) bugs with php 5.5, before php 5.6 will land (that will not resume to only deprecated errors).

3. I'm sure you're aware both major competitors (joomla & drupal) already support other databases (like PostgreSQL) - drupal via PDO, or that Oracle has a history of making weird decisions regarding mysql, or that both major Linux distributions and Google NOW prefer MariaDB over mysql.

But I hope you'd also consider when choosing between mysqli and PDO which solution is the most future-proof. Maybe at some point Oracle will give everyone reasons to look for alternatives, and when that time comes, a good decision taken now would help. For example, the possibility to use mysqli as an extension for php also with MariaDB is important, but is MariaDB sufficient when it comes to alternatives for mysql?

comment:113 nacin7 months ago

WordPress supports MySQL-like databases. That includes MariaDB and Percona Server. In fact, WordPress.org uses Percona for quite a bit. Both work just fine with WordPress as-is, right now — even with ext-mysql or ext-mysqli — and shouldn't impact any decision points here.

Also, I think "full" support for PHP 5.5 is already built-in. The main reason why the PHP core developers were convinced they should finally deprecate ext-mysql is because a WordPress lead developer told them to just rip the band-aid off and do it already. WordPress would then adapt to the changing landscape because that's what we were good at. But ext-mysql still works in 5.5; you're just annoyed by some deprecated messages. Anyone operating PHP 5.5 — which is basically no one at under 0.1% of WordPress sites — knows what they're doing, and they can best serve the WordPress project by testing out markoheijnen's plugin.

comment:114 nacin7 months ago

  • Milestone changed from Future Release to 3.8
  • Summary changed from Use PDO for MySQL queries when PDO is available to Use PDO or mysqli for MySQL queries when available

My take on the current work here: While we should look into adding this in 3.8, I think it needs to be a narrow approach. I dislike the idea of any future improvements in wp-db.php needing to be written in two places, as would occur if it were a second file that extends wpdb and overrides methods. (Obviously, that makes the most sense for a plugin, but isn't necessarily the way core should do it.)

I think a "driver" situation is pretty robust, but that the current proposals are actually more robust than what we need to simply add mysqli support. Based on some conversations I've had with marko and others, targeting mysqli seems like a good first step, versus more general DB abstraction. Then we can see how well that works, and maybe get to the point where we have multiple drivers with an interface. I hope to engage with scribu and marko at WordCamp Europe this weekend; maybe we can get something committed to trunk once 3.7 is branched.

comment:115 tomdxw6 months ago

  • Cc tom@… added

comment:116 markoheijnen6 months ago

I'm unsure about first targeting mysqli. If something need to be changed on current approach then we should try to work on that. I rather do everything in one step.

comment:117 aaroncampbell6 months ago

It looks like Ubuntu 13.10 shipped with PHP 5.5.3, so people are going to start running into this quite a bit more now. Everything works, but all dev environments with WP_DEBUG on get "Deprecated: mysql_connect(): The mysql extension is deprecated and will be removed in the future: use mysqli or PDO instead in /path/to/wp/wp-includes/wp-db.php on line 1142"

Nacin: What was the outcome of your talks at WordCamp Europe?

Honestly, with the plugin existing and seemingly stable, I think it would be nice to go ahead and move to supporting mysqli as well as PDO.

It also looks like there's a problem with the .7 patch. The files wp-includes/wp-db-driver.interface.php, wp-includes/wp-db-driver-mysql.php, wp-includes/wp-db-driver-mysqli.php, and wp-includes/wp-db-driver-pdo_mysql.php all seem to have triple copies of the code in them.

comment:118 markoheijnen6 months ago

The code from the patches are old. I can create a new one from the plugin. That one is more up to date.

comment:119 in reply to: ↑ 51 atimmer6 months ago

Replying to bpetty:

Replying to scribu:

I don't think we should waste effort on making plugins/themes that still don't use $wpdb work in PHP 5.4.

By my count, I've found 1,722 plugins in the WP.org plugin repository that make raw calls to mysql_*() functions. Some might have nothing to to with the WP main connection or not actually be calls to core PHP methods, but most of them likely are.

That's 7.4% of all plugins.

Using the slurper I have run that stats just now, there are some plugins that failed downloading but here are the stats for me as of right now:
Total plugins: 33263
Plugins using mysql_query: 1019 (files found: 2061)
Percentage: 3,06%

Last edited 6 months ago by atimmer (previous) (diff)

comment:120 valeriosza6 months ago

#5932 was marked as a duplicate.

comment:121 danielbachhuber5 months ago

  • Cc daniel@… added

comment:122 follow-up: wonderboymusic5 months ago

Can someone throw up their patch or summarize the approach that was decided? It would suck if this missed the boat again. The Deprecated: mysql_connect() notice everywhere in PHP 5.5 is harshing my mellow

comment:123 in reply to: ↑ 122 ; follow-up: kurtpayne5 months ago

Replying to wonderboymusic:

Can someone throw up their patch

https://github.com/markoheijnen/wp-db-driver
http://wordpress.org/plugins/wp-db-driver/

or summarize the approach that was decided?

No final decision as far as I know. The current "driver" idea as implemented above prizes compatibility by supporting mysql, mysqli, and pdo possible (thus increasing maintenance costs). It increases cost with no real access to the "candy" parts of PDO or mysqli. It may be smarter to do a hard cut and say "okay, we're now PDO now" and make wp-db only support pdo.

It would suck if this missed the boat again. The Deprecated: mysql_connect() notice everywhere in PHP 5.5 is harshing my mellow

Agreed.

wonderboymusic5 months ago

comment:124 follow-up: wonderboymusic5 months ago

Added a patch based on the latest code on GitHub. I added a wpdb_drivers filter in wpdb::set_driver() to allow someone to shove another slug in there. I ran unit tests with each driver, everything works.

Feedback more than welcome, just wanted to get the ball rolling.

comment:125 in reply to: ↑ 124 ; follow-up: kurtpayne5 months ago

Replying to wonderboymusic:

I added a wpdb_drivers filter in wpdb::set_driver() to allow someone to shove another slug in there.

This is going to be hard to access. Plugins and mu-plugins aren't loaded until after a database connection is made, so what could set that filter? If this is in core, it would probably only be code in db.php. The current plugin on github relies on a constant named WPDB_DRIVER (presumably in wp-config.php) if the user wants to pick a particular driver.

Thoughts?

And thanks for adding the patch :-)

Last edited 5 months ago by kurtpayne (previous) (diff)

comment:126 in reply to: ↑ 123 Denis-de-Bernardy5 months ago

Replying to kurtpayne:

Replying to wonderboymusic:

or summarize the approach that was decided?

No final decision as far as I know. The current "driver" idea as implemented above prizes compatibility by supporting mysql, mysqli, and pdo possible (thus increasing maintenance costs). It increases cost with no real access to the "candy" parts of PDO or mysqli. It may be smarter to do a hard cut and say "okay, we're now PDO now" and make wp-db only support pdo.

+1 with the idea of sticking to PDO only. It would be really sweet if this change opened the venue to make WP compatible with e.g. Postgres down the road, instead of saddling us with new technical debt.

comment:127 in reply to: ↑ 125 ; follow-up: markoheijnen5 months ago

Replying to kurtpayne:

Replying to wonderboymusic:

I added a wpdb_drivers filter in wpdb::set_driver() to allow someone to shove another slug in there.

This is going to be hard to access. Plugins and mu-plugins aren't loaded until after a database connection is made, so what could set that filter? If this is in core, it would probably only be code in db.php. The current plugin on github relies on a constant named WPDB_DRIVER (presumably in wp-config.php) if the user wants to pick a particular driver.

Thoughts?

And thanks for adding the patch :-)

Wouldn't it still be possible if you use the dropin? Since there you still need to set the $wpdb variable and when you don't do that WordPress will load it's own class.

And yes thanks for adding the patch.

comment:128 in reply to: ↑ 127 kurtpayne5 months ago

Replying to markoheijnen:

Wouldn't it still be possible if you use the dropin?

Yes, sorry, that's what I meant by db.php.

comment:129 3flex5 months ago

  • Cc 3flex added

comment:130 omarabid5 months ago

  • Cc contact@… added

comment:131 follow-up: wonderboymusic5 months ago

21663-PDO-merge.diff​ is my first attempt to merge the PDO driver into wpdb, which was way more brutal a task than it looks in the diff. Took me like 7 tries. A Meta Query unit test fails, but everything else passes.

Adding this as a conversation piece.

comment:132 in reply to: ↑ 131 Denis-de-Bernardy5 months ago

Replying to wonderboymusic:

21663-PDO-merge.diff​ is my first attempt to merge the PDO driver into wpdb, which was way more brutal a task than it looks in the diff. Took me like 7 tries. A Meta Query unit test fails, but everything else passes.

Adding this as a conversation piece.

Emulated prepared statements are an essential flag in my own experience, and it usually is what is needed, wanted and expected. It produces better query plans, too.

http://stackoverflow.com/questions/10113562/pdo-mysql-use-pdoattr-emulate-prepares-or-not

comment:133 wycks5 months ago

OS stats for 5.5 +:

Fedora 19 - Current Stable - PHP 5.5.0
Arch Linux - Current Stable - PHP 5.5.6
Ubuntu 13.10 (Saucy) - Current Stable - PHP 5.5.3

Debian 8 (Jessie) Testing - PHP 5.5.0 - release unknown..2015 ?
Red Hat/CentOS ...many years away...(..shared hosting..)

comment:134 markoheijnen5 months ago

1&1 is planning to introduce PHP 5.5 next year and I would love that this is then addressed. I think its weird that there isn't any major discussion about this topic yet. We should already had a plan on how we are going to do this instead of focussing on adding new functionality to WordPress.

aaroncampbell5 months ago

comment:135 aaroncampbell5 months ago

21663.2.diff is a quick replacement of all mysql_* functions with their mysqli_* counterparts. It only does this if mysqli_connect() exists and PHP >= 5.5. There is a filter called use_mysqli to override. There are a couple issues still, and that is what to do with MYSQL_NEW_LINK and MYSQL_CLIENT_FLAGS. We default new link to true, which is what mysqli does by default, but I'm not sure that MySQLi has an override. I also didn't have time to dig into the various possible flags yet, but I wanted to get the patch up so people could play with it.

aaroncampbell5 months ago

comment:136 aaroncampbell5 months ago

There were a couple places I missed yesterday. One in wpdb::flush() for freeing a result, one in wpdb::print_error() to grab the most recent error, and a couple references in the PHPDoc for wpdb::_real_escape(). These have all been taken care of in 21663.3.diff

comment:137 dd325 months ago

Quick thoughts on 21663.3.diff, Perhaps it'd be best to stick to full if's rather than ternaries? It makes the diff nice and short, but would be much more readable with full if's instead.

comment:138 follow-up: wonderboymusic5 months ago

You can set the flags if you you use mysql_real_connect() - see here where I did this at the end of 3.7 for fun: http://core.trac.wordpress.org/attachment/ticket/25282/25282.diff

comment:139 in reply to: ↑ 138 ; follow-up: aaroncampbell5 months ago

Replying to dd32:

Quick thoughts on 21663.3.diff, Perhaps it'd be best to stick to full if's rather than ternaries? It makes the diff nice and short, but would be much more readable with full if's instead.

After creating the diff I thought about that. I stuck with the ternaries because they're beneficial for the while (such as while ( $row = ( $this->use_mysqli )? @mysqli_fetch_object( $this->result ) : @mysql_fetch_object( $this->result ) ) ) and using them everywhere kept things uniform. I can break most of them into full ifs though.

Replying to wonderboymusic:

You can set the flags if you you use mysql_real_connect() - see here where I did this at the end of 3.7 for fun: http://core.trac.wordpress.org/attachment/ticket/25282/25282.diff

Thanks. I'll look into mysqli_init() and mysql_real_connect()

comment:140 in reply to: ↑ 139 dd325 months ago

Replying to aaroncampbell:

Replying to dd32:

Quick thoughts on 21663.3.diff, Perhaps it'd be best to stick to full if's rather than ternaries? It makes the diff nice and short, but would be much more readable with full if's instead.

After creating the diff I thought about that. I stuck with the ternaries because they're beneficial for the while (such as while ( $row = ( $this->use_mysqli )? @mysqli_fetch_object( $this->result ) : @mysql_fetch_object( $this->result ) ) ) and using them everywhere kept things uniform. I can break most of them into full ifs though.

Ah didn't notice those. I think going for a full if () { while(){}; } else { while(){}; } would be more readable anyway, just my thoughts though.

aaroncampbell5 months ago

comment:141 aaroncampbell5 months ago

21663.4.diff handles the MYSQL_CLIENT_FLAGS. It still uses ternaries.

aaroncampbell5 months ago

comment:142 aaroncampbell5 months ago

21663.5.diff Moves away from ternaries.

comment:143 nacin5 months ago

21663.5.diff looks solid. Some thoughts:

  • The filter is too early for a plugin to modify it. We should remove it. If you want to use mysqli, then for 3.8, you have to use the latest version of PHP. Or, we *could* add a constant here, as I don't think it would be difficult to keep around in a backwards compatible manner.
  • The mysqli_set_charset conditional likely still needs the has_cap() check. Or was it removed deliberately?
  • I double-checked that the client flags are the same value, whether MYSQL_* or MYSQLI_*, so that's cool.
  • The wpdb::load_col_info() should do the for loop inside the use_mysqli conditionals. It merges to conditionals into one and makes it easier to read.
  • Let's set $this->last_error = mysqli_error( $this->dbh ) in its own conditional, rather than duplicating the logic inside both branches of the if statement.
  • It may be cleaner to reverse the nesting of WP_DEBUG and use_mysqli, that way the mysqli commands are together. mysqli_init() also doesn't need to be silenced, so this will reduce that to one line.

comment:144 markoheijnen5 months ago

We should not introduce any new constant or filter since this patch to me is just a hack that need to be revisit in 3.9

comment:145 HypertextRanch5 months ago

Is the idea that we use 21663.5.diff​ as a stopgap patch in 3.8 then introduce the more robust DB driver approach from @markoheijnen's plugin in 3.9?

aaroncampbell5 months ago

comment:146 aaroncampbell5 months ago

In 21663.6.diff:

  • I removed the filter and opted to NOT introduce a new constant. Constants have been quite a pain for us in the not-too-distant past, and since this didn't seem necessary I thought we'd see if we came up with a better solution later instead of rushing to add it now.
  • The mysqli_set_charset call happens inside if ( $this->has_cap( 'collation' ) && ! empty( $charset ) ) so the cap check is still in place.
  • I had checked the flags too. Seems OK. A bunch of the functions themselves took reversed arguments (handle first for mysqli), so I was worried...but it turned out OK.
  • I changed load_col_info to put the for loops in the if. Definitely looks cleaner
  • Cleaned up the last_error logic.
  • I swapped out the WP_DEBUG and use_mysqli conditionals in db_connect()

wonderboymusic5 months ago

wonderboymusic5 months ago

wonderboymusic5 months ago

comment:147 wonderboymusic5 months ago

  • Milestone changed from 3.8 to Future Release

we will do #26322 in 3.8

comment:148 markmont4 months ago

So what is the current thinking for future releases? Plugins, only mysqli, only PDO, or something else?

comment:149 wonderboymusic4 months ago

If we use the OO versions of mysql and mysqli, we won't have to branch the code as much - the constructor would branch, but most of the methods are named the same and take the same args. That's probably lowest hanging fruit.

comment:150 wonderboymusic4 months ago

\ scratch that - I was thinking of something else

Last edited 4 months ago by wonderboymusic (previous) (diff)

comment:151 ircbot4 months ago

This ticket was mentioned in IRC in #wordpress-dev by miss_jwo. View the logs.

comment:152 follow-up: yandod3 months ago

PHP5.6 may be removing ext/mysql. because php team already voted for it, that why it is triggering E_DEPRECATED.
http://news.php.net/php.internals/71222

Could you reactivate this conversation and catch up PHP5.6 out?
I don't know next version of WordPress come earlier than PHP5.6. But it might be time.

comment:153 in reply to: ↑ 152 dd323 months ago

Replying to yandod:

PHP5.6 may be removing ext/mysql. because php team already voted for it, that why it is triggering E_DEPRECATED.
http://news.php.net/php.internals/71222

Could you reactivate this conversation and catch up PHP5.6 out?
I don't know next version of WordPress come earlier than PHP5.6. But it might be time.

Reading the linked posting & RFC, you can see that they only voted on adding a deprecated notice in 5.5, no discussion on removing it in 5.6 was discussed/voted on which is what would normally happen if they were planning an immediate removal. Due to the sheer amount of users of the extension, I wouldn't expect PHP to remove the extension in 5.6 at all. It'd be put to a RFC vote first and we'd have plenty of notice.

That being said, the discussion on this, and the development that occurred on this during 3.7/3.8 is still ongoing and not forgotten. "Future Release" is simply our broad milestone, we'll pull it to the 3.9 milestone if enough development work happens that a change will make it into the release.

comment:154 yandod3 months ago

dd32:
That's right. It is not confirmed yet.
But at least is is mentioned, I shared this because it should worth link on this ticket.

Thanks.

comment:155 RicaNeaga3 months ago

I really hoped this would have been included and fixed in WP 3.8, either via mysqli or either PDO. Now RHEL 7 is just inches from its stable launch (followed soon by its CentOS more popular alternative), with MariaDB coming as the default database.

After Plesk, also Cpanel has full support for MariaDB planned, so everybody has (or will soon have) OFFICIAL alternatives to MySQL, that are easy to implement (MariaDB is considered by everybody a drop-in replacement for MySQL).

In this context, when almost everybody is running away from MySQL, is 2014 the year when wordpress will fully support MariaDB (with a mention of this in its requirements page, that's very important for non-tech-savvy-users / late adopters) via mysqli (or PDO)? I mean, I see it's not even considered worthy for 3.9... :(

Last edited 3 months ago by RicaNeaga (previous) (diff)

comment:156 markoheijnen3 months ago

Core does lack having attention for this issue. I totally forgot to mention this in the chat yesterday. That said you can already use MariaDB. I already do that for one site. You normally do get a warning that the version doesn't match warning . I'm not sure if that is fixable with mysql_* functions so I do use PDO for that by using db.php dropin.

If not then please mention what doesn't work.

comment:157 RicaNeaga3 months ago

I'm a little discouraged by the potential issues that may pop up after mysqli is implemented, if I would be a MariaDB early adopter. I am a little tech-savvy, but I don't want to spend time fixing problems of my own doing.

Sorry for the little offtopic, hope the development on this will restart soon, but please think about this, there are many non-tech-savvy users out there that want a replacement for MySQL NOW, but cannot act in this manner, since the requirements page only mention MySQL.

comment:158 markoheijnen3 months ago

From own experience there aren't many non-tech-savvy users out there that want to replace MySQL. Like they have no idea which version of MySQL they are running on. And that is totally fine.

I have no issues with using MariaDB and it was always compatible with MySQL. I guess the reason why it's not on the requirement page is that we don't test on it. But to me that is not a reason for not using it.

comment:159 follow-up: yandod3 months ago

Instead of just abandoning mysql, Can WordPress have both implementation of mysql and mysqli.

Then it will determine by like
define('WP_DB_USE_CLASSIC_MYSQL', true);

Least mysqli will be blessed as modern default, but classic option still will be available.

comment:160 in reply to: ↑ 159 ; follow-up: adegans3 months ago

Replying to yandod:

Instead of just abandoning mysql, Can WordPress have both implementation of mysql and mysqli.

Then it will determine by like
define('WP_DB_USE_CLASSIC_MYSQL', true);

Least mysqli will be blessed as modern default, but classic option still will be available.

Better yet - Just have a switch there
define('WP_DB_TYPE', 'mysqli'); mysql, mysqli, postgres (?) Whatever else kind of types you wanna support.

comment:161 in reply to: ↑ 160 yandod3 months ago

Replying to adegans:

Better yet - Just have a switch there
define('WP_DB_TYPE', 'mysqli'); mysql, mysqli, postgres (?) Whatever else kind of types you wanna support.

it makes sense.
For now, saving red data animal mysql is primary purpose of this switch.
if the switch was false, we can use PDO or better wp_db which supports multiple abstracted database layer.

main part of this idea is, WordPress can still bundle current wp_db with mysql module and use it with some switch.
And WordPress can also implement modern wp_db which takes better solution we want.

Replacing mysql with X type discussion seems never ending.
Having gurantee of diversity on wp_db could be minimum improvement for future.

comment:162 markoheijnen3 months ago

Could you both look into the patches and the plugins and give your opinion on that. Since all those questions/comments are addressed there.

Postgres is something for the future and not for this ticket what is already complex enough to be taken care of.

pento3 months ago

comment:163 pento3 months ago

  • Milestone changed from Future Release to 3.9

attachment:21663.10.diff

  • Updates attachment:21663.7.diff to apply cleanly against trunk
  • Remove the temporary E_DEPRECATED check added in 3.8
  • Fix a bug where set_charset() wouldn’t work for MySQL 5.0.0 -> 5.0.5

I’m a big fan of eventually moving to a driver-style WPDB, like markoheijnen’s plugin, but I don’t think we’re ready for that, yet. Changes to WPDB can have far reaching effects, so we need to move in baby steps, and I like aaroncampbell’s approach as a first step.

This patch will need to be updated for #26847 and (hopefully) #5932, but I’d like to see it get into 3.9 early, so we can give it as much soaking time as possible. Changing over to mysqli will probably break any plugins that call the mysql_*() functions directly, so we’ll need to give the plugin authors as much time as possible to update their plugins.

comment:164 follow-up: markoheijnen3 months ago

Baby steps to me would mean moving to a driver-style WPDB and have it always use mysql_*. We could then test everything out and people who want to use it can do it in a better way.

comment:165 in reply to: ↑ 164 ; follow-up: pento3 months ago

Replying to markoheijnen:

Baby steps to me would mean moving to a driver-style WPDB

Moving to a driver-style WPDB is a pretty big step. Not because of the changes to the code, but because of the changes to how WPDB can be extended.

Releasing a driver interface is telling the world (even if it isn’t explicitly announced) “here is our DB interface, you can extend it to add new drivers”. Locking down an interface we’re comfortable supporting for a long time is not something we can do quickly or easily.

comment:166 Denis-de-Bernardy3 months ago

Short comment on 21663.10.diff: mysqli_set_charset() always exists in PHP 5.2.4: http://hu1.php.net/mysqli_set_charset

comment:167 in reply to: ↑ 165 ; follow-up: markoheijnen3 months ago

Replying to pento:

Replying to markoheijnen:

Baby steps to me would mean moving to a driver-style WPDB

Moving to a driver-style WPDB is a pretty big step. Not because of the changes to the code, but because of the changes to how WPDB can be extended.

I do get that it's a bigh step. I already had that experience with WP_Image_Editor. I do believe that time is irrelevant. It needs a lot of attention to get how we want it and currently that is the major issue. We only want to do ugly hot fixes instead of making the code great.

comment:168 follow-up: knutsp3 months ago

MySQL 5.0.6 was released in May 2005. Should earlier versions make WordPress not use set_charset()? How many users are on MySQL <= 5.0.5?

comment:169 in reply to: ↑ 168 ; follow-up: Denis-de-Bernardy3 months ago

Replying to knutsp:

MySQL 5.0.6 was released in May 2005. Should earlier versions make WordPress not use set_charset()? How many users are on MySQL <= 5.0.5?

My bad. I missed the MySQL version requirement.

comment:170 in reply to: ↑ 169 knutsp3 months ago

Replying to Denis-de-Bernardy:

Replying to knutsp:

MySQL 5.0.6 was released in May 2005. Should earlier versions make WordPress not use set_charset()? How many users are on MySQL <= 5.0.5?

My bad. I missed the MySQL version requirement.

Not bad. WordPress still not requiring 5.0.6 is bad, if WordPress really needs this to be better and consistent regarding charset. I'm ok with just requiring 5.0.x in general, but wonder how bad it would be to raise the requirement just a bit.

I may not understand the implications, but will learn. Requirements have to change from time to time, or else we are in trouble.

comment:171 in reply to: ↑ 167 ; follow-up: pento3 months ago

Replying to markoheijnen:

It needs a lot of attention to get how we want it and currently that is the major issue. We only want to do ugly hot fixes instead of making the code great.

There are two major issues:

  • We need to move away from ext/mysql as soon as possible, due to it being deprecated.
  • We need to modernise WPDB.

This ticket is about moving away from ext/mysql as quickly and with as little pain as possible, and the easiest way to do that is with as few changes to the codebase as possible, which is the aim of the current patch.

I understand that you would prefer to have the correctly engineered solution, that would be my first preference as well. But the more pressing need is to have 3.9 be able to use mysqli, which we won’t be able to do if we implement it as driver solution.

Replying to knutsp:

Not bad. WordPress still not requiring 5.0.6 is bad, if WordPress really needs this to be better and consistent regarding charset. I’m ok with just requiring 5.0.x in general, but wonder how bad it would be to raise the requirement just a bit.

We do upgrade the minimum requirements as we need to, usually when the usage levels are negligible. I don’t have the usage breakdown handy, nacin will know the answer to that.

That said, however, for the sake of a few lines of code, I’m not inclined to upgrade the minimum MySQL requirements from 5.0.0 to 5.0.6. Perhaps when we decide to make use of views (added in MySQL 5.0.3), we’ll upgrade it.

comment:172 in reply to: ↑ 171 markoheijnen2 months ago

Replying to pento:

There are two major issues:

  • We need to move away from ext/mysql as soon as possible, due to it being deprecated.
  • We need to modernise WPDB.

This ticket is about moving away from ext/mysql as quickly and with as little pain as possible, and the easiest way to do that is with as few changes to the codebase as possible, which is the aim of the current patch.

I understand that you would prefer to have the correctly engineered solution, that would be my first preference as well. But the more pressing need is to have 3.9 be able to use mysqli, which we won’t be able to do if we implement it as driver solution.

I disagree that this ticket is about moving away from ext/mysql. I rather see it as hijacked to be like that. That also makes this ticket now hard to follow since there are patches with the ticket number doing different things. One focusses on one file with mysqli support and the other going for the driver approach.

The problem I'm having now is that we don't focus on the best solution. That happend in 3.8 and now also again for 3.9. With the last sentence you mean if we go for the driver approach we will not hit 3.9?

I believe the driver approach is really stable but on the API side it can be that it isn't something we want. The real issue is never tackled here and that is what to do with plugins that still do raw calls to mysql_query() for example.

comment:173 nacin2 months ago

  • Keywords commit added; dev-feedback needs-testing removed

This ticket was opened to use a non-deprecated API when available. I'm going to modify that a bit, but the general goal is the same. Here are the goals for 3.9:

  • Use mysqli when PHP 5.5+, assuming it is available.
  • Offer the ability to not use ext/mysql even when PHP 5.4 or below, but this will not be the default.

pento is right: We should move away from ext/mysql as soon as possible. I will say I don't think it's simply because PHP has decided it's now deprecated. Honestly, this matters fairly little. Rather, eventually we're going to need mysqli to be our default, and in order to get there in a sane fashion, we need to get this in core at the PHP 5.5+ level first.

Why? (And why 5.5?) Well, one, WordPress already works pretty well under ext/mysql, and we've gotten used to its quirks, and I wouldn't want to suddenly break a site that's been working fine under PHP 5.2-5.4. Two, the bigger issue is that a lot of plugins are going to be affected, and that's what's probably gonna break a site. Easing this in (5.5 only) is the only way to do this without causing a complete disaster.

A driver is out of scope for this ticket. The only goal of this ticket is to get our foot in the door and to signal to plugin developers they need to stop using direct mysql_* functions. (We can email the author of every plugin that uses mysql_* functions, FWIW, and we will do so.)

So, again, the plan for 3.9:

  • Use mysqli when PHP 5.5+, assuming it is available.
  • 21663.10.diff is ready for commit.

comment:174 follow-ups: adegans2 months ago

What about plugins that not use $wpdb... They dont strife to be compatible if they don't. WordPress or it's team doesn't have to concern themselves with those.
So either ban them or break them... They should get in line and use the API in a meaningfull way or they become irrelevant.

That's a short and narrow view, even for me. But it will force devs to improve and get with the times.
Just imagine how many plugins are insecure because a dev is too lazy to use the API. And how many inefficient code exists because the dev is not aware of $wpdb.
I believe (strongly) that breaking compatibility with plugins not using $wpdb will benefit every user in the long run. All be it a bit messy during the transition.

Wordpress offers a rubost and (I think) beautiful API for databases. Everyone who doesn't use it is not seeking compatibility and ease of use.

comment:175 in reply to: ↑ 174 nacin2 months ago

Replying to adegans:

What about plugins that not use $wpdb... They dont strife to be compatible if they don't. WordPress or it's team doesn't have to concern themselves with those.
So either ban them or break them... They should get in line and use the API in a meaningfull way or they become irrelevant.

I'm very concerned with those. We don't break backwards compatibility just to spite plugin authors. Some of them may have been using mysql_* accidentally, or recklessly, or even intentionally (say, for something not [easily] done with wpdb). And it's been working for them for 11 years without any problem. We're not just going to indiscriminately break plugins (and, thus, sites) when we have the option to not do so. It's not our style and it doesn't sit well with our philosophies.

comment:176 adegans2 months ago

True, but at the same time you shouldn't let them hinder the progress of Wordpress as a whole. If WP is looking to drop support for mysql_* they are directly slowing down development of WP in a big way.
If that's allowed, this whole ticket and discussion becomes pointless.

So while I agree that my earlier comment is short and perhaps too strict. A clear stance has to be taken towards (against?) such plugins.

comment:177 in reply to: ↑ 174 ; follow-up: Otto422 months ago

Replying to adegans:

But it will force devs to improve and get with the times.

You get more flies with honey than with vinegar.

A better long term way would be:

  1. Migrate: Start including experimental support for the new methods on a limited basis, with planned expansion of the methods later. This also lets you modify the methods later if it turns out you have to do so, without breaking huge amounts of new development based on it.
  1. Teach: Find the most common dependencies that will break because of it on those cases. Make documentation somewhere detailing these cases, and providing examples of how to change the dependent code effectively.
  1. Notify: Make a list of code that will be affected, mass email the authors, point them to said documentation and other resources.
  1. Deprecate: After sufficient time or after enough measurable migration has occurred, deprecate support for the older methods, stop using old methods by default, but perhaps with sufficient backwards compatibility to make some cases not break totally, or to otherwise minimize impact to users.

Any big change has to be this way. You can't just break the world because it's the right thing to do, because it's never really the right thing to do.

comment:178 in reply to: ↑ 177 nacin2 months ago

Replying to Otto42:

You can't just break the world because it's the right thing to do, because it's never really the right thing to do.

This.

comment:179 brody1822 months ago

I agree with Otto, nacin seems to be stuck on php 5.2

how do you know that plugin authors are using (mysql) and not mysqli? have you checked everyone? or maybe they are just waiting for you to update

Last edited 2 months ago by brody182 (previous) (diff)

comment:180 Otto422 months ago

A lot of people still use php 5.2, including me. It's standard in a lot of hosting situations.

comment:181 brody1822 months ago

you are wrong otto, every hosting company has switched to php 5.3, 5.4 or 5.5

if some hosting company out there hasn't switched yet its because WordPress is holding them back

comment:182 nacin2 months ago

Enough. This ticket is not for these conversations. Go use a mailing list or Twitter.

Do not reply and say "OK", it triggers unnecessary notifications to dozens of people. Just do not reply.

The only thing on-topic for this ticket is implementing 21663.10.diff and it's derivatives in 3.9. That's it.

comment:183 ircbot2 months ago

This ticket was mentioned in IRC in #wordpress-dev by DH-Shredder. View the logs.

pento8 weeks ago

comment:184 pento8 weeks ago

attachment:21663.11.diff updates the patch to allow for [27072] and [27075]. It also updates the unit test added it [27075], to check the use_mysqli flag.

comment:185 follow-up: markoheijnen8 weeks ago

Pento: Your last changes will not work since use_mysqli is a private property. Also the patch is not backwards compatible when people use socket connections and it's not the default one in php.ini. See https://github.com/markoheijnen/wp-db-driver/commit/ac7700d4eb20d05dd91edf0ead63727153bb7e12.

I still am curious why you used mysqli_real_connect.

comment:186 wonderboymusic8 weeks ago

mysqli_real_connect() accepts flags that mysqli_connect() does not

comment:187 markoheijnen8 weeks ago

Ah yeah correct. We use the flags argument. We kind a miss use MYSQL_CLIENT_FLAGS but the internals of PHP make things like MYSQLI_CLIENT_COMPRESS the same as MYSQL_CLIENT_COMPRESS

comment:188 in reply to: ↑ 185 pento8 weeks ago

Replying to markoheijnen:

Your last changes will not work since use_mysqli is a private property.

Are you referring to the unit test? It does work, because of wpdb::__get().

Also the patch is not backwards compatible when people use socket connections and it's not the default one in php.ini. See https://github.com/markoheijnen/wp-db-driver/commit/ac7700d4eb20d05dd91edf0ead63727153bb7e12.

Good catch, I'll have a play around with this.

comment:189 markoheijnen8 weeks ago

ah crap, forgot about that one. Moments I hate get()

pento8 weeks ago

comment:190 pento8 weeks ago

attachment:21663.12.diff adds support for host:port, host:/path/to/mysql.sock, and host:port:/path/to/mysql.sock style host definitions, same as mysql_connect() supports.

Implementation was taken from PHP's mysql_connect source.

comment:191 nacin8 weeks ago

In 27250:

Use ext/mysqli in PHP 5.5 or greater. Expect minor explosions.

props aaroncampbell, pento.
see #21663.

comment:192 Denis-de-Bernardy8 weeks ago

Could we get a means to force wpdb to use mysqli please?

comment:193 follow-up: pento7 weeks ago

Yes! Maybe.

We want to make the method backwards compatible for when we move to a driver system, I like the WPDB_DRIVER define from markoheijnen's WP DB Driver plugin.

aaroncampbell, you mentioned previously that we've had problems with adding defines in the past - are there particular problems we've had that you think would apply to this situation? Given how early $wpdb is created, it seems a define would be the only practical way for a site to be able to change the driver.

comment:194 nacin7 weeks ago

In 27257:

Use mysqli for WordPress development versions, regardless of PHP version, to increase testing footprint.

Allow the lack of ext/mysql to pass wp_check_php_mysql_versions().

see #21663.

nacin7 weeks ago

comment:195 follow-up: nacin7 weeks ago

[27257] additionally forces mysqli for development environments. This should increase testing during 3.9 beta.

21663.13.diff adds a constant called USE_EXT_MYSQL. Rather than the more forward-thinking driver approach, this simply restricts it to applying to the enabling/disabling of ext/mysql.

I actually first had it only allow for the disabling of ext/mysql via USE_EXT_MYSQL = false. USE_EXT_MYSQL = true wouldn't force ext/mysql on PHP 5.5. The patch does this, however, since it may be useful to force ext/mysql for development/testing purposes, and because it's inevitable this will break a PHP 5.5 site, so let's ensure we have easy recourse.

comment:196 aaroncampbell7 weeks ago

It seems like USE_EXT_MYSQL is backwards in the .13 patch. Setting it to true causes mysqli to be used, right?

$this->use_mysqli = (bool) USE_EXT_MYSQL;

Seems like we should rename it to USE_EXT_MYSQLI or use !USE_EXT_MYSQL to set use_mysqli

comment:197 in reply to: ↑ 195 Denis-de-Bernardy7 weeks ago

Replying to nacin:

21663.13.diff adds a constant called USE_EXT_MYSQL. Rather than the more forward-thinking driver approach, this simply restricts it to applying to the enabling/disabling of ext/mysql.

$this->use_mysqli = (bool) USE_EXT_MYSQL; 

I think you meant:

$this->use_mysqli = !USE_EXT_MYSQL; 

That said, methinks the define should actually be USE_EXT_MYSQLI to *enable* MySQLi, rather than USE_EXT_MYSQL to *disable* MySQL.

Imho, the odds are ridiculously low that someone would actually go in his wp-config file and define a constant to use the clunky old library, whereas I can readily imagine a number of hackers going in there and happily define a constant that allows to use the more modern library.

comment:198 in reply to: ↑ 193 ; follow-up: aaroncampbell7 weeks ago

Replying to pento:

aaroncampbell, you mentioned previously that we've had problems with adding defines in the past - are there particular problems we've had that you think would apply to this situation? Given how early $wpdb is created, it seems a define would be the only practical way for a site to be able to change the driver.

Sorry, I missed this with my last comment. The problem has been mostly in unit testing. If you define something like USE_EXT_MYSQL to make a unit test to see that it works, you can't then change it during the testing so that another unit test can test the other option. If filters can't be run at the point where the DB is loaded though, we don't have a lot of options.

nacin7 weeks ago

comment:199 in reply to: ↑ 198 ; follow-up: Denis-de-Bernardy7 weeks ago

Replying to aaroncampbell:

Sorry, I missed this with my last comment. The problem has been mostly in unit testing. If you define something like USE_EXT_MYSQL to make a unit test to see that it works, you can't then change it during the testing so that another unit test can test the other option. If filters can't be run at the point where the DB is loaded though, we don't have a lot of options.

For unit testing purposes, couldn't we simply add a filter that we explicitly document as e.g. "internal, for unit testing purposes only — this hook cannot be used by plugins, because these are not loaded at this stage".

Alternatively, and perhaps more ideally, we could tweak the load procedure so to ensure that add_filter() exists when wp-config.php is loaded. It's a matter of including a couple of files in wp-load.php, and changing the calls to require_once (instead of require) in wp-settings.php. That would ensure backwards compatibility with plugins that mindlessly include wp-config.php, and forward compatibility of the needed filter.

comment:200 follow-up: nacin7 weeks ago

Correct, there are a few issues with my patch. Fixed in 21663.14.diff.

USE_EXT_MYSQLI means if we ever add PDO, we would need to continue to force MySQLi in order to obey their settings. It would be a permanent fixture. An ext/mysql constant, however, would eventually just fade away.

I can readily imagine a number of hackers going in there and happily define a constant that allows to use the more modern library.

Honestly, the only reason why I'm even considering the constant is for development/testing purposes and easy recourse for broken sites. For this situation, said hackers should update to PHP 5.5. If they want to use a more modern library, then the onus should be on them to use a more modern version of PHP.

For unit testing, we can extend wpdb and set use_mysqli to true. That property should probably be protected, not private.

comment:201 in reply to: ↑ description AzizSaleh7 weeks ago

Replying to scottconnerly:

the mysql_* functions are officially deprecated for PHP 5.4 and will begin throwing E_DEPRECATED errors in the next version of PHP.
http://marc.info/?l=php-internals&m=131031747409271&w=2

Wordpress should use PDO by default, but fall back to mysql_* when PDO is not present.

See also: #11622 for last year's discussion.

I have created a PDO backend for mysql_* functions for exiting old projects which I didn't want to have to go through the code, update, and re-test:

https://github.com/AzizSaleh/mysql

Hopefully it helps someone else.

comment:202 in reply to: ↑ 199 Denis-de-Bernardy7 weeks ago

Replying to Denis-de-Bernardy:

Replying to aaroncampbell:

Sorry, I missed this with my last comment. The problem has been mostly in unit testing. If you define something like USE_EXT_MYSQL to make a unit test to see that it works, you can't then change it during the testing so that another unit test can test the other option. If filters can't be run at the point where the DB is loaded though, we don't have a lot of options.

(…)

Alternatively, and perhaps more ideally, we could tweak the load procedure so to ensure that add_filter() exists when wp-config.php is loaded. It's a matter of including a couple of files in wp-load.php, and changing the calls to require_once (instead of require) in wp-settings.php. That would ensure backwards compatibility with plugins that mindlessly include wp-config.php, and forward compatibility of the needed filter.

Ticket with patch: #27208

comment:203 in reply to: ↑ 200 bpetty7 weeks ago

Replying to nacin:

USE_EXT_MYSQLI means if we ever add PDO, we would need to continue to force MySQLi in order to obey their settings. It would be a permanent fixture. An ext/mysql constant, however, would eventually just fade away.

This is a good idea.

Would the code for switching to mysqli in development versions also disappear during RC or code freeze? Just curious since it would mean the unit tests would also always use mysqli across the board (including with PHP 5.2), which will be great for testing, but leaves us without results on ext/mysql all the way up to the very final tagged release even though it's still going to be used with 99% of installations right now. I'm not really too concerned about it, but it's something to think about.

For this situation, said hackers should update to PHP 5.5. If they want to use a more modern library, then the onus should be on them to use a more modern version of PHP.

Completely agreed here. If there's actually anyone in a situation where they could legitimately use mysqli enhancements or optimizations, the choice of upgrading to PHP 5.5 should make much more sense anyway, and they most likely already have the power to do this on their server(s). I'm honestly having a hard time even coming up with some use cases that might cover more than even 0.1% of users, and for that many, installing a DB drop-in is still a perfectly acceptable solution.

comment:204 bpetty7 weeks ago

@nacin: Just FYI, 21663.14.diff contains [27258], I assume on accident.

pento7 weeks ago

comment:205 pento7 weeks ago

attachment:21663.15.diff adds the USE_EXT_MYSQL define, and fixes some warnings caused by wpdb::$dbh not being empty when the connection fails using mysqli - mysqli doesn't set the handle to null like ext/mysql does.

comment:206 nacin7 weeks ago

In 27277:

wpdb: set dbh to null when the mysqli connection fails. see #21663.

comment:207 nacin7 weeks ago

In 27278:

Add a constant to disable mysqli for testing purposes. see #21663.

comment:208 in reply to: ↑ 1 AzizSaleh7 weeks ago

Replying to kurtpayne:

Would it make sense to create translation functions for mysql_* if they're not available?

I can see this being a saving grace for plugins / themes that do not use $wpdb. Example plugin

Incomplete non-functioning concept code:

<?php

if (!function_exists('mysql_query')) {
        $pdo_conn = null;
        function mysql_connect($host = 'localhost', $user = '', $pass = '' ) {
                global $pdo_conn = new PDO('mysql:;host=' . $host, $user, $pass);
        }
        function mysql_query($sql) {
                global $pdo_conn;
                return $pdo_conn->query($sql);
        }
        function mysql_fetch_assoc($rs) {
                return $rs->fetch(PDO::FETCH_ASSOC);
        }
        // ... etc.
}

That is exactly what I did for my library to enable sites to work without mysql_* functions using PDO as a backend:

https://github.com/AzizSaleh/mysql

Copy those files to a folder and then in your wordpress wp-includes/load.php file replace the following two lines:

<?php
wp_load_translations_early();
die( __( 'Your PHP installation appears to be missing the MySQL extension which is required by WordPress.' ) );

With the following:

<?php
$pathToMySQL = 'C:\\xampp\\htdocs\\mysql\\';
require_once($pathToMySQL . 'MySQL_Definitions.php');
require_once($pathToMySQL . 'MySQL.php');
require_once($pathToMySQL . 'MySQL_Functions.php');
Last edited 7 weeks ago by AzizSaleh (previous) (diff)

comment:209 ircbot7 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by DH-Shredder. View the logs.

comment:210 follow-up: nacin6 weeks ago

  • Summary changed from Use PDO or mysqli for MySQL queries when available to Use mysqli for MySQL queries when available

In terms of offering a mysql_* fallback for plugins, I don't see the benefit. This will only help when ext/mysql is missing, and it never is. ext/mysql will start to be disabled more often over time, but by the time that's any measurable footprint, we should be able to have moved the needle on plugins using MySQL directly.

There is little left here (please weigh in on anything I'm missing):

  • Email the authors of all plugins that use mysql_* functions directly.
  • Get Travis CI to run the test suite under MySQLi. It'd double the time it takes to do the tests, but it's probably worth it at least for now.

comment:211 in reply to: ↑ 210 rmccue6 weeks ago

Replying to nacin:

Get Travis CI to run the test suite under MySQLi. It'd double the time it takes to do the tests, but it's probably worth it at least for now.

Actually, if it's matrixed properly, it should be able to run in parallel and (if mysqli turns out being faster) will probably finish first.

comment:212 follow-up: benjamin45 weeks ago

I'm using Zend Server (in my Dev environment) and this ticket broke the database connection ("Warning: mysqli_real_connect(): [2002] No such file or directory (trying to connect via unix:///tmp/mysql.sock)"). I had to change my wp-config to:

define('DB_HOST', 'localhost:/var/run/mysqld/mysqld.sock');

See this forum discussion for more infos:

http://forums.zend.com/viewtopic.php?f=44&t=568

Would it be possible to include the socket syntax in the error message?

arnee5 weeks ago

comment:213 arnee5 weeks ago

Please allow plugins to check if wpdb uses mysqli or mysql, since some functions are not covered by wpdb yet (like unbuffered queries using MYSQLI_USE_RESULT).

comment:214 jb_wisemo4 weeks ago

Given the decision not to do the full abstraction this time around, please carefully test and consider how this will affect the existing database abstraction plugins that many sites have installed in order to support databases other than MySQL.

One such plugin is "Wordpress Database Abstraction" ( http://wordpress.org/extend/plugins/wordpress-database-abstraction/ ), which (though not updated for a few years) is still working nicely with WordPress 3.8.1 . With the original developers of that plugin apparently gone, a way forward is needed to continue using alternative databases (such as MSSQL) with WordPress 3.9 .

One option would be for markoheijnen's plugin to be updated and to merge in the extra features from "Wordpress Database Abstraction", especially the query adjustment logic they put in their MSSQL driver (they apparently cared little for getting their other drivers right anyway).

comment:215 in reply to: ↑ 212 ; follow-up: pento3 weeks ago

Replying to benjamin4:

I'm using Zend Server (in my Dev environment) and this ticket broke the database connection ("Warning: mysqli_real_connect(): [2002] No such file or directory (trying to connect via unix:///tmp/mysql.sock)").

What version of Zend Server are you using to get this error message?

Replying to arnee:

Please allow plugins to check if wpdb uses mysqli or mysql

You should be able to check this with $wpdb->use_mysqli. Is this not the case for you?

comment:216 markoheijnen3 weeks ago

What benjamin4 is seeing can be totally fine. You can specify a different default host in php.ini for MySQL and MySQLi. And if you only changed one then you can experience this weirdness.

I would say that it is a configuration mistake by the user since when using a socket connection you should never trust the default setting and always specify the location yourself.

comment:217 benjamin43 weeks ago

I'm using Zend Server 6.1.0 (Free Version).

I agree it is an configuration issue, however IMO it took me too much time to figure this out.
If I hadn't enabled WP_DEBUG, I wouldn't have seen the notice of mysqli_real_connect(), but only the generic "Error establishing a database connection"-Site, where sockets aren't even mentioned.

So instead of preventing the error (which we can't), we should improve its documentation. My suggestions:

  • wp-config.php could include Syntax examples for valid uses of DB_HOST
  • The error site on /wp-admin/ could link to a specific troubleshooting site on Codex instead of linking to the forum in general.
  • It could also mention the recent move to Mysqli as possible cause.

comment:218 in reply to: ↑ 215 ; follow-up: bpetty3 weeks ago

Replying to pento:

You should be able to check this with $wpdb->use_mysqli. Is this not the case for you?

This is a private property, so this won't work.

comment:219 in reply to: ↑ 218 nacin3 weeks ago

Replying to bpetty:

Replying to pento:

You should be able to check this with $wpdb->use_mysqli. Is this not the case for you?

This is a private property, so this won't work.

It actually will, because of the existing magic getter.

comment:220 ircbot3 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:221 samuelsidler3 weeks ago

  • Keywords needs-testing added; has-patch commit removed

pento2 weeks ago

comment:222 pento2 weeks ago

To try and workaround situations where mysqli fails to connect, but mysql probably works, attachment:21663.17.diff implements a fallback to mysql, if the first attempt to connect through mysqli fails.

This patch doesn't try to figure out why the mysqli connection failed - if it's a connection configuration issue as described in comment:212, then it should just fallback nicely to mysql. If it's a more serious issue (ie, the server has gone away), mysql will return the same error as mysqli.

pento2 weeks ago

comment:223 pento2 weeks ago

attachment:21663.18.diff adds a couple of extra sanity checks, that ext/mysql is loaded, and that USE_EXT_MYSQL hasn't been explicitly set to false.

comment:224 nacin2 weeks ago

In 27935:

Database: Fall back from ext/mysqli to ext/mysql if the connection fails.

This allows us to avoid breaking a site that works under ext/mysql but is misconfigured for ext/mysqli.

props pento.
see #21663.

comment:225 nacin2 weeks ago

What's left here? I got:

  • Don't force ext/mysqli in RC versions, as we had at one point discussed.
  • Documentation on this new complexity in our database layer.
  • Email the authors of all plugins that use mysql_* functions directly.
  • Get Travis CI to run the test suite under both MySQL and MySQLi. (As noted by pento, if we didn't force ext/mysqli in development versions, this would already just happen automatically for PHP 5.2/5.3/5.4 versus PHP 5.5. I'm fine with Travis running like that if we can make it work.)

comment:227 nacin11 days ago

In 28022:

Rename USE_EXT_MYSQL to WP_USE_EXT_MYSQL. see #21663.

comment:228 nacin11 days ago

Calling this fixed. We won't be un-forcing ext/mysqli until post RC1, that way plugin developers get the brunt of mysqli for testing.

comment:229 nacin11 days ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.