Change mysql CLI call to native PHP function#158
Change mysql CLI call to native PHP function#158danielbachhuber merged 6 commits intowp-cli:mainfrom wojtekn:update/mysql-connection-check
Conversation
|
@wojtekn , can you please provide some test instructions here? |
src/Config_Command.php
Outdated
| // phpcs:disable WordPress.DB.RestrictedFunctions,WordPress.PHP.NoSilencedErrors.Discouraged | ||
| $mysql = mysqli_init(); | ||
| if ( ! @mysqli_real_connect( $mysql, $assoc_args['dbhost'], $assoc_args['dbuser'], $assoc_args['dbpass'] ) ) { | ||
| die( 'Error (' . mysqli_connect_errno() . ') ' . mysqli_connect_error() ); |
There was a problem hiding this comment.
| die( 'Error (' . mysqli_connect_errno() . ') ' . mysqli_connect_error() ); | |
| die( 'Database connection error (' . mysqli_connect_errno() . ') ' . mysqli_connect_error() ); |
There was a problem hiding this comment.
I've read some caution against using @ too liberally, and I think part of the reason is that error handlers still execute, and also that it can trap errors that we don't want to trap.
Not that it would demand any blocker on this PR, but it could bring some benefit to try and catch this in some other way. Potentially by setting mysqli_report( MYSQLI_REPORT_STRICT ) we could wrap this in a try block and bypass generating warnings while also only trapping errors related to the connection.
mysqli_report( MYSQLI_REPORT_STRICT );
try {
mysqli_real_connect( … );
} catch ( mysqli_sql_exception $e ) {
die( "Database connection error ({$e->message})" );
}There was a problem hiding this comment.
Thanks, I've adjusted the code based on the proposed approach.
src/Config_Command.php
Outdated
| // Check DB connection. | ||
| if ( ! Utils\get_flag_value( $assoc_args, 'skip-check' ) ) { | ||
| Utils\run_mysql_command( '/usr/bin/env mysql --no-defaults', $mysql_db_connection_args ); | ||
| // phpcs:disable WordPress.DB.RestrictedFunctions,WordPress.PHP.NoSilencedErrors.Discouraged |
There was a problem hiding this comment.
it would be awesome here if we provided a description in a comment explaining why the command is calling a restricted function, similar to what's in the PR description.
dmsnell
left a comment
There was a problem hiding this comment.
I'm not sure who should review this, but from my perspective it looks like a positive change.
danielbachhuber
left a comment
There was a problem hiding this comment.
Amazingly, this seems to work fine against all PHP versions we support. Thanks for the pull request!
It fixes #152
In this diff, I propose to change the way of validating MySQL connection from the CLI command call to the native PHP function.
$wpdbis not accessible, so I used the native mysqli function, similar to what happens in$wpdb->db_connect().Testing steps
wp-config.phpif it exists