Merge pull request #118 from gsteel/allow-list

[RFC] Clean up `AllowList` Filter as an example of general modernisation of all filters
laminas · Jun 29, 2024 · 0e6a88d · 0e6a88d
2 parents db97199 + 86f91b8
commit 0e6a88d
Show file tree

Hide file tree

Showing 13 changed files with 156 additions and 136 deletions.
diff --git a/psalm-baseline.xml b/psalm-baseline.xml
@@ -23,15 +23,6 @@
       <code><![CDATA[is_object($options)]]></code>
     </RedundantConditionGivenDocblockType>
   </file>
-  <file src="src/AllowList.php">
-    <PossiblyUnusedMethod>
-      <code><![CDATA[setList]]></code>
-      <code><![CDATA[setStrict]]></code>
-    </PossiblyUnusedMethod>
-    <RedundantCastGivenDocblockType>
-      <code><![CDATA[(bool) $strict]]></code>
-    </RedundantCastGivenDocblockType>
-  </file>
   <file src="src/Callback.php">
     <MixedFunctionCall>
       <code><![CDATA[call_user_func_array($this->options['callback'], $params)]]></code>
@@ -775,17 +766,6 @@
     </PossiblyUnusedMethod>
   </file>
   <file src="test/AllowListTest.php">
-    <InvalidArgument>
-      <code><![CDATA[$strict]]></code>
-    </InvalidArgument>
-    <MixedArrayAccess>
-      <code><![CDATA[$expected]]></code>
-      <code><![CDATA[$value]]></code>
-    </MixedArrayAccess>
-    <MixedAssignment>
-      <code><![CDATA[$data]]></code>
-      <code><![CDATA[[$value, $expected]]]></code>
-    </MixedAssignment>
     <PossiblyUnusedMethod>
       <code><![CDATA[defaultTestProvider]]></code>
       <code><![CDATA[listTestProvider]]></code>
@@ -1083,26 +1063,11 @@
       <code><![CDATA[returnUnfilteredDataProvider]]></code>
     </PossiblyUnusedMethod>
   </file>
-  <file src="test/TestAsset/Alpha.php">
-    <MixedArgumentTypeCoercion>
-      <code><![CDATA[$value]]></code>
-    </MixedArgumentTypeCoercion>
-  </file>
   <file src="test/TestAsset/CallbackClass.php">
     <PossiblyUnusedMethod>
       <code><![CDATA[objectCallbackWithParams]]></code>
     </PossiblyUnusedMethod>
   </file>
-  <file src="test/TestAsset/LowerCase.php">
-    <MixedArgument>
-      <code><![CDATA[$value]]></code>
-    </MixedArgument>
-  </file>
-  <file src="test/TestAsset/StripUpperCase.php">
-    <MixedArgument>
-      <code><![CDATA[$value]]></code>
-    </MixedArgument>
-  </file>
   <file src="test/ToFloatTest.php">
     <PossiblyUnusedMethod>
       <code><![CDATA[filterableValuesProvider]]></code>

diff --git a/src/AbstractFilter.php b/src/AbstractFilter.php
@@ -21,6 +21,7 @@
 
 /**
  * @template TOptions of array
+ * @implements FilterInterface<mixed>
  */
 abstract class AbstractFilter implements FilterInterface
 {

diff --git a/src/AllowList.php b/src/AllowList.php
@@ -5,92 +5,48 @@
 namespace Laminas\Filter;
 
 use Laminas\Stdlib\ArrayUtils;
-use Traversable;
 
+use function array_values;
 use function in_array;
-use function is_array;
 
 /**
  * @psalm-type Options = array{
  *     strict?: bool,
- *     list?: array,
- *     ...
+ *     list?: iterable<array-key, mixed>,
  * }
- * @extends AbstractFilter<Options>
+ * @implements FilterInterface<null>
  */
-final class AllowList extends AbstractFilter
+final class AllowList implements FilterInterface
 {
-    /** @var bool */
-    protected $strict = false;
+    private readonly bool $strict;
+    /** @var list<mixed> */
+    private readonly array $list;
 
-    /** @var array */
-    protected $list = [];
-
-    /**
-     * @param null|array|Traversable $options
-     */
-    public function __construct($options = null)
-    {
-        if (null !== $options) {
-            $this->setOptions($options);
-        }
-    }
-
-    /**
-     * Determine whether the in_array() call should be "strict" or not. See in_array docs.
-     *
-     * @param bool $strict
-     */
-    public function setStrict($strict = true): void
+    /** @param Options $options */
+    public function __construct(array $options = [])
     {
-        $this->strict = (bool) $strict;
+        $this->strict = $options['strict'] ?? false;
+        $list         = ArrayUtils::iteratorToArray($options['list'] ?? []);
+        $this->list   = array_values($list);
     }
 
     /**
-     * Returns whether the in_array() call should be "strict" or not. See in_array docs.
-     *
-     * @return bool
-     */
-    public function getStrict()
-    {
-        return $this->strict;
-    }
-
-    /**
-     * Set the list of items to allow
-     *
-     * @param array|Traversable $list
-     */
-    public function setList($list = []): void
-    {
-        if (! is_array($list)) {
-            $list = ArrayUtils::iteratorToArray($list);
-        }
-
-        $this->list = $list;
-    }
-
-    /**
-     * Get the list of items to allow
+     * {@inheritDoc}
      *
-     * @return array
+     * Will return $value if its present in the allow-list. If $value is rejected then it will return null.
      */
-    public function getList()
+    public function filter(mixed $value): mixed
     {
-        return $this->list;
+        return in_array($value, $this->list, $this->strict) ? $value : null;
     }
 
     /**
      * {@inheritDoc}
      *
      * Will return $value if its present in the allow-list. If $value is rejected then it will return null.
-     *
-     * @template T
-     * @param T $value
-     * @return T|null
      */
-    public function filter(mixed $value): mixed
+    public function __invoke(mixed $value): mixed
     {
-        return in_array($value, $this->getList(), $this->getStrict()) ? $value : null;
+        return $this->filter($value);
     }
 }
diff --git a/src/BaseName.php b/src/BaseName.php
@@ -7,15 +7,16 @@
 use function basename;
 use function is_string;
 
-/** @psalm-immutable */
+/**
+ * @psalm-immutable
+ * @implements FilterInterface<string>
+ */
 final class BaseName implements FilterInterface
 {
     /**
      * Returns basename($value).
      *
      * If the value provided is non-string, the value will remain unfiltered
-     *
-     * @psalm-return ($value is string ? string : mixed)
      */
     public function filter(mixed $value): mixed
     {

diff --git a/src/Boolean.php b/src/Boolean.php
@@ -30,6 +30,7 @@
  *     casting: bool,
  *     translations: array<string, bool>,
  * }
+ * @implements FilterInterface<bool>
  */
 final class Boolean implements FilterInterface
 {

diff --git a/src/FilterInterface.php b/src/FilterInterface.php
@@ -4,14 +4,26 @@
 
 namespace Laminas\Filter;
 
+/** @template TFilteredValue */
 interface FilterInterface
 {
     /**
      * Returns the result of filtering $value
      *
+     * @template T
+     * @param T $value
+     * @return TFilteredValue|T
      * @throws Exception\RuntimeException If filtering $value is impossible.
      */
     public function filter(mixed $value): mixed;
 
+    /**
+     * Returns the result of filtering $value
+     *
+     * @template T
+     * @param T $value
+     * @return TFilteredValue|T
+     * @throws Exception\RuntimeException If filtering $value is impossible.
+     */
     public function __invoke(mixed $value): mixed;
 }
diff --git a/src/ForceUriScheme.php b/src/ForceUriScheme.php
@@ -16,6 +16,7 @@
 
 /**
  * @psalm-type Options = array{scheme: non-empty-string}
+ * @implements FilterInterface<string>
  */
 final class ForceUriScheme implements FilterInterface
 {

diff --git a/test/AllowListTest.php b/test/AllowListTest.php
@@ -6,11 +6,14 @@
 
 use Laminas\Filter\AllowList as AllowListFilter;
 use Laminas\Stdlib\ArrayObject;
-use Laminas\Stdlib\Exception;
 use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\TestCase;
+use Throwable;
+use TypeError;
 
+use function assert;
 use function gettype;
+use function is_array;
 use function sprintf;
 use function var_export;
 
@@ -23,16 +26,15 @@ public function testConstructorOptions(): void
             'strict' => true,
         ]);
 
-        self::assertSame(true, $filter->getStrict());
-        self::assertSame(['test', 1], $filter->getList());
+        self::assertNull($filter->filter('1'), 'Strict options infer that string 1 is not in the list');
+        self::assertSame(1, $filter->filter(1));
+        self::assertSame('test', $filter->filter('test'));
     }
 
     public function testConstructorDefaults(): void
     {
         $filter = new AllowListFilter();
-
-        self::assertSame(false, $filter->getStrict());
-        self::assertSame([], $filter->getList());
+        self::assertNull($filter->filter('anything'));
     }
 
     public function testWithPluginManager(): void
@@ -43,39 +45,49 @@ public function testWithPluginManager(): void
         self::assertInstanceOf(AllowListFilter::class, $filter);
     }
 
-    public function testNullListShouldThrowException(): void
+    public function testTraversableConvertsToArray(): void
     {
-        $this->expectException(Exception\InvalidArgumentException::class);
-        new AllowListFilter([
-            'list' => null,
+        $filter = new AllowListFilter([
+            'list' => new ArrayObject(['test', 1]),
         ]);
+
+        self::assertSame('1', $filter->filter('1'), 'The filter should be non-strict by default');
+        self::assertSame(1, $filter->filter(1));
     }
 
-    public function testTraversableConvertsToArray(): void
+    public function testSetStrictShouldBeBoolean(): void
     {
-        $array  = ['test', 1];
-        $obj    = new ArrayObject(['test', 1]);
-        $filter = new AllowListFilter([
-            'list' => $obj,
+        $this->expectException(TypeError::class);
+        /** @psalm-suppress InvalidArgument */
+        new AllowListFilter([
+            'strict' => 1,
         ]);
-        self::assertSame($array, $filter->getList());
     }
 
-    public function testSetStrictShouldCastToBoolean(): void
+    public function testListOptionShouldBeIterable(): void
     {
-        $filter = new AllowListFilter([
-            'strict' => 1,
+        /**
+         * Throwable is expected because the actual exception will come from StdLib. In future, this might/should become
+         * a TypeError
+         */
+        $this->expectException(Throwable::class);
+        /** @psalm-suppress InvalidArgument */
+        new AllowListFilter([
+            'list' => 'foo',
         ]);
-        self::assertSame(true, $filter->getStrict());
     }
 
     #[DataProvider('defaultTestProvider')]
-    public function testDefault(mixed $value): void
+    public function testFilterWillReturnNullForAnyValueWhenNoListHasBeenSupplied(mixed $value): void
     {
-        $filter = new AllowListFilter();
+        $filter = new AllowListFilter(['list' => []]);
         self::assertNull($filter->filter($value));
     }
 
+    /**
+     * @param list<mixed> $list
+     * @param array{0: mixed, 1: mixed} $testData
+     */
     #[DataProvider('listTestProvider')]
     public function testList(bool $strict, array $list, array $testData): void
     {
@@ -84,13 +96,14 @@ public function testList(bool $strict, array $list, array $testData): void
             'list'   => $list,
         ]);
         foreach ($testData as $data) {
+            assert(is_array($data));
             [$value, $expected] = $data;
             $message            = sprintf(
                 '%s (%s) is not filtered as %s; type = %s',
                 var_export($value, true),
                 gettype($value),
                 var_export($expected, true),
-                $strict
+                $strict ? 'strict' : 'non-strict',
             );
             self::assertSame($expected, $filter->filter($value), $message);
         }
@@ -108,7 +121,7 @@ public static function defaultTestProvider(): array
         ];
     }
 
-    /** @return list<array{0: bool, 1: array, 2: array}> */
+    /** @return list<array{0: bool, 1: list<mixed>, 2: list<array{0: mixed, 1: mixed}>}> */
     public static function listTestProvider(): array
     {
         return [
@@ -139,4 +152,10 @@ public static function listTestProvider(): array
             ],
         ];
     }
+
+    public function testFilterCanBeInvoked(): void
+    {
+        $filter = new AllowListFilter(['list' => ['foo']]);
+        self::assertSame('foo', $filter->__invoke('foo'));
+    }
 }