[Form] Simplify request handling #5493

webmozart · 2012-09-11T18:25:48Z

We could simplify the form request handling by

supporting a "method" option
supporting a "request" option that binds the request if its method matches "method"
supporting an "action" option and helpers for opening and closing tags

We previously had this idea and dropped it. I was inspired again by Zend Framework 2's API and thought that it might be a good idea. Kudos to them.

Proposed simplification (fully BC):

<?php
// in the controller
$form = $this->createForm('my_form', null, array(
    'action' => $this->generateUrl('process_form'),
    'method' => 'POST',
    'request' => $request,
));

// Alternative 1: short
if ($form->isValid()) {
    // process form
}

// Alternative 2: self-describing
if ($form->isBound() && $form->isValid()) {
    // process form
}

Note that the check for $request->getMethod() is not necessary anymore in the above snippet. Also, bind($request) is done implicitely by passing the "request" option (explicit binding is also possible, of course).

{# in the template #}
{{ form_start(form) }}
    {{ form_widget(form) }}
    <input type="submit" />
{{ form_end(form) }}

{# alternative with explicit method or action #}
{{ form_start(form, { method: "GET" }) }}

Thoughts? Preferences for Alternative 1 or 2?

The text was updated successfully, but these errors were encountered:

havvg · 2012-09-11T18:31:53Z

Do form_start and form_end support the same behavior like form_widget when it gets to finding the correct template/block to render?

In the example a lookup for block my_form_form_start etc?

webmozart · 2012-09-11T18:35:11Z

@havvg Probably no, for performance reasons. Needs more research though.

webmozart · 2012-09-11T18:44:34Z

Thinking a bit further, this could also be supported for plain PHP without HttpFoundation:

<?php

// will automatically bind $_POST[$form->getName()] if set
$form = $this->createForm('my_form', null, array(
    'action' => $this->generateUrl('process_form'),
    'method' => 'POST',
));

// BC behavior by dropping "method"
$form = $this->createForm('my_form', null, array(
    'action' => $this->generateUrl('process_form'),
));

// BC behavior by unsetting "auto_bind"
$form = $this->createForm('my_form', null, array(
    'action' => $this->generateUrl('process_form'),
    'method' => 'POST',
    'auto_bind' => false,
));

bamarni · 2012-09-11T19:07:11Z

This looks great!

Both alternatives are some nice shortcuts, my first impression was "alternative 1 is too magic", but if I look at it like "when using the auto_bind option, having a boundable form is the first condition for its validity", then alternative 1 makes sense to me.

I can see the submit button isn't rendered by form_end, will it only render the closing form tag? If so, I don't understand why we would need this helper.

webmozart · 2012-09-11T19:08:29Z

I can see the submit button isn't rendered by form_end, will it only render the closing form tag? If so, I don't understand why we would need this helper.

For symmetry, simply. If you'd write out </form> explicitly, IDEs would complain about a missing start tag.

webmozart · 2012-09-11T19:09:45Z

I can see the submit button isn't rendered by form_end, will it only render the closing form tag?

We could also do form_rest() within form_end().

bamarni · 2012-09-11T19:12:19Z

We could also do form_rest() within form_end() so that people don't have to call it themselves.

Wouldn't that suppose to put the submit button in form_end?

webmozart · 2012-09-11T19:15:37Z

Wouldn't that suppose to put the submit button in form_end?

No, otherwise people couldn't freely place the submit button anymore. form_start and form_end would also be used when coding the rest of the form markup manually.

bamarni · 2012-09-11T19:30:31Z

Hum then I think I'm missing something... Usually isn't the submit button rendered between form_rest and the closing form tag? So if form_rest is done within form_end, when using form_end, how can we render the submit button between them?

webmozart · 2012-09-11T19:47:12Z

Usually isn't the submit button rendered between form_rest and the closing form tag?

The order of the submit button and form_rest() doesn't matter. The submit button might be placed anywhere in the markup. form_end() would just render form_rest() and the closing form tag.

michelsalib · 2012-09-11T20:58:21Z

I really like the idea of having the action url generated by the controller, this way the controller which is already handling the form creation and binding is also responsible for pointing creation to binding.

I prefer option 2 with explicit binding, this way there is no ambiguity about when the form is bonded.

I don't like the idea when automatically using the $_POST variable. It looks dirty and HttpFundation is here for that. Not injecting the bonded data is really against the whole DI/IOC precept.

Finally, I like the idea of form_start(). And agree that form_end should include form_rest just in case...

I just want to make a proposal for another syntax that looks cleaner to me:

{% form form with {method: 'POST', attr: { 'class': 'my_class'}} %}
    {{ form_widget(form) }}
    <input type="submit" />
{% endform %}

I a not sure here that the use of a {% with a form keyword is really a good idea, but it looks more like a block with an end and a start and feels nicer.

Tobion · 2012-09-12T06:07:21Z

In general I like these shortcuts. I prefer alternative 2 because its more explicit. I already see developer issues with alternative 1 that could arise, which would be wrong:

if (form->isValid()) { 
    //process form
} else {
    // wrongly add flash message to signal error
}

Furthermore, I think it should be possible to disable auto-rendering of form_rest in form_end.

some people might not want to render all fields
in HTML5 it's possible to render inputs outside the form scope with the "form" attribute (which would otherwise not be possible anymore)

webmozart · 2012-09-12T06:20:09Z

I don't like the idea when automatically using the $_POST variable. It looks dirty and HttpFoundation is here for that.

This is only half-true. From the Form components POV, HttpFoundation is optional, not mandatory. Features supported with HttpFoundation should, if possible, also be supported if not using HttpFoundation.

webmozart · 2012-09-12T06:21:56Z

@Tobion Valid points. I guess form_rest() should then remain to be called manually.

webmozart · 2012-09-12T06:25:42Z

@michelsalib I like your Twig syntax. I think it makes sense. Form theming could also be integrated there.

{% form form with { method: 'POST', theme: _self } %}
    {{ form_widget(form) }}
    <input type="submit" />
{% endform %}

Tobion · 2012-09-12T06:50:13Z

The form tag makes sense semantically, as a good twig editor would recognize a missing endtag (in contrast to form_end).
But on the other hand, tags usually don't output data. But this one does.

Another idea: {% form form with { autorest: true/false } %} Probably defaults to true and would output the form_rest at the end.

michelsalib · 2012-09-12T07:11:17Z

The matter is the '{%' is about logic, not about display...

Michel Salib

De : Tobias Schultze
Envoyé : 12/09/2012 08:50
À : symfony/symfony
Cc : Michel Salib
Objet : Re: [symfony] [Form] Simplify request handling (#5493)

{% form form with { autorest: true/false } %} Probably defaults to true.

Reply to this email directly or view it on GitHub:
#5493 (comment)

Tobion · 2012-09-12T07:16:30Z

@michelsalib Yes, then your own suggestion is not valid.

michelsalib · 2012-09-12T07:24:17Z

That is what bothered me at the beginning. It has the advantage of syntactically enforce the use of a 'endform' keyword.
But it is too confusing to have a '{%' that provoque a display. So let's go back to the original proposal.

Michel Salib

De : Tobias Schultze
Envoyé : 12/09/2012 09:16
À : symfony/symfony
Cc : Michel Salib
Objet : Re: [symfony] [Form] Simplify request handling (#5493)

@michelsalib Yes, then your own suggestion is not valid.

Reply to this email directly or view it on GitHub:
#5493 (comment)

bamarni · 2012-09-12T08:40:46Z

Indeed @Tobion has a good use case against alternative 1. Then what about making $form->isValid() return null when it is not bound?

This would require a strict compare when checking for an invalid form, but I don't think it's such a common use case, usually you want to do something when the form is valid, I can't even see any example checking for an invalid form in the docs.

Furthermore, if by default (so for most people) forms are auto-bounded, the binding would become an internal stuff, then calling a method named isBound is also missleading, because this binding notion doesn't appear anywhere else in the framework userland code.

webmozart · 2012-09-12T08:49:48Z

Furthermore, if by default (so for most people) forms are auto-bounded, the binding would become an internal stuff, then calling a method named isBound is also missleading, because this binding notion doesn't appear anywhere else in the framework userland code.

I thought about that too. One solution would be to deprecate the notion "bind" and rename it to "submit", but that's a very delicate topic. I'm not sure how such a deprecation would be received.

<?php

if ($form->isSubmitted() && $form->isValid()) {
}

// or

if ($form->isSubmitted()) {
    if ($form->isValid()) {
    } else {
    }
}

// or

$form->submit($request);

// or

$form->submit($_POST[$form->getName()]);

bamarni · 2012-09-12T09:14:40Z

Indeed that's a huge deprecation... I think it's really explicit to check (isSubmitted) but weird when binding, $form->submit() makes me think to a crawler or something like that.

But are you against making isValid return null when the form is not bound? I really find alternative 1 simple and powerful.

webmozart · 2012-09-12T09:24:16Z

But are you against making isValid return null when the form is not bound?

Yes. It really hides what is going on. Having two separate checks for these separate conditions is much easier to read IMHO.

webmozart · 2012-10-02T17:34:54Z

Follow-up thoughts: Automatically binding a form is hard, because it is difficult to determine which form should be automatically bound (which is the root form?). An alternative is an explicitly called handleRequest($request = null) method in FormInterface, which passes the given data to a FormRequestHandlerInterface instance that replaces the current BindRequestSubscriber.

Resulting API:

<?php

$form = $this->createForm('my_form', null, array(
    'action' => $this->generateUrl('process_form'),
    'method' => 'POST',
));

// with HttpFoundation
$form->handleRequest($request);

// without HttpFoundation (uses superglobals)
$form->handleRequest();

if ($form->isSubmitted() && $form->isValid()) {
   // do stuff
}

Tobion · 2012-10-02T17:48:02Z

Btw, IMO the term isSubmitted is worse than isBound. When you use a form for a GET request nothing needs to be submitted. I think it's irritating.

bamarni · 2012-10-02T18:24:58Z

Maybe handleRequest could return wether or not the request has been handled?

<?php

$form = $this->createForm('my_form', null, array(
    'action' => $this->generateUrl('process_form'),
    'method' => 'POST',
));

if ($form->handleRequest($request) && $form->isValid()) {
   // do stuff
}

Trainmaster · 2012-10-10T22:27:13Z

What about:

if ($form->isValid($request)) {
    // ...
}

?

stof · 2012-10-10T23:07:55Z

@Trainmaster isValid is just a getter currently. It does not perform any validation itself. Changing it would be a BC break

webmozart · 2012-11-07T23:07:09Z

We could also shorten handleRequest() to simply handle():

// with HttpFoundation
if ($form->handle($request)->isValid()) {
    ...
}

// without HttpFoundation
if ($form->handle()->isValid()) {
    ...
}

or process():

// with HttpFoundation
if ($form->process($request)->isValid()) {
    ...
}

// without HttpFoundation
if ($form->process()->isValid()) {
    ...
}

bamarni · 2012-11-08T10:53:09Z

@bschussek : you were saying you wanted to keep 2 different methods in order to check if the form is submitted and if it's valid, instead of delegating both to isValid().

webmozart · 2012-11-08T10:57:39Z

@bamarni We can have these two methods. For the majority of cases it might be easier to just call isValid() though (which currently - unnecessarily? - throws an exception if the form is not submitted).

bamarni · 2012-11-08T11:10:22Z

you're right, I also think most of the times isValid is enough and if people want more subtle checks they can just split their checks, I'm +1 with your last snippet with handle() this is a great API.

Tobion · 2012-11-08T11:13:53Z

process without arguments makes more sense than handle()

ericclemmons · 2012-11-09T16:22:57Z

I may be late to the party here, but I'm not sure about the auto_bind part. I would assume binding would occur if request is set, but if left null, then the user would have to call $form->submit($request) explicitly.

Also, I may be using this wrong, but in some instances I bind a form using GET and POST data alike, except I disable all validation for GET requests. This way, 3rd parties can link to a form with data already pre-filled in the URL, but no errors are rendered in the view. To save the form, a POST (+ validation) is still required.

webmozart · 2012-11-09T16:35:59Z

@ericclemmons Caling $form->submit($request) will also be deprecated. Essentially the API will allow you to do these two things, depending on the level of detail you want to work on:

Option 1: Submit manually, you must pass an array or a simple value to submit (as provided by the HTTP client)

if (/* submitted */) {
    $form->submit(array(...));
    $form->submit($_POST[$form->getName()]);
    $form->submit($request->request->get($form->getName()));

    if ($form->isValid()) {
        // ...
    }
}

Option 2: Let Symfony deal with the above.

if ($form->process()->isValid()) {
    // ...
}
if ($form->process($request)->isValid()) {
    // ...
}

// or, if you want to check submission too
if ($form->process()->isSubmitted()) {
    // ...
    if ($form->isValid()) {
        // ...
    }
}

This will refer the processing to an internal component, such as FormProcessor, which defines the accepted parameters (Request for RequestFormProcessor, null for NativeFor F438 mProcessor).

ericclemmons · 2012-11-09T17:03:11Z

I also wanted to add some thoughts on this rename from bind to submit (pre-filling a form with GET data would need to be considered)...

Coming from a ZF1.x background myself, it makes sense to submit client data.

I think bind was fitting when the form component was released, as no other framework was transforming & mapping the client data back to the model. Basically, it made early Symfony users realize they were using forms differently.

Option 1 makes sense when POSTing & subsequently saving a form. Assuming that there would be a FormEvent for this, then it would help clean up the heavy-lifting in the controller as well when submitted successfully to listeners. (Well, its a possibility!)

However, Option 2 is more vague, which would allow, in my case, a GET request to be "bound", but validation ignored, while a POST request would be bound and validated, and a PATCH request would be bound & validated but with allow_missing (ignore_missing) enabled.

I hope this feedback helps!

webmozart · 2012-11-09T17:57:00Z

@ericclemmons Both of these options would be supported. If you don't care about the details, you'll use Option 2. And if you want to do specific stuff, you will do Option 1.

webmozart · 2012-11-09T18:01:19Z

I updated the code sample in the previous comment to be clearer.

This PR was merged into the master branch. Discussion ---------- [2.3] [Form] Implemented form processors Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: partially #5493 Todo: - License of the code: MIT Documentation PR: symfony/symfony-docs#2092 Commits ------- 11fee06 [TwigBridge] Removed duplicate entries from the CHANGELOG 68f360c [Form] Moved upgrade nodes to UPGRADE-3.0 01b71a4 [Form] Removed trigger_error() for deprecations as of 3.0 81f8c67 [Form] Implemented form processors 0ea75db [Form] Improved FormRenderer::renderBlock() to be usable outside of form blocks

This PR was merged into the master branch. Discussion ---------- [Form] Deprecated bind() and isBound() in favor of submit() and isSubmitted() | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | yes (*) | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #5493 | License | MIT | Doc PR | TODO This change was discussed for a while in #5493. **(*)** It breaks BC *only for people who implemented* `FormInterface` *manually* (not a lot, so I hope). These can fix the problem by simply renaming `bind()` and `isBound()` in their implementation to `submit()` and `isSubmitted()`. The main rationale is that with the request handlers introduced in #6522, people won't be confronted with the term "binding" anymore. As such, `isBound()` will be a very strange name to new users that have never used `bind()` manually. See this code sample as example: ```php $form = $this->createForm(...); $form->handleRequest($request); // Imagine you have never heard about bind() or binding. What does this mean? if ($form->isBound()) { // ... } ``` In reality, `bind()` submits a form. Where-ever I renamed "bind" to "submit" in the comments, "submit" made actually much more sense. So it does in the code sample above: ```php $form = $this->createForm(...); $form->handleRequest($request); // Aha! if ($form->isSubmitted()) { // ... } ``` Also when using `submit()` directly, the code makes much more sense now: ```php $text = $this->createForm('text'); $text->submit('New Value'); ``` For current users, the current naming will be supported until 3.0. Commits ------- 41b0127 [Form] Deprecated bind() and isBound() in favor of submit() and isSubmitted()

webmozart · 2013-09-13T15:17:22Z

Closed in favor of #9032.

This was referenced Dec 13, 2012

[Form] Rename "always_empty" in PasswordType to "reset_on_submit" #6325

Closed

[RFC] [Form] Add Partial Bind Support #5576

Closed

This was referenced Dec 30, 2012

[2.3] [Form] Implemented form processors #6522

Merged

Added information about the new form(), form_start() and form_end() helpers symfony/symfony-docs#2092

Merged

webmozart mentioned this issue Apr 20, 2013

[Form] Deprecated bind() and isBound() in favor of submit() and isSubmitted() #7736

Merged

webmozart mentioned this issue Sep 13, 2013

[Form] Simplify request handling #2 #9032

Closed

webmozart closed this as completed Sep 13, 2013

stof mentioned this issue Jan 17, 2025

[Form] Add a method to submit a request and return the valid method in one #59537

Open

Uh oh!

[Form] Simplify request handling #5493

[Form] Simplify request handling #5493

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!