Skip to content

Conversation

@mrxkon
Copy link
Contributor

@mrxkon mrxkon commented Jan 12, 2021

Fixes #110

Adds functionality to remove test tables from the database during cleanup.

Fixes WordPress#110

Adds functionality via `mysqli` on functions & cleanup to remove test tables from the database.
new line eof
@getsource
Copy link
Member

Thanks much for the PR!

I took a look, and I think this wouldn't work as expected when the database is only accessible from the test environment (when the test environment is not the same as the prepare one).

You can see an example in cleanup.php of how it's done for removing files.

Maybe something similar to that would work?

@mrxkon
Copy link
Contributor Author

mrxkon commented Jan 15, 2021

Hey @getsource , sure yeah, that's why I mentioned it in the issue comment since I'm only working locally :D .

If mysql is in general accessible everywhere ( since we're either way using shell_exec and other direct cli access as well ) we can go for oneliners like mysql -u $user -p$pass $db -e "drop table if exists $table;" and just iterate with that as a direct shell (and overssh) command.

This would work both locally and remotely I suppose.

@getsource
Copy link
Member

Ah, sorry, I missed the issue comment!

Yes, I think something like that makes sense. The only thing that comes to mind is that I think that would result in the MySQL password being stored in logs.

Copy link
Contributor

@grooverdan grooverdan left a comment

Choose a reason for hiding this comment

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

could get into issues if ever $prefix needed SQL escaping, but seems unlikely.

mysql as a command generally got phased out of MariaDB-11.0+ an use mariadb instead.

MYSQL_PWD as an env variable for password still generally works, though could create a configuration file with [mysql]\npassword=.... and pass that to the command.

@kittenkamala
Copy link
Contributor

This might be mergable but I think it needs a rebase and to be tested again.

Copy link
Member

@Crixu Crixu left a comment

Choose a reason for hiding this comment

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

All looks good and straight forwards. As @kittenkamala mentioned we need some testers to run this to see if it still works

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

If cleanup.php doesn't fully wipe the DB, it can cause failures.

6 participants