Unnecessary check of sql modes in query command

[mrsdizzie]

Passing additional options to wp db query doesn't work because it runs get_sql_mode_query which ignores all of those options when checking sql modes and then dies.

db-command/src/DB_Command.php

Lines 501 to 520 in c821b92

	public function query( $args, $assoc_args ) {
	$command = sprintf(
	'/usr/bin/env %s%s --no-auto-rehash',
	$this->get_mysql_command(),
	$this->get_defaults_flag_string( $assoc_args )
	);
	WP_CLI::debug( "Running shell command: {$command}", 'db' );
	$assoc_args['database'] = DB_NAME;
	// The query might come from STDIN.
	if ( ! empty( $args ) ) {
	$assoc_args['execute'] = $args[0];
	}
	if ( isset( $assoc_args['execute'] ) ) {
	// Ensure that the SQL mode is compatible with WPDB.
	$assoc_args['execute'] = $this->get_sql_mode_query( $assoc_args ) . $assoc_args['execute'];
	}

And

db-command/src/DB_Command.php

Lines 2152 to 2162 in c821b92

	protected function get_current_sql_modes( $assoc_args ) {
	static $modes = null;
	// Make sure the provided arguments don't interfere with the expected
	// output here.
	$args = [];
	foreach ( [] as $arg ) {
	if ( isset( $assoc_args[ $arg ] ) ) {
	$args[ $arg ] = $assoc_args[ $arg ];
	}
	}

So if I run:

isla $ wp db query "SELECT 1" --host=dev.example.com --defaults --debug
Debug (bootstrap): Running command: db query (0.116s)
Debug (db): Running shell command: /usr/bin/env mariadb --no-auto-rehash (0.15s)
Debug (db): Final MySQL command: /usr/bin/env mariadb --no-defaults --no-auto-rehash --batch --skip-column-names --host='localhost' --user='test' --default-character-set='utf8' --execute='SELECT @@SESSION.sql_mode' (0.15s)
Error: Failed to get current SQL modes. Reason: ERROR 1045 (28000): Access denied for user 'test'@'localhost' (using password: YES)

because it has stripped out --host=dev.example.com --defaults.

It does if you pipe it in like:

 $ echo "SELECT 1" | wp db query --host=dev.example.com --defaults --debug
Debug (db): Associative arguments: {"host":"dev.example.com","database":"test"} (0.099s)
Debug (db): Final MySQL command: /usr/bin/env mariadb --no-auto-rehash --host='dev.example.com' --user='test' --default-character-set='utf8' --database='test' (0.099s)
1
1

I'm pretty sure this is just wrong:

if ( isset( $assoc_args['execute'] ) ) { 
 		// Ensure that the SQL mode is compatible with WPDB. 
 		$assoc_args['execute'] = $this->get_sql_mode_query( $assoc_args ) . $assoc_args['execute']; 
 	} 

because wp db query will never use wpdb and nothing else in this chain looks at $assoc_args['execute'] that I can see. Blame suggests the original commit: 75d4cdf

which says

* This includes necessary setup to make sure the queries behave similar
* to what WPDB produces.

But that doesn't seem very helpful or necessary here. Given that and how it already skips this check when using STDIN I'd vote to simply remove it so that the more standard wp db query "SELECT 1" works as expected without unnecessary complication.

[mrsdizzie]

Similar to #237 I feel like this is unnecessary complication which causes many real issues in an effort to avoid some sort of hypothetical problems. Several open issues are about errors with get_sql_mode_query for commands that never use wpdb at all (#288 #278, #218).

This all comes from #168 but unfortunately there isn't a good description of why here and the links to examples all don't work anymore : (

Though I think the sated reason of

However, for the commands that use the mysql binary directly to execute queries, this is not done yet. This can lead to problems where a query that would work within WordPress would not work with a command like wp db query ....

Isn't enough to be causing all of these bugs.

[swissspidy]

Thanks for the writeup!

Maybe @schlessera can shed some light on this?