[Validator] Added allowEmpty option to File constraint #10615
Conversation
megazoll
commented
Apr 2, 2014
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | |
| License | MIT |
| Doc PR | symfony/symfony-docs#3746 |
| public $notFoundMessage = 'The file could not be found.'; | ||
| public $notReadableMessage = 'The file is not readable.'; | ||
| public $maxSizeMessage = 'The file is too large ({{ size }} {{ suffix }}). Allowed maximum size is {{ limit }} {{ suffix }}.'; | ||
| public $mimeTypesMessage = 'The mime type of the file is invalid ({{ type }}). Allowed mime types are {{ types }}.'; | ||
| public $allowEmptyMessage = 'The file could not be empty.'; |
There was a problem hiding this comment.
probably better $disallowEmptyMessage
There was a problem hiding this comment.
The string could be Empty file is not allowed.
There was a problem hiding this comment.
Usually messages vars are equals with their options but with postfix.
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraints/Image.php#L40
There was a problem hiding this comment.
I agree with @romainneutron this name is confusing. One could also rename the option to disallowEmpty to avoid the problem.
| @@ -59,6 +59,7 @@ CHANGELOG | |||
| * added `Util\PropertyPath` | |||
| * made the PropertyAccess component an optional dependency | |||
| * deprecated `ValidatorBuilder::setPropertyAccessor()` | |||
| * added `allowEmpty` to File validator | |||
There was a problem hiding this comment.
Can you move this to a new 2.6 section?
|
What is the use-case for this? Uploading a file with only 1 byte is probably also invalid in your case. So after all the file must be validated anyway. And a more general |
|
I agree with Fabien that minSize is also awkward. Also the message could then include an explanation of the possible problem like: "Empty file is not allowed. Maybe it's locked by another program." |
|
Closing for reasons exposed by @Tobion |
|
@megazoll would you make a PR for the different fix? |