Filter empty database parameters to prevent MySQL command failures #308
Conversation
Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to prevent MySQL command failures by filtering out empty database parameters. The approach is sound, but the current implementation has a flaw: it filters parameters from wp-config.php before merging them with command-line arguments. This means the fix can be bypassed if an empty value is provided as a command-line argument. I've suggested applying the filter after all arguments are merged to create a more robust solution.
Additionally, the new test case for an empty DB_CHARSET doesn't actually exercise the new filtering logic due to a pre-existing condition. I've recommended an alternative test scenario that would better validate the changes.
| // Filter out empty string values to avoid passing empty parameters to MySQL commands | ||
| // which can cause errors like "Character set '' is not a compiled character set" | ||
| $required = array_filter( | ||
| $required, | ||
| static function ( $value ) { | ||
| return null !== $value && '' !== $value; | ||
| } | ||
| ); | ||
|
|
||
| $final_args = array_merge( $required, $assoc_args ); |
There was a problem hiding this comment.
The current implementation filters empty values from the $required array before merging with $assoc_args. However, this approach has two issues:
- It doesn't filter empty values that might come from command-line arguments (i.e., in
$assoc_args). For example,wp db check --host=''would still pass an empty host parameter. - Values in
$assoc_argsoverwrite values from$requiredduring the merge. If an empty value is passed via the command line for a parameter that has a non-empty value inwp-config.php, the filtering on$requiredbecomes ineffective.
To ensure all empty parameters are correctly filtered, regardless of their origin, the filtering should be applied to the $final_args array after merging.
$final_args = array_merge( $required, $assoc_args );
// Filter out empty string values to avoid passing empty parameters to MySQL commands
// which can cause errors like "Character set '' is not a compiled character set"
$final_args = array_filter(
$final_args,
static function ( $value ) {
return null !== $value && '' !== $value;
}
);| Scenario: Empty DB credentials should not cause empty parameter errors | ||
| Given a WP install | ||
| And a wp-config.php file: | ||
| """ | ||
| <?php | ||
| define( 'DB_NAME', 'wp_cli_test' ); | ||
| define( 'DB_USER', 'wp_cli_test' ); | ||
| define( 'DB_PASSWORD', 'password1' ); | ||
| define( 'DB_HOST', 'localhost' ); | ||
| define( 'DB_CHARSET', '' ); | ||
| define( 'DB_COLLATE', '' ); | ||
| $table_prefix = 'wp_'; | ||
| require_once ABSPATH . 'wp-settings.php'; | ||
| """ | ||
|
|
||
| When I run `wp db check --debug` | ||
| Then STDERR should contain: | ||
| """ | ||
| Debug (db): Final MySQL command: | ||
| """ | ||
| And STDERR should not contain: | ||
| """ | ||
| --default-character-set='' | ||
| """ | ||
| And STDERR should not contain: | ||
| """ | ||
| --default-character-set= | ||
| """ | ||
| And the return code should be 0 | ||
| And STDOUT should contain: | ||
| """ | ||
| Success: Database checked. | ||
| """ | ||
|
|
There was a problem hiding this comment.
This test scenario is a great addition for verifying the behavior with empty credentials. However, it specifically tests an empty DB_CHARSET, which is already handled by existing logic in DB_Command::run() (&& constant( 'DB_CHARSET' )) and doesn't actually exercise the new array_filter logic.
To create a more effective test that validates the new filtering mechanism, consider testing a scenario where DB_HOST or DB_USER is an empty string. This would directly test whether the array_filter correctly removes the empty parameter before it's passed to the MySQL command.
For example, you could add a scenario like this:
Scenario: Empty DB_HOST should not be passed as a command-line argument
Given a WP install
And a wp-config.php file:
"""
<?php
define( 'DB_NAME', 'wp_cli_test' );
define( 'DB_USER', 'wp_cli_test' );
define( 'DB_PASSWORD', 'password1' );
define( 'DB_HOST', '' );
define( 'DB_CHARSET', 'utf8' );
$table_prefix = 'wp_';
require_once ABSPATH . 'wp-settings.php';
"""
When I run `wp db check --debug`
Then STDERR should not contain "--host=''"
And STDERR should not contain "--host="
# Depending on environment, an empty host might default to localhost socket and succeed.
And the return code should be 0
MySQL commands fail when
DB_HOST,DB_USER,DB_PASSWORD, orDB_CHARSETare defined as empty strings inwp-config.php. Empty values are passed as--host= --user= --default-character-set=, which MySQL rejects with errors likeCharacter set '' is not a compiled character set.Changes
DB_Command::run(): Addedarray_filterto strip empty strings and null values from the$requiredarray before passing to MySQL commands'0'and integer0are kept (valid for ports/values)DB_CHARSETdoesn't generate invalid command-line argumentsImplementation
Applies to all commands using the
run()method:check,optimize,repair,cli,query,export,import, etc.Original prompt
This section details on the original issue you should resolve
<issue_title>mysqlcheck: Character set '' is not a compiled character set</issue_title>
<issue_description>## Bug Report
When running commands related to
dbat hostinger, I get the following error.Hostinger support said it's a bug with the wp-cli, and my debug shows the same..
Describe the current, buggy behavior
When running
wp db checkin any of my wp installs at hostinger, I get the following:With debug...
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.