[RFC] Clean up AllowList Filter as an example of general modernisation of all filters

[gsteel]

This patch is indicative of the plans for all the shipped filters where feasible:

• Removes inheritance
• Private properties instead of protected
• Native return types
• Native parameter types
• Options can no longer be Traversable

The reason options can no longer be Traversable is because we can rely on static analysis by using the documented array shape. Allowing Traversable will require a lot of type juggling and coercion, and it becomes difficult to know where to stop, for example, given the strict option here, previously, at least bool|int<0,1>|'0'|'1' were acceptable, so why not also 'y'|'n'|'yes'|'no'|'true'|'false' too? We can all agree that native types are an improvement, so we might as well get rid of the idea that Options arrays|Traversables can have mixed values too.

Questions…

Option value setters and getters. Can we get rid of them?

It's unlikely that filters are mutated at runtime. Most consumers will be using them via an InputFilter as part of an array spec such as:

[
  'filters' => [
    [
      'name' => AllowList::class,
      'options' => [
        'strict' => true,
        'list' => ['some', 'list'],
      ],
    ],
  ],
];

Handling the option values in __construct and setOptions seems sufficient, and the getters just feel like a testing convenience.

We'll also still have runtime type safety via native property types.

Inheritance Removal…

… might be invalid in this case. I can think of 2 reasons why a parent abstract might be useful:

• To provide __invoke(mixed):mixed (without useful type inference) that just proxies to filter()
• To call setOptions during __construct.

Therefore, might it be better to strip everything else out of the AbstractFilter bar these 2 functions?

Introduce an AcceptsOptionsInterface

something along the lines of

/** @template Options of array<string, mixed> */
interface AcceptsOptionsInterface {
  
  /** @param Options $options */
  public function setOptions(array $options): self; /** preferably void */
  
}

This patch is cut from #117 so it'll be easier to review just fab100b in isolation.

[boesing]

Overall, having final around works for me pretty well.

I'd remove setOptions and/or all the setter methods at all.

All these mutable classes are painful and when I have a look into our project, no1 ever used those setters.

Having immutable filters makes absolutely sense to me and thus I'd be fine with changing that. But if we want to keep setter methods, lets at least remove setOptions, no?

[gsteel]

Gentle nudge for review from @boesing and/or @Ocramius 🫣

[Ocramius]

Shouldn't this be TFilteredValue&T?

We're trying to restrict the type, not expand it 🤔

This can be verified wtih $v = (new AllowList(['list' => ['a', 'b', 'c']))->filter(123), which becomes 123|'a'|'b'|'c'|null here, while we want 'a'|'b'|'c'|null to be the actual returned type.

I believe we need to check this a bit better 🔍

[Ocramius]

Is every filter supposed to return |null BTW? Unsure about that detail...

[gsteel]

This filter is a bit odd. Typically, filter failure returns the un-filtered value, so $stringToLower->filter($someArray): array. So, StringToLower for example would be @implements FilterInterface<string> and return string|T - this AllowList filter is odd because in a success scenario, it returns T vs null in a failure scenario, hence the reversal of the failure/success type.

The AllowList filter specifically could be improved with a template for the list perhaps something like:

/**
 * @template MyList of list<mixed>
 * @implements FilterInterface<value-of<MyList>>
 */
final class AllowList implements FilterInterface
{
  /** @param MyList $list */
  public function __construct(private array $list) {}
}

And I guess we'd then need to override the filter method docs to return TFilteredValue|null. That would be more precise right?

I assumed that the current docs as they are here would have yielded:

$v = (new AllowList(['list' => ['a', 'b', 'c']))->filter('a'); // 'a'|null
$v = (new AllowList(['list' => ['a', 'b', 'c']))->filter('z'); // 'z'|null

[gsteel]

Actually, I don't think it's possible to restrict the return type to members of the list, and it does currently restrict the type to T|null which accurately describes the behaviour of the filter.

Adding a template to describe the list is the only way I know of to get @return value-of<TList> to work, i.e. @return value-of<$this->property> doesn't work, (or I'm wrong 🤷‍♂️) - If I use a template, you'd have to annotate every usage of the filter with @var AllowList<list{'a'|'b'|'c'}> right?

I might need a bit of help here to refine the filter to return anything beyond T|null

[Ocramius]

Is value-of<TList> not an actual template type?

I need to think of it myself too btw, just thinking that current union type is a bit too wide

[Ocramius]

@gsteel does this example of yours work BTW?

/**
 * @template MyList of list<mixed>
 * @implements FilterInterface<value-of<MyList>>
 */
final class AllowList implements FilterInterface
{
  /** @param MyList $list */
  public function __construct(private array $list) {}
}

I think this is very close to what we want, and verifiable via static analysis test code

[gsteel]

No - it doesn't work… I tried a bunch of things but couldn't figure out how to get inference from value-of<$this->list> (value-of<$property> is not supported afaict), also, without annotating all of the filters you use to satisfy TList, you end up with mixed anyhow, and a bunch of psalm issues for missing templates. It's not like we can put the template on the constructor either, because then inference is lost in the filter method which is where we want it - i.e. we can't do constructor property promotion because the convention of a single $options argument.

TBH, the other filters should be a lot more straightforward - I started with this one because it's the first one in the list alphabetically