From 163f07400a27376b3ccaa6d75cf3e163ed6fbf30 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Fri, 13 May 2022 14:02:43 +0200 Subject: [PATCH] Fix support for named closures --- .github/workflows/ci.yml | 44 ++----------- doc/filters/format_datetime.rst | 4 +- src/ExtensionSet.php | 2 - src/Node/Expression/CallExpression.php | 83 ++++++++++-------------- tests/Fixtures/functions/magic_call.test | 2 + tests/IntegrationTest.php | 1 + tests/Node/Expression/FilterTest.php | 25 ++++++- tests/Node/Expression/FunctionTest.php | 6 +- tests/Node/Expression/TestTest.php | 2 +- 9 files changed, 78 insertions(+), 91 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index df60b6a3d..266f6cd17 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,36 +26,23 @@ jobs: - '7.4' - '8.0' - '8.1' - composer-options: [''] experimental: [false] steps: - name: "Checkout code" - uses: actions/checkout@v2.3.3 + uses: actions/checkout@v2 - name: "Install PHP with extensions" - uses: shivammathur/setup-php@2.7.0 + uses: shivammathur/setup-php@v2 with: coverage: "none" php-version: ${{ matrix.php-version }} ini-values: memory_limit=-1 - tools: composer:v2 - name: "Add PHPUnit matcher" run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json" - - name: "Set composer cache directory" - id: composer-cache - run: echo "::set-output name=dir::$(composer config cache-files-dir)" - - - name: "Cache composer" - uses: actions/cache@v2.1.2 - with: - path: ${{ steps.composer-cache.outputs.dir }} - key: ${{ runner.os }}-${{ matrix.php-version }}-composer-${{ hashFiles('composer.json') }} - restore-keys: ${{ runner.os }}-${{ matrix.php-version }}-composer- - - - run: composer install ${{ matrix.composer-options }} + - run: composer install - name: "Install PHPUnit" run: vendor/bin/simple-phpunit install @@ -92,35 +79,22 @@ jobs: - 'extra/markdown-extra' - 'extra/string-extra' - 'extra/twig-extra-bundle' - composer-options: [''] experimental: [false] steps: - name: "Checkout code" - uses: actions/checkout@v2.3.3 + uses: actions/checkout@v2 - name: "Install PHP with extensions" - uses: shivammathur/setup-php@2.7.0 + uses: shivammathur/setup-php@v2 with: coverage: "none" php-version: ${{ matrix.php-version }} ini-values: memory_limit=-1 - tools: composer:v2 - name: "Add PHPUnit matcher" run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json" - - name: "Set composer cache directory" - id: composer-cache - run: echo "::set-output name=dir::$(composer config cache-files-dir)" - - - name: "Cache composer" - uses: actions/cache@v2.1.2 - with: - path: ${{ steps.composer-cache.outputs.dir }} - key: ${{ runner.os }}-${{ matrix.php-version }}-${{ matrix.extension }}-${{ hashFiles('composer.json') }} - restore-keys: ${{ runner.os }}-${{ matrix.php-version }}-${{ matrix.extension }}- - - run: composer install - name: "Install PHPUnit" @@ -129,10 +103,6 @@ jobs: - name: "PHPUnit version" run: vendor/bin/simple-phpunit --version - - if: matrix.extension == 'extra/markdown-extra' && matrix.php-version == '8.0' - working-directory: ${{ matrix.extension}} - run: composer config platform.php 7.4.99 - - name: "Composer install" working-directory: ${{ matrix.extension}} run: composer install @@ -158,10 +128,10 @@ jobs: steps: - name: "Checkout code" - uses: actions/checkout@v2.3.3 + uses: actions/checkout@v2 - name: "Install PHP with extensions" - uses: shivammathur/setup-php@2.7.0 + uses: shivammathur/setup-php@v2 with: coverage: "none" extensions: "gd, pdo_sqlite" diff --git a/doc/filters/format_datetime.rst b/doc/filters/format_datetime.rst index 5d438b6e4..6f2c8cfd9 100644 --- a/doc/filters/format_datetime.rst +++ b/doc/filters/format_datetime.rst @@ -30,8 +30,8 @@ You can tweak the output for the date part and the time part: Supported values are: ``none``, ``short``, ``medium``, ``long``, and ``full``. -For greater flexibility, you can even define your own pattern (see the `ICU -user guide`_ for supported patterns). +For greater flexibility, you can even define your own pattern +(see the `ICU user guide`_ for supported patterns). .. code-block:: twig diff --git a/src/ExtensionSet.php b/src/ExtensionSet.php index dc25b133b..deba573fb 100644 --- a/src/ExtensionSet.php +++ b/src/ExtensionSet.php @@ -149,8 +149,6 @@ final class ExtensionSet throw new \LogicException(sprintf('Unable to register extension "%s" as it is already registered.', $class)); } - // For BC/FC with namespaced aliases - $class = (new \ReflectionClass($class))->name; $this->extensions[$class] = $extension; } diff --git a/src/Node/Expression/CallExpression.php b/src/Node/Expression/CallExpression.php index 7de754a9d..6d105313c 100644 --- a/src/Node/Expression/CallExpression.php +++ b/src/Node/Expression/CallExpression.php @@ -24,21 +24,21 @@ abstract class CallExpression extends AbstractExpression { $callable = $this->getAttribute('callable'); - $closingParenthesis = false; - $isArray = false; if (\is_string($callable) && false === strpos($callable, '::')) { $compiler->raw($callable); } else { - list($r, $callable) = $this->reflectCallable($callable); - if ($r instanceof \ReflectionMethod && \is_string($callable[0])) { - if ($r->isStatic()) { + [$r, $callable] = $this->reflectCallable($callable); + + if (\is_string($callable)) { + $compiler->raw($callable); + } elseif (\is_array($callable) && \is_string($callable[0])) { + if (!$r instanceof \ReflectionMethod || $r->isStatic()) { $compiler->raw(sprintf('%s::%s', $callable[0], $callable[1])); } else { $compiler->raw(sprintf('$this->env->getRuntime(\'%s\')->%s', $callable[0], $callable[1])); } - } elseif ($r instanceof \ReflectionMethod && $callable[0] instanceof ExtensionInterface) { - // For BC/FC with namespaced aliases - $class = (new \ReflectionClass(\get_class($callable[0])))->name; + } elseif (\is_array($callable) && $callable[0] instanceof ExtensionInterface) { + $class = \get_class($callable[0]); if (!$compiler->getEnvironment()->hasExtension($class)) { // Compile a non-optimized call to trigger a \Twig\Error\RuntimeError, which cannot be a compile-time error $compiler->raw(sprintf('$this->env->getExtension(\'%s\')', $class)); @@ -48,17 +48,11 @@ abstract class CallExpression extends AbstractExpression $compiler->raw(sprintf('->%s', $callable[1])); } else { - $closingParenthesis = true; - $isArray = true; - $compiler->raw(sprintf('call_user_func_array($this->env->get%s(\'%s\')->getCallable(), ', ucfirst($this->getAttribute('type')), $this->getAttribute('name'))); + $compiler->raw(sprintf('$this->env->get%s(\'%s\')->getCallable()', ucfirst($this->getAttribute('type')), $this->getAttribute('name'))); } } - $this->compileArguments($compiler, $isArray); - - if ($closingParenthesis) { - $compiler->raw(')'); - } + $this->compileArguments($compiler); } protected function compileArguments(Compiler $compiler, $isArray = false) @@ -245,10 +239,7 @@ abstract class CallExpression extends AbstractExpression private function getCallableParameters($callable, bool $isVariadic): array { - list($r) = $this->reflectCallable($callable); - if (null === $r) { - return [[], false]; - } + [$r, , $callableName] = $this->reflectCallable($callable); $parameters = $r->getParameters(); if ($this->hasNode('node')) { @@ -275,11 +266,6 @@ abstract class CallExpression extends AbstractExpression array_pop($parameters); $isPhpVariadic = true; } else { - $callableName = $r->name; - if ($r instanceof \ReflectionMethod) { - $callableName = $r->getDeclaringClass()->name.'::'.$callableName; - } - throw new \LogicException(sprintf('The last parameter of "%s" for %s "%s" must be an array with default value, eg. "array $arg = []".', $callableName, $this->getAttribute('type'), $this->getAttribute('name'))); } } @@ -293,30 +279,33 @@ abstract class CallExpression extends AbstractExpression return $this->reflector; } - if (\is_array($callable)) { - if (!method_exists($callable[0], $callable[1])) { - // __call() - return [null, []]; - } - $r = new \ReflectionMethod($callable[0], $callable[1]); - } elseif (\is_object($callable) && !$callable instanceof \Closure) { - $r = new \ReflectionObject($callable); - $r = $r->getMethod('__invoke'); - $callable = [$callable, '__invoke']; - } elseif (\is_string($callable) && false !== $pos = strpos($callable, '::')) { - $class = substr($callable, 0, $pos); - $method = substr($callable, $pos + 2); - if (!method_exists($class, $method)) { - // __staticCall() - return [null, []]; - } - $r = new \ReflectionMethod($callable); - $callable = [$class, $method]; - } else { - $r = new \ReflectionFunction($callable); + if (\is_string($callable) && false !== $pos = strpos($callable, '::')) { + $callable = [substr($callable, 0, $pos), substr($callable, 2 + $pos)]; } - return $this->reflector = [$r, $callable]; + if (\is_array($callable) && method_exists($callable[0], $callable[1])) { + $r = new \ReflectionMethod($callable[0], $callable[1]); + + return $this->reflector = [$r, $callable, $r->class.'::'.$r->name]; + } + + $r = new \ReflectionFunction(\Closure::fromCallable($callable)); + + if (false !== strpos($r->name, '{closure}')) { + return $this->reflector = [$r, $callable, 'Closure']; + } + + if ($object = $r->getClosureThis()) { + $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; + } else { + $callable = $callableName = $r->name; + } + + return $this->reflector = [$r, $callable, $callableName]; } } diff --git a/tests/Fixtures/functions/magic_call.test b/tests/Fixtures/functions/magic_call.test index 4dd5e2704..f802de804 100644 --- a/tests/Fixtures/functions/magic_call.test +++ b/tests/Fixtures/functions/magic_call.test @@ -2,7 +2,9 @@ __call calls --TEMPLATE-- {{ 'foo'|magic_call }} +{{ 'foo'|magic_call_closure }} --DATA-- return [] --EXPECT-- magic_foo +magic_foo diff --git a/tests/IntegrationTest.php b/tests/IntegrationTest.php index bcadb3615..4bb727c03 100644 --- a/tests/IntegrationTest.php +++ b/tests/IntegrationTest.php @@ -168,6 +168,7 @@ class TwigTestExtension extends AbstractExtension new TwigFilter('static_call_string', 'Twig\Tests\TwigTestExtension::staticCall'), new TwigFilter('static_call_array', ['Twig\Tests\TwigTestExtension', 'staticCall']), new TwigFilter('magic_call', [$this, 'magicCall']), + new TwigFilter('magic_call_closure', \Closure::fromCallable([$this, 'magicCall'])), new TwigFilter('magic_call_string', 'Twig\Tests\TwigTestExtension::magicStaticCall'), new TwigFilter('magic_call_array', ['Twig\Tests\TwigTestExtension', 'magicStaticCall']), new TwigFilter('*_path', [$this, 'dynamic_path']), diff --git a/tests/Node/Expression/FilterTest.php b/tests/Node/Expression/FilterTest.php index ccac2ab82..21efa3c1b 100644 --- a/tests/Node/Expression/FilterTest.php +++ b/tests/Node/Expression/FilterTest.php @@ -13,6 +13,7 @@ namespace Twig\Tests\Node\Expression; use Twig\Environment; use Twig\Error\SyntaxError; +use Twig\Extension\AbstractExtension; use Twig\Loader\ArrayLoader; use Twig\Loader\LoaderInterface; use Twig\Node\Expression\ConstantExpression; @@ -39,8 +40,23 @@ class FilterTest extends NodeTestCase { $environment = new Environment($this->createMock(LoaderInterface::class)); $environment->addFilter(new TwigFilter('bar', 'twig_tests_filter_dummy', ['needs_environment' => true])); + $environment->addFilter(new TwigFilter('bar_closure', \Closure::fromCallable(twig_tests_filter_dummy::class), ['needs_environment' => true])); $environment->addFilter(new TwigFilter('barbar', 'Twig\Tests\Node\Expression\twig_tests_filter_barbar', ['needs_context' => true, 'is_variadic' => true])); + $extension = new class() extends AbstractExtension { + public function getFilters(): array + { + return [ + new TwigFilter('foo', \Closure::fromCallable([$this, 'foo'])), + ]; + } + + public function foo() + { + } + }; + $environment->addExtension($extension); + $tests = []; $expr = new ConstantExpression('foo', 1); @@ -77,12 +93,15 @@ class FilterTest extends NodeTestCase // filter as an anonymous function $node = $this->createFilter(new ConstantExpression('foo', 1), 'anonymous'); - $tests[] = [$node, 'call_user_func_array($this->env->getFilter(\'anonymous\')->getCallable(), ["foo"])']; + $tests[] = [$node, '$this->env->getFilter(\'anonymous\')->getCallable()("foo")']; // needs environment $node = $this->createFilter($string, 'bar'); $tests[] = [$node, 'twig_tests_filter_dummy($this->env, "abc")', $environment]; + $node = $this->createFilter($string, 'bar_closure'); + $tests[] = [$node, twig_tests_filter_dummy::class.'($this->env, "abc")', $environment]; + $node = $this->createFilter($string, 'bar', [new ConstantExpression('bar', 1)]); $tests[] = [$node, 'twig_tests_filter_dummy($this->env, "abc", "bar")', $environment]; @@ -104,6 +123,10 @@ class FilterTest extends NodeTestCase ]); $tests[] = [$node, 'Twig\Tests\Node\Expression\twig_tests_filter_barbar($context, "abc", "1", "2", [0 => "3", "foo" => "bar"])', $environment]; + // from extension + $node = $this->createFilter($string, 'foo'); + $tests[] = [$node, sprintf('$this->extensions[\'%s\']->foo("abc")', \get_class($extension)), $environment]; + return $tests; } diff --git a/tests/Node/Expression/FunctionTest.php b/tests/Node/Expression/FunctionTest.php index d0ca83dc9..8c9beb370 100644 --- a/tests/Node/Expression/FunctionTest.php +++ b/tests/Node/Expression/FunctionTest.php @@ -36,6 +36,7 @@ class FunctionTest extends NodeTestCase { $environment = new Environment($this->createMock(LoaderInterface::class)); $environment->addFunction(new TwigFunction('foo', 'twig_tests_function_dummy', [])); + $environment->addFunction(new TwigFunction('foo_closure', \Closure::fromCallable(twig_tests_function_dummy::class), [])); $environment->addFunction(new TwigFunction('bar', 'twig_tests_function_dummy', ['needs_environment' => true])); $environment->addFunction(new TwigFunction('foofoo', 'twig_tests_function_dummy', ['needs_context' => true])); $environment->addFunction(new TwigFunction('foobar', 'twig_tests_function_dummy', ['needs_environment' => true, 'needs_context' => true])); @@ -46,6 +47,9 @@ class FunctionTest extends NodeTestCase $node = $this->createFunction('foo'); $tests[] = [$node, 'twig_tests_function_dummy()', $environment]; + $node = $this->createFunction('foo_closure'); + $tests[] = [$node, twig_tests_function_dummy::class.'()', $environment]; + $node = $this->createFunction('foo', [new ConstantExpression('bar', 1), new ConstantExpression('foobar', 1)]); $tests[] = [$node, 'twig_tests_function_dummy("bar", "foobar")', $environment]; @@ -94,7 +98,7 @@ class FunctionTest extends NodeTestCase // function as an anonymous function $node = $this->createFunction('anonymous', [new ConstantExpression('foo', 1)]); - $tests[] = [$node, 'call_user_func_array($this->env->getFunction(\'anonymous\')->getCallable(), ["foo"])']; + $tests[] = [$node, '$this->env->getFunction(\'anonymous\')->getCallable()("foo")']; return $tests; } diff --git a/tests/Node/Expression/TestTest.php b/tests/Node/Expression/TestTest.php index d8fc291c3..97955cb62 100644 --- a/tests/Node/Expression/TestTest.php +++ b/tests/Node/Expression/TestTest.php @@ -48,7 +48,7 @@ class TestTest extends NodeTestCase // test as an anonymous function $node = $this->createTest(new ConstantExpression('foo', 1), 'anonymous', [new ConstantExpression('foo', 1)]); - $tests[] = [$node, 'call_user_func_array($this->env->getTest(\'anonymous\')->getCallable(), ["foo", "foo"])']; + $tests[] = [$node, '$this->env->getTest(\'anonymous\')->getCallable()("foo", "foo")']; // arbitrary named arguments $string = new ConstantExpression('abc', 1);