Skip to content
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

Smarty v5 built-in json_encode modifier doesn't take Smarty $_CHARSET encoding into account, and can't be overridden using registerPlugin() #1011

Open
cmanley opened this issue May 6, 2024 · 18 comments

Comments

@cmanley
Copy link

cmanley commented May 6, 2024

I manage a couple of websites, some of which use cp1252 encoding.
It's in these websites, the json_encode modifier sometimes returns false when given cp1252 encoded data.
This used to work with Smarty v3 and v4 where I registered my own json_encode modifier that does the input encoding conversion automatically based on the Smarty $_CHARSET encoding.
After some debugging, I noticed that my custom and registered modifier isn't being called at all in v5, but instead the built-in Smarty json_encode() modifier is being used.

I've tried to unregister the built-in json_encode modifier first before registering my custom modifier but that doesn't seem to have any effect:

$smarty->unregisterPlugin('modifier',  'json_encode');
$smarty->unregisterPlugin('function', 'json_encode');
$smarty->registerPlugin('modifier', 'json_encode', [$this, 'modifier_json_encode']);

If the json_encode modifier is to ignore the $_CHARSET encoding by design, then how do I override a built-in modifier/function with a customized one in v5?

@wisskid
Copy link
Contributor

wisskid commented May 6, 2024

You can find clues on how to do this in #1006

@wisskid
Copy link
Contributor

wisskid commented May 6, 2024

TL;DR. Create a custom Extension. Register it before the BCPluginsAdapter Extension.

@wisskid
Copy link
Contributor

wisskid commented May 6, 2024

But if you consider this to be a bug in the default implementation, we should probably fix that instead?

@cmanley
Copy link
Author

cmanley commented May 6, 2024

Thanks. I'll give the setExtension() route a try. I wish it were somewhat easier though.
B.t.w. I consider it a bug, but it also depends on what you consider it to be.

@wisskid
Copy link
Contributor

wisskid commented May 6, 2024

I'd be happy to have a look, but I need some more specifics. Can you share a reproduction scenario and your fixed version of the modifier?

@cmanley
Copy link
Author

cmanley commented May 6, 2024

Wow, I got it to work using 2 anonymous classes.
$this is in a child class of \Smarty\Smarty here:

		parent::__construct();
		$this->setExtensions([
			new \Smarty\Extension\CoreExtension(),
			new class extends \Smarty\Extension\Base {
				public function getModifierCompiler(string $modifier): ?\Smarty\Compile\Modifier\ModifierCompilerInterface {
					if ($modifier == 'json_encode') {
						return new class extends \Smarty\Compile\Modifier\Base {
							public function compile($params, \Smarty\Compiler\Template $compiler) {
								return 'json_enc(' . $params[0] . ')';	# my custom version of json_encode()
							}
						};
					}
					return null;
				}
			},
			new \Smarty\Extension\DefaultExtension(),
			new \Smarty\Extension\BCPluginsAdapter($this)
		]);

Now, if only there were an easier API :)
Should you want to see how I encode the data before json_encode(), I can paste it or send that to you.

@cmanley
Copy link
Author

cmanley commented May 6, 2024

Thanks. I sent my custom version of the json_encode modifier to your support@ email address.

@wisskid
Copy link
Contributor

wisskid commented May 6, 2024

Uhm. What address is that? 🤔

@cmanley
Copy link
Author

cmanley commented May 6, 2024

At the top of this page where the envelope is: https://github.com/iwink

@wisskid
Copy link
Contributor

wisskid commented May 6, 2024

I got a hold of your email. It seems to me you are fixing something in your templates that ought to be fixed before the data is passed to Smarty in the first place. Wouldn't it be a lot easier to convert your input data to the correct charset before you assign it to a Smarty variable?

@cmanley
Copy link
Author

cmanley commented May 7, 2024

In my opinion no. The use of Smarty isn't necessarily the final stage in a process flow. That means the data being read by Smarty, may also be used after Smarty has done it's job, or by other template parts, or even the session write handler (not in my case, but it's possible).

From what I can see in v5, within the domain of Smarty, \Smarty::\Smarty::$CHARSET defines the expected encoding of all data to be processed by Smarty. This is somewhat subjective and what I interpret as the 'correct charset' within the Smarty domain. That encoding (charset is actually a misnomer) is passed to all the mb*(), htmlentities() etc. functions Smarty calls for example. It would be strange to make an exception for a single Smarty modifier (json_encode in this case). While it is true that JSON is UTF-8 by definition, that has no bearing on the data to be encoded.

Within the broader application domain I use the ini setting default_charset as the application encoding and assign mb_internal_encoding() to that during start up, mysql database connections use SET NAMES to transcode (if necessary) to/from the application's encoding, and Smarty's static $_CHARSET is assigned that encoding too before construction. Thankfully, Smarty v5 doesn't change mb_internal_encoding() anymore as previous versions did.

But to get back to the 2 issues in the title:

  1. Is it possible to make registerPlugin() override existing built-in plugins having the same name (as was the case prior to v5), or if not, then at least throw an exception so that users don't incorrectly assume that their plugin is being used.
  2. For modifiers such as json_encode (it's probably the only one) that only accept UTF-8 input, then automatically convert the encoding of the template variable being passed to it from \Smarty::\Smarty::$_CHARSET to UTF-8 first (if they differ) ?

For now my work around was rename my custom modifier json_encode to to_json. I chose this route above the large setExtensions chunk of code above because the prior is less likely to break in future major version updates.

@wisskid
Copy link
Contributor

wisskid commented May 7, 2024

Ah I think I see what you mean now.

So, basically what you are saying is that you are running Smarty in another charset and all input data is passed to Smarty in that charset, but json_encode doesn't work properly because it expects UTF-8 no matter what?

How about the output of json_encode? We won't have to convert it back to the original charset?

@cmanley
Copy link
Author

cmanley commented May 7, 2024

Yes exactly. The output of json_encode must remain UTF-8 because that is in the definition of JSON. That is a known fact that the template user should be aware of. The output is typically fed into javascript which should parse it anyway without problems, since the default behavior of json_encode if you don't pass any flags in the 2nd argument, is to escape multibyte characters as \uXXXX.

@wisskid
Copy link
Contributor

wisskid commented May 7, 2024

Ah perfect. Would you like to create the PR?

@cmanley
Copy link
Author

cmanley commented May 7, 2024

I don't mind doing so, but I'd have to dive into the code to figure out where to place the transcoding helper function/method (that I mailed you) and where/how to write the new json_encode modifier, but that costs time which I don't have much of at the moment. If you know where/how already, then you can create the PR too if you like. Otherwise I'll give it a try later when I have some spare time.

@cmanley
Copy link
Author

cmanley commented May 7, 2024

I've taken a quick look at the Smarty code and I think I know what needs to be done to fix json_encode. Please tell me if I'm on the right track:
In Smarty\Extension\DefaultExtension, move json_encode from \getModifierCompiler() into getModifierCallback(), and define a new public function smarty_modifier_json_encode(...)
Remove JsonEncodeModifierCompiler.php.
In functions.php, add the helper function I sent you for deeply transcoding data of any encoding into UTF-8.
That'll be called from smarty_modifier_json_encode() if the \Smarty\Smarty:$_CHARSET != 'UTF-8'.
Is that correct? I assume functions.php is where all the generic helper functions reside.
If that's correct, then I'll make a PR.

@wisskid
Copy link
Contributor

wisskid commented May 7, 2024

Mostly correct, but just add your modifier as a method to DefaultExtension. I try to avoid polluting the global namespace and have been reducing the amount of global functions. What is left in functions.php wasn't easy to remove yet, but may be in the future.

@cmanley
Copy link
Author

cmanley commented May 14, 2024

I just created the PR: #1016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants