-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change mysql CLI call to native PHP function #158
Change mysql CLI call to native PHP function #158
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure who should review this, but from my perspective it looks like a positive change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
$wpdb
is not accessible, so I used the native mysqli function, similar to what happens in$wpdb->db_connect()
.Testing steps
wp-config.php
if it exists