Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
COil opened this issue Jun 14, 2019 · 11 comments
Closed

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

COil opened this issue Jun 14, 2019 · 11 comments
Labels
Bug Form Status: Needs Review

Comments

@COil
Copy link
Contributor

COil commented Jun 14, 2019 •
edited
Loading

Symfony version(s) affected: 4.3.1

Description
I have a double file size validation when file is beyond the 'upload_max_filesize' parameter.
In this case the max size is set to 500ko in the form and the max size allowed by php 2mo. Additionally the error message isn't translated.

sf-bug

How to reproduce

/**
 * Add or edit the avatar.
 */
class AvatarFormType extends AbstractType
{
    private const IMAGE_MAX_SIZE = 512000;  
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder->add('avatar', FileType::class, [
            'required' => true,
            'constraints' => [
                new NotBlank(),
                new Image([
                    'maxSize' => self::IMAGE_MAX_SIZE,
                ]),
            ],
        ]);
    }
}

and php setting :
upload_max_filesize | 2M | 2M

Possible Solution
Detect if a maxSize constraint already exists and is below the upload_max_filesize value?

See you. COil.
@COil COil changed the title Double file size validation when file is beyond the 'upload_max_filesize' parameter [FORM] Double file size validation when file is beyond the 'upload_max_filesize' parameter Jun 14, 2019
@COil COil changed the title [FORM] Double file size validation when file is beyond the 'upload_max_filesize' parameter [Form] Double file size validation when file is beyond the 'upload_max_filesize' parameter Jun 14, 2019
@xabbuh
Copy link
Member

xabbuh commented Jun 15, 2019

The fact that the message is not translated looks like a bug to me. Can you check in the profiler if it reports any missing translation?

Though I wonder why the size constraint is triggered at all. If the file cannot be uploaded, this shouldn't happen.

@COil
Copy link
Contributor Author

COil commented Jun 17, 2019

Hi Xabbuh, well it's weird, look at this, in the validator panel I can only see my constraint :

bug02

But in the form panel, I can see the second message but there isn't constraint violation associated to it (at least in the debug panel)

bug01

There isn't i18n warning in the debug bar regarding the message.

@xabbuh
Copy link
Member

xabbuh commented Jun 17, 2019

That the filesize check is not listed in the validator panel is expected as that error is caught on the form level and attached as an error to the form without using the validator component.

I am not sure how we would solve the Image constraint validation though. We could skip validation if the file is an UploadedFile instance whose error is not 0. However, that would require hardcoding knowledge about the HttpFoundation in the validator component.

@COil
Copy link
Contributor Author

COil commented Jun 19, 2019 •
edited
Loading

hi Xabbuh. So, what's your suggestion? I can try to debug this tomorrow.

Why FileType doesn't use the public constants of File for translations?

How to pass the translator object to the file type? The translator object is null, that's why the message isn't translated.

Someone had the same the same problem :

@xabbuh xabbuh mentioned this issue Jun 20, 2019
Merged
@xabbuh
Copy link
Member

xabbuh commented Jun 20, 2019

@COil I found out why the translation didn't happen (see #32116).

Though I have no idea yet how to prevent the double error message.

fabpot added a commit that referenced this issue Jun 20, 2019
@fabpot
bug #32116 [FrameworkBundle] tag the FileType service as a form type …
370682c 
…(xabbuh)

This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle] tag the FileType service as a form type

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | part of #32045
| License       | MIT
| Doc PR        |

In #30961 we undeprecated the `form.type.file` service as we need to pass the translator to the form type. But we forgot to add back the `form.type` tag which means that the form registry actually never used our service but instantiated the `FileType` itself and thus the type was never able to use a translator.

Commits
-------

ea5b1f4 tag the FileType service as a form type
@COil
Copy link
Contributor Author

COil commented Jun 20, 2019 •
edited
Loading

@xabbuh there is also an error in the translation domain :

Should be :

$message = $this->translator->trans($messageTemplate, $messageParameters, 'validators');

in FileType.php.

I have tested and it works now with this last fix.

@xabbuh xabbuh mentioned this issue Jun 20, 2019
Merged
xabbuh added a commit that referenced this issue Jun 21, 2019
@xabbuh
bug #32123 [Form] fix translation domain (xabbuh)
4c088b6 
This PR was merged into the 3.4 branch.

Discussion
----------

[Form] fix translation domain

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32045 (comment)
| License       | MIT
| Doc PR        |

Commits
-------

74387cf fix translation domain
@o-alquimista
Copy link

@COil Did you find a workaround for the duplicated error message? I can reproduce this.

@COil
Copy link
Contributor Author

COil commented Oct 10, 2019

No. :) As it's not critical. But may be you can change the 'upload_max_filesize' on the fly just for the action.

@COil
Copy link
Contributor Author

COil commented Oct 19, 2019 •
edited
Loading

Just a comment, obviously we can't change the upload_max_filesize ini setting at runtime so this trick will not function.

@xabbuh xabbuh mentioned this issue Apr 20, 2020
Closed
@lugosium
Copy link

lugosium commented Jun 7, 2020

Hello,

Got duplicate error message too.

This is confusing for the user i think, If i limit the image to 2mo, users does not have to know my upload_max_filesize value.

The only case is when the upload_max_filesize value is lower than maxSize.

@xabbuh xabbuh mentioned this issue Nov 19, 2020
Merged
@xabbuh
Copy link
Member

xabbuh commented Nov 19, 2020

Can you please check if #39119 fixes this?

@fabpot fabpot closed this as completed Nov 27, 2020
fabpot added a commit that referenced this issue Nov 27, 2020
@fabpot
bug #39119 [Form] prevent duplicated error message for file upload li…
66e76d1 
…mits (xabbuh)

This PR was merged into the 4.4 branch.

Discussion
----------

[Form] prevent duplicated error message for file upload limits

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

Commits
-------

fe6a2dd prevent duplicated error message for file upload limits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Form Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@fabpot @javiereguiluz @COil @xabbuh @lugosium @carsonbot @o-alquimista