[Form] Moved POST_MAX_SIZE validation from FormValidator to request handler #11924
Conversation
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #11729, #11877 |
| License | MIT |
| Doc PR | - |
47f5fde to
c9bda8b
Compare
| array('{{ max }}' => $this->serverParams->getNormalizedIniPostMaxSize()) | ||
| )); | ||
|
|
||
| // The form stays unsubmitted, since we have no data to submit |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Should we submit it as empty form and suppress all other validation to prevent further error messages?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Just to confirm: This means that every 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?
c9bda8b to
49abec9
Compare
|
ping @symfony/deciders |
49abec9 to
b7ead29
Compare
|
👍 |
| @@ -53,6 +68,25 @@ public function handleRequest(FormInterface $form, $request = null) | |||
| $data = $request->query->get($name); | |||
| } | |||
| } else { | |||
| // Mark the form with an error if the uploaded size was too large | |||
There was a problem hiding this comment.
This code block is duplicated. So I guess one could refactor this. E.g. HttpFoundationRequestHandler extends NativeRequestHandler and then overwrites certains methods.
There was a problem hiding this comment.
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.
|
👍 |
|
Oh wait, this is for 2.3? So I don't agree to deprecate |
|
@Tobion Good point, thanks! I'll remove the deprecation note. I moved |
b7ead29 to
759ae1a
Compare
|
Updated. |
|
Thank you @webmozart. |
…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 handler 4780210 [Form] Add a form error if post_max_size has been reached.
…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
|
I've updated to Errors that are added via an event listener in the FormType do also still occur. So it seems |
|
I was curious to see that error and also tried triggering it by setting |
…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
…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 symfony/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