Skip to content

[9.x] Add zero-width space to trimmed characters in TrimStrings middleware - #44906

Merged
taylorotwell merged 2 commits into
laravel:9.xfrom
tomcodes:9.x
Nov 11, 2022
Merged

taylorotwell merged 2 commits into
laravel:9.xfrom
tomcodes:9.x

Conversation

@tomcodes

@tomcodes tomcodes commented Nov 11, 2022 •

Copy link
Copy Markdown
Contributor

I had a weird bug recently when adding a job to the queue when the content had a zero-width space present somewhere.

The bug I had was: Unable to JSON encode payload. Error code: 5

It took me a while to corner this issue to a zero-width space present in a user firstname.

I was wondering also if would be interesting to add a middleware to actually remove those characters also from within the fields instead of just the side. I am willing to create another pull request if that need can benefit more people.

Github does not show the diff, so here is a visual diff:

Before
image

After
image

@tomcodes

Copy link
Copy Markdown
Contributor Author

Also, I can try and add a test for this if necessary.

@tomcodes tomcodes changed the title Add zero-width space to trimmed characters in TrimStrings middleware [9.x] Add zero-width space to trimmed characters in TrimStrings middleware Nov 11, 2022
@taylorotwell

Copy link
Copy Markdown
Member

Yeah we would definitely want a test for this I think.

@taylorotwell
taylorotwell marked this pull request as draft November 11, 2022 14:46
@taylorotwell

Copy link
Copy Markdown
Member

Please mark as ready for review when it has a test.

@tomcodes

Copy link
Copy Markdown
Contributor Author

I tried to think of multiple situations with the tests, I hope it's fine like this!

@tomcodes
tomcodes marked this pull request as ready for review November 11, 2022 15:37
@taylorotwell
taylorotwell merged commit 5199140 into laravel:9.x Nov 11, 2022
@tomcodes

Copy link
Copy Markdown
Contributor Author

woops, I have noticed a tiny typo issue in the last test, should I open a new pull request?

The title should not be This title contains a zero-width non-breakable space at the end, it should be This title contains a combination of zero-width non-breakable space and zero-widh spaces characters at the begining and the end instead.

The test is still valid, it's just the title that does not fit the test name.

@driesvints

Copy link
Copy Markdown
Member

@tomcodes yes feel free to send in a new pr

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.

3 participants