Add the Drush db-drop command

[ElusiveMind]

I have created Pull request #201 to add the db-drop command to Bee. This also allows Bee to be bootstrapped with an empty database for the purposes of creating tables. I welcome comments and feedback as to the approach.

[yorkshire-pudding]

Hi @ElusiveMind - it would be good to document here a bit more about any potential use cases for this. It's obviously a dangerous command so if we did include this I think it would benefit from an "Are you sure?" prompt.

Firstly, though, please could you explain the use case? I have used db-import to overwrite a database, so I'm not sure of the need.

[ElusiveMind]

Hey @yorkshire-pudding - db-drop clears out all the tables. You can (most times) do a db-import to over-write tables, but depending on how the export happened, it may or may not delete and re-create the tables. A db-drop makes sure all the old data is removed before importing new data making sure all old-data is removed.

[yorkshire-pudding]

Thanks @ElusiveMind - that's helpful

I presume you've tested that bee will still work to import the replacement database? What do you think to the idea of a prompt? Another thought is whether as part of the prompt the user could be given an option to export the existing database first?

[ElusiveMind]

In keeping things in line with drush, I did not do a prompt nor export the database. I could add those, but would those be redundant since there are already bee commands to do that? A confirmation prompt, I agree, could be useful. Let me know how I should proceed. I would like to work toward getting this merged for some projects I am working on so I am happy to make changes. I just want to agree on those changes. Peace.

[yorkshire-pudding]

@ElusiveMind - You're right about not duplicating export. Perhaps a confirmation yes or no? There are existing patterns for confirmation that allow the user to add a --yes or -y flag to automatically tick yes, so this would allow people to avoid the prompt if they want to.

[yorkshire-pudding]

Thank you @ElusiveMind - I've done some testing and it seems to work well. There are a few improvements in the code review. Testing and reviewing this has helped to see how useful this function could be. I've suggested a reworded description to be more precise about the potential use case.

[ElusiveMind]

I have made the requested changes. I also added one more. For db-export I was getting the semi-new mysql/mariadb error:

mysqldump: Couldn't execute 'SELECT COLUMN_NAME,                       JSON_EXTRACT(HISTOGRAM, '$."number-of-buckets-specified"')                FROM information_schema.COLUMN_STATISTICS                WHERE SCHEMA_NAME = 'backdrop' AND TABLE_NAME = 'batch';': Unknown table 'column_statistics' in information_schema (1109)

I added two new options: --column-statistics --no-tablespaces

I tried to add this as '--extra-dump' with the arguments in quotes, but something wasn't working with that due to the way command line parsing was happening.

That code is included for revision since it could be needed to do exports before running drops to test.

[yorkshire-pudding]

@ElusiveMind Hi Michael Something's not right here; I checked your changes, but you're working against quite an old version of the code (looking at your repo, you're 10 commits behind which includes changes to the db command that you will need). Before we can progress this you need to:

• pull down all the latest commits from the backdrop-contrib/bee/1.x-1.x branch
• open a separate issue for the export issue with clear steps to reproduce, expected results and actual results (I have never seen that kind of issue with the export function so we need to understand that better); remove the export changes from this.

I don't think you need to accept a yes argument in the db_bee_command() function as that is a global option. What you will need is to test $_bee_yes_mode and if so then not prompt for confirmation. While there doesn't seem to be any examples of using this, there are examples of getting input from users (install/cache-clear).

I agree that we'll need tests for db-drop that will need to be wrapped around with db-export and db-import but I tested these manually and there was no issue. We therefore need separate issue(s) for any feature requests and any bug reports so we can keep PRs focussed on one issue at a time.

Thank you for your work on this; I do think this will be a valuable addition to bee.

[ElusiveMind]

I've had to add one function for the dropping and creating of databases as per the new paradigm. I couldn't do this with the existing db_bee_mysql_options function because it would not be compatible with mysqldump.

/**
 * Returns a decoded options string for use with the mysql command.
 *
 * @param array $db_info
 *   Connection information for the current database. Generally obtained via
 *   Database::getConnectionInfo().
 * 
 * @param string $command
 *   A sql command to be run once the options have been passed in. Note that
 *   the database is omitted here so that this can run create and destroy
 *   queries. To specify database to run command in, utilize "use <tablename>;"
 *   before the rest of the command string.
 *
 * @return string
 *   A string of command options that can be appended to 'mysql'.
 */
function db_bee_mysql_command($db_info, $command) {
  $options = '--user=' . rawurldecode($db_info['username']);
  $options .= " --password='" .  rawurldecode($db_info['password']) . "'";
  $options .= ' --host=' . $db_info['host'];
  $options .= ' -e "' . $command . '"';

  return $options;
}

This then allows me to call it (with confirm checking):

  if (empty($_bee_yes_mode)) {
    // Prompt to continue.
    if (!bee_confirm(bt('Are you sure you want to drop the contents of !database ?', array(
        '!database' => $db_info['database']
      )), FALSE)) {
      return;
    }
  }

  // Drop the existing backdrop database as configured.
  $command = 'mysql ' . db_bee_mysql_command($db_info, 'drop database ' . $db_database);
  $result = proc_close(proc_open($command, array(STDIN, STDOUT, STDERR), $pipes));

Would like to get thoughts before I push the change up for review. I have removed the secondary code also.

[yorkshire-pudding]

@ElusiveMind Thanks Michael. I think we should probably keep it within the db-drop function as it starts to get very complicated; we need to factor in the rawurldecode() for the database name and there is no easy way to do that in a generic way that a helper function would need to do. I don't want to change db_bee_mysql_options in that way because if we allow commands that include the database name we will have issues if that name contains characters that should be decoded; it will break.

In db-drop you know the commands being done and can decode the db name before inserting it in. What I'd like to see is --user=' . $db_username . ' --password=' . $db_password . ' --host=' . $db_info['host'] replaced by $options and keeping $db_database = rawurldecode($db_info['database']); as it is so you can insert that into each statement.

$options could be built re-using the same code from db_bee_mysql_options():

  $options = '--user=' . rawurldecode($db_info['username']);
  $options .= " --password='" .  rawurldecode($db_info['password']) . "'";
  $options .= ' --host=' . $db_info['host'];

As $_bee_yes_mode is boolean, the test should be if (!$_bee_yes_mode)

Thanks

[ElusiveMind]

@yorkshire-pudding - Thanks for all the feedback. I think i've done the implementation as suggested. My test indicate it is working also. Let me know how I can best serve

[yorkshire-pudding]

@ElusiveMind - Hi Michael. I don't think we're too far off. There are a few bits of feedback on the PR.

Are you happy to continue? If you're short on time at the moment but you'd like to pickup in the near future, let me know. Also, if for some reason you don't wish to continue, let me know and I'll look at getting it the rest of the way.

Thanks for all your patience and work on this. Martin

[ElusiveMind]

Hey @yorkshire-pudding - I am happy to continue with this. I should have some more time after this weekend. I have a work deadline I need to make that is taking me 12-14 hours a day through at least Monday. Will that work?

[yorkshire-pudding]

Hi @ElusiveMind - no problem - there's no rush, I just wanted to know your plans.

[yorkshire-pudding]

I'm going to be revisiting this again as there have been quite a few changes to the way db commands work. Also, #280, when merged will removes the need to change miscellaneous.inc

Also, to note, in the #280 discussion, @indigoxela raised the point which is relevant to this feature that we shouldn't assume the user has both the DROP and CREATE permissions. I think we'll need to do something with SHOW GRANTS to test permission.

[yorkshire-pudding]

@ElusiveMind - see my comments on the PR.

Also, to note, in the https://github.com/backdrop-contrib/bee/issues/280 discussion, @indigoxela raised the point which is relevant to this feature that we shouldn't assume the user has both the DROP and CREATE permissions. I think we'll need to do something with SHOW GRANTS to test permission.

EDIT - See comment below - no need to do any checks for SHOW GRANTS.

I've done some investigation but can't see a simple way to do this. Therefore I propose that we do this ticket as is with my comments on the PR and also some tests. I'll then create a follow up ticket so the idea is not forgotten and perhaps someone who can workout how best to parse the output from SHOW GRANTS to test for these permissions can tackle that aspect. (Note - other tables are not suitable because the backdrop db user would not necessarily have access to them).

[indigoxela]

Based on our findings (problems with drop/create will be rather an edge-case) I think, there's no need for an extra check for GRANTs.

As far as I can see, the result is already evaluated (when dropping), so everything looks fine for me. (Note: I'll probably never need that command, as I prefer to use the mysql shell directly, when dropping and creating, so my opinion might not actually be relevant here.)

[alanmels]

Weren't the following commands:

bee sql-drop
bee sql-dump
bee sql-cli

introduced by commit on https://github.com/backdrop-contrib/bee/issues/129

[yorkshire-pudding]

Hi @alanmels It looks like sql-drop was in this commit

But I can't see any PR with that commit (even though you reference a PR) and perhaps that's why it got overlooked. The later PR by @BWPanda does not have this in. I wasn't on the project then, but I am sorry that your contribution didn't get added.

Thanks for your interest in bee - I hope you feel inspired to look at other tickets if you have time.