add missing rawurlencode

lekoala commented 6 months ago

Description

Encodes toast messages, which will be decoded again on the other end

Things still need to be decoded properly though, but that's the first step. Another PR is coming up for the js side of things.

Outside the scope of this pr: it would probably make a lot of sense to standardize these calls instead of having the burden on the developer to remember to actually encode this stuff. rawurlencode is used everywhere anywhay so that's probably worth it!

Manual testing steps

Issues

https://github.com/silverstripe/silverstripe-admin/issues/1639

Pull request checklist

[x] The target branch is correct
- See picking the right version
[x] All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
- Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
[x] The commit messages follow our commit message guidelines
[x] The PR follows our contribution guidelines
[x] Code changes follow our coding conventions
[x] This change is covered with tests (or tests aren't necessary for this change)
[x] Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
[x] CI is green

GuySartorelli commented 6 months ago

Hiya, First off, thank you for this contribution.

We added a new pull request template to this repository yesterday - but it looks like your PR description isn't using it. Did the template show up when you created this pull request?

lekoala commented 6 months ago

hi @GuySartorelli , see here https://github.com/silverstripe/silverstripe-admin/pull/1640#issuecomment-1866899691 same explanation :)

GuySartorelli commented 6 months ago

I've added the template back in - please add any manual testing steps, or if there aren't any that are specific to this PR, please point back at the reproduction steps in the issue.

Please also tick all of the boxes that you have done, and do whatever is still not done based on the checklist.

Please also add some extra information to the description about whether this same encoding is done everywhere else toasts are sent, or if not, why this is being done in these specific places.

lekoala commented 6 months ago

done unit test not necessary, but maybe some e2e testing that can actually capture the toast message. that seems like a painful things to do in behat so i won't dive into that :-) i didn't read the guidelines but i'm pretty sure it's ok :-)

GuySartorelli commented 5 months ago

This looks generally okay. Can you please update the commit message to include FIX at the start (see commit message guidelines)?

Also, just for my own understanding, why do we need to encode the value at all, if we're decoding it in JS? Do header values not handle utf8 characters well?

lekoala commented 5 months ago

@GuySartorelli basically only ascii characters are supported by the specs in http headers. There is no guarantee that utf8 encoding will be respected in http headers (it's up to the browser). Therefore, you need to encode the headers. For some characters, it's not required, but for single quote for example, url encoding is needed. Or even more extreme cases like emojis.

lekoala commented 4 months ago

I took the initiative to align this PR with the behavior in CMSMain

https://github.com/silverstripe/silverstripe-cms/blob/cb46fd8aac90b6a1f08542b92cefac841f64a469/code/Controllers/CMSMain.php#L1779-L1795

This way, everthing works without any change on the js side of things

silverstripe / silverstripe-framework