Version 3.4 broke undefined function callback support

[ju1ius]

Hi,

I'm using twig inside WordPress. As the number of WordPress templating functions is enormous, it is not humanly possible to redefine all of them as twig functions. Therefore, I use the registerUndefinedFunctionCallback() recipe mentioned in the docs:

$environment->registerUndefinedFunctionCallback(static function($name) {
    if (function_exists($name)) {
        return new TwigFunction($name, $name);
    }
    return false;
});

So that these kind of things work out of the box:

<footer>
  {% do do_action('wp_footer') %}
  {{ apply_filters('foo', bar) }}
</footer>

I know it's not a goot practice, but it is a very important feature when working on legacy applications. It allows integrating twig progressively without requiring a huge amount of work upfront.

Anyway, it used to work until version 3.3.10 (included). I could even compile the templates «offline» (without loading the whole WordPress core). After upgrading to version 3.4.1, compiling the templates throws an exception:

In CallExpression.php line 293:
  [TypeError]
  Failed to create closure from callable: function "do_action" not found or invalid function name

Indeed, we can see here that the compiler does:

$r = new \ReflectionFunction(\Closure::fromCallable($callable));

But the callable does not exist at that time, so a TypeError is thrown. In version 3.3.10, this kind of compilation errors were somehow converted to a Twig\Error, which was later catched in the TemplateCacheWarmer::warmUp() method in symfony/twig-bundle, and silently ignored so that the template could be compiled later, at runtime. In version 3.4, the thrown TypeError is not catched, so the cache:clear command fails.

[stof]

Well, if you were returning a TwigFunction for a non-existent callable in your registerUndefinedFunctionCallback callback, that would still have broken several features before (named argument support for instance).

Note that if you return a function name that exists, the support of the registerUndefinedFunctionCallback callback should not be broken.

It seems to me that all that reflection magic should fail gracefuly (as it did in previous versions) if a function does not exist at compile time (that's the point of the registerUndefined*Callback() methods right?).

registerUndefinedFunctionCallback is meant to be able to handle gracefully the case where no TwigFunction is registered for that name (either by providing one dynamically or by throwing a more meaningful error message explaining what prerequisites are needed to make it available for instance). But I don't think it was ever meant to support returning a TwigFunction based on a non-existent callable.

[ju1ius]

@stof OK I get your point, however until version 3.3.10, the fact that the callable does not exist at compile time caused the cache warmer of the twig bundle to just skip the template, and the template would be compiled later at runtime. See this comment in symfony/twig-bundle:

/*
 * Problem during compilation, give up for this template (e.g. syntax errors).
 * Failing silently here allows to ignore templates that rely on functions that aren't available in
 * the current environment. For example, the WebProfilerBundle shouldn't be available in the prod
 * environment, but some templates that are never used in prod might rely on functions the bundle provides.
 * As we can't detect which templates are "really" important, we try to load all of them and ignore
 * errors. Error checks may be performed by calling the lint:twig command.
 */

Now the cache:clear command fails with an exception....

[stof]

Well, maybe we should throw a Twig error in that case instead of the TypeError of PHP, to keep that skipping behavior.

[ju1ius]

That seems to be the issue indeed.

[fabpot]

@ju1ius Would you like to work on a PR?

[ju1ius]

@fabpot Sure, why not? I'm not super familiar with the codebase however. Where should the tests for that use case go?

[ju1ius]

After a bit of step-debugging, it seems that the issue comes from here: https://github.com/twigphp/Twig/blob/a1f8d3f91a0d28c258e81607e6711387a46f1705/src/Environment.php#L520 Since TypeError extends \Error, not \Exception it should catch \Throwable instead.

[ju1ius]

@ju1ius Would you like to work on a PR?

@fabpot Here you go!

[ju1ius]

After deeper investigations, it turns out the unexpected behaviour has nothing to do with undefined callback support. Therefore I'm closing this issue in favour of #3708.