smarty-php / smarty

Smarty is a template engine for PHP, facilitating the separation of presentation (HTML/CSS) from application logic.
Other
2.22k stars 701 forks source link

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 2 months ago

cmanley commented 2 months ago

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 commented 2 months ago

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

wisskid commented 2 months ago

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

wisskid commented 2 months ago

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

cmanley commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

wisskid commented 2 months ago

Uhm. What address is that? 🤔

cmanley commented 2 months ago

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

wisskid commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Ah perfect. Would you like to create the PR?

cmanley commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

I just created the PR: https://github.com/smarty-php/smarty/pull/1016