smarty icon indicating copy to clipboard operation
smarty copied to clipboard

Necessity of mb_internal_encoding in Smarty::__construct() is questionable

Open AnrDaemon opened this issue 5 years ago • 2 comments
trafficstars

While working on a fix for #549, I did a short investigation of mbstring extension usage across Smarty codebase. What I've found is that in almost all cases, Smarty explicitly provides charset for relevant operations. Where it does not, the charset does not matter.

My main objection against the use of mb_internal_encoding in Smarty::__construct() is its shady nature. It silently changes the behavior of a whole application without affecting the behavior of Smarty itself, as it seems.

AnrDaemon avatar May 05 '20 17:05 AnrDaemon

@AnrDaemon how far are you on this one? If this requires making a breaking change (I could imagine some older projects implicitly relying on Smarty to change the internal encoding for them) we could introduce it in Smarty 4?

wisskid avatar Feb 07 '21 21:02 wisskid

This could be a breaking change for existing projects (not for Smarty itself) when templates using encoding that is not matching script encoding. But it could be fixed using PHP configuration or code change just prior to calling new Smarty().

If we are to go further on this one, for 4.0 I suggest making Smarty::$_CHARSET unavailable for changing, and set its value in constructor (passing explicitly or just read from mb_internal_encoding). This would force users to use same encoding for all templates (right now it could be different, which makes little sense, but possible) and won't change the rest of the library's behavior.

Running ag --php --color --pager=less -i '(?<!\.)\bmb_\w+' libs/ yield just a few places, where encoding is relevant, and none of them makes assumptions.

AnrDaemon avatar Feb 08 '21 12:02 AnrDaemon

I've removed mb_internal_encoding calls in v5.0.

wisskid avatar Jan 31 '23 10:01 wisskid