Skip to content

[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

Closed
wants to merge 1 commit into from

Conversation

HeahDude
Copy link
Contributor

@HeahDude 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

@HeahDude
Copy link
Contributor Author

HeahDude commented Mar 8, 2016

It would be better if we could use $form->getConfig()->getForceSubmit() but changing FormConfigInterface would break BC.

$this->setRequestData($method, array());

$form->expects($this->once())
->method('submit');
Copy link
Contributor

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.

@webmozart
Copy link
Contributor

Thanks for working on this @HeahDude! :)

@HeahDude
Copy link
Contributor Author

HeahDude commented Mar 8, 2016

@webmozart alright, comments addressed!

@webmozart
Copy link
Contributor

Looks good, thanks @HeahDude! :) 👍

Is anybody taking care of the failed tests?

@HeahDude
Copy link
Contributor Author

HeahDude commented Mar 9, 2016

@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.

@xabbuh
Copy link
Member

xabbuh commented Mar 10, 2016

Yes, failures are unrelated.

@xabbuh
Copy link
Member

xabbuh commented Mar 10, 2016

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).

@HeahDude
Copy link
Contributor Author

Ok.

Status: Needs Work

@HeahDude HeahDude force-pushed the feature-form-force_submit branch from 7748f5e to d354997 Compare March 12, 2016 01:05
HeahDude added a commit to HeahDude/symfony-docs that referenced this pull request Mar 12, 2016
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.
@HeahDude HeahDude force-pushed the feature-form-force_submit branch from d354997 to 9ead4ac Compare March 12, 2016 06:48
HeahDude added a commit to HeahDude/symfony-docs that referenced this pull request Mar 12, 2016
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.
@HeahDude HeahDude changed the title [WIP] [Form] add 'force_submit' option [Form] add 'force_submit' option to FormType Mar 12, 2016
@HeahDude
Copy link
Contributor Author

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.
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.

Copy link
Member

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.

Copy link
Contributor Author

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 ?

Copy link
Member

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.
@HeahDude HeahDude force-pushed the feature-form-force_submit branch from 9ead4ac to 85bf807 Compare March 13, 2016 22:17
HeahDude added a commit to HeahDude/symfony-docs that referenced this pull request Mar 13, 2016
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.
@webmozart
Copy link
Contributor

Status: Reviewed

👍

@javiereguiluz
Copy link
Member

I know I'm very late to this PR ... but I have some concerns about the force_submit name. Without reading the code or the docs, force_submit can be understood as "send the form even if it contains errors", "send the form even if some field is missing", "send the form even if the CSRF token doesn't match", "keep retrying form submission until there is no error", etc.

But this option in fact just removes the form name from the submitted values. Instead of user[name] you can use name.

@HeahDude
Copy link
Contributor Author

@javiereguiluz Hopefully one would read the docs before using a new option :) If you think otherwise, do you have a suggestion ?

@webmozart
Copy link
Contributor

@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 submit_every_request?

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 null here?

ping @guilhermeblanco

@webmozart
Copy link
Contributor

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 RequestHandlerInterface implementation, if it needs to be reused).

@xabbuh
Copy link
Member

xabbuh commented Mar 25, 2016

@webmozart Afaics #12528 would (at least partly) be solvable with this change too.

@HeahDude
Copy link
Contributor Author

Yes I agree it's more accurate to use submit_every_request.

What about force_submit_request ?

@guilhermeblanco
Copy link
Contributor

@webmozart I think the name submit_every_request is better than originally proposed one.
Also, the motivation is mainly about having a GET request with an un-named form, where every regular request should submit the form.
Now taking the GET call together with the form with fields that have default values (empty_data), let's say page=1, perPage=50. Hitting the controller without submitting anything should still submit the form, even though it doesn't have any form data. Also, $form->isValid() must be true (which is not right now because $form->isSubmitted() is false). No matter what we discuss, this is a valid situation where I could force submission.

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.

@HeahDude
Copy link
Contributor Author

What about adding FormConfig::getSubmitEveryRequest() then pushing the method to the FormConfigInterface in 3.4 ? It looks more like a "hack" actually.

@HeahDude
Copy link
Contributor Author

I agree it can be useful for API in general and API with GET in particular.

@HeahDude
Copy link
Contributor Author

What's the final call on this ? Changing the name is easy to get done for tomorrow :)

@javiereguiluz
Copy link
Member

@HeahDude Bernhard commented the following:

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 null here?

So, if this option is about submitting the request, why not calling it submit_request ?

@HeahDude
Copy link
Contributor Author

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:

  • force_submit_request (because data keys mismatch) or force_submit_data
  • submit_all_request or submit_every_request
  • submit_as_root
  • submit_unmapped
  • submit_no_name
  • submit_anonymous
  • anonymous_form
  • 'force_submit_anonymous`

??

@webmozart
Copy link
Contributor

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 handleRequest() by submit(). There's nothing we need to change as far as I can tell. :)

@webmozart webmozart closed this Mar 31, 2016
@HeahDude
Copy link
Contributor Author

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 ?

@HeahDude
Copy link
Contributor Author

@webmozart What do you think about having in 4.0 some clear equivalence:

// Does not work
$dataToSubmit = $form->getViewData();

$form->submit($dataToSubmit); // would submit pre set data

// Works
$emptyData = $form->getConfig()->getEmptyData();

$form->submit($emptyData); // <=> $form->submit(null);

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.

@webmozart
Copy link
Contributor

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.

compatibility of dataset and data mapping and the way view data is handled throw the global form lifecycle.

What do you mean? What do you see that should be improved?

@HeahDude
Copy link
Contributor Author

HeahDude commented Apr 1, 2016

Thanks for your response :)

I mean currently $viewData unless using transformers is equal to model data.

My guess is that it should match the real view value: compound ? array : string

@HeahDude
Copy link
Contributor Author

HeahDude commented Apr 2, 2016

On second thought to be able to do that, the data mapping should be done with normalized data, shouldn't it ?

@HeahDude HeahDude deleted the feature-form-force_submit branch April 9, 2016 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants