Drop support for using PHP functions as modifiers

[wisskid]

In order to be able to provide a stable interface, we should drop support for using PHP functions (actually, any callable) as a modifier and require that any modifier is either part of Smarty by default or added as a plugin. If not, we will continue to have to chase every change PHP decides to make and wrap it in backwards compatibility layers.

[wisskid]

#814 was incorrectly marked to close this. It's only the first step.

[timmit-nl]

Hi,

We see the deprecated notices and we see we use a small list not jet covered:
'array_reverse',
'array_slice',
'empty',
'floatval',
'htmlentities',
'is_array',
'json_encode',
'md5',
'print_r',
'strpos',
'strstr',
'substr',
'ucfirst',

Some need to be changed in the templates to use for example {md5($var)} instead of {$var|md5} but some we just need or make sense as modifier.

What is the best practice to make it ("modifier" vs "modifiercompiler")? Then we can make some and push those to smarty.

[wisskid]

You can just register them as custom modifier plugins, no need to push them to Smarty.

[timmit-nl]

I understand we can make them as custom privately only for us (we already do that for other modifiers), but sharing is caring.

[wisskid]

That is much appreciated, but part of the reason we are doing this is to reduce the responsibility of Smarty to maintain backward compatibility on all of PHPs functions. If we add these wrappers, this responsibility is reintroduced too.

[timmit-nl]

I understand your standpoint, but some of these modifier are normal to use in templates. Thinking of:
'substr',
'ucfirst',

So completly remove those as default is cripling smarty in my opinion.

(edditted after studing our codebase)

[lkppo]

@timmit-nl

I never used 'substr' and 'ucfirst'. i don't see them as important.

And the correct thing should be 'mb_substr' in some case, so it's a bad idea to reintroduce them.

Registering is ok.

[scottchiefbaker]

I agree with @wisskid that these are better served as separate modifier plug-ins. Perhaps a contrib folder could be added to the repo for third party (non-supported) add-ons?

[xtcodecom]

I agree with @wisskid that these are better served as separate modifier plug-ins. Perhaps a contrib folder could be added to the repo for third party (non-supported) add-ons?

+1
I hope they will provide a separate repo with all modifiers.
Really needed for legacy projects.

Anyway, this update is confusing.

[alexgit2k]

libs/debug.tpl still contains "|md5"

[AdrienPoupa]

I struggled a bit to identify which functions would trigger this warning so I wrote a regex to match custom modifiers, excluding built-in modifiers, in case anybody finds it useful

\{(.+(?=\|))\|*(\b(?!capitalize|count|date_format|debug_print_var|escape|explode|mb_wordwrap|number_format|regex_replace|replace|spacify|truncate|cat|count_characters|count_paragraphs|count_sentences|count_words|default|escape|from_charset|indent|lower|nl2br|noprint|round|str_repeat|string_format|strip|strip_tags|strlen|to_charset|unescape|upper|wordwrap|trimwhitespace\b)[A-Za-z0-9_:$'->?]+)\}

https://regexr.com/7413e

[scorninpc]

This is nice change, but i think the error maybe can show caller. It's a problem with large/dynamics layouts

[ppp0]

I'm not sure but shouldn't adding a SmartySecurity policy with php_modifiers = [...$modifier] mark a modifier as trusted and thus exclude it from the deprecation note? Here, any trusted modifier is throwing the exception: https://github.com/smarty-php/smarty/pull/814/files#diff-0715c61a907d5b514aff3ea91f145275090d21e72496f68f298b7a7564500c06R110

Shouldn't lines 109-111 read

if (!is_object($compiler->smarty->security_policy)
                                || !$compiler->smarty->security_policy->isTrustedPhpModifier($modifier, $compiler)
                            )

?

[ophian]

@ppp0 I agree with you adding the "!" for the current release, ...even if I think it is intentional, as stated by wisskid.

But I still think the main issue on this is that Smarty should not think that it has to be responsible about user code that does not match newer PHP version deprecations or removes, e.g. by adding (string) casts to string parameters, etc.

All these default PHP functions as modifiers in use by Smarty default and as custom trusted PHP modifiers should still be usable and should not be marked as deprecated to "reduce the responsibility of Smarty to maintain backward compatibility on all of PHPs functions", when not being a security issue or so. It is totally valid to use them as modifiers against functions because then they (may) better match template interpretor needs. This is one of the values that makes Smarty to be a good template engine.

IMHO Smarty is an excellent interpretor of PHP as a template engine and not there to fix up the currently used framework code for PHP versions (getting better and more strict by types etc.).
So in my eyes this deprecation and some other latterly added Smarty type fixes for PHP 8+ are the wrong way, because they suppress the message. We better have to say to the users: Fix up your templates and frameworks to match newer PHP versions getting better. Smarty is going to throw PHP messages when they are valid and will appear in plain PHP also too.
So my believe on this is:
If there really is a urgent need to support old "forgiving" code usage, the framework using Smarty as its template engine has to care about workarounds or custom errorhandlers themselves. Otherwise they (the developers, if they know about it) probably just go the lazy way until the removal is getting into effect!

This is much more straithforward and Smarty will not be responsible to support wrong or outdated code out there over the time.

Please forgive this "meta" here. I just thought it might be useful to bring Smarty into future. 🙂

[wisskid]

@ophian I used to feel the same way, but now I feel otherwise. People writing Smarty templates are not writing PHP code. They might even be unaware of what happens under the hood.

Also, if you have tooling (such as PhpStorm) helping you to upgrade from one PHP version to the next, it will not warn you about Smarty modifiers.

Therefore, I feel Smarty is a language in its own right and should define its own rules.

[ophian]

Just to say this before, I don't want to start a debate on things you don't want to have.
Maybe there are more Developers like me that would just like to add their view of (future) usage.

Why should it warn about Smarty modifiers? Translated to PHP these are just plain functions/methods. The good thing is that Smarty supports using them as modifiers, in example for |sprintf:'foo':'bar', which is much easier to understand for a non PHP savvy than using it as a function, because they all run this way, like |escape:pa:ra:me:ters a.s.o. You can say, well in this case the framework has to define custom registered modifiers as I said and all is well.

But this is - like already happened in this thread - just more work, and people ask for repositories to copy from. Currently or until now we had some defaults and were able to add some more trusted custom modifiers per name as well. This was easy, secure and extendable. These allowed modifiers were parsed by Smarty to render to functions. Easy and in one hand! Now this would move into userland and start to defrag. What is the real benefit ? Is it faster, ...is it better ?

Frameworks using Smarty are build for templates/themes. They know what they need and deliver some core themes to start learning from for Newbies building custom copies or even new templates. So in my experience I have seen more people knowing or learning PHP building themes, than plain theme Designers doing so. And if the latter did, they followed the rules of Smarty documentation and the frameworks documentation and needs. Using Smarty on its own as a language outside a framework is not a real use case, for me... (unless I am mistaken).

[ppp0]

The solution was pretty easy: just wrap the incriminated functions in a Smarty modifier plugin. Thanks for having a look

[ophian]

I agree with you adding the "!" for the current release, ...

No, thats a bad idea. Just @ silence the trigger_error().

[wisskid]

It would be extremely easy to create a "wildcard" extension that will allow you to call any callable PHP function as a modifier in 5.0. For example:

class WildcardExtension extends \Smarty\Extension\Base {

	public function getModifierCallback(string $modifierName) {
		if (is_callable($modifierName)) {
			return $modifierName;
		}
		return null;
	}

}

and then:

$this->smarty->addExtension(new WildcardExtension());
$this->smarty->fetch('string:{"blah"|substr:1:2}');
$this->smarty->fetch('string:{"blah"|ucfirst}');
$this->smarty->fetch('string:{"blah"|md5}');

all will work.

[scottchiefbaker]

@AdrienPoupa how did you use that regexp to find things in your codebase?

[AdrienPoupa]

@AdrienPoupa how did you use that regexp to find things in your codebase?

Search everywhere with PHPStorm ;)

[LDerkzenSawiday]

@wisskid

As I understand from the first comment on this PR is that support for using PHP functions as modifiers will be dropped. However, after seeing #871 I'm not clear on what exactly is affected by this change. As it looks like md5() isn't used as a 'modifier' in #871.

Could you please clarify which uses of PHP functions would fail in the future?

[wisskid]

@LDerkzenSawiday the goal is to be able to provide a stable interface. Therefore we will drop support for using PHP functions (actually, any callable) in templates and require that it is either part of Smarty by default or added as a plugin.

After #814, Smarty is now throwing deprecation notice when using unregistered PHP functions as modifiers. I will check if using them as function in expressions also throws a notice. If not, we'll have to add that.

(Update: it does not.)

[OhBenHof]

I really don't understand the reasoning for any of this and the stated logic by wisskid is equally fallacious. wisskid stated that he didn't want to allow PHP modifiers because Smarty would have to maintain backwards compatibility for updated functions. This is a wish on the part of wisskid, not a technical requirement or concern.

@ophian ophian stated: "IMHO Smarty is an excellent interpretor of PHP as a template engine and not there to fix up the currently used framework code for PHP versions"

wisskid replied: "@ophian I used to feel the same way, but now I feel otherwise. People writing Smarty templates are not writing PHP code. They might even be unaware of what happens under the hood."

So, the answer is to make people who can't code enough to know what is going on under the hood to make their own custom plugins and register them with Smarty? So, because they can't code, we'll make them code more?

It should at the very least be something we can turn on and off like changing the template directory. It's our code that we let you play in. We should have complete autonomy over how we work with Smarty. Don't babysit your users.

[wisskid]

@OhBenHof I don't think it's fallacious at all. The goal is actually to provide a stable interface, so they'll have to change code less in the future.

It's not just a "wish" either: actual bugreports have been filed for changes in PHP function signatures. And other template engines follow the same principles.

Registering a php function is an extremely simple one-liner. You don't have to write a plugin. And if that's too complex for you, just don't upgrade then. You'll be fine.

[scottchiefbaker]

I'm slightly hesitant on this change too. What other template engines have made this change?

[wisskid]

Twig decided to never implement support for PHP functions. For Python, Django and Jinja don't support it either. Not aware if they have supported it in the past.

[scottchiefbaker]

What PHP functions have changed signatures that prompted this change?

[wisskid]

strftime is deprecated in php8.1, join no longer supports parameters switched out, many php functions throw warnings when called with unexpected params such as null since php8.0.

[kenorbi87]

if i use

$this->smarty->addExtension(new WildcardExtension());

there is a memory leak in smarty, my memory usage just goes up

[wisskid]

@kenorbi87 are you repeatedly adding the extension? You're supposed to only add it once.

[kenorbi87]

Thank you for your response, i have a static class that returns a smarty refference like this:

// App.php
<?php 

use \Smarty\Smarty;

class App
{
  public static function getSmarty()
  {
      static $smarty = null;
  
      if (is_null($smarty)) {
          $smarty = new Smarty();
          $smarty->addExtension(new \SmartyCallableFunctions());
      }
  
      // Set template dir and stuff...
  
      $smartyRef =& $smarty;
  
      // return smarty ref
      return $smartyRef;
  }
}

// index.php
<?php

// ...
$smarty = App::getSmarty();

// assign variable + display

the code it's working, all php functions, are working, but memory inceeeses and never drops

[kenorbi87]

I have a screenshot with memory usage before and after switching to smarty5

[https://drive.google.com/file/d/1Jw_4qA1NApfVbuWejgp1AnyZWDMjvAM3/view?usp=sharing]

any ideas?

[wisskid]

@kenorbi87 not exactly. You might want to drop the smarty ref, just return $smarty;. That's not useful anymore since PHP7. But otherwise, this should be fine.

[kenorbi87]

@wisskid i've removed the ref, nothing changed, if you take a look at the memory usage diagram here

https://drive.google.com/file/d/1Jw_4qA1NApfVbuWejgp1AnyZWDMjvAM3/view?usp=sharing

the memory usage is only increeasing after we implemented smarty5.

Smarty was installed via composer, do you have any idea how to deal with increased memory usage?

[wisskid]

@kenorbi87 I've tried to reproduce this using your code, creating a simple forever running while loop that displays a single template that uses a callback modifier. But memory usage is constant. Can you create a simplified reproduction scenario that triggers a memory build-up?

[wblessen]

@kenorbi87 Maybe the cause lies in the reference in the static class?

I am always working like this, to have one instance:

// App.php
 <?php 
 
 use \Smarty\Smarty;
 
 class App
 {
     protected $smarty = null;
     public  function smarty()
    {
       if (is_null($this->smarty)) {
           $this->smarty = new Smarty();
           $this->smarty->addExtension(new \SmartyCallableFunctions());
       }
       return $this->smarty;
   }
 }
 
// index.php
<?php
 $smarty = App->smarty();
 // assign variable + display

[kenorbi87]

Hi,
we did a lot of experiments, upgrades for other modules, and the final result is that smarty 5 has a memory leak. We needed to restart our server once every 2-3 days because exponencially increesing memory usage.
So we downgraded to smarty 4.3.4 and memory usage came back to normal values (about 7% constant).

memory usage with smarty 5:

memory usage with smarty 4.3.4:

[wisskid]

@kenorbi87 I've tried to reproduce this using your code, creating a simple forever running while loop that displays a single template that uses a callback modifier. But memory usage is constant. Can you create a simplified reproduction scenario that triggers a memory build-up?

@kenorbi87 please see my previous reply. If you have a example causing the leak, I'll be happy to have a look.

[ccerrillo]

I struggled a bit to identify which functions would trigger this warning so I wrote a regex to match custom modifiers, excluding built-in modifiers, in case anybody finds it useful

\{(.+(?=\|))\|*(\b(?!capitalize|count|date_format|debug_print_var|escape|explode|mb_wordwrap|number_format|regex_replace|replace|spacify|truncate|cat|count_characters|count_paragraphs|count_sentences|count_words|default|escape|from_charset|indent|lower|nl2br|noprint|round|str_repeat|string_format|strip|strip_tags|strlen|to_charset|unescape|upper|wordwrap|trimwhitespace\b)[A-Za-z0-9_:$'->?]+)\}

https://regexr.com/7413e

I have modified your regular expression because it did not take into account chained modifiers, that is, something like this: {$var|intval|default:null}

\|(?!(\$|capitalize|count|date_format|debug_print_var|escape|explode|mb_wordwrap|number_format|regex_replace|replace|spacify|truncate|cat|count_characters|count_paragraphs|count_sentences|count_words|default|escape|from_charset|indent|lower|nl2br|noprint|round|str_repeat|string_format|strip|strip_tags|strlen|to_charset|unescape|upper|wordwrap|trimwhitespace)\b)([^|\s]+)

[vojtasvoboda]

Could you please add support for count to the v4 branch? It looks like, it will be supported in v5 (https://smarty-php.github.io/smarty/5.x/designers/language-modifiers/language-modifier-count/).

Using the example below:

{if count($myVar) > 3}4 or more{/if}

It gives me Using unregistered function "count" in a template is deprecated and will be removed in a future release. for Smarty 4.5.3.

[wisskid]

@vojtasvoboda fixed it in #1054