Make WordPress Core

Opened 12 years ago

Closed 11 years ago

Last modified 10 years ago

#19324 closed feature request (fixed)

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

Reported by: otto42's profile Otto42 Owned by: nacin's profile 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 12 years ago.
patch to add DB_NEW_LINK and DB_CLIENT_FLAGS defines
db.php (1.8 KB) - added by Otto42 12 years ago.
Custom db.php file that incorporates the patch as an extension to the class

Download all attachments as: .zip

Change History (27)

#1 @avcascade
12 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 12 years ago by avcascade (previous) (diff)

#2 @dd32
12 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.

#3 follow-up: @sirzooro
12 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.

#4 in reply to: ↑ 3 @nacin
12 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.

#5 @JustinK101
12 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);

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 : 0 );
	} else {
		$this->dbh = @mysql_connect( $this->dbhost, $this->dbuser, $this->dbpassword, true, WP_MYSQL_CLIENT_COMPRESS ? WP_MYSQL_CLIENT_COMPRESS : 0 );
	}
Last edited 12 years ago by JustinK101 (previous) (diff)

@Otto42
12 years ago

patch to add DB_NEW_LINK and DB_CLIENT_FLAGS defines

#6 @Otto42
12 years ago

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

#7 @Otto42
12 years ago

  • Keywords needs-refresh needs-patch removed

#8 @JustinK101
12 years ago

  • Version changed from 3.3.1 to 3.4

Awesome, when will this make it into live?

#9 @Otto42
12 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.

#10 @Otto42
12 years ago

  • Version changed from 3.3.1 to 3.2

#11 @chrisvanpatten
12 years ago

  • Cc chris@… added

#12 @chrisvanpatten
12 years 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?

@Otto42
12 years ago

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

#13 @Otto42
12 years 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 12 years ago by Otto42 (previous) (diff)

#14 @PeterUpfold
12 years ago

  • Cc strategyoracle@… added

#15 @chrisvanpatten
12 years 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.

#16 @pento
12 years ago

  • Keywords has-patch added

A good patch. Let's do it.

#17 @nacin
12 years ago

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

#18 @nacin
12 years ago

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

#19 @pento
12 years ago

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

#20 @nacin
12 years 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.

#21 @ticoombs
11 years 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 11 years ago by ticoombs (previous) (diff)

#22 @Otto42
11 years 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.

#23 follow-up: @ticoombs
11 years 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 11 years ago by ticoombs (previous) (diff)

#24 in reply to: ↑ 23 @SergeyBiryukov
11 years 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.

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


10 years ago

Note: See TracTickets for help on using tickets.