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

Support all `mysql` flags while importing database #123

Merged
merged 4 commits into from Dec 3, 2018

Conversation

2 participants
@abhijitrakas
Copy link

abhijitrakas commented Nov 14, 2018

Commit for wp-cli/ideas#112

@schlessera
Copy link
Member

schlessera left a comment

Thanks for this first iteration, @abhijitrakas !

First of all, we should add at least 1 test in the Behat suite to test and assert that the arguments are actually passed onto mysql.

Then I have a few thoughts regarding the implementation. You have directly implemented this as part of the db import command now, but I think that every command that works by providing arguments to run the mysql binary would be abel to benefit from this improvement. So I think it would be preferable to implement this in a separate method called get_mysql_args( $assoc_args ) (similar to get_dbuser_dbpass_args( $assoc_args )), and then feed the resulting array of arguments into the run() or run_query() method.

Show resolved Hide resolved src/DB_Command.php Outdated
@abhijitrakas

This comment has been minimized.

Copy link

abhijitrakas commented Nov 15, 2018

@schlessera Ok, cool. So we allow only flags which are mention in MySql Doc check Table 4.12 mysqlimport Options table. Correct me if I am wrong.

@schlessera

This comment has been minimized.

Copy link
Member

schlessera commented Nov 15, 2018

@abhijitrakas No, we are actually using the mysql binary, not the mysqlimport one. So the options would be the ones documented here: https://dev.mysql.com/doc/refman/8.0/en/mysql-command-options.html

@abhijitrakas

This comment has been minimized.

Copy link

abhijitrakas commented Nov 22, 2018

@schlessera Please check updated PR.

Abhijit Rakas
Show resolved Hide resolved src/DB_Command.php Outdated
Show resolved Hide resolved src/DB_Command.php Outdated
Show resolved Hide resolved src/DB_Command.php Outdated
Show resolved Hide resolved src/DB_Command.php Outdated
Show resolved Hide resolved src/DB_Command.php Outdated
Show resolved Hide resolved src/DB_Command.php Outdated
Abhijit Rakas

@schlessera schlessera added this to the 2.0.1 milestone Dec 3, 2018

@schlessera

This comment has been minimized.

Copy link
Member

schlessera commented Dec 3, 2018

Thanks for the PR, @abhijitrakas !

@schlessera schlessera merged commit 99188aa into wp-cli:master Dec 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@schlessera schlessera changed the title Allow all mysql flags while importing database Support all `mysql` flags while importing database Dec 3, 2018

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