Add PSR-7 support for FileValidator #15421
Conversation
|
For adding a test, you could create a mock UploadedFileInterface object that returns credible values for the method used in the validator. |
|
the way this is implemented means that supporting PSR-7 requires to have HttpFoundation installed. Wouldn't it be better to support using PSR-7 directly rather than converting it to HrrpFoundation ? |
| @@ -51,6 +52,10 @@ public function validate($value, Constraint $constraint) | |||
| return; | |||
| } | |||
|
|
|||
| if ($value instanceof UploadedFileInterface) { | |||
| $value = new UploadedFile('', $value->getClientFilename(), $value->getClientMediaType(), $value->getSize(), $value->getError()); | |||
There was a problem hiding this comment.
passing an empty path looks wrong here. It will break things when retrieving the path to the file later in the validation logic to perform the assertions
There was a problem hiding this comment.
If this approach is followed, wouldn't it be better to use the bridge instead of duplicating the logic?
But I agree with @stof, a validator without dependency to HttpFoundation would be better (better performance, less complexity and dependencies).
|
Take a look at the PSR-7 bridge for tests examples. |
595c0c9 to
745094a
Compare
|
Looked at the code again after my week off and discovered that 90% of the validator is about files in the filesystem and only a small section about uploaded files. The I've now rewritten some lines in the validator so that the upload errors are now validated for both HttpFoundatio 8000 n's uploaded file as PSR-7 uploaded file. I'm not sure if this is really the direction we want to go, as this means the FileValidator without HttpFoundation is quite useless (it's the mime type validation and such that make this validator interesting). |
| @@ -292,6 +307,33 @@ private static function moreDecimalsThan($double, $numberOfDecimals) | |||
| return strlen((string) $double) > strlen(round($double, $numberOfDecimals)); | |||
| } | |||
|
|
|||
| private static function getMaxFilesize() | |||
There was a problem hiding this comment.
This method is equal to HttpFoundation\File\UploadedFile::getMaxFilesize(). It exists here in order to remove the dependency on HttpFoundation for creating the error message.
There was a problem hiding this comment.
But we still have a dependency on the component, so what's the advantage?
There was a problem hiding this comment.
nevermind - it's a dev dependency. I'm with you :)
745094a to
fad4551
Compare
| @@ -29,7 +29,8 @@ | |||
| "symfony/expression-language": "~2.4|~3.0.0", | |||
| "doctrine/annotations": "~1.0", | |||
| "doctrine/cache": "~1.0", | |||
| "egulias/email-validator": "~1.2,>=1.2.1" | |||
| "egulias/email-validator": "~1.2,>=1.2.1", | |||
| "psr/http-message": "~1.0" | |||
There was a problem hiding this comment.
Shouldn't this go into suggest too?
There was a problem hiding this comment.
It needs to be here because we reference the Psr UploadedFileInterface
There was a problem hiding this comment.
I've now also added it to the suggests section as well.
|
ping @symfony/deciders What about this one? I don't this we should bother too much about this now. So, I'm -0 |
|
👍 on my side. It doesn't mean I'm yet convinced by psr-7 but this opens the use cases with very small changes. |
|
I don't really care either way on this one either - but it is a small patch to accomplish this. So 👍 - it's consistent with our decision to add support PSR-7 to components when we can do so without breaking BC. |
| return new FileValidator(); | ||
| } | ||
|
|
||
| public function testValidUploadedFile() |
There was a problem hiding this comment.
why not putting them in the FileValidatorTest class directly ?
There was a problem hiding this comment.
There is not one FileValidatorTest, there are 2: FileValidatorObjectTest, FileValidatorPathTest, both extending FileValidatorTest. It makes no sense to put it in FileValidatorTest, as it would be run twice with exact the same input/output.
fad4551 to
a6950c3
Compare
|
Re-reading this PR now and I think I'm 👎 now. If you are using PSR-7, you don't have mime-type validation, size validation, ... Having a validator that does not do the same thing for PSR-7 and HttpFoundation looks dangerous to me, especially as there is nothing warning the developer about the different behavior. As we decided to not support PSR-7 in 3.0, I propose to close this one. |
|
Ah, good point. I agree - unless they work perfectly the same with both Request inputs, it seems like a bad idea. You could even configure some mimeType stuff, but then have it totally ignored. |
I couldn't find an easy way to add tests for this, as the base tests are about files in the filesystem and not uploaded files.