Skip to content
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

Don't hardcode MySQL command names #275

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

mrsdizzie
Copy link
Member

The compatibility links MariaDB provides for MySQL commands are deprecated and will be removed in the future. We should use the MariaDB command name if that is the current MySQL provider.

Fixes #271

(We still use mysql all over the place in wp-cli, but that isn't going away from MariaDB.)

The compatibility links MariaDB provides for MySQL commands are
deprecated and will be removed in the future. We should use the MariaDB
command name if that is the current MySQL provider.

Fixes wp-cli#271
@marcwieland95
Copy link

@mrsdizzie How am I able to test your fix? I get the same issue on wp db import or wp db export after updating to PHP 8.4 and new bundles on the server.

@mrsdizzie
Copy link
Member Author

@mrsdizzie How am I able to test your fix? I get the same issue on wp db import or wp db export after updating to PHP 8.4 and new bundles on the server.

This PR hasn't been merged yet so outside of checking out my personal branch here you'd need to wait until it gets reviewed and merged

@mrsdizzie mrsdizzie requested a review from swissspidy December 18, 2024 17:34
@swissspidy
Copy link
Member

If you create branch in this repo instead of forking, people could install it via wp package or composer with the dev-<branchname> syntax

@swissspidy swissspidy merged commit e9c4e8a into wp-cli:main Dec 18, 2024
25 of 35 checks passed
@mrsdizzie mrsdizzie deleted the fix-mariadb-warnings branch December 18, 2024 17:54
@marcwieland95
Copy link

marcwieland95 commented Dec 19, 2024

I just updated to the latest db_command package and ran a wp db reset. I still get the same warning and the command fails. I think WP CLI doesn't get the correct system information. Check the screenshot below, especially the MySQL binary in the info section. The fix of this pull request is still running with mysql instead of mariadb in my case for that reason, I guess.

CleanShot 2024-12-19 at 11 24 00@2x

@swissspidy
Copy link
Member

I don't think the PR actually touched wp db reset itself. It only changed mysqlcheck and mysqldump, as that's what the original report was about. It did not touch mysql. We use that in a few places still:

$command = sprintf( '/usr/bin/env mysql%s --no-auto-rehash', $this->get_defaults_flag_string( $assoc_args ) );
WP_CLI::debug( "Running shell command: {$command}", 'db' );

$command = sprintf( '/usr/bin/env mysql%s --no-auto-rehash', $this->get_defaults_flag_string( $assoc_args ) );
WP_CLI::debug( "Running shell command: {$command}", 'db' );

list( $stdout, $stderr, $exit_code ) = self::run(
sprintf(
'/usr/bin/env mysql%s --no-auto-rehash --batch --skip-column-names',
$this->get_defaults_flag_string( $assoc_args )
),
[ 'execute' => $query ],
false
);

$command = sprintf( '/usr/bin/env mysql%s --no-auto-rehash', $this->get_defaults_flag_string( $assoc_args ) );

db-command/src/DB_Command.php

Lines 1735 to 1741 in e9c4e8a

self::run(
sprintf(
'/usr/bin/env mysql%s --no-auto-rehash',
$this->get_defaults_flag_string( $assoc_args )
),
array_merge( [ 'execute' => $query ], $mysql_args )
);

^ this one is ultimately used by wp db reset

db-command/src/DB_Command.php

Lines 2135 to 2142 in e9c4e8a

list( $stdout, $stderr, $exit_code ) = self::run(
sprintf(
'/usr/bin/env mysql%s --no-auto-rehash --batch --skip-column-names',
$this->get_defaults_flag_string( $assoc_args )
),
array_merge( $args, [ 'execute' => 'SELECT @@SESSION.sql_mode' ] ),
false
);

@mrsdizzie wanna do a follow-up PR?

@mrsdizzie
Copy link
Member Author

I don't think the PR actually touched wp db reset itself. It only changed mysqlcheck and mysqldump, as that's what the original report was about. It did not touch mysql. We use that in a few places still:

@mrsdizzie wanna do a follow-up PR?

Yes, I'll think on the best way to do this since wp cli really does assume and use the mysql binary in many places. Probably an initial PR to wp-cli itself for a new utils function that returns the correct binary based on some heuristics (presumably people will have both installed at some point if they are intentionally separating to not be a drop in replacement anymore).

@pluraltouch
Copy link

I still getting Deprecated warnings as far as I know I am using the latest wp cli docker image...

$ wp cli info
OS: Linux 6.8.0-1020-azure #23-Ubuntu SMP Mon Dec 9 16:58:58 UTC 2024 x86_64
Shell:
PHP binary: /usr/local/bin/php
PHP version: 8.2.27
php.ini used:
MySQL binary: /usr/bin/mysql
MySQL version: mysql from 11.4.4-MariaDB, client 15.2 for Linux (x86_64) using readline 5.1
SQL modes:
WP-CLI root dir: phar://wp-cli.phar/vendor/wp-cli/wp-cli
WP-CLI vendor dir: phar://wp-cli.phar/vendor
WP_CLI phar path: /var/www/html
WP-CLI packages dir:
WP-CLI cache dir: /tmp/.wp-cli/cache
WP-CLI global config:
WP-CLI project config:
WP-CLI version: 2.11.0

@swissspidy
Copy link
Member

Yeah I think we still need that follow-up PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wp-cli returns FAIL notice when using mariadb symlinked mysql* commands; request for native mdb support
4 participants