Refactor RenameUpload filter to implement FlterInterface #223
Conversation
| * @template TOptions of Options | ||
| * @extends AbstractFilter<TOptions> | ||
| * @implements FilterInterface<mixed> | ||
| * @final |
There was a problem hiding this comment.
Used this @final instead of making the class final because otherwise we could not overwrite the move_uploaded_file function.
Or do you prefer to pass a dependency via option that could change the behavior via DependencyInjection? And should the option be documented as well?
There was a problem hiding this comment.
I added the deprecated aswell for these methods here #224
There was a problem hiding this comment.
We should be able to make this final without any problems… and IMO it should be @implements FilterInterface<string>, and operate on a single upload at a time. If it allows multiple uploads, then we should change that.
| && array_key_exists('name', $value) | ||
| ) { | ||
| /** @var UploadedFile $uploadedFile */ | ||
| $uploadedFile = $value; |
There was a problem hiding this comment.
i don't know why psalm will not detect it if i add the @var annotation directly to $value
There was a problem hiding this comment.
There's a value object you can use in Filter\File\FileInformation
if (! FileInformation::isPossibleFile($value)) {
return $value; // not an upload
}
$file = FileInformation::factory($value);This should also negate the need for the other PSR related deps.
Psalms refusal to accept the forced @var is probably because of the templates
| * @psalm-suppress MixedReturnStatement | ||
| * @template T | ||
| * @param T $value | ||
| * @return (T is UploadedFileInterface |
There was a problem hiding this comment.
sadly i had to duplicate this doctype because otherwise psalm will detect the mixed return type and mark that as an error for all following calls in the Tests. This comes from the FilterInterface @return TFilteredValue|T but i could not define it strict. So i need to use @implements FilterInterface<mixed> in this class and overwrite the return type here manually
| * } | ||
| * @template TOptions of Options | ||
| * @extends RenameUpload<TOptions> | ||
| * @psalm-suppress InvalidExtendClass |
There was a problem hiding this comment.
Expected error because the RenameUpload is marked as @final
Signed-off-by: Marcel Thole <[email protected]>
marcelthole
force-pushed
the
refactor-rename-upload
branch
from
7396466 to
850defd
Compare
|
@marcelthole I'm not going to review this just yet, because I think that discussion in #222 is relevant here too |
Description
Move RenameUpload filter also to the implementation via FilterInterface and removed the AbstractFilter