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

Issue #181 - Fixes failure of database functions if special characters used #183

Merged
19 commits merged into from
May 19, 2022

Conversation

yorkshire-pudding
Copy link
Collaborator

Fixes #181 - Database functions fail if db_password contains special characters
Used the rawurlencode and rawurldecode that core uses

I've tested using the lando recipe in the module and they all pass (details below).

PHPUnit 8.5.26 #StandWithUkraine

Bee Core
✔ Bee is installed correctly
✔ Unknown command triggers an error
✔ Missing required argument triggers an error
✔ Commands can be called via an alias
✔ Root global option works
✔ Yes global option works
✔ Debug global option works

Cache Commands
✔ Cache clear command works

Config Commands
✔ Config get command works
✔ Config set command works
✔ Config export command works
✔ Config import command works

Cron Commands
✔ Cron command works

DBCommands
✔ Db export command works
✔ Db import command works

DBLog Commands
✔ Log command works

Download Commands
✔ Download command works
✔ Download core command works

Help Commands
✔ Help command works

PHPCommands
✔ Eval command works

Projects Commands
✔ Projects command works
✔ Enable command works
✔ Disable command works
✔ Uninstall command works
✔ Theme default command works
✔ Theme admin command works

Update Commands
✔ Update db command works

User Commands
✔ Users command works
✔ User password command works
✔ User login command works

Time: 43.26 seconds, Memory: 10.00 MB

OK (30 tests, 89 assertions)
PHPUnit 8.5.26 #StandWithUkraine

Multisite
✔ Site global option works

Multisite Install Commands
✔ Install command works

Multisite Download Commands
✔ Download command works

Time: 15.84 seconds, Memory: 10.00 MB

OK (3 tests, 20 assertions)

What I couldn't quite figure out is how to work with the `.lando.yml` and `setup.sh` script that calls the `install.sh` to test with a password that contains special characters

Perhaps this needs some dedicated tests writing?

I'll try and test on my hosting where I can more easily set a complex password than in lando

@yorkshire-pudding
Copy link
Collaborator Author

Tested on host.
Works with complex password in interactive mode but not in auto mode. I think this is because the parse_url function doesn't work with characters that are invalid as part of a URL, whereas with interactive we encode each element.
I came up with a different idea initially but thought parse_url was more efficient. I will revisit that previous idea as we want it to work with secure passwords in auto mode.
The other option is a breaking change and splitting the constituent parts of the db option into individual options so they can be encoded individually.

@yorkshire-pudding yorkshire-pudding marked this pull request as draft April 9, 2022 15:47
@yorkshire-pudding
Copy link
Collaborator Author

Tested this with the lando recipe

PHPUnit 8.5.26 #StandWithUkraine

Bee Core
✔ Bee is installed correctly
✔ Unknown command triggers an error
✔ Missing required argument triggers an error
✔ Commands can be called via an alias
✔ Root global option works
✔ Yes global option works
✔ Debug global option works

Cache Commands
✔ Cache clear command works

Config Commands
✔ Config get command works
✔ Config set command works
✔ Config export command works
✔ Config import command works

Cron Commands
✔ Cron command works

DBCommands
✔ Db export command works
✔ Db import command works

DBLog Commands
✔ Log command works

Download Commands
✔ Download command works
✔ Download core command works

Help Commands
✔ Help command works

PHPCommands
✔ Eval command works

Projects Commands
✔ Projects command works
✔ Enable command works
✔ Disable command works
✔ Uninstall command works
✔ Theme default command works
✔ Theme admin command works

Update Commands
✔ Update db command works

User Commands
✔ Users command works
✔ User password command works
✔ User login command works

Time: 49.68 seconds, Memory: 10.00 MB

OK (30 tests, 89 assertions)
PHPUnit 8.5.26 #StandWithUkraine

Multisite
✔ Site global option works

Multisite Install Commands
✔ Install command works

Multisite Download Commands
✔ Download command works

Time: 16.71 seconds, Memory: 10.00 MB

OK (3 tests, 20 assertions)

Also tested on host and this now works with complex password both auto and interactively.
The only (unlikely) condition I can see tripping this up, and would be tricky to accommodate is if someone put a : character in their database username.

@yorkshire-pudding yorkshire-pudding marked this pull request as ready for review April 9, 2022 18:19
@yorkshire-pudding yorkshire-pudding requested a review from a user April 9, 2022 20:36
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks @yorkshire-pudding. It took me a while to get my head around all this. Mainly because I didn't notice at first the different places where you used rawurlencode() and rawurldecode() (I thought they were all decoding, and that threw me for a while). But finally worked it out and now left some comments/requested changes for 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/install.bee.inc Outdated Show resolved Hide resolved
@yorkshire-pudding
Copy link
Collaborator Author

@BWPanda - As per our discussion, I've changed the code to split the database settings. I've also changed the test (multi-site) where it uses this command.

I've run tests using the updated lando recipe:

PHPUnit 8.5.26 #StandWithUkraine

Bee Core
✔ Bee is installed correctly
✔ Unknown command triggers an error
✔ Missing required argument triggers an error
✔ Commands can be called via an alias
✔ Root global option works
✔ Yes global option works
✔ Debug global option works

Cache Commands
✔ Cache clear command works

Config Commands
✔ Config get command works
✔ Config set command works
✔ Config export command works
✔ Config import command works

Cron Commands
✔ Cron command works

DBCommands
✔ Db export command works
✔ Db import command works

DBLog Commands
✔ Log command works

Download Commands
✔ Download command works
✔ Download core command works

Help Commands
✔ Help command works

PHPCommands
✔ Eval command works

Projects Commands
✔ Projects command works
✔ Enable command works
✔ Disable command works
✔ Uninstall command works
✔ Theme default command works
✔ Theme admin command works

Update Commands
✔ Update db command works

User Commands
✔ Users command works
✔ User password command works
✔ User login command works

Time: 41.96 seconds, Memory: 10.00 MB

OK (30 tests, 89 assertions)
PHPUnit 8.5.26 #StandWithUkraine

Multisite
✔ Site global option works

Multisite Install Commands
✔ Install command works

Multisite Download Commands
✔ Download command works

Time: 15.69 seconds, Memory: 10.00 MB

OK (3 tests, 20 assertions)

@yorkshire-pudding
Copy link
Collaborator Author

@BWPanda - please can you review the latest commit

@ghost
Copy link

ghost commented Apr 27, 2022

It's on my to do list.

@yorkshire-pudding yorkshire-pudding requested a review from a user May 10, 2022 20:11
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

A few things to change please.

commands/db.bee.inc Outdated Show resolved Hide resolved
commands/db.bee.inc Outdated Show resolved Hide resolved
commands/install.bee.inc Outdated Show resolved Hide resolved
commands/install.bee.inc Outdated Show resolved Hide resolved
@yorkshire-pudding
Copy link
Collaborator Author

Thanks @BWPanda for your feedback. I've implemented these.

commands/db.bee.inc Outdated Show resolved Hide resolved
commands/install.bee.inc Outdated Show resolved Hide resolved
commands/install.bee.inc Outdated Show resolved Hide resolved
@yorkshire-pudding
Copy link
Collaborator Author

@BWPanda - Done those.

Regarding checking the db name, are there any general principles about when you would test once and store in a variable versus checking the same test each time? I'm learning plenty, but sometimes I can't see why one approach would be favoured over another and would like to understand more.

@ghost
Copy link

ghost commented May 18, 2022

@yorkshire-pudding I understand where you're coming from. If it were performing a DB query or reading from config files, then yes, doing that once and then storing the value in a local variable would be better. But in this case, checking the DB name is just checking a variable (albeit an array rather than a string), so there's no real benefit storing this value in a second variable for later. Hope that makes sense...?

@yorkshire-pudding
Copy link
Collaborator Author

Thanks. That's a good explanation. Makes sense.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Sorry, looking more closely I found a few more things to change.

commands/db.bee.inc Outdated Show resolved Hide resolved
commands/install.bee.inc Outdated Show resolved Hide resolved
commands/install.bee.inc Outdated Show resolved Hide resolved
commands/install.bee.inc Outdated Show resolved Hide resolved
commands/install.bee.inc Outdated Show resolved Hide resolved
commands/install.bee.inc Outdated Show resolved Hide resolved
commands/install.bee.inc Outdated Show resolved Hide resolved
@yorkshire-pudding
Copy link
Collaborator Author

Hi @BWPanda - I have done those.

@ghost ghost merged commit b29e71d into backdrop-contrib:1.x-1.x May 19, 2022
klonos added a commit that referenced this pull request Jul 25, 2022
Hey @BWPanda and/or @yorkshire-pudding 👋🏼 ...I've noticed that this was missed when merging #183
@yorkshire-pudding yorkshire-pudding deleted the issue_181 branch October 3, 2022 14:48
This pull request was closed.
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.

Database functions fail if db_password contains special characters
1 participant