-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add a command to drop and recreate the configured database #201
Add a command to drop and recreate the configured database #201
Conversation
Allow bee to bootstrap with an empty database
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ElusiveMind - I've left some feedback to help get this up to standard.
…dded flags for database export required for some configurations of MariaDB and MySQL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ElusiveMind
Thank you Michael. This is starting to look really good.
I've suggested an extra example to make clear that we can bypass the confirmation.
I've added two suggestions to remove the initialisation of the $pipes
array. While it is normally best practice to declare arrays, the PHP documentation page does not include this in any examples. Additionally, it was not used previously in bee and did not cause a problem that I am aware of and wherever possible we want to keep the coding consistent throughout bee, not changing patterns unless there is a very good reason.
Next step is to add a test to tests/backdrop/DBCommandsTest.php
; I think we want to check that the success message is received and as previously discussed wrap the test for this in an export and import so it is non-destructive. Probably want to remove the export once successfully imported.
@ElusiveMind - once you're back from your travels, we should look at getting this over the line. We're almost there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ElusiveMind
Hi Michael
Following a number of changes to the database operations, namely #261 , #280 and #282 I have revisited this and made a number of further suggestions.
It would be great to get this in and we're not far off. You can add each suggestion to a batch and then apply, or I can do this.
We also need a test for this and I don't think it should be too onerous. Feel free to have a go adding into tests/backdrop/DBCommandsTest.php
- I can always offer suggestions if not quite right.
Thanks for your patience and hard work on this.
533b0ed
into
backdrop-contrib:issue-202-drop-recreate-db
* Add a command to drop and recreate the configured database (#201) Merging into a branch so can add tests separately. Tests are not running (see #287) but have all run locally, and PHPCS issues all resolved so that test is passing * Added a command to drop the database. Allow bee to bootstrap with an empty database * Some re-factoring. Inclusion of confirmation on database drop. Also added flags for database export required for some configurations of MariaDB and MySQL. * Updates to descriptions before refactoring * Removed command logic * Apply suggestions from code review * Apply some PHPCS fixes --------- Co-authored-by: Martin Price <github@systemhorizons.co.uk> * Add tests to db-drop function (#291) --------- Co-authored-by: Michael Bagnall <ElusiveMind@users.noreply.github.com>
Fixes #202
Allow bee to bootstrap with an empty database