-
Notifications
You must be signed in to change notification settings - Fork 715
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
Fix for issue 1011: add 3rd input encoding parameter to json_encode modifier and let it default to \Smarty\Smarty::$_CHARSET #1016
base: master
Are you sure you want to change the base?
Conversation
…lt to \Smarty\Smarty::$_CHARSET
…n most common cases
…ial characters in both UTF-8 and other encodings
… in that encoding
…prevent other tests from failing.
src/Extension/DefaultExtension.php
Outdated
} | ||
elseif (is_array($value) || is_object($value)) { | ||
static $transcoder; # this closure will be assigned once, and then persist in memory | ||
if (is_null($transcoder)) { |
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.
You probably did this because of this comment but I'm afraid this is a bit much. Can you refactor the transcoder into a class file under src/Extension/DefaultExtension/DeepTranscoder.php
or something like that?
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.
Alright, perhaps DeepJsonTranscode.php then since it's specific to transcoding for json_encode() as indicated by a comment in the code where JsonSerializable is checked.
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.
I just pushed a new commit where the json_encode() modifier uses the new class src/Extension/DefaultExtension/RecursiveTranscoder.php, and added a little to the json_encode unit tests.
} # / elseif (is_array($value) || is_object($value)) | ||
} # / if input encoding != UTF-8 | ||
|
||
return \json_encode($value, $flags); # string|false |
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.
My IDE tells me that \json_encode (and \JsonSerializable::class) are not part of the core of PHP prior to PHP8. We should probably check for function_exists('json_encode')
and do something accordingly.
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.
It's strange that someone would want to use the |json_encode modifier without having the json extension installed. But what for exception (and message) to you suggest to throw if it's not present? The check can be done once (using a static boolean var) the first time the modifier is called.
…ltExtension/RecursiveTranscoder.php
See #1011