Don't hardcode MySQL command names, part 2

[swissspidy]

This is a follow-up to #275 which addressed the mysqlcheck and mysqldump usage, but not usage of mysql itself.

There was a suggestion to move this utility to the main framework (maybe updating Utils/get_mysql_binary_path()?) which we could do in a future step.

Then, we could also add a get_db_type() util or so to avoid repeating ( strpos( Utils\get_mysql_version(), 'MariaDB' ) !== false ) everywhere.

[mrsdizzie]

this doesn't work as we would hope because:

private function get_mysql_command() {
		return ( strpos( Utils\get_mysql_version(), 'MariaDB' ) !== false ) ? 'mariadb' : 'mysql';
	}

just calls the mysql binary which is what generates the warnings about using mysql itself.

The problem is that because mariadb has decided to deprecate calling mysql, we would need to have a way to determine what binary to call before we call one at all. So this will need some thinking as the best way to do that (and then replace the very many uses of mysql in wp-cli so it probably should be in the wp cli project itself).

A simple way is to just assume if mariadb is there to prefer that instead -- but im not sure how accurate that might be. Or to add a new environment variable that lets the user decide?

[swissspidy]

Right... 🤔 💡

So basically we need /usr/bin/env which mariadb to get the MariaDB, which could be its own util a la get_mariadb_binary_path().

(and then replace the very many uses of mysql in wp-cli so it probably should be in the wp cli project itself).

Not sure if there are really that many, see https://github.com/search?q=org%3Awp-cli+%22%2Fusr%2Fbin%2Fenv+mysql%22&type=code

A simple way is to just assume if mariadb is there to prefer that instead -- but im not sure how accurate that might be. Or to add a new environment variable that lets the user decide?

The former could have some unexpected results if suddenly a different binary is used. So the latter seems safer.

[mrsdizzie]

So basically we need /usr/bin/env which mariadb to get the MariaDB, which could be its own util a la get_mariadb_binary_path().

Yea I think something like that is needed! I'm not sure if it is common to have both mariadb and mysql installed -- but even with the deprecated mysql binary that mariadb provides (which causes these warnings) we would still get valid return values for both which mariadb and which mysql so we would need some way of deciding which is the proper one to pick.

So really the problem is on a system that is going to have both a mariadb and mysql binary, what steps do we take to determine the one to use? Is it safe enough to assume that since mariadb provides a mysql binary, but mysql doesn't provide a mariadb binary, that pretty much any case of both existing are going to be a mariadb install?

Maybe a more general get_db_type() check that could include sqlite as well? I don't think we have anything like that but it would be a good first step for either fixing or giving better error messages for most of the commands in this repo ..

[swissspidy]

^ @schlessera any thoughts on the above suggestion and the issue at hand?

[swissspidy]

Yea I think something like that is needed! I'm not sure if it is common to have both mariadb and mysql installed -- but even with the deprecated mysql binary that mariadb provides (which causes these warnings) we would still get valid return values for both which mariadb and which mysql so we would need some way of deciding which is the proper one to pick.

So really the problem is on a system that is going to have both a mariadb and mysql binary, what steps do we take to determine the one to use? Is it safe enough to assume that since mariadb provides a mysql binary, but mysql doesn't provide a mariadb binary, that pretty much any case of both existing are going to be a mariadb install?

Perhaps we could do this:

1. If MySQL is not installed but MariaDB is -> use MariaDB
2. If MySQL is installed and mysql --version says "from 11.7.2-MariaDB" -> use MariaDB
   (For this we would run /usr/bin/env mysql --version and simply ignore/silence the deprecation warning)
3. If MySQL is installed and it's the "real" one -> use MySQL
4. If MySQL is not installed, well, good luck 😄

This way there shouldn't be any breaking change.

Maybe a more general get_db_type() check that could include sqlite as well? I don't think we have anything like that but it would be a good first step for either fixing or giving better error messages for most of the commands in this repo ..

We could check for SQLite support depending on the SQLITE_DB_DROPIN_VERSION constant. But that of course works only when dealing with an actual WordPress install with the SQLite plugin.

[swissspidy]

Now tests fail because run_mysql_command adds /usr/bin/env to every command...

[mrsdizzie]

Now tests fail because run_mysql_command adds /usr/bin/env to every command...

Probably something like this needed going forward:

db-command/features/db-check.feature

Line 131 in 19b2555

Then STDERR should match #Debug \(db\): Running shell command: /usr/bin/env (mysqlcheck|mariadb-check) %s#

[swissspidy]

Yeah good point… So I should not use the fully resolved cmd path to make that work, but still /usr/bin/env (mysql|mariadb)