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

Check for the presence of the MySQL binaries before calling them #389

Merged

Conversation

gwolf
Copy link
Contributor

@gwolf gwolf commented Jun 20, 2024

Fixes #388
Patch to verify for the presence of mysql and mysqldump (and gunzip, as it was in the same command construction) before actually calling it.

Thanks!

Copy link
Collaborator

@yorkshire-pudding yorkshire-pudding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gwolf

I haven't yet tested this (I don't have any environments without these on to test). I will run the automated tests again once the changes are made, but might also see if anyone else is able to test the negative conditions. Most changes are about using FALSE instead of null, and making the bee_message strings translatable. Also a function docblock required.

Thank you

commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
Use FALSE instead of null.
Write a proper docblock for the introduced function.
@gwolf
Copy link
Contributor Author

gwolf commented Jun 21, 2024

Thanks!
I think I have addressed the needed changes for the coding standards. Sorry, I'm not used to PHP development, and of course, am completely new to this project.

Aren\'t we a bit over-pedantic? 😉
@gwolf
Copy link
Contributor Author

gwolf commented Jun 21, 2024

All tests passed! \o/

Copy link
Collaborator

@yorkshire-pudding yorkshire-pudding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @gwolf - a bit more to do. After thinking about the name of the helper function, I've decided it would be better to go into includes/filesystem.inc. There's also a few tweaks with the function doc bloc and variable names.

commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
Change variable names to full words instead of abbr
Rename introduced function to a more project-wide descriptive one
Move introduced function to the filesystem includes
@yorkshire-pudding
Copy link
Collaborator

Thanks @gwolf - this is looking good. I tested the negative condition by temporarily changing the defined executable in the db_export_bee_callback() function so it couldn't be found. The function works well.

Thank you for your patience in getting this over the line.

@yorkshire-pudding yorkshire-pudding merged commit f59c259 into backdrop-contrib:1.x-1.x Jun 24, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does not verify whether the MySQL client is installed
2 participants