-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] add 'force_submit' option to FormType #18053
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
Conversation
HeahDude
commented
Mar 8, 2016
Q | A |
---|---|
Branch | master |
Bug fix? | no |
New feature? | yes |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | #16491 |
License | MIT |
Doc PR | symfony/symfony-docs#6353 |
It would be better if we could use |
$this->setRequestData($method, array()); | ||
|
||
$form->expects($this->once()) | ||
->method('submit'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should add a with()
block here.
Thanks for working on this @HeahDude! :) |
6d9895f
to
c5034e6
Compare
@webmozart alright, comments addressed! |
c5034e6
to
7748f5e
Compare
Looks good, thanks @HeahDude! :) 👍 Is anybody taking care of the failed tests? |
@webmozart thanks! I think appveyor was fixed by @nicolas-grekas in #18054 and for travis on php 7 @xabbuh should have fixed it in #18037. |
Yes, failures are unrelated. |
Imo this needs to be added to the changelog (to document the new feature) and to the upgrade file as (as people will have to update their own request handler if they built one). |
Ok. Status: Needs Work |
7748f5e
to
d354997
Compare
related to symfony/symfony#18053 Document the new option ‘force_submit’ added to FormType in 3.1 It allows request handlers to submit data that does not hold the form name.
d354997
to
9ead4ac
Compare
related to symfony/symfony#18053 Document the new option ‘force_submit’ added to FormType in 3.1 It allows request handlers to submit data that does not hold the form name.
Status: Needs Review |
@@ -18,6 +18,8 @@ Form | |||
* Support for data objects that implements both `Traversable` and `ArrayAccess` | |||
in `ResizeFormListener::preSubmit` method has been deprecated and will be | |||
removed in Symfony 4.0. | |||
* A new "force_submit" option has been added to allow request handlers to | |||
submit a form even if its name is not a key of the submitted data array. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the new option doesn't force the user to do anything when upgrading it shouldn't be added to the upgrade file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, we need to add it to the upgrade file as one needs to change their custom request handlers. Please excuse the confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not mandatory. The option is just available for anyone who wants to use it, but is totally transparent for pure BC.
Final words ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think@webmozart?
closes symfony#16491. Add a `force_submit` option to FormType with default false, allowing request handlers to submit a form even if its name is not a key of the submitted data array.
9ead4ac
to
85bf807
Compare
related to symfony/symfony#18053 Document the new option ‘force_submit’ added to FormType in 3.1 It allows request handlers to submit data that does not hold the form name.
Status: Reviewed 👍 |
I know I'm very late to this PR ... but I have some concerns about the But this option in fact just removes the form name from the submitted values. Instead of |
@javiereguiluz Hopefully one would read the docs before using a new option :) If you think otherwise, do you have a suggestion ? |
@javiereguiluz That's a very good point, thanks for pointing it out. So in effect what we are controlling is: // When
$form->handleRequest($request);
// force_submit=true: only submit if $form->getName() present in request
$form->submit($request->request->get($form->getName()));
// force_submit=true: submit even if $form->getName() not present in request
$form->submit($request->request->all()); What about As seen in this snippet, another difference is that - with the option set - we do not submit the data of the form, but the complete request. Does this make sense? Shouldn't we rather submit ping @guilhermeblanco |
Furthermore, do we really need this? So far, @guilhermeblanco was the only one who ever needed this feature, and it can easily be fixed by implementing the functionality of the request handler yourself in the controller (or as another |
@webmozart Afaics #12528 would (at least partly) be solvable with this change too. |
Yes I agree it's more accurate to use What about |
@webmozart I think the name There's an alternative, implementing my own RequestHandlerInterface, but still, it takes a couple time to digest what you did wrong when working on a day by day basis and experiencing this issue. |
What about adding |
I agree it can be useful for API in general and API with GET in particular. |
What's the final call on this ? Changing the name is easy to get done for tomorrow :) |
@HeahDude Bernhard commented the following:
So, if this option is about submitting the request, why not calling it |
I agree with all the reasoning here, I would just say I'm not against your proposal, but I don't find it explicit since all form get submitted through request then I go in circle again:
?? |
I think we're overengineering. This use case can be solved as simply as: $form = $this->createForm(/* ... */);
$form->submit($request->query->all());
if ($form->isValid()) {
// ...
} Just replace |
I may be wrong but the point of this PR is that we cannot do it if the form name is not the key holding the data to submit, because the request handler will not map it, right ? |
@webmozart What do you think about having in 4.0 some clear equivalence:
Don't get me wrong, I'm not talking about modifying the way APIs work but about compatibility of dataset and data mapping and the way view data is handled throw the global form lifecycle. |
The request handler is simply a more user friendly interface to submit(). If you have more elaborate use cases, using submit() directly is simple enough.
What do you mean? What do you see that should be improved? |
Thanks for your response :) I mean currently My guess is that it should match the real view value: |
On second thought to be able to do that, the data mapping should be done with normalized data, shouldn't it ? |