Fix possible json encoding error #13823
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
some API's in tests don't return a response cause they fail to json_encode because of `Malformed UTF-8 characters, possibly incorrectly encoded`. Tested this fix and it worked for me. Would maybe also need to create a general JSON class for this that always takes care of this in case we have same problem somewhere else? Not sure how good a fix it is as the input might not be utf8? In test case there was eg `/page/index.htm?q=non unicode keyword ��������";s:7:"segment";s:103:"pageUrl==http%3A%2F%2Fexample.org%2Fpage%2Findex.htm%3Fq%3Dnon+unicode+keyword+%EC%E5%F8%EA%EE%E2%FB%E5";}i` and it was converted to `??????`
tsteur
added
not-in-changelog
tsteur
commented
Dec 11, 2018
core/DataTable/Renderer/Json.php
Outdated
| $array[$key] = self::makeArrayUtf8($value); | ||
| } | ||
| } elseif (is_string($array)) { | ||
| return mb_convert_encoding($array, 'UTF-8', 'UTF-8'); |
There was a problem hiding this comment.
or maybe return mb_convert_encoding($array, 'UTF-8', 'auto'); is better?
diosmosis
reviewed
Dec 11, 2018
core/DataTable/Renderer/Json.php
Outdated
| @@ -73,9 +73,30 @@ protected function renderTable($table) | |||
| // silence "Warning: json_encode(): Invalid UTF-8 sequence in argument" | |||
| $str = @json_encode($array); | |||
|
|
|||
| if ($str === false && json_last_error() === JSON_ERROR_UTF8) { | |||
| $str = @json_encode(self::makeArrayUtf8($array)); | |||
There was a problem hiding this comment.
Maybe we shouldn't silence this second function call? Just so any other issues won't be silenced.
|
fyi we use slightly different version in https://github.com/matomo-org/matomo/blame/3.x-dev/core/DataTable/Renderer/Csv.php#L477 but same version for https://github.com/matomo-org/matomo/blame/3.x-dev/core/DataTable/Renderer.php#L188-L193 not sure if any is better... the one in data table renderer should be fine though I suppose |
|
Tests are passing, feel free to merge (not sure if you want to change to use the DataTable\Renderer code). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
some API's in tests don't return a response cause they fail to json_encode because of
Malformed UTF-8 characters, possibly incorrectly encoded. Tested this fix and it worked for me. Would maybe also need to create a general JSON class for this that always takes care of this in case we have same problem somewhere else? Not sure how good a fix it is as the input might not be utf8?In test case there was eg
/page/index.htm?q=non unicode keyword ��������";s:7:"segment";s:103:"pageUrl==http%3A%2F%2Fexample.org%2Fpage%2Findex.htm%3Fq%3Dnon+unicode+keyword+%EC%E5%F8%EA%EE%E2%FB%E5";}iand it was converted to??????