WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#31644 closed defect (bug) (fixed)

mysqli_get_client_version in wp-db

Reported by: filipac Owned by: pento
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.2
Component: Database Keywords: has-patch
Focuses: Cc:

Description

If you are checking the error logs (especially when running hhvm) you get this error:
Warning: mysqli_get_client_version() expects exactly 0 parameters, 1 given in

I would suggest to use the function without an argument as suggested on the internet:

if ( $this->use_mysqli ) {
					return mysqli_get_client_version() >= 50503;
				} else {
					return mysql_get_client_version() >= 50503;
				}

Attachments (2)

31644.patch (530 bytes) - added by SergeyBiryukov 6 years ago.
31644.diff (640 bytes) - added by MattyRob 6 years ago.

Download all attachments as: .zip

Change History (22)

#1 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 4.2

Introduced in [31391].

#2 follow-up: @pento
6 years ago

This seems to be a bug in HHVM, their mysqli_get_client_version() definition doesn't allow the link parameter to be passed.

Could you link to where on the internet that solution was suggested (as opposed to fixing HHVM's buggy implementation)? I'm not wild about it, as we explicitly pass the DB handle in every other function call.

#3 in reply to: ↑ 2 @SergeyBiryukov
6 years ago

Replying to pento:

Could you link to where on the internet that solution was suggested (as opposed to fixing HHVM's buggy implementation)?

The php.net example suggests that connection is not required to determine the version of MySQL client library:

<?php

/* We don't need a connection to determine
   the version of mysql client library */

printf("Client library version: %d\n", mysqli_get_client_version());
?>

It also looks like mysql_get_client_version() (non-MySQLi) does not exist. $wpdb->has_cap( 'utf8mb4' ) produces a fatal error with WP_USE_EXT_MYSQL set to true:

Fatal error: Call to undefined function mysql_get_client_version() in wp-includes/wp-db.php on line 2810

mysql_get_client_info() does exist though. See 31644.patch.

#4 @SergeyBiryukov
6 years ago

Comparing mysql_get_client_info() to integer version (50503) is obviously incorrect though, ignore the patch :)

#5 @pento
6 years ago

Actually, I thought about this further, and we don't actually need to be passing the DB handle - the handle is passed so that it goes to the correct DB server, whereas there's only one client lib that it can go through, the one loaded by PHP.

I'll fix this and the ext/mysql bug tomorrow.

#6 @pento
6 years ago

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

In 31783:

WPDB: HHVM doesn't support passing a DB link to mysqli_get_client_version(). While we usually pass a DB link to every ext/mysql and mysqli function call, we don't really need to do that here, as there's no way for the client library to change mid page load.

Another fun fact is that mysql_get_client_version() doesn't exist, but mysql_get_client_info() (along with `mysqli_get_client_info()') do. So, we're switching to them, in order to add a pleasing symmetry to the client version check.

Fixes #31644

#7 follow-up: @MattyRob
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Please can I get a quick sanity check on this issue.

mysql_get_client_version() and mysqli_get_client_info() on my testing sites return an entirely different number to the MySQL database version. I'm using PHP 5.6.7 and MySQL 5.6.23 but these functions both return 5.0.11 which is the version number of the client library (mysqlnd) on my system.

According to this page:
http://php.net/manual/en/mysqlnd.plugin.obtaining.php

PHP versions and the library version numbers are not the same and so the comparison a little later in the code comparing the client version to 5.5.3 is always returning a false.

Should the code not use $client_version = mysqli_get_server_info( $this->dbh ); for example?

#8 in reply to: ↑ 7 @dd32
6 years ago

Replying to MattyRob:

Please can I get a quick sanity check on this issue.
...
Should the code not use $client_version = mysqli_get_server_info( $this->dbh ); for example?

Unfortunately not, as for the connection to support the feature, it needs both Client and Server to support the feature being tested.

However, what you have run into is a legitimate bug I think, 5.0.11 is the latest version of mysqlnd in existance, and is the same that's currently in PHP trunk. I'll leave the digging to @pento as to if it supports the features we need..

#9 @dd32
6 years ago

If we have to do some branching to support two version checks, the MySQLnd Stats functions can be used to detect which library is in use (libmysql doesn't provide the stats functions, so function_exists should work) https://php.net/manual/en/mysqlnd.stats.php

#10 @pento
6 years ago

mysqlnd supported utf8mb4 from version 5.0.9, or PHP 5.4.0:

https://github.com/php/php-src/commit/41ddca803d7adeec6023665441e9573efae4b8b6

Checking for mysqli_get_client_stats() will work for mysqli, but there was no equivalent function created for ext/mysql.

#11 follow-up: @dd32
6 years ago

Checking for mysqli_get_client_stats() will work for mysqli, but there was no equivalent function created for ext/mysql.

Ugh, I missed that ext/mysql also used mysqlnd. I guess that means we'd have to check the ini config settings?

#12 @MattyRob
6 years ago

I guess this is an issue because we are tring to be function consistent between mysqli and mysql but unfortunately the functions don't work the same or maybe don't exist. How about accepting that limitation and documenting it:

if ( $this->use_mysqli ) {
	// using mysqli allows direct comparison of the client mysqldn verison
	return mysqli_get_client_version( $this->dbh ) >= 50503;
} else {
	// using mysql fall back functions
	return version_compare( mysql_get_client_info(), '5.5.3', '>=' );
}

#13 in reply to: ↑ 11 ; follow-up: @pento
6 years ago

Replying to dd32:

Ugh, I missed that ext/mysql also used mysqlnd. I guess that means we'd have to check the ini config settings?

That assumes that mysqlnd is the driver for all extensions - you can choose which extensions use mysqlnd. :-)

@MattyRob: Both functions return the same number, just in a different format. The problem is that mysqlnd's version numbers don't match MySQL's, so we need to detect when mysqlnd is being used.

#14 in reply to: ↑ 13 @MattyRob
6 years ago

Replying to pento:

@MattyRob: Both functions return the same number, just in a different format. The problem is that mysqlnd's version numbers don't match MySQL's, so we need to detect when mysqlnd is being used.

In my testing I'm not seeing the same number returned. On on site where mysql is used I get 5.5.42 returned which is the same as the server MySQL version. While on a local host testing environment at home I get a long string of the mysqlnd version including 5.0.11 which is different from the MySQL version. (PHP 5.6.7 and MySQL 5.6.23)

Can we check for the presence of the 'mysqlnd' string in the version string, does that cover all possibilities?

if ( false !== stristr( $client_version, 'mysqlnd' ) ) {
	$client_version = preg_replace( '/[^0-9.].*/', '', $client_version );
	return version_compare( $client_version, '5.0.9', '>=' );
} else {
	return version_compare( $client_version, '5.5.3', '>=' );
}

#15 @MattyRob
6 years ago

Okay, in the example text above it turns out that the reggae was totally wrong, the attached is better but uses pre_match() rather than pre_replace(). That said it works.

I've created a new database in WordPress 4.1.1, then upgrade to the latest trunk with the attached patch and the tables and rows were updated to utf8mb4_unicode_ci as expected.

@MattyRob
6 years ago

#16 @pento
5 years ago

  • Keywords has-patch added

It looks like mysqlnd (at least since it was first imported into PHP trunk) has always included the string "mysqlnd" in the version, so this solution works for me.

#17 @pento
5 years ago

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

In 31939:

WPDB: When we're checking to see if the MySQL client library supports utf8mb4, we need a separate check for mysqlnd versions, which using different version numbering to libmysqlclient.

Props MattyRob.

Fixes #31644.

#18 @ramiy
5 years ago

I think the "Requirements" page should be updated too.

From MySQL 5.5 or greater
To MySQL 5.5.3 or greater

#19 @dd32
5 years ago

I think we can skip changing the Recommended MySQL version.

For those using MySQL 5.5, 99.997% are on 5.5.5+ with ~96%+ on 5.5.30+ (There's currently 42 5.5 releases)

The number of hosts running an initial release of 5.5 is so insignificant that adding the extra version level adds almost no extra context to a host, or end-user.

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.