Refactor Dir filter to implement FlterInterface #216
Conversation
There was a problem hiding this comment.
Sorry @marcelthole - reviewed a couple of days ago and never submitted it 🤦♂️
| { | ||
| /** | ||
| * Defined by Laminas\Filter\FilterInterface | ||
| * | ||
| * Returns dirname($value) | ||
| * | ||
| * @psalm-return ($value is scalar ? string : mixed) | ||
| */ | ||
| public function filter(mixed $value): mixed | ||
| { |
There was a problem hiding this comment.
Can you change this conditional to if (! is_string($value) && ! is_numeric($value)) and amend the tests to suit please?
I can nearly see a use case for numeric directory names, but casting bool to string and finding the parent directory? Just weird.
There was a problem hiding this comment.
As this is a refactor, test clean up would be appreciated too:
- Change loops to data providers and include more cases
- Ensure
__invokeis covered
There was a problem hiding this comment.
I updated the testcase.
For the numeric value i made this filter more strict and only allow strings because i guess that the behavior with numeric values is a little bit strange. https://3v4l.org/5rHL2
i didn't expect to get the value "." as string if i pass 1 as integer value to the filter. If i really want to use that filter for the value i should cast it to a string on my side
There was a problem hiding this comment.
Yes, dirname will return '.' for anything in getcwd, any non-string input and any non-existent directory/file. It's only remotely useful if it receives something that looks like a fully qualified path, existing on disk or not.
As it stands, the Dir filter is not really useful at all as far as I can tell.
Signed-off-by: Marcel Thole <[email protected]>
marcelthole
force-pushed
the
refactor-dir-filter
branch
from
aa957ac to
527193b
Compare
| self::assertSame($expected, $filter->filter($input)); | ||
| } | ||
|
|
||
| public static function defaultSettingsDataProvider(): array |
There was a problem hiding this comment.
For future reference, please annotate data providers with a precise return types:
| public static function defaultSettingsDataProvider(): array | |
| /** @return list<array{0: string, 1: string}> */ | |
| public static function defaultSettingsDataProvider(): array |
This helps document intent, especially when new items are added or when a larger number of arguments are required.
Description
Move Dir filter also to the implementation via FilterInterface and removed the AbstractFilter