-
Notifications
You must be signed in to change notification settings - Fork 827
Forms: Fix decoding issue #44927
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
Forms: Fix decoding issue #44927
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 2 files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work fine, but I'm not sure I have an actual case to test. I tried adding weird characters and tilde things on the fields and responses, it all worked out.
Tests also pass.
I can't be sure how to test one of the real cases. Maybe we can get some feedback post content and paste it?
@CGastrell The way to test it would be to revert back to the point where we were creating V2 versions of the feedback. Then load up this PR and notice that you see them now. |
d1ed69f
to
679bd24
Compare
public static function normalize_unicode( $string ) { | ||
// Case 1: JSON-style escapes, e.g. "\u003cstrong\u003e" or "\ud83d\ude48" | ||
if ( strpos( $string, '\u' ) !== false ) { | ||
$decoded = json_decode( '"' . $string . '"' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could extract
if ( $decoded !== null && json_last_error() === JSON_ERROR_NONE ) {
return $decoded;
}```
into it's own method as we do the same in the following if?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. We can do this in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't make it fail. Tried with bogus words and country flags in unicode. Worked well.
Adds normalization for Unicode characters in feedback field values, especially for legacy and v2 feedback formats that may contain escaped or raw Unicode sequences. Updates Feedback_Field and Feedback classes to support this, introduces new parsing logic for v3 feedback, and adds comprehensive tests to ensure correct handling of special characters and emojis.
679bd24
to
1fd0210
Compare
This PR fixed the endocing issue by making sure that v2 version of the encoding gets.
The problem was that in V2 unicode strings such as special characters (emojies or languages such as japanse )
stripped the / from the content. This made it so that we don't know that the characters were unicode when the conversion happened in decode_json.
Fixes FORMS-271
Proposed changes:
addslashes()
before saving the content in V3Other information:
Jetpack product discussion
p1755930976939539-slack-C086RGTJT1D
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Do the tests pass?
On a post with special characters. Do you see the content as expected now?