Repository: twigphp/Twig (php)

Issue:
Named Arguments of Filters and Functions are not passed as-is and forced to snake_case
## Problem Description
I stumbled upon this super odd behaviour when implementing a custom function in Twig and realised that given the template
```twig
{{ foo(barValue='bar', booValue='boo') }}
```
the two named arguments, `barValue` and `booValue` will be passed to the `foo` function as `bar_value` and `boo_value`. This is due to the "name normalization" done in the [`CallExpression` on line 242](https://github.com/twigphp/Twig/blob/4a47f0f5b4bf2400ed1e3abecb486655e3970cdb/src/Node/Expression/CallExpression.php#L242) in the current 3.x branch. This has been introduced in a59dcde3, around 8 years ago and was part of the initial named arguments implementation.

As this behaviour is undocumented, technically unnecessary (named arguments are passed as array keys and PHP allows all strings to be array keys) and defnitely unexpected, I'm suggesting to classifiy this as a bug.

## Proposed Fix
To fix it without introducing backwards incompatible changes, I'm proposing a new option for functions and filters that is called `legacy_named_arguments` and that defaults to `true` in 3.x and can be set to `false` in upcoming major releases.

- Setting `legacy_named_arguments` to `true` means: keep the current behaviour. In my opinion it makes sense to trigger a `E_USER_DEPRECATED` when a variable key has been "normalized" (i. e. input key !== output key). However, this might have a very big impact on a lot of existing Symfony installations.

- Setting `legacy_named_arguments` to `false` means: leave the input argument as-is. The lexer decides whether a variable name is valid or not (which it already does implicitly [here](https://github.com/twigphp/Twig/blob/3.x/src/Lexer.php#L363)).


I am happy to contribute in a PR once you've confirmed the issue.

Pull request:
Allow Twig callable arguments to use camel or snake names
Closes #3475

Twig callables (functions/filters/tests) now accept snake and camel argument names independently of the name of the underlying PHP callable parameter names.


Patch:
diff --git a/CHANGELOG b/CHANGELOG
index ad3841a5..ea986469 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,5 +1,7 @@
 # 3.15.0 (2024-XX-XX)
 
+ * Allow Twig callable argument names to be free-form (snake-case or camelCase) independently of the PHP callable signature
+   They were automatically converted to snake-cased before
  * Deprecate the `attribute` function; use the `.` notation and wrap the name with parenthesis instead
  * Add support for argument unpackaging
  * Add JSON support for the file extension escaping strategy
diff --git a/doc/deprecated.rst b/doc/deprecated.rst
index 7222b4f6..3a80f632 100644
--- a/doc/deprecated.rst
+++ b/doc/deprecated.rst
@@ -286,3 +286,6 @@ Functions/Filters/Tests
       $twig->addFunction(new TwigFunction('upper', 'upper', [
           'deprecation_info' => new DeprecatedCallableInfo('twig/twig', '3.12'),
       ]));
+
+* For variadic arguments, use snake-case for the argument name to ease the
+  transition to 4.0.
diff --git a/src/Util/CallableArgumentsExtractor.php b/src/Util/CallableArgumentsExtractor.php
index fedee621..d8625169 100644
--- a/src/Util/CallableArgumentsExtractor.php
+++ b/src/Util/CallableArgumentsExtractor.php
@@ -40,22 +40,25 @@ final class CallableArgumentsExtractor
     public function extractArguments(Node $arguments): array
     {
         $extractedArguments = [];
+        $extractedArgumentNameMap = [];
         $named = false;
         foreach ($arguments as $name => $node) {
             if (!\is_int($name)) {
                 $named = true;
-                $name = $this->normalizeName($name);
             } elseif ($named) {
                 throw new SyntaxError(\sprintf('Positional arguments cannot be used after named arguments for %s "%s".', $this->twigCallable->getType(), $this->twigCallable->getName()), $this->node->getTemplateLine(), $this->node->getSourceContext());
             }
 
-            $extractedArguments[$name] = $node;
+            $extractedArguments[$normalizedName = $this->normalizeName($name)] = $node;
+            $extractedArgumentNameMap[$normalizedName] = $name;
         }
 
         if (!$named && !$this->twigCallable->isVariadic()) {
             $min = $this->twigCallable->getMinimalNumberOfRequiredArguments();
             if (\count($extractedArguments) < $this->rc->getReflector()->getNumberOfRequiredParameters() - $min) {
-                throw new SyntaxError(\sprintf('Value for argument "%s" is required for %s "%s".', $this->rc->getReflector()->getParameters()[$min + \count($extractedArguments)]->getName(), $this->twigCallable->getType(), $this->twigCallable->getName()), $this->node->getTemplateLine(), $this->node->getSourceContext());
+                $argName = $this->toSnakeCase($this->rc->getReflector()->getParameters()[$min + \count($extractedArguments)]->getName());
+
+                throw new SyntaxError(\sprintf('Value for argument "%s" is required for %s "%s".', $argName, $this->twigCallable->getType(), $this->twigCallable->getName()), $this->node->getTemplateLine(), $this->node->getSourceContext());
             }
 
             return $extractedArguments;
@@ -76,7 +79,7 @@ final class CallableArgumentsExtractor
         $optionalArguments = [];
         $pos = 0;
         foreach ($callableParameters as $callableParameter) {
-            $callableParameterName = $this->normalizeName($callableParameter->name);
+            $callableParameterName = $callableParameter->name;
             if (\PHP_VERSION_ID >= 80000 && 'range' === $callable) {
                 if ('start' === $callableParameterName) {
                     $callableParameterName = 'low';
@@ -86,8 +89,9 @@ final class CallableArgumentsExtractor
             }
 
             $callableParameterNames[] = $callableParameterName;
+            $normalizedCallableParameterName = $this->normalizeName($callableParameterName);
 
-            if (\array_key_exists($callableParameterName, $extractedArguments)) {
+            if (\array_key_exists($normalizedCallableParameterName, $extractedArguments)) {
                 if (\array_key_exists($pos, $extractedArguments)) {
                     throw new SyntaxError(\sprintf('Argument "%s" is defined twice for %s "%s".', $callableParameterName, $this->twigCallable->getType(), $this->twigCallable->getName()), $this->node->getTemplateLine(), $this->node->getSourceContext());
                 }
@@ -95,13 +99,13 @@ final class CallableArgumentsExtractor
                 if (\count($missingArguments)) {
                     throw new SyntaxError(\sprintf(
                         'Argument "%s" could not be assigned for %s "%s(%s)" because it is mapped to an internal PHP function which cannot determine default value for optional argument%s "%s".',
-                        $callableParameterName, $this->twigCallable->getType(), $this->twigCallable->getName(), implode(', ', $callableParameterNames), \count($missingArguments) > 1 ? 's' : '', implode('", "', $missingArguments)
+                        $callableParameterName, $this->twigCallable->getType(), $this->twigCallable->getName(), implode(', ', array_map([$this, 'toSnakeCase'], $callableParameterNames)), \count($missingArguments) > 1 ? 's' : '', implode('", "', $missingArguments)
                     ), $this->node->getTemplateLine(), $this->node->getSourceContext());
                 }
 
                 $arguments = array_merge($arguments, $optionalArguments);
-                $arguments[] = $extractedArguments[$callableParameterName];
-                unset($extractedArguments[$callableParameterName]);
+                $arguments[] = $extractedArguments[$normalizedCallableParameterName];
+                unset($extractedArguments[$normalizedCallableParameterName]);
                 $optionalArguments = [];
             } elseif (\array_key_exists($pos, $extractedArguments)) {
                 $arguments = array_merge($arguments, $optionalArguments);
@@ -118,7 +122,7 @@ final class CallableArgumentsExtractor
 
                 $missingArguments[] = $callableParameterName;
             } else {
-                throw new SyntaxError(\sprintf('Value for argument "%s" is required for %s "%s".', $callableParameterName, $this->twigCallable->getType(), $this->twigCallable->getName()), $this->node->getTemplateLine(), $this->node->getSourceContext());
+                throw new SyntaxError(\sprintf('Value for argument "%s" is required for %s "%s".', $this->toSnakeCase($callableParameterName), $this->twigCallable->getType(), $this->twigCallable->getName()), $this->node->getTemplateLine(), $this->node->getSourceContext());
             }
         }
 
@@ -128,7 +132,13 @@ final class CallableArgumentsExtractor
                 if (\is_int($key)) {
                     $arbitraryArguments->addElement($value);
                 } else {
-                    $arbitraryArguments->addElement($value, new ConstantExpression($key, $this->node->getTemplateLine()));
+                    $originalKey = $extractedArgumentNameMap[$key];
+                    if ($originalKey !== $this->toSnakeCase($originalKey)) {
+                        trigger_deprecation('twig/twig', '3.15', \sprintf('Using "snake_case" for variadic arguments is required for a smooth upgrade with Twig 4.0; rename "%s" to "%s" in "%s" at line %d.', $originalKey, $this->toSnakeCase($originalKey), $this->node->getSourceContext()->getName(), $this->node->getTemplateLine()));
+                    }
+                    $arbitraryArguments->addElement($value, new ConstantExpression($this->toSnakeCase($originalKey), $this->node->getTemplateLine()));
+                    // I Twig 4.0, don't convert the key:
+                    // $arbitraryArguments->addElement($value, new ConstantExpression($originalKey, $this->node->getTemplateLine()));
                 }
                 unset($extractedArguments[$key]);
             }
@@ -151,7 +161,7 @@ final class CallableArgumentsExtractor
             throw new SyntaxError(
                 \sprintf(
                     'Unknown argument%s "%s" for %s "%s(%s)".',
-                    \count($extractedArguments) > 1 ? 's' : '', implode('", "', array_keys($extractedArguments)), $this->twigCallable->getType(), $this->twigCallable->getName(), implode(', ', $callableParameterNames)
+                    \count($extractedArguments) > 1 ? 's' : '', implode('", "', array_keys($extractedArguments)), $this->twigCallable->getType(), $this->twigCallable->getName(), implode(', ', array_map([$this, 'toSnakeCase'], $callableParameterNames))
                 ),
                 $unknownArgument ? $unknownArgument->getTemplateLine() : $this->node->getTemplateLine(),
                 $unknownArgument ? $unknownArgument->getSourceContext() : $this->node->getSourceContext()
@@ -163,7 +173,12 @@ final class CallableArgumentsExtractor
 
     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));
+        return strtolower(str_replace('_', '', $name));
+    }
+
+    private function toSnakeCase(string $name): string
+    {
+        return strtolower(preg_replace(['/([A-Z]+)([A-Z][a-z])/', '/([a-z0-9])([A-Z])/'], '\1_\2', $name));
     }
 
     private function getCallableParameters(): array


Test patch:
diff --git a/tests/Util/CallableArgumentsExtractorTest.php b/tests/Util/CallableArgumentsExtractorTest.php
index cfea0f8a..18228318 100644
--- a/tests/Util/CallableArgumentsExtractorTest.php
+++ b/tests/Util/CallableArgumentsExtractorTest.php
@@ -12,15 +12,20 @@ namespace Twig\Tests\Util;
  */
 
 use PHPUnit\Framework\TestCase;
+use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
 use Twig\Error\SyntaxError;
 use Twig\Node\Expression\ConstantExpression;
 use Twig\Node\Expression\FunctionExpression;
+use Twig\Node\Expression\VariadicExpression;
 use Twig\Node\Node;
+use Twig\Source;
 use Twig\TwigFunction;
 use Twig\Util\CallableArgumentsExtractor;
 
 class CallableArgumentsExtractorTest extends TestCase
 {
+    use ExpectDeprecationTrait;
+
     public function testGetArguments()
     {
         $this->assertEquals(['U', null], $this->getArguments('date', 'date', ['format' => 'U', 'timestamp' => null]));
@@ -29,7 +34,7 @@ class CallableArgumentsExtractorTest extends TestCase
     public function testGetArgumentsWhenPositionalArgumentsAfterNamedArguments()
     {
         $this->expectException(SyntaxError::class);
-        $this->expectExceptionMessage('Positional arguments cannot be used after named arguments for function "date".');
+        $this->expectExceptionMessage('Positional arguments cannot be used after named arguments for function "date" in "test.twig" at line 2.');
 
         $this->getArguments('date', 'date', ['timestamp' => 123456, 'Y-m-d']);
     }
@@ -37,7 +42,7 @@ class CallableArgumentsExtractorTest extends TestCase
     public function testGetArgumentsWhenArgumentIsDefinedTwice()
     {
         $this->expectException(SyntaxError::class);
-        $this->expectExceptionMessage('Argument "format" is defined twice for function "date".');
+        $this->expectExceptionMessage('Argument "format" is defined twice for function "date" in "test.twig" at line 2.');
 
         $this->getArguments('date', 'date', ['Y-m-d', 'format' => 'U']);
     }
@@ -80,6 +85,54 @@ class CallableArgumentsExtractorTest extends TestCase
         $this->assertEquals(['arg1'], $this->getArguments('custom_static_function', __CLASS__.'::customStaticFunction', ['arg1' => 'arg1']));
     }
 
+    /**
+     * @dataProvider getGetArgumentsConversionData
+     */
+    public function testGetArgumentsConversion($arg1, $arg2)
+    {
+        $this->assertEquals([null], $this->getArguments('custom', eval("return fn (\$$arg1) => '';"), [$arg1 => null]));
+        $this->assertEquals([null], $this->getArguments('custom', eval("return fn (\$$arg2) => '';"), [$arg2 => null]));
+        $this->assertEquals([null], $this->getArguments('custom', eval("return fn (\$$arg1) => '';"), [$arg2 => null]));
+        $this->assertEquals([null], $this->getArguments('custom', eval("return fn (\$$arg2) => '';"), [$arg1 => null]));
+    }
+
+    public static function getGetArgumentsConversionData()
+    {
+        yield ['some_name', 'some_name'];
+        yield ['someName', 'some_name'];
+        yield ['no_svg', 'noSVG'];
+        yield ['error_404', 'error404'];
+        yield ['errCode_404', 'err_code_404'];
+        yield ['errCode404', 'err_code_404'];
+        yield ['aBc', 'a_b_c'];
+        yield ['aBC', 'a_b_c'];
+    }
+
+    /**
+     * @group legacy
+     */
+    public function testGetArgumentsConversionForVariadics()
+    {
+        $this->expectDeprecation('Since twig/twig 3.15: Using "snake_case" for variadic arguments is required for a smooth upgrade with Twig 4.0; rename "someNumberVariadic" to "some_number_variadic" in "test.twig" at line 2.');
+
+        $this->assertEquals([
+            new ConstantExpression('a', 0),
+            new ConstantExpression(12, 0),
+            new VariadicExpression([
+                new ConstantExpression('some_text_variadic', 2), new ConstantExpression('a', 0),
+                new ConstantExpression('some_number_variadic', 2), new ConstantExpression(12, 0),
+            ], 2),
+        ], $this->getArguments('custom', eval("return fn (string \$someText, int \$some_number, ...\$args) => '';"), ['some_text' => 'a', 'someNumber' => 12, 'some_text_variadic' => 'a', 'someNumberVariadic' => 12], true));
+    }
+
+    public function testGetArgumentsError()
+    {
+        $this->expectException(SyntaxError::class);
+        $this->expectExceptionMessage('Value for argument "some_name" is required for function "custom_static_function" in "test.twig" at line 2.');
+
+        $this->getArguments('custom_static_function', [$this, 'customFunctionSnakeCamel'], ['someCity' => 'Paris']);
+    }
+
     public function testResolveArgumentsWithMissingParameterForArbitraryArguments()
     {
         $this->expectException(SyntaxError::class);
@@ -119,6 +172,10 @@ class CallableArgumentsExtractorTest extends TestCase
     {
     }
 
+    public function customFunctionSnakeCamel($someName, $some_city)
+    {
+    }
+
     public function customFunctionWithArbitraryArguments()
     {
     }
@@ -126,14 +183,15 @@ class CallableArgumentsExtractorTest extends TestCase
     private function getArguments(string $name, $callable, array $args, bool $isVariadic = false): array
     {
         $function = new TwigFunction($name, $callable, ['is_variadic' => $isVariadic]);
-        $node = new ExpressionCall($function, new Node([]), 0);
+        $node = new ExpressionCall($function, new Node([]), 2);
+        $node->setSourceContext(new Source('', 'test.twig'));
         foreach ($args as $name => $arg) {
             $args[$name] = new ConstantExpression($arg, 0);
         }
 
         $arguments = (new CallableArgumentsExtractor($node, $function))->extractArguments(new Node($args));
         foreach ($arguments as $name => $argument) {
-            $arguments[$name] = $argument->getAttribute('value');
+            $arguments[$name] = $isVariadic ? $argument : $argument->getAttribute('value');
         }
 
         return $arguments;