Fix support for named closures

[nicolas-grekas]

As spotted by @stof on symfony/symfony#46335 (comment)

[nicolas-grekas]

not needed because $class = \get_class($extension), which means aliases are already resolved

[nicolas-grekas]

PHP 7.1 means we can remove using call_user_func_array() and use an inline call instead.

[nicolas-grekas]

a reflection object is now always returned, even for magic methods

[stof]

This needs to add tests in FunctionTest showing that using Closure::fromCallable on an extension method actually compiles to the optimized $this->extensions['extension_class_name']->method() call rather than to $this->env->getFunction('foo')->getCallable() which triggers a full initialization of extensions.

[nicolas-grekas]

Test added.

[stof]

The tests you added don't cover the case of calling extension methods

[nicolas-grekas]

Test added :)

[fabpot]

Thank you @nicolas-grekas.

[LukeTowers]

@nicolas-grekas this PR has caused an issue with callables that point to Laravel facades in Winter CMS: wintercms/winter#557 (comment). Do you have any idea what would be causing that and how we can go about fixing it?

[nicolas-grekas]

It looks the code should be changed like this:

--- a/src/Node/Expression/CallExpression.php
+++ b/src/Node/Expression/CallExpression.php
@@ -305,8 +305,8 @@ abstract class CallExpression extends AbstractExpression
             $callable = [$object, $r->name];
             $callableName = (\function_exists('get_debug_type') ? get_debug_type($object) : \get_class($object)).'::'.$r->name;
         } elseif ($class = $r->getClosureScopeClass()) {
-            $callable = [$class, $r->name];
-            $callableName = $class.'::'.$r->name;
+            $callable = [$class->name, $r->name];
+            $callableName = $class->name.'::'.$r->name;
         } else {

Would you be available to send this with a test case, please?

[LukeTowers]

Off the top of my head, it would probably look something like this:

class CallStaticCallable
{
    public static function __callStatic($name, $arguments)
    {
        if ($name === 'magicMethod') {
            return 'mischief managed';
        }
    }
}

$callable = [CallStaticCallable::class, 'magicMethod'];

Is that enough for you to go off of @nicolas-grekas?

[nicolas-grekas]

That should definitely help. Would you be up to submit a PR with everything wrapped up?

[LukeTowers]

At the moment I don't have the time unfortunately, however perhaps @mjauvin, @damsfx, @WebVPF, or @arvislacis would be willing to as the issue is directly impacting them.

[bennothommo]

@nicolas-grekas here's a patch that has the full test case:

diff --git a/tests/Node/Expression/FilterTest.php b/tests/Node/Expression/FilterTest.php
index ef7c82cb..1a86e800 100644
--- a/tests/Node/Expression/FilterTest.php
+++ b/tests/Node/Expression/FilterTest.php
@@ -49,6 +49,7 @@ class FilterTest extends NodeTestCase
                 return [
                     new TwigFilter('foo', \Closure::fromCallable([$this, 'foo'])),
                     new TwigFilter('foobar', \Closure::fromCallable([$this, 'foobar'])),
+                    new TwigFilter('alias_bar', ['Twig_Test_Alias_Class', 'test']),
                 ];
             }
 
@@ -135,6 +136,10 @@ class FilterTest extends NodeTestCase
         $node = $this->createFilter($string, 'foobar');
         $tests[] = [$node, '$this->env->getFilter(\'foobar\')->getCallable()("abc")', $environment];
 
+        // aliased class
+        $node = $this->createFilter($string, 'alias_bar');
+        $tests[] = [$node, '$this->env->getFilter(\'alias_bar\')->getCallable()("abc")', $environment];
+
         return $tests;
     }
 
@@ -190,3 +195,31 @@ function twig_tests_filter_dummy()
 function twig_tests_filter_barbar($context, $string, $arg1 = null, $arg2 = null, array $args = [])
 {
 }
+
+// Sample class with a magic method
+class Twig_Test_Parent_Class
+{
+    public static function getAccessor()
+    {
+        throw new \Exception('Accessor not set');
+    }
+
+    public static function __callStatic($name, $arguments)
+    {
+        $accessor = static::getAccessor();
+
+        if ($name === 'test') {
+            return 'received ' . $arguments[0];
+        }
+    }
+}
+
+class Twig_Test_Child_Class extends Twig_Test_Parent_Class
+{
+    public static function getAccessor()
+    {
+        return 'child';
+    }
+}
+
+class_alias(Twig_Test_Child_Class::class, 'Twig_Test_Alias_Class');

It passes in the test case, in the sense that it creates the compiled getFilter call just fine - the issue would only manifest if one were to run the actual template.

What seems to happen is that the $r->getClosureScopeClass() call here returns the class as Twig_Test_Parent_Class. I assume this is because that is the class that will actually fulfill the call. The problem is calling that class directly would fail because of the getAccessor method, whereas calling it in the context of the child class (or the alias) would be fine normally. This is similar to how Laravel facades would work.

I hope that makes sense - let me know if you need any further info.

[stof]

Could you open an issue with the details instead of commenting on a merged PR ? It would avoid loosing track of the issue.