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

[Form] Moved POST_MAX_SIZE validation from FormValidator to request handler#11924

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 2 commits intosymfony:2.3fromwebmozart:issue11877

Sep 24, 2014

Merged

[Form] Moved POST_MAX_SIZE validation from FormValidator to request handler#11924

fabpot merged 2 commits intosymfony:2.3fromwebmozart:issue11877

Sep 24, 2014

Conversation

Copy link

Contributor

webmozart commentedSep 15, 2014

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#11729,#11877
License	MIT
Doc PR	-

webmozart mentioned this pull request

Sep 15, 2014

[Form] Add a form error if post_max_size has been reached.#11877

Closed

webmozart force-pushed theissue11877 branch from47f5fde toc9bda8bCompare

September 15, 2014 14:51

stof reviewed

Sep 15, 2014

View reviewed changes

src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php Outdated

Copy link

Member

Choose a reason for hiding this comment

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

this looks wrong to me. It should be marked as submitted but invalid IMO, as this case means that the client tried to submit it

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Should we submit it as empty form and suppress all other validation to prevent further error messages?

Copy link

Member

Choose a reason for hiding this comment

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

Well, submitting it as an empty form would make thing more complex in case of an edit form, as it would empty all fields rather than keeping the previous values.
I think we should mark the form as submitted, but without submitting the data for children (it could rely on the partial binding though)

Copy link

ContributorAuthor

webmozartSep 15, 2014

Choose a reason for hiding this comment

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

Exactly. If we disable the$clearMissing parameter, as in PATCH requests, that should work. I don't really want to add a way for marking a form as submitted, that could be misused quite heavily.

Copy link

ContributorAuthor

webmozartSep 17, 2014

Choose a reason for hiding this comment

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

Just to confirm: This means thatevery form with a matching request method (for example, if there are multiple POST forms on one page) will show the error and report being submitted. Is this desired?

Copy link

ContributorAuthor

webmozartSep 23, 2014

Choose a reason for hiding this comment

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

Updated now.

stof added the Form label

Sep 20, 2014

[Form] Add a form error if post_max_size has been reached.

4780210

webmozart force-pushed theissue11877 branch fromc9bda8b to49abec9Compare

September 23, 2014 11:13

Copy link

ContributorAuthor

webmozart commentedSep 23, 2014

ping @symfony/deciders

webmozart force-pushed theissue11877 branch from49abec9 tob7ead29Compare

September 23, 2014 11:14

Copy link

Member

fabpot commentedSep 23, 2014

👍

Tobion reviewed

Sep 23, 2014

View reviewed changes

src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php

Copy link

Contributor

TobionSep 23, 2014

Choose a reason for hiding this comment

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

This code block is duplicated. So I guess one could refactor this. E.g. HttpFoundationRequestHandler extends NativeRequestHandler and then overwrites certains methods.

Copy link

ContributorAuthor

webmozartSep 23, 2014

Choose a reason for hiding this comment

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

Yes, there are a lot of similarities between the two request handlers. On the other hand, there are lots of tiny differences, so we'd end up with a pretty complex class + control flow if we remove all the duplications. Both classes are covered by the same test case, so if tests are added, both classes need to be adapted.

Feel free to try to remove the duplication in a new PR, there we can discuss how to do that well and whether it is really worth it.

Copy link

Contributor

Tobion commentedSep 23, 2014

👍

Copy link

Contributor

Tobion commentedSep 23, 2014

Oh wait, this is for 2.3? So I don't agree to deprecateForm/Extension/Validator/Util/ServerParams in a maintainance branch. Why is that necessary?

Copy link

ContributorAuthor

webmozart commentedSep 23, 2014

@Tobion Good point, thanks! I'll remove the deprecation note.

I movedServerParams to the globalUtil namespace since it's not just used for theValidatorExtension now.

[Form] Moved POST_MAX_SIZE validation from FormValidator to request h…

759ae1a

…andler

webmozart force-pushed theissue11877 branch fromb7ead29 to759ae1aCompare

September 23, 2014 12:27

Copy link

ContributorAuthor

webmozart commentedSep 23, 2014

Updated.

Copy link

Member

fabpot commentedSep 24, 2014

Thank you@webmozart.

fabpot merged commit759ae1a intosymfony:2.3

Sep 24, 2014

fabpot added a commit that referenced this pull request

Sep 24, 2014

bug#11924[Form] Moved POST_MAX_SIZE validation from FormValidator t…

6ad5d31

…o request handler (rpg600, webmozart)This PR was merged into the 2.3 branch.Discussion----------[Form] Moved POST_MAX_SIZE validation from FormValidator to request handler| Q             | A| ------------- | ---| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#11729,#11877| License       | MIT| Doc PR        | -Commits-------759ae1a [Form] Moved POST_MAX_SIZE validation from FormValidator to request handler4780210 [Form] Add a form error if post_max_size has been reached.

webmozart mentioned this pull request

Sep 25, 2014

[Form] Removed constructor argument from FormTypeHttpFoundationExtension for forward compatibility with 2.5#12033

Merged

fabpot added a commit that referenced this pull request

Sep 25, 2014

minor#12033[Form] Removed constructor argument from FormTypeHttpFou…

b2524d4

…ndationExtension for forward compatibility with 2.5 (webmozart)This PR was merged into the 2.3 branch.Discussion----------[Form] Removed constructor argument from FormTypeHttpFoundationExtension for forward compatibility with 2.5| Q             | A| ------------- | ---| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -This argument was introduced in#11924. No release was made of the 2.3 branch after merging that PR.Since a different constructor argument (`$requestHandler`) was added to FormTypeHttpFoundationExtension in the 2.5 branch, we cannot merge this forward in a BC fashion. For this reason, I removed the argument again.Commits-------6cbc862 [Form] Removed constructor argument from FormTypeHttpFoundationExtension for forward compatibility with 2.5

Copy link

Contributor

rvanlaak commentedSep 29, 2014

I've updated to2.5.5 and tested this implementation. The MAX_POST_SIZE error is added, but the CSRF-invalid error also gets added. So, it seems this check occurs after the CSRF-check (?).

Errors that are added via an event listener in the FormType do also still occur.

$builder->addEventListener(FormEvents::POST_SET_DATA, array($this, 'isFiletypeValid'));

So it seemsFormEvents::POST_SET_DATA is still triggered when the form already has an error. Does the latter event also trigger the CSRF-check?

Copy link

Contributor

craue commentedSep 30, 2014

I was curious to see that error and also tried triggering it by settingpost_max_size = 1M inphp.ini and uploading a larger file. But may it be with or without this PR, all I get is a plain PHP warning likeWarning: POST Content-Length of 4242283 bytes exceeds the limit of 1048576 bytes in Unknown on line 0 printed on top of the rendered page. So in which case is this error message meant to be shown?

webmozart deleted the issue11877 branch

October 22, 2014 17:20

jakzal mentioned this pull request

Jan 30, 2015

Form using Validator Constraint File - Max File Size not validating correctly, element returns NULL#13291

Closed

gharlan mentioned this pull request

Sep 3, 2015

Message post_max_size_message not translated#15479

Closed

Tobion mentioned this pull request

Oct 2, 2015

[Form] Forward Form::submit(Request) to Form::HandleRequest#16088

Closed

DavidBadura mentioned this pull request

Apr 15, 2016

[FORM] fix post_max_size_message translation#18543

Closed

HeahDude mentioned this pull request

Apr 11, 2020

[Form] Deprecated unused oldServerParams util#36426

Merged

fabpot added a commit that referenced this pull request

Apr 12, 2020

feature#36426[Form] Deprecated unused oldServerParamsutil (Heah…

7800a2a

…Dude)This PR was merged into the 5.1-dev branch.Discussion----------[Form] Deprecated unused old `ServerParams` util| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| Deprecations? | yes| Tickets       | ~| License       | MIT| Doc PR        | ~A left over after#11924 (comment), this class is unused since Symfony 2.3 ^^'. I've noticed it before but never took the time to submit a PR because this is very minor IMHO. But since this deprecation should not impact anyone, let's keep the codebase clean and remove it in 6.0.Commits-------e05e924 [Form] Deprecated unused old `ServerParams` util

symfony-splitter pushed a commit to symfony/form that referenced this pull request

Apr 12, 2020

feature #36426 [Form] Deprecated unused oldServerParams util (Heah…

bf4b3a8

…Dude)This PR was merged into the 5.1-dev branch.Discussion----------[Form] Deprecated unused old `ServerParams` util| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| Deprecations? | yes| Tickets       | ~| License       | MIT| Doc PR        | ~A left over aftersymfony/symfony#11924 (comment), this class is unused since Symfony 2.3 ^^'. I've noticed it before but never took the time to submit a PR because this is very minor IMHO. But since this deprecation should not impact anyone, let's keep the codebase clean and remove it in 6.0.Commits-------e05e924c5a [Form] Deprecated unused old `ServerParams` util

Labels

Form

Movatterモバイル変換

Uh oh!

[Form] Moved POST_MAX_SIZE validation from FormValidator to request handler#11924

[Form] Moved POST_MAX_SIZE validation from FormValidator to request handler#11924

Uh oh!

Conversation

webmozart commentedSep 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmozart commentedSep 23, 2014

Uh oh!

fabpot commentedSep 23, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tobion commentedSep 23, 2014

Uh oh!

Tobion commentedSep 23, 2014

Uh oh!

webmozart commentedSep 23, 2014

Uh oh!

webmozart commentedSep 23, 2014

Uh oh!

fabpot commentedSep 24, 2014

Uh oh!

rvanlaak commentedSep 29, 2014

Uh oh!

craue commentedSep 30, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants