WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 18 months ago

#27933 closed defect (bug) (fixed)

MYSQL_NEW_LINK constant is not honored with MySQLi

Reported by: doublesharp Owned by: nacin
Milestone: 4.0 Priority: normal
Severity: normal Version: 3.9
Component: Database Keywords: has-patch commit
Focuses: docs Cc:
PR Number:

Description

Using WordPress 3.9 and PHP5.5, the MySQLi extension is used in lieu of the deprecated mysql_* extensions, however the MYSQL_NEW_LINK constant is not honored. When this constant is set to false it would cause the $new_link passed to [mysql_connect()](http://us2.php.net/function.mysql-connect) to indicate that an already opened link should be returned.

For MySQLi this is accomplishing by prepending a p: to the hostname - see details on [The mysqli Extension and Persistent Connections](http://www.php.net/manual/en/mysqli.persistconns.php) page. Dynamically prepending p: to the $host based on the value of MYSQL_NEW_LINK to maintain compatibility would be ideal.

Attachments (6)

wp-db.php.patch (494 bytes) - added by doublesharp 6 years ago.
Prepend $host with "p:" for persistent mysqli connections
wp-db.php.2.patch (562 bytes) - added by doublesharp 6 years ago.
check for PHP 5.3+
wp-db.php.with_debug.patch (697 bytes) - added by doublesharp 6 years ago.
check for PHP 5.3+, print debug message if false
wp-db.php.3.patch (725 bytes) - added by doublesharp 6 years ago.
use prefered functions and code styling
wp-db.php.4.patch (727 bytes) - added by doublesharp 6 years ago.
corret styling standards
27933.diff (522 bytes) - added by DrewAPicture 6 years ago.

Download all attachments as: .zip

Change History (25)

@doublesharp
6 years ago

Prepend $host with "p:" for persistent mysqli connections

#1 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 3.9.1

#2 @pento
6 years ago

  • Keywords has-patch added

Nice find. :-)

I like where you're going with this patch, but it also needs to do a PHP version check. The p: syntax was introduced in PHP 5.3, but WordPress still supports PHP 5.2.

Also, a minor coding standard thing - there's no need to use the curly brackets in "p:{$host}".

I'm okay with PHP 5.2 users who've deliberately forced mysqli to be used to not have persistent connections.

#3 @doublesharp
6 years ago

Good call on the PHP version, didn't occur to me that someone would still be running PHP 5.2. I'm going to attach two updated patches for your consideration with slightly different functionality.

The first is based on your exact recommendations to include a check for PHP 5.3+ and to nix the curly braces.

if ( ! $new_link && version_compare(PHP_VERSION, '5.3.0') >= 0 ){
        $host = "p:$host";
}

The second does basically the same thing, but prints a message indicating that persistent connections are not supported when WP_DEBUG==true and MYSQL_NEW_LINK==false.

if ( ! $new_link ){
        if ( version_compare(PHP_VERSION, '5.3.0') >= 0 ){
                $host = "p:$host";
        } elseif ( WP_DEBUG ){
                _e( 'MySQLi persistent connections are not supported with your version of PHP.' );
        }
}

@doublesharp
6 years ago

check for PHP 5.3+

@doublesharp
6 years ago

check for PHP 5.3+, print debug message if false

#4 @pento
6 years ago

For the version_compare() call, putting the operator in the function call would make it more readable:

version_compare( PHP_VERSION, '5.3.0', '>=' )

For consistency with the other PHP version check in wpdb, I'd use phpversion() instead of PHP_VERSION.

I like the warning, though instead of the WP_DEBUG check, you can just do a _doing_it_wrong() call instead:

} else {
    _doing_it_wrong( 'wpdb::db_connect', __( 'Disabling MYSQL_NEW_LINK with MySQLi requires PHP 5.3 or later' ), '3.9.1' );
}

(I'm not wild about that particular error message, @DrewAPicture, do you have some thoughts on this?)

And for a minor coding standard reminder, don't forget your whitespace.

#5 @doublesharp
6 years ago

Attaching a new patch with the latest suggestions:

if ( ! $new_link ){
        if ( version_compare( phpversion(), '5.3.0', '>=' ) ){
                $host = "p:$host";
        } else {
                _doing_it_wrong( 'wpdb::db_connect', __( 'Disabling MYSQL_NEW_LINK with MySQLi requires PHP 5.3 or later.' ), '3.9.1' );
        }
}

Thanks for the coding standards link @pento, probably should have looked at it first.

@doublesharp
6 years ago

use prefered functions and code styling

#6 @pento
6 years ago

Looking good! Just a couple of small changes for spacing - there should be a space before the { in each of the if() statements.

#7 @doublesharp
6 years ago

Fourth time's the charm?

if ( ! $new_link ) {
        if ( version_compare( phpversion(), '5.3.0', '>=' ) ) {
                $host = "p:$host";
        } else {
                _doing_it_wrong( 'wpdb::db_connect', __( 'Disabling MYSQL_NEW_LINK with MySQLi requires PHP 5.3 or later.' ), '3.9.1' );
        }
}

@doublesharp
6 years ago

corret styling standards

#8 @pento
6 years ago

  • Keywords needs-docs added

Looking good!

I'm still not wild about the message, so we'll wait for a docs review, but this patch is ready to go!

#9 @nacin
6 years ago

Looking through the ext/mysql source, there seems to be some differences between mysql_pconnect() and mysql_connect( new_link = false ). mysqli's p: seems to be the former. Is it expected to apply MYSQL_NEW_LINK in this case?

#10 @pento
6 years ago

  • Keywords needs-refresh added; has-patch needs-docs removed

Well, today has been a day for digging through PHP source.

Nacin is correct, new_link = false is different to mysql_pconnect(), and mysqli's p: notation is the equivalent of mysql_pconnect().

new_link = false is for forcing subsequent calls of mysql_connect(), in the same process, to use an established link that matches the connect parameters of that call. mysqli doesn't have an equivalent to this.

With that in mind, attachment:wp-db.php.4.patch is unfortunately the incorrect solution for this problem - instead, MYSQL_NEW_LINK is deprecated for use with mysqli, as of 3.9.

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


6 years ago

#12 @nacin
6 years ago

  • Owner set to DrewAPicture
  • Status changed from new to assigned

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


6 years ago

@DrewAPicture
6 years ago

#14 @DrewAPicture
6 years ago

  • Focuses docs added
  • Keywords has-patch added; needs-refresh removed

I don't think we necessarily need to do anything beyond adding an inline comment describing that the value is deprecated in 3.9+.

This is done in 27933.diff

#15 @nacin
6 years ago

  • Milestone changed from 3.9.1 to 4.0

#16 @DrewAPicture
5 years ago

  • Owner changed from DrewAPicture to nacin
  • Status changed from assigned to reviewing

Will 27933.diff suffice here?

#17 @SergeyBiryukov
5 years ago

  • Keywords commit added

#18 @DrewAPicture
5 years ago

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

In 28657:

Mark the MYSQL_NEW_LINK constant as deprecated in 3.9+ as no equivalent to the $new_link parameter exists in mysqli_* functions.

Fixes #27933.

This ticket was mentioned in Slack in #core-php by sergey. View the logs.


18 months ago

Note: See TracTickets for help on using tickets.