NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:4.4fromxabbuh:issue-32045

Nov 27, 2020

Merged

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

fabpot merged 1 commit intosymfony:4.4fromxabbuh:issue-32045

Nov 27, 2020

Conversation

Copy link

Member

xabbuh commentedNov 19, 2020

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

xabbuh added this to the4.4 milestone

Nov 19, 2020

xabbuh requested a review fromyceruto as acode owner

November 19, 2020 18:57

carsonbot added Status: Needs Review Form Bug labels

Nov 19, 2020

This was referencedNov 19, 2020

File Constraint not overwrite custom message#39107

Closed

[Form] Double file size validation when file is beyond the 'upload_max_filesize' parameter#32045

Closed

Duplicate uploadIniSizeError if entity form is nested in the form of another entity#36503

Closed

xabbuh force-pushed theissue-32045 branch fromcbaf76e tob38ba40Compare

November 19, 2020 18:59

yceruto approved these changes

Nov 19, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Nov 19, 2020

Copy link

thewalkingcoder commentedNov 20, 2020

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

Copy link

MemberAuthor

xabbuh commentedNov 20, 2020

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

Copy link

thewalkingcoder commentedNov 20, 2020

@xabbuh ok i try to create that today.

Copy link

thewalkingcoder commentedNov 20, 2020

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

stof reviewed

Nov 20, 2020

View reviewed changes

src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php Outdated


		// 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

MemberAuthor

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 theFile constraint we could potentially remove an error here that we would have to keep.

Copy link

MemberAuthor

xabbuh commentedNov 20, 2020

Status: Needs work

carsonbot added Status: Needs Work and removed Status: Reviewed labels

Nov 20, 2020

prevent duplicated error message for file upload limits

fe6a2dd

xabbuh force-pushed theissue-32045 branch fromb38ba40 tofe6a2ddCompare

November 21, 2020 10:45

Copy link

MemberAuthor

xabbuh commentedNov 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?

Copy link

MemberAuthor

xabbuh commentedNov 21, 2020

Status: Needs Review

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Nov 21, 2020

Copy link

thewalkingcoder commentedNov 21, 2020

@xabbuh thanks for your works.

Your commit resolve the duplicated message and so resolve#39107

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

normally is

Custom message max size.

other issue ? or side effect with the PR ?

Copy link

MemberAuthor

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 theFileValidator will not perform any additional checks.

Copy link

thewalkingcoder commentedNov 21, 2020•
edited
Loading

@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

Copy link

MemberAuthor

xabbuh commentedNov 22, 2020

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