Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Form] prevent duplicated error message for file upload limits #39119

Merged
merged 1 commit into from Nov 27, 2020

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Nov 19, 2020

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #32045, #36503, #39107
License MIT
Doc PR

@thewalkingcoder
Copy link

@xabbuh thanks for your pull request, as expected i try with context #39107 same issue for me.

image

@xabbuh
Copy link
Member Author

xabbuh commented Nov 20, 2020

Thank you for the feedback. Could you create a small example application that allows me to reproduce and debug the issue?

@thewalkingcoder
Copy link

@xabbuh ok i try to create that today.

@thewalkingcoder
Copy link

@xabbuh
you can try here ;-)
https://github.com/thewalkingcoder/uploadfile

@@ -124,6 +126,17 @@ public function mapViolation(ConstraintViolation $violation, FormInterface $form

// Only add the error if the form is synchronized
if ($this->acceptsErrors($scope)) {
if (File::TOO_LARGE_ERROR === $violation->getCode()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a better check could be $violation->getCause() instanceof File to account for the different error codes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. If there are multiple file upload files and one of them fails with another check of the File constraint we could potentially remove an error here that we would have to keep.

@xabbuh
Copy link
Member Author

xabbuh commented Nov 20, 2020

Status: Needs work

@xabbuh
Copy link
Member Author

xabbuh commented Nov 21, 2020

@thewalkingcoder Thanks, based on your example I was able to find a few issue with my initial solution. Can you try the updated patch again?

@xabbuh
Copy link
Member Author

xabbuh commented Nov 21, 2020

Status: Needs Review

@thewalkingcoder
Copy link

@xabbuh thanks for your works.

Your commit resolve the duplicated message and so resolve #39107
image

but if i remove

uploadIniSizeErrorMessage
    /**
     * @var array<UploadedFile>

     * @Assert\NotBlank(message="Veuillez sélectionner un fichier.")
     * @Assert\All({
     *     @Assert\File(
     *         maxSize = "500k",
     *         maxSizeMessage = "Custom message max size.",
     *         uploadFormSizeErrorMessage = "Custom message max size form",
     *     )
     * })
     */
    public $files;

error message is

image

normally is

Custom message max size.

other issue ? or side effect with the PR ?

@xabbuh
Copy link
Member Author

xabbuh commented Nov 21, 2020

If I understand you correctly, that's expected. The file couldn't be uploaded as the maximum file limit configured for PHP is reached and thus the FileValidator will not perform any additional checks.

@thewalkingcoder
Copy link

thewalkingcoder commented Nov 21, 2020

@xabbuh i say just i don't understand the behaviour

config value
php.ini 2mo
Assert maxSize 500ko
Assert maxSizeMessage Custom Max file 500ko
Assert uploadIniSizeErrorMessage Custom message Max file php ini
file.pdf 6mo
result Custom message Max file php ini

for me this PR resolve duplicated error Message.

but if i remove Assert uploadIniSizeErrorMessage

config value
php.ini 2mo
Assert maxSize 500ko
Assert maxSizeMessage Custom Max file 500ko size
file.pdf 6mo
result the file is too large. Allowed maximum size is 0.5Mo

default message maxSize is applied instead MaxSizeMessage

For me, the expected behaviour is

config value
php.ini 2mo
Assert maxSize 500ko
file.pdf 6mo
result the file is too large. Allowed maximum size is 0.5Mo (default message Assert MaxSize)
behaviour ok actually
config value
php.ini 2mo
Assert maxSize 500ko
Assert maxSizeMessage Custom Max file 500ko size
file.pdf 6mo
result Custom Max file 500ko size (custom message with maxSizeMessage)
behaviour not the same actually
config value
php.ini 2mo
Assert maxSize 500ko
Assert maxSizeMessage Custom Max file 500ko size
Assert uploadIniSizeErrorMessage Custom message Max file php ini
file.pdf 6mo
result Custom Max file 500ko size (custom message with maxSizeMessage)
behaviour not the same actually
config value
php.ini 2mo
Assert uploadIniSizeErrorMessage Custom message Max file php ini
Assert maxSize 4mo
file.pdf 6mo
result Custom message Max file php ini (custom message with uploadIniSizeErrorMessage)
behaviour ok actually

I'm not sure is a side effect with this PR

@xabbuh
Copy link
Member Author

xabbuh commented Nov 22, 2020

Well, if the file exceeds the limit configured with upload_max_filesize the uploadIniSizeErrorMessage is used. If you do not configure it explicitly, its default value The file is too large. Allowed maximum size is {{ limit }} {{ suffix }}. will be used. The message configured with maxSizeMessage is only taken into account if the file size is below the limit configured with upload_max_filesize, but exceeds the maxSize value of the File constraint. So the results that you observe look expected to me.

@fabpot
Copy link
Member

fabpot commented Nov 27, 2020

Thank you @xabbuh.

@fabpot fabpot merged commit 66e76d1 into symfony:4.4 Nov 27, 2020
@xabbuh xabbuh deleted the issue-32045 branch November 27, 2020 08:05
This was referenced Nov 29, 2020
@COil
Copy link
Contributor

COil commented Dec 5, 2020

Hi, I can confirm it is fixed in 5.1.9. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants