WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 7 months ago

Last modified 3 months ago

#19324 closed feature request (fixed)

Allow more flexible mysql_connect configuration (compression, ssl, etc)

Reported by: Otto42 Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.2
Component: Database Keywords: commit has-patch
Focuses: Cc:

Description

In wp-db.php, mysql is connected like so:

if ( WP_DEBUG ) {
	$this->dbh = mysql_connect( $this->dbhost, $this->dbuser, $this->dbpassword, true );
} else {
	$this->dbh = @mysql_connect( $this->dbhost, $this->dbuser, $this->dbpassword, true );
}

The "true" forces a new link to be established (not allowing connection reuse), and no fifth parameter means that one cannot use the mysql client constants, such as MYSQL_CLIENT_SSL for SSL support or MYSQL_CLIENT_COMPRESS for compression. Things like that.

Suggest adding filters or some kind of hooks here to allow these to be defined/overridden by plugins as well. SSL support especially would be nice for people who have separate DB servers and want a small extra smidge of security over that link.

Currently the only way to really do this and be semi-forward-compatible is to copy wp-db.php into wp-content/db.php and modify the file directly. Not exactly upgrade friendly.

Attachments (2)

19324.diff (985 bytes) - added by Otto42 2 years ago.
patch to add DB_NEW_LINK and DB_CLIENT_FLAGS defines
db.php (1.8 KB) - added by Otto42 23 months ago.
Custom db.php file that incorporates the patch as an extension to the class

Download all attachments as: .zip

Change History (27)

comment:1 avcascade2 years ago

This is a *great* idea. I'd like to see this made a priority for 3.4, and failing that, 3.5 at least.

I especially concur with this:

SSL support especially would be nice for people who have separate DB servers and want a small extra smidge of security over that link.

Support for secure MySQL connections really needs to be in core. Security-minded WordPress users who do the work of getting their host to properly configure MySQL to accept secure connections shouldn't have to rewrite wp-db.php to get things working on the WordPress end.

Given the significant and growing popularity of WordPress, I'm sure there are many cases in which people are connecting to databases that are not on the same machines as their WordPress filesystem. It would be best if that traffic were encrypted.

Last edited 2 years ago by avcascade (previous) (diff)

comment:2 dd322 years ago

Instead of copying the entire file, you can simply have that file require_once wpdb, followed by extending the class and replacing the function required.

Still not as clean as it could be, but works never-the-less.

comment:3 follow-up: sirzooro2 years ago

The easiest way seems to add new define (e.g. WP_DB_CLIENT_FLAGS) with default value 0, and allow to specify custom value in wp-config.php.

comment:4 in reply to: ↑ 3 nacin2 years ago

Replying to sirzooro:

The easiest way seems to add new define (e.g. WP_DB_CLIENT_FLAGS) with default value 0, and allow to specify custom value in wp-config.php.

Something along those lines, yeah. The connection runs too early for plugins to run, so filters won't help much.

comment:5 JustinK1012 years ago

  • Cc kobenews@… added
  • Keywords needs-refresh needs-patch 2nd-opinion added
  • Version set to 3.3.1

I use MYSQL_CLIENT_COMPRESS, it reduces the amount of traffic between the web server and database significantly. Currently I have to modify core, and change /wp-includes/wp-db.php every release. It quite annoying. I propose a new configuration parameter:

    define(WP_MYSQL_CLIENT_COMPRESS, true);

By default its false. Which then in wp-db.php I purpose the following change:

	if ( WP_DEBUG ) {
		$this->dbh = mysql_connect( $this->dbhost, $this->dbuser, $this->dbpassword, true, WP_MYSQL_CLIENT_COMPRESS ? WP_MYSQL_CLIENT_COMPRESS : null );
	} else {
		$this->dbh = @mysql_connect( $this->dbhost, $this->dbuser, $this->dbpassword, true, WP_MYSQL_CLIENT_COMPRESS ? WP_MYSQL_CLIENT_COMPRESS : null );
	}
Version 1, edited 2 years ago by JustinK101 (previous) (next) (diff)

Otto422 years ago

patch to add DB_NEW_LINK and DB_CLIENT_FLAGS defines

comment:6 Otto422 years ago

Attached patch to add DB_NEW_LINK and DB_CLIENT_FLAGS defines to allow changing of these two settings.

comment:7 Otto422 years ago

  • Keywords needs-refresh needs-patch removed

comment:8 JustinK1012 years ago

  • Version changed from 3.3.1 to 3.4

Awesome, when will this make it into live?

comment:9 Otto422 years ago

  • Version changed from 3.4 to 3.3.1

The version number on the ticket should not be changed, it should reflect the original reported version.

comment:10 Otto422 years ago

  • Version changed from 3.3.1 to 3.2

comment:11 chrisvanpatten2 years ago

  • Cc chris@… added

comment:12 chrisvanpatten23 months ago

We have had this patch tested in production for several weeks now and have had no problems (running Ubuntu 12.04 on two Linode VPSs, using MYSQL_CLIENT_SSL to connect between the two). It works flawlessly. I'd love to see this fix make it in to 3.4.1, so we can stop running patched core!

How we can help make that happen?

Otto4223 months ago

Custom db.php file that incorporates the patch as an extension to the class

comment:13 Otto4223 months ago

I attached a db.php file which you can drop into the wp-content directory. It basically extends the wpdb class to create a custom_wpdb class, which incorporates the patch.

This will let you run the same patch without hacking core. It's not a great solution, but I can't think of a better one because as nacin said, plugins don't have the ability to easily modify that section of code (making filters useless) and using defines are rather unpleasant at best.

Still, having a drop in like this to do that sort of thing is better than hacking core.

Last edited 23 months ago by Otto42 (previous) (diff)

comment:14 PeterUpfold23 months ago

  • Cc strategyoracle@… added

comment:15 chrisvanpatten23 months ago

Thanks Otto! I still think this belongs in core but this is a nice way to make it work without patches/hacks until it makes it in.

comment:16 pento21 months ago

  • Keywords has-patch added

A good patch. Let's do it.

comment:17 nacin21 months ago

  • Keywords commit added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to 3.5

comment:18 nacin20 months ago

Because these are MySQL-specific, I kind of want to call them MYSQL_* rather than DB_*. Any preference?

comment:19 pento20 months ago

MYSQL_NEW_LINK and MYSQL_CLIENT_FLAGS works for me. These match the default parameter names for mysql_connect().

comment:20 nacin20 months ago

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

In [21609]:

Basic support for the mysql_connect() new_link and client_flags arguments. props Otto42, fixes #19324.

comment:21 ticoombs7 months ago

  • Cc ticoombs added
  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version 3.2 deleted
  • redacted as relized it was already fixed. -
Last edited 7 months ago by ticoombs (previous) (diff)

comment:22 Otto427 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from 3.5 to 3.2
  • Resolution set to fixed
  • Status changed from reopened to closed

No, this seems to work correctly, as intended.

This line sets the $client_flags to whatever the value of the MYSQL_CLIENT_FLAGS is defined as:

$client_flags = defined( 'MYSQL_CLIENT_FLAGS' ) ? MYSQL_CLIENT_FLAGS : 0;

Same goes for $new_link and MYSQL_NEW_LINK.

comment:23 follow-up: ticoombs7 months ago

Edit:
Just realised the line should read:
$client_flags = defined( 'MYSQL_CLIENT_FLAGS' ) ? $MYSQL_CLIENT_FLAGS : 0;
$MYSQL_CLIENT_FLAGS not MYSQL_CLIENT_FLAGS.

Then setting the variable in wp-config works. (This is the same for newlink, as it will just be text not the variable you assigned)

Last edited 7 months ago by ticoombs (previous) (diff)

comment:24 in reply to: ↑ 23 SergeyBiryukov7 months ago

  • Milestone changed from 3.2 to 3.5
  • Version set to 3.2

Replying to ticoombs:

Then setting the variable in wp-config works. (This is the same for newlink, as it will just be text not the variable you assigned)

It's a constant, not a variable. Constants do not need to be prefixed with the $ sign.

comment:25 ircbot3 months ago

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

Note: See TracTickets for help on using tickets.