Throw a SyntaxError exception at compile time when a Twig callable ha…

…s not the minimum number of required arguments
twigphp · Aug 15, 2024 · 0823d23 · 0823d23
1 parent 55733a2
commit 0823d23
Show file tree

Hide file tree

Showing 12 changed files with 87 additions and 74 deletions.
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,5 +1,6 @@
 # 3.12.0 (2024-XX-XX)
 
+ * Throw a SyntaxError exception at compile time when a Twig callable has not the minimum number of required arguments
  * Add a `CallableArgumentsExtractor` class
  * Deprecate passing a name to `FunctionExpression`, `FilterExpression`, and `TestExpression`;
    pass a `TwigFunction`, `TwigFilter`, or `TestFilter` instead

diff --git a/src/AbstractTwigCallable.php b/src/AbstractTwigCallable.php
@@ -128,4 +128,9 @@ public function getAlternative(): ?string
     {
         return $this->options['alternative'];
     }
+
+    public function getMinimalNumberOfRequiredArguments(): int
+    {
+        return ($this->options['needs_charset'] ? 1 : 0) + ($this->options['needs_environment'] ? 1 : 0) + ($this->options['needs_context'] ? 1 : 0) + \count($this->arguments);
+    }
 }
diff --git a/src/TwigCallableInterface.php b/src/TwigCallableInterface.php
@@ -46,4 +46,6 @@ public function getDeprecatingPackage(): string;
     public function getDeprecatedVersion(): string;
 
     public function getAlternative(): ?string;
+
+    public function getMinimalNumberOfRequiredArguments(): int;
 }
diff --git a/src/TwigFilter.php b/src/TwigFilter.php
@@ -61,4 +61,9 @@ public function getPreEscape(): ?string
     {
         return $this->options['pre_escape'];
     }
+
+    public function getMinimalNumberOfRequiredArguments(): int
+    {
+        return parent::getMinimalNumberOfRequiredArguments() + 1;
+    }
 }
diff --git a/src/Util/CallableArgumentsExtractor.php b/src/Util/CallableArgumentsExtractor.php
@@ -49,7 +49,8 @@ public function __construct(
      */
     public function extractArguments(Node $arguments): array
     {
-        $parameters = [];
+        $rc = new ReflectionCallable($this->twigCallable->getCallable(), $this->type, $this->name);
+        $extractedArguments = [];
         $named = false;
         foreach ($arguments as $name => $node) {
             if (!\is_int($name)) {
@@ -59,24 +60,27 @@ public function extractArguments(Node $arguments): array
                 throw new SyntaxError(\sprintf('Positional arguments cannot be used after named arguments for %s "%s".', $this->type, $this->name), $this->node->getTemplateLine(), $this->node->getSourceContext());
             }
 
-            $parameters[$name] = $node;
+            $extractedArguments[$name] = $node;
         }
 
         if (!$named && !$this->twigCallable->isVariadic()) {
-            return $parameters;
+            $min = $this->twigCallable->getMinimalNumberOfRequiredArguments();
+            if (count($extractedArguments) < $rc->getReflector()->getNumberOfRequiredParameters() - $min) {
+                throw new SyntaxError(\sprintf('Value for argument "%s" is required for %s "%s".', $rc->getReflector()->getParameters()[$min + count($extractedArguments)]->getName(), $this->type, $this->name), $this->node->getTemplateLine(), $this->node->getSourceContext());
+            }
+
+            return $extractedArguments;
         }
 
         if (!$callable = $this->twigCallable->getCallable()) {
             if ($named) {
-                $message = \sprintf('Named arguments are not supported for %s "%s".', $this->type, $this->name);
-            } else {
-                $message = \sprintf('Arbitrary positional arguments are not supported for %s "%s".', $this->type, $this->name);
+                throw new SyntaxError(\sprintf('Named arguments are not supported for %s "%s".', $this->type, $this->name));
             }
 
-            throw new \LogicException($message);
+            throw new SyntaxError(\sprintf('Arbitrary positional arguments are not supported for %s "%s".', $this->type, $this->name));
         }
 
-        [$callableParameters, $isPhpVariadic] = $this->getCallableParameters();
+        [$callableParameters, $isPhpVariadic] = $this->getCallableParameters($rc);
         $arguments = [];
         $names = [];
         $missingArguments = [];
@@ -94,8 +98,8 @@ public function extractArguments(Node $arguments): array
 
             $names[] = $name;
 
-            if (\array_key_exists($name, $parameters)) {
-                if (\array_key_exists($pos, $parameters)) {
+            if (\array_key_exists($name, $extractedArguments)) {
+                if (\array_key_exists($pos, $extractedArguments)) {
                     throw new SyntaxError(\sprintf('Argument "%s" is defined twice for %s "%s".', $name, $this->type, $this->name), $this->node->getTemplateLine(), $this->node->getSourceContext());
                 }
 
@@ -107,37 +111,37 @@ public function extractArguments(Node $arguments): array
                 }
 
                 $arguments = array_merge($arguments, $optionalArguments);
-                $arguments[] = $parameters[$name];
-                unset($parameters[$name]);
+                $arguments[] = $extractedArguments[$name];
+                unset($extractedArguments[$name]);
                 $optionalArguments = [];
-            } elseif (\array_key_exists($pos, $parameters)) {
+            } elseif (\array_key_exists($pos, $extractedArguments)) {
                 $arguments = array_merge($arguments, $optionalArguments);
-                $arguments[] = $parameters[$pos];
-                unset($parameters[$pos]);
+                $arguments[] = $extractedArguments[$pos];
+                unset($extractedArguments[$pos]);
                 $optionalArguments = [];
                 ++$pos;
             } elseif ($callableParameter->isDefaultValueAvailable()) {
                 $optionalArguments[] = new ConstantExpression($callableParameter->getDefaultValue(), -1);
             } elseif ($callableParameter->isOptional()) {
-                if (empty($parameters)) {
+                if (!$extractedArguments) {
                     break;
-                } else {
-                    $missingArguments[] = $name;
                 }
+
+                $missingArguments[] = $name;
             } else {
                 throw new SyntaxError(\sprintf('Value for argument "%s" is required for %s "%s".', $name, $this->type, $this->name), $this->node->getTemplateLine(), $this->node->getSourceContext());
             }
         }
 
         if ($this->twigCallable->isVariadic()) {
             $arbitraryArguments = $isPhpVariadic ? new VariadicExpression([], -1) : new ArrayExpression([], -1);
-            foreach ($parameters as $key => $value) {
+            foreach ($extractedArguments as $key => $value) {
                 if (\is_int($key)) {
                     $arbitraryArguments->addElement($value);
                 } else {
                     $arbitraryArguments->addElement($value, new ConstantExpression($key, -1));
                 }
-                unset($parameters[$key]);
+                unset($extractedArguments[$key]);
             }
 
             if ($arbitraryArguments->count()) {
@@ -146,22 +150,22 @@ public function extractArguments(Node $arguments): array
             }
         }
 
-        if (!empty($parameters)) {
-            $unknownParameter = null;
-            foreach ($parameters as $parameter) {
-                if ($parameter instanceof Node) {
-                    $unknownParameter = $parameter;
+        if ($extractedArguments) {
+            $unknownArgument = null;
+            foreach ($extractedArguments as $extractedArgument) {
+                if ($extractedArgument instanceof Node) {
+                    $unknownArgument = $extractedArgument;
                     break;
                 }
             }
 
             throw new SyntaxError(
                 \sprintf(
                     'Unknown argument%s "%s" for %s "%s(%s)".',
-                    \count($parameters) > 1 ? 's' : '', implode('", "', array_keys($parameters)), $this->type, $this->name, implode(', ', $names)
+                    \count($extractedArguments) > 1 ? 's' : '', implode('", "', array_keys($extractedArguments)), $this->type, $this->name, implode(', ', $names)
                 ),
-                $unknownParameter ? $unknownParameter->getTemplateLine() : $this->node->getTemplateLine(),
-                $unknownParameter ? $unknownParameter->getSourceContext() : $this->node->getSourceContext()
+                $unknownArgument ? $unknownArgument->getTemplateLine() : $this->node->getTemplateLine(),
+                $unknownArgument ? $unknownArgument->getSourceContext() : $this->node->getSourceContext()
             );
         }
 
@@ -173,11 +177,9 @@ private function normalizeName(string $name): string
         return strtolower(preg_replace(['/([A-Z]+)([A-Z][a-z])/', '/([a-z\d])([A-Z])/'], ['\\1_\\2', '\\1_\\2'], $name));
     }
 
-    private function getCallableParameters(): array
+    private function getCallableParameters(ReflectionCallable $rc): array
     {
-        $rc = new ReflectionCallable($this->twigCallable->getCallable(), $this->type, $this->name);
         $r = $rc->getReflector();
-        $callableName = $rc->getName();
 
         $parameters = $r->getParameters();
         if ($this->node->hasNode('node')) {
@@ -206,7 +208,7 @@ private function getCallableParameters(): array
                 array_pop($parameters);
                 $isPhpVariadic = true;
             } else {
-                throw new \LogicException(\sprintf('The last parameter of "%s" for %s "%s" must be an array with default value, eg. "array $arg = []".', $callableName, $this->type, $this->name));
+                throw new SyntaxError(\sprintf('The last parameter of "%s" for %s "%s" must be an array with default value, eg. "array $arg = []".', $rc->getName(), $this->type, $this->name));
             }
         }
 

diff --git a/tests/ErrorTest.php b/tests/ErrorTest.php
@@ -234,28 +234,6 @@ public function testTwigArrayReduceThrowsRuntimeExceptions()
         }
     }
 
-    public function testTwigArgumentCountErrorThrowsRuntimeExceptions()
-    {
-        $loader = new ArrayLoader([
-            'argument-error.html' => <<<EOHTML
-{# max requires at least one argument #}
-{{ max() }}
-EOHTML
-        ]);
-
-        $twig = new Environment($loader, ['debug' => true, 'cache' => false]);
-
-        $template = $twig->load('argument-error.html');
-        try {
-            $template->render();
-
-            $this->fail();
-        } catch (RuntimeError $e) {
-            $this->assertEquals(2, $e->getTemplateLine());
-            $this->assertEquals('argument-error.html', $e->getSourceContext()->getName());
-        }
-    }
-
     public function getErroredTemplates()
     {
         return [

diff --git a/tests/Fixtures/functions/cycle_without_enough_args.test b/tests/Fixtures/functions/cycle_without_enough_args.test
@@ -0,0 +1,8 @@
+--TEST--
+"cycle" function without enough args and a named argument
+--TEMPLATE--
+{{ cycle(position=2) }}
+--DATA--
+return []
+--EXCEPTION--
+Twig\Error\SyntaxError: Value for argument "values" is required for function "cycle" in "index.twig" at line 2.
diff --git a/tests/Fixtures/functions/max_without_args.test b/tests/Fixtures/functions/max_without_args.test
@@ -0,0 +1,8 @@
+--TEST--
+"max" function without an argument throws a compile time exception
+--TEMPLATE--
+{{ max() }}
+--DATA--
+return []
+--EXCEPTION--
+Twig\Error\SyntaxError: Value for argument "value" is required for function "max" in "index.twig" at line 2.
diff --git a/tests/Node/DeprecatedTest.php b/tests/Node/DeprecatedTest.php
@@ -67,7 +67,7 @@ public function getTests()
         ];
 
         $environment = new Environment(new ArrayLoader());
-        $environment->addFunction($function = new TwigFunction('foo', 'foo', []));
+        $environment->addFunction($function = new TwigFunction('foo', 'Twig\Tests\Node\foo', []));
 
         $expr = new FunctionExpression($function, new Node(), 1);
         $node = new DeprecatedNode($expr, 1, 'deprecated');
@@ -80,11 +80,15 @@ public function getTests()
 
         $tests[] = [$node, <<<EOF
 // line 1
-\$$varName = foo();
+\$$varName = Twig\Tests\Node\\foo();
 trigger_deprecation("twig/twig", "1.1", \$$varName." in \"foo.twig\" at line 1.");
 EOF
             , $environment];
 
         return $tests;
     }
 }
+
+function foo()
+{
+}
diff --git a/tests/Node/Expression/FilterTest.php b/tests/Node/Expression/FilterTest.php
@@ -86,13 +86,13 @@ public function getTests()
 
         // needs environment
         $node = $this->createFilter($environment, $string, 'bar');
-        $tests[] = [$node, 'twig_tests_filter_dummy($this->env, "abc")', $environment];
+        $tests[] = [$node, 'Twig\Tests\Node\Expression\twig_tests_filter_dummy($this->env, "abc")', $environment];
 
         $node = $this->createFilter($environment, $string, 'bar_closure');
         $tests[] = [$node, twig_tests_filter_dummy::class.'($this->env, "abc")', $environment];
 
         $node = $this->createFilter($environment, $string, 'bar', [new ConstantExpression('bar', 1)]);
-        $tests[] = [$node, 'twig_tests_filter_dummy($this->env, "abc", "bar")', $environment];
+        $tests[] = [$node, 'Twig\Tests\Node\Expression\twig_tests_filter_dummy($this->env, "abc", "bar")', $environment];
 
         // arbitrary named arguments
         $node = $this->createFilter($environment, $string, 'barbar');
@@ -170,7 +170,7 @@ protected function getEnvironment()
     {
         $env = new Environment(new ArrayLoader());
         $env->addFilter(new TwigFilter('anonymous', function () {}));
-        $env->addFilter(new TwigFilter('bar', 'twig_tests_filter_dummy', ['needs_environment' => true]));
+        $env->addFilter(new TwigFilter('bar', 'Twig\Tests\Node\Expression\twig_tests_filter_dummy', ['needs_environment' => true]));
         $env->addFilter(new TwigFilter('bar_closure', \Closure::fromCallable(twig_tests_filter_dummy::class), ['needs_environment' => true]));
         $env->addFilter(new TwigFilter('barbar', 'Twig\Tests\Node\Expression\twig_tests_filter_barbar', ['needs_context' => true, 'is_variadic' => true]));
         $env->addFilter(new TwigFilter('magic_static', __NAMESPACE__.'\ChildMagicCallStub::magicStaticCall'));

diff --git a/tests/Node/Expression/FunctionTest.php b/tests/Node/Expression/FunctionTest.php
@@ -38,31 +38,31 @@ public function getTests()
         $tests = [];
 
         $node = $this->createFunction($environment, 'foo');
-        $tests[] = [$node, 'twig_tests_function_dummy()', $environment];
+        $tests[] = [$node, 'Twig\Tests\Node\Expression\twig_tests_function_dummy()', $environment];
 
         $node = $this->createFunction($environment, 'foo_closure');
         $tests[] = [$node, twig_tests_function_dummy::class.'()', $environment];
 
         $node = $this->createFunction($environment, 'foo', [new ConstantExpression('bar', 1), new ConstantExpression('foobar', 1)]);
-        $tests[] = [$node, 'twig_tests_function_dummy("bar", "foobar")', $environment];
+        $tests[] = [$node, 'Twig\Tests\Node\Expression\twig_tests_function_dummy("bar", "foobar")', $environment];
 
         $node = $this->createFunction($environment, 'bar');
-        $tests[] = [$node, 'twig_tests_function_dummy($this->env)', $environment];
+        $tests[] = [$node, 'Twig\Tests\Node\Expression\twig_tests_function_dummy($this->env)', $environment];
 
         $node = $this->createFunction($environment, 'bar', [new ConstantExpression('bar', 1)]);
-        $tests[] = [$node, 'twig_tests_function_dummy($this->env, "bar")', $environment];
+        $tests[] = [$node, 'Twig\Tests\Node\Expression\twig_tests_function_dummy($this->env, "bar")', $environment];
 
         $node = $this->createFunction($environment, 'foofoo');
-        $tests[] = [$node, 'twig_tests_function_dummy($context)', $environment];
+        $tests[] = [$node, 'Twig\Tests\Node\Expression\twig_tests_function_dummy($context)', $environment];
 
         $node = $this->createFunction($environment, 'foofoo', [new ConstantExpression('bar', 1)]);
-        $tests[] = [$node, 'twig_tests_function_dummy($context, "bar")', $environment];
+        $tests[] = [$node, 'Twig\Tests\Node\Expression\twig_tests_function_dummy($context, "bar")', $environment];
 
         $node = $this->createFunction($environment, 'foobar');
-        $tests[] = [$node, 'twig_tests_function_dummy($this->env, $context)', $environment];
+        $tests[] = [$node, 'Twig\Tests\Node\Expression\twig_tests_function_dummy($this->env, $context)', $environment];
 
         $node = $this->createFunction($environment, 'foobar', [new ConstantExpression('bar', 1)]);
-        $tests[] = [$node, 'twig_tests_function_dummy($this->env, $context, "bar")', $environment];
+        $tests[] = [$node, 'Twig\Tests\Node\Expression\twig_tests_function_dummy($this->env, $context, "bar")', $environment];
 
         // named arguments
         $node = $this->createFunction($environment, 'date', [
@@ -105,11 +105,11 @@ protected function getEnvironment()
     {
         $env = new Environment(new ArrayLoader());
         $env->addFunction(new TwigFunction('anonymous', function () {}));
-        $env->addFunction(new TwigFunction('foo', 'twig_tests_function_dummy', []));
+        $env->addFunction(new TwigFunction('foo', 'Twig\Tests\Node\Expression\twig_tests_function_dummy', []));
         $env->addFunction(new TwigFunction('foo_closure', \Closure::fromCallable(twig_tests_function_dummy::class), []));
-        $env->addFunction(new TwigFunction('bar', 'twig_tests_function_dummy', ['needs_environment' => true]));
-        $env->addFunction(new TwigFunction('foofoo', 'twig_tests_function_dummy', ['needs_context' => true]));
-        $env->addFunction(new TwigFunction('foobar', 'twig_tests_function_dummy', ['needs_environment' => true, 'needs_context' => true]));
+        $env->addFunction(new TwigFunction('bar', 'Twig\Tests\Node\Expression\twig_tests_function_dummy', ['needs_environment' => true]));
+        $env->addFunction(new TwigFunction('foofoo', 'Twig\Tests\Node\Expression\twig_tests_function_dummy', ['needs_context' => true]));
+        $env->addFunction(new TwigFunction('foobar', 'Twig\Tests\Node\Expression\twig_tests_function_dummy', ['needs_environment' => true, 'needs_context' => true]));
         $env->addFunction(new TwigFunction('barbar', 'Twig\Tests\Node\Expression\twig_tests_function_barbar', ['is_variadic' => true]));
 
         return $env;

diff --git a/tests/Util/CallableArgumentsExtractorTest.php b/tests/Util/CallableArgumentsExtractorTest.php
@@ -82,7 +82,7 @@ public function testGetArgumentsForStaticMethod()
 
     public function testResolveArgumentsWithMissingParameterForArbitraryArguments()
     {
-        $this->expectException(\LogicException::class);
+        $this->expectException(SyntaxError::class);
         $this->expectExceptionMessage('The last parameter of "Twig\\Tests\\Util\\CallableArgumentsExtractorTest::customFunctionWithArbitraryArguments" for function "foo" must be an array with default value, eg. "array $arg = []".');
 
         $this->getArguments('foo', [$this, 'customFunctionWithArbitraryArguments'], [], true);
@@ -97,15 +97,15 @@ public function testGetArgumentsWithInvalidCallable()
 
     public function testResolveArgumentsWithMissingParameterForArbitraryArgumentsOnFunction()
     {
-        $this->expectException(\LogicException::class);
+        $this->expectException(SyntaxError::class);
         $this->expectExceptionMessageMatches('#^The last parameter of "Twig\\\\Tests\\\\Util\\\\custom_call_test_function" for function "foo" must be an array with default value, eg\\. "array \\$arg \\= \\[\\]"\\.$#');
 
         $this->getArguments('foo', 'Twig\Tests\Util\custom_call_test_function', [], true);
     }
 
     public function testResolveArgumentsWithMissingParameterForArbitraryArgumentsOnObject()
     {
-        $this->expectException(\LogicException::class);
+        $this->expectException(SyntaxError::class);
         $this->expectExceptionMessageMatches('#^The last parameter of "Twig\\\\Tests\\\\Util\\\\CallableTestClass\\:\\:__invoke" for function "foo" must be an array with default value, eg\\. "array \\$arg \\= \\[\\]"\\.$#');
 
         $this->getArguments('foo', new CallableTestClass(), [], true);