feature #48525 [HttpFoundation] Add ParameterBag::getString() and d…

…eprecate accepting invalid values (GromNaN)

This PR was merged into the 6.3 branch.

Discussion
----------

[HttpFoundation] Add `ParameterBag::getString()` and deprecate accepting invalid values

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | Fix #44787 #48219
| License       | MIT
| Doc PR        | symfony/symfony-docs#...

There were a lot of discussions on #44787 regarding the implementation. The main debate is to determine the right behavior when the value is invalid: try to convert the value, throw an exception, return the default value or the falsy?

I added plenty of test cases for the methods `getAlpha`, `getAlnum`, `getDigits`, `getInt`, ~`getInteger`~, `getString` so that we can discuss the expected result for each input value.

My goals:
- Provide safe methods to get values in the expected type. Example: The parameters being generally of type `string`, the `getString` method is useful to be sure we don't get an array.
- Reduce deprecations on methods that exist since Symfony 2.0, in one of the most popular Symfony component.

How are these getter methods used?
- Most of the time, parameter values are `string` (from routing, query string, request payload). `get` is used but does not validate the return type.
- `getInt` is used for [pagination (UX Autocomplete)](https://github.com/symfony/ux-autocomplete/blob/697f1cb4914480b811d978efe031a6f4a0dc3814/src/Controller/EntityAutocompleteController.php#L41) or [getting an index (EasyAdmin)](https://github.com/EasyCorp/EasyAdminBundle/blob/f210bd1e494b699dec54d2ef29302db1211267b5/src/Context/AdminContext.php#L157-L158)
- I think there was an unidentified breaking change when we introduced return types [#42507](https://github.com/symfony/symfony/pull/42507/files#diff-04f3b8ea71029f48853b804129631aeb9bf3dad7a7a398feb9568bf5397d0e69L117). Methods `getAlpha`, `getAlnum` and `getDigits` could return an array, but their return type have been modified to `string`, resulting in a `TypeError`. [example of use](https://github.com/asaaa1997/Licencjat/blob/8c10abaf87120c904557977ae14284c7d443c9a7/src/Controller/QuizController.php#L164)

Commits
-------

172f0a7 [HttpFoundation] Add `ParameterBag::getString()` and deprecate accepting invalid values

UPGRADE-6.3.md

@@ -63,6 +63,12 @@ HttpFoundation
 
  * `Response::sendHeaders()` now takes an optional `$statusCode` parameter
 
+HttpFoundation
+--------------
+
+ * Deprecate conversion of invalid values in `ParameterBag::getInt()` and `ParameterBag::getBoolean()`
+ * Deprecate ignoring invalid values when using `ParameterBag::filter()`, unless flag `FILTER_NULL_ON_FAILURE` is set
+
 HttpKernel
 ----------
 

src/Symfony/Component/HttpFoundation/CHANGELOG.md

@@ -4,10 +4,14 @@ CHANGELOG
 6.3
 ---
 
+ * Calling `ParameterBag::getDigit()`, `getAlnum()`, `getAlpha()` on an `array` throws a `UnexpectedValueException` instead of a `TypeError`
+ * Add `ParameterBag::getString()` to convert a parameter into string and throw an exception if the value is invalid
  * Add `ParameterBag::getEnum()`
  * Create migration for session table when pdo handler is used
  * Add support for Relay PHP extension for Redis
  * The `Response::sendHeaders()` method now takes an optional HTTP status code as parameter, allowing to send informational responses such as Early Hints responses (103 status code)
+ * Deprecate conversion of invalid values in `ParameterBag::getInt()` and `ParameterBag::getBoolean()`,
+ * Deprecate ignoring invalid values when using `ParameterBag::filter()`, unless flag `FILTER_NULL_ON_FAILURE` is set
 
 6.2
 ---

src/Symfony/Component/HttpFoundation/InputBag.php

@@ -92,6 +92,15 @@ public function getEnum(string $key, string $class, \BackedEnum $default = null)
         }
     }
 
+    /**
+     * Returns the parameter value converted to string.
+     */
+    public function getString(string $key, string $default = ''): string
+    {
+        // Shortcuts the parent method because the validation on scalar is already done in get().
+        return (string) $this->get($key, $default);
+    }
+
     public function filter(string $key, mixed $default = null, int $filter = \FILTER_DEFAULT, mixed $options = []): mixed
     {
         $value = $this->has($key) ? $this->all()[$key] : $default;
@@ -109,6 +118,22 @@ public function filter(string $key, mixed $default = null, int $filter = \FILTER
             throw new \InvalidArgumentException(sprintf('A Closure must be passed to "%s()" when FILTER_CALLBACK is used, "%s" given.', __METHOD__, get_debug_type($options['options'] ?? null)));
         }
 
-        return filter_var($value, $filter, $options);
+        $options['flags'] ??= 0;
+        $nullOnFailure = $options['flags'] & \FILTER_NULL_ON_FAILURE;
+        $options['flags'] |= \FILTER_NULL_ON_FAILURE;
+
+        $value = filter_var($value, $filter, $options);
+
+        if (null !== $value || $nullOnFailure) {
+            return $value;
+        }
+
+        $method = debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS | \DEBUG_BACKTRACE_PROVIDE_OBJECT, 2)[1];
+        $method = ($method['object'] ?? null) === $this ? $method['function'] : 'filter';
+        $hint = 'filter' === $method ? 'pass' : 'use method "filter()" with';
+
+        trigger_deprecation('symfony/http-foundation', '6.3', 'Ignoring invalid values when using "%s::%s(\'%s\')" is deprecated and will throw a "%s" in 7.0; '.$hint.' flag "FILTER_NULL_ON_FAILURE" to keep ignoring them.', $this::class, $method, $key, BadRequestException::class);
+
+        return false;
     }
 }

src/Symfony/Component/HttpFoundation/ParameterBag.php

@@ -114,40 +114,53 @@ public function remove(string $key)
      */
     public function getAlpha(string $key, string $default = ''): string
     {
-        return preg_replace('/[^[:alpha:]]/', '', $this->get($key, $default));
+        return preg_replace('/[^[:alpha:]]/', '', $this->getString($key, $default));
     }
 
     /**
      * Returns the alphabetic characters and digits of the parameter value.
      */
     public function getAlnum(string $key, string $default = ''): string
     {
-        return preg_replace('/[^[:alnum:]]/', '', $this->get($key, $default));
+        return preg_replace('/[^[:alnum:]]/', '', $this->getString($key, $default));
     }
 
     /**
      * Returns the digits of the parameter value.
      */
     public function getDigits(string $key, string $default = ''): string
     {
-        // we need to remove - and + because they're allowed in the filter
-        return str_replace(['-', '+'], '', $this->filter($key, $default, \FILTER_SANITIZE_NUMBER_INT));
+        return preg_replace('/[^[:digit:]]/', '', $this->getString($key, $default));
+    }
+
+    /**
+     * Returns the parameter as string.
+     */
+    public function getString(string $key, string $default = ''): string
+    {
+        $value = $this->get($key, $default);
+        if (!\is_scalar($value) && !$value instanceof \Stringable) {
+            throw new \UnexpectedValueException(sprintf('Parameter value "%s" cannot be converted to "string".', $key));
+        }
+
+        return (string) $value;
     }
 
     /**
      * Returns the parameter value converted to integer.
      */
     public function getInt(string $key, int $default = 0): int
     {
-        return (int) $this->get($key, $default);
+        // In 7.0 remove the fallback to 0, in case of failure an exception will be thrown
+        return $this->filter($key, $default, \FILTER_VALIDATE_INT, ['flags' => \FILTER_REQUIRE_SCALAR]) ?: 0;
     }
 
     /**
      * Returns the parameter value converted to boolean.
      */
     public function getBoolean(string $key, bool $default = false): bool
     {
-        return $this->filter($key, $default, \FILTER_VALIDATE_BOOL);
+        return $this->filter($key, $default, \FILTER_VALIDATE_BOOL, ['flags' => \FILTER_REQUIRE_SCALAR]);
     }
 
     /**
@@ -178,7 +191,8 @@ public function getEnum(string $key, string $class, \BackedEnum $default = null)
     /**
      * Filter key.
      *
-     * @param int $filter FILTER_* constant
+     * @param int                                     $filter  FILTER_* constant
+     * @param int|array{flags?: int, options?: array} $options Flags from FILTER_* constants
      *
      * @see https://php.net/filter-var
      */
@@ -196,11 +210,31 @@ public function filter(string $key, mixed $default = null, int $filter = \FILTER
             $options['flags'] = \FILTER_REQUIRE_ARRAY;
         }
 
+        if (\is_object($value) && !$value instanceof \Stringable) {
+            throw new \UnexpectedValueException(sprintf('Parameter value "%s" cannot be filtered.', $key));
+        }
+
         if ((\FILTER_CALLBACK & $filter) && !(($options['options'] ?? null) instanceof \Closure)) {
             throw new \InvalidArgumentException(sprintf('A Closure must be passed to "%s()" when FILTER_CALLBACK is used, "%s" given.', __METHOD__, get_debug_type($options['options'] ?? null)));
         }
 
-        return filter_var($value, $filter, $options);
+        $options['flags'] ??= 0;
+        $nullOnFailure = $options['flags'] & \FILTER_NULL_ON_FAILURE;
+        $options['flags'] |= \FILTER_NULL_ON_FAILURE;
+
+        $value = filter_var($value, $filter, $options);
+
+        if (null !== $value || $nullOnFailure) {
+            return $value;
+        }
+
+        $method = debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS | \DEBUG_BACKTRACE_PROVIDE_OBJECT, 2)[1];
+        $method = ($method['object'] ?? null) === $this ? $method['function'] : 'filter';
+        $hint = 'filter' === $method ? 'pass' : 'use method "filter()" with';
+
+        trigger_deprecation('symfony/http-foundation', '6.3', 'Ignoring invalid values when using "%s::%s(\'%s\')" is deprecated and will throw an "%s" in 7.0; '.$hint.' flag "FILTER_NULL_ON_FAILURE" to keep ignoring them.', $this::class, $method, $key, \UnexpectedValueException::class);
+
+        return false;
     }
 
     /**

src/Symfony/Component/HttpFoundation/Tests/InputBagTest.php

@@ -12,12 +12,15 @@
 namespace Symfony\Component\HttpFoundation\Tests;
 
 use PHPUnit\Framework\TestCase;
+use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
 use Symfony\Component\HttpFoundation\Exception\BadRequestException;
 use Symfony\Component\HttpFoundation\InputBag;
 use Symfony\Component\HttpFoundation\Tests\Fixtures\FooEnum;
 
 class InputBagTest extends TestCase
 {
+    use ExpectDeprecationTrait;
+
     public function testGet()
     {
         $bag = new InputBag(['foo' => 'bar', 'null' => null, 'int' => 1, 'float' => 1.0, 'bool' => false, 'stringable' => new class() implements \Stringable {
@@ -36,6 +39,58 @@ public function __toString(): string
         $this->assertFalse($bag->get('bool'), '->get() gets the value of a bool parameter');
     }
 
+    /**
+     * @group legacy
+     */
+    public function testGetIntError()
+    {
+        $this->expectDeprecation('Since symfony/http-foundation 6.3: Ignoring invalid values when using "Symfony\Component\HttpFoundation\InputBag::getInt(\'foo\')" is deprecated and will throw a "Symfony\Component\HttpFoundation\Exception\BadRequestException" in 7.0; use method "filter()" with flag "FILTER_NULL_ON_FAILURE" to keep ignoring them.');
+
+        $bag = new InputBag(['foo' => 'bar']);
+        $result = $bag->getInt('foo');
+        $this->assertSame(0, $result);
+    }
+
+    /**
+     * @group legacy
+     */
+    public function testGetBooleanError()
+    {
+        $this->expectDeprecation('Since symfony/http-foundation 6.3: Ignoring invalid values when using "Symfony\Component\HttpFoundation\InputBag::getBoolean(\'foo\')" is deprecated and will throw a "Symfony\Component\HttpFoundation\Exception\BadRequestException" in 7.0; use method "filter()" with flag "FILTER_NULL_ON_FAILURE" to keep ignoring them.');
+
+        $bag = new InputBag(['foo' => 'bar']);
+        $result = $bag->getBoolean('foo');
+        $this->assertFalse($result);
+    }
+
+    public function testGetString()
+    {
+        $bag = new InputBag(['integer' => 123, 'bool_true' => true, 'bool_false' => false, 'string' => 'abc', 'stringable' => new class() implements \Stringable {
+            public function __toString(): string
+            {
+                return 'strval';
+            }
+        }]);
+
+        $this->assertSame('123', $bag->getString('integer'), '->getString() gets a value of parameter as string');
+        $this->assertSame('abc', $bag->getString('string'), '->getString() gets a value of parameter as string');
+        $this->assertSame('', $bag->getString('unknown'), '->getString() returns zero if a parameter is not defined');
+        $this->assertSame('foo', $bag->getString('unknown', 'foo'), '->getString() returns the default if a parameter is not defined');
+        $this->assertSame('1', $bag->getString('bool_true'), '->getString() returns "1" if a parameter is true');
+        $this->assertSame('', $bag->getString('bool_false', 'foo'), '->getString() returns an empty empty string if a parameter is false');
+        $this->assertSame('strval', $bag->getString('stringable'), '->getString() gets a value of a stringable paramater as string');
+    }
+
+    public function testGetStringExceptionWithArray()
+    {
+        $bag = new InputBag(['key' => ['abc']]);
+
+        $this->expectException(BadRequestException::class);
+        $this->expectExceptionMessage('Input value "key" contains a non-scalar value.');
+
+        $bag->getString('key');
+    }
+
     public function testGetDoesNotUseDeepByDefault()
     {
         $bag = new InputBag(['foo' => ['bar' => 'moo']]);
@@ -126,4 +181,34 @@ public function testGetEnumThrowsExceptionWithInvalidValue()
 
         $this->assertNull($bag->getEnum('invalid-value', FooEnum::class));
     }
+
+    public function testGetAlnumExceptionWithArray()
+    {
+        $bag = new InputBag(['word' => ['foo_BAR_012']]);
+
+        $this->expectException(BadRequestException::class);
+        $this->expectExceptionMessage('Input value "word" contains a non-scalar value.');
+
+        $bag->getAlnum('word');
+    }
+
+    public function testGetAlphaExceptionWithArray()
+    {
+        $bag = new InputBag(['word' => ['foo_BAR_012']]);
+
+        $this->expectException(BadRequestException::class);
+        $this->expectExceptionMessage('Input value "word" contains a non-scalar value.');
+
+        $bag->getAlpha('word');
+    }
+
+    public function testGetDigitsExceptionWithArray()
+    {
+        $bag = new InputBag(['word' => ['foo_BAR_012']]);
+
+        $this->expectException(BadRequestException::class);
+        $this->expectExceptionMessage('Input value "word" contains a non-scalar value.');
+
+        $bag->getDigits('word');
+    }
 }