Skip to content

[2.3] [Form] Implemented form processors #6522

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

Merged
merged 5 commits into from
Apr 19, 2013
Merged

Conversation

webmozart
Copy link
Contributor

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

}

$name = $form->getName();
$method = $form->getConfig()->getMethod();
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be moved after if below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean line above 😮

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a rather pointless micro optimization.

@webmozart
Copy link
Contributor Author

Added the helpers form_start() and form_end() which were missing before.

After:

```
if ($form->process($request)->isValid()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The example in Before and After will probably confuse people that use the else part of the condition. So it should make clear that one must check isBound before falsely adding flash messages about an invalid form (that is not even bound).

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

@webmozart
Copy link
Contributor Author

We discussed in #5493 whether form_end() should also imply form_rest(). @Tobion made the valid point that people might not always want to render all fields. I think this is an edge case tough, so I propose to imply form_rest() by default and make it possible to deactivate this using a variable render_rest.

{{ form_start(form) }}
   {# ... #}
{{ form_end(form) }}

or

{{ form_start(form) }}
   {# ... #}
{{ form_end(form, { 'render_rest': false }) }}

What do you think?

@webmozart
Copy link
Contributor Author

Another question that remains to be answered:

With the current implementation, the attributes passed in the attr option are rendered both in the <form> tag (new) and in the <div>/<table> tag of the form (for BC).

$form = $this->createForm('my_form', $formData, array(
    'attr' => array(
        'id' => 'foo',
        'class' => 'bar',
    ),
));
{{ form_start(form) }}
    {{ form_widget(form) }}
    <input type="submit">
{{ form_end(form) }}

->

<form id="foo" class="bar">
    <div id="foo" class="bar">
        <!-- rows -->
    </div>
    <input type="submit">
</form>

Solutions that come to mind:

(a) Move the rendering of the attributes from the <div>/<table> tag to the <form> tag for the root form. This breaks BC because the form_widget() call for the root form does not output the attributes anymore then. Nested forms are not affected.

<form id="foo" class="bar">
    <div><!-- no attrs! -->
        <!-- rows -->
    </div>
    <input type="submit">
</form>

(b) Don't render a <div> tag for the root form and do as in (a) for the table layout. This obviously also breaks BC. Nested forms are not affected.

<form id="foo" class="bar">
    <!-- rows -->
    <input type="submit">
</form>

(c) Don't render any attributes in the <form> tag. Maintains full BC.

<form>
    <div id="foo" class="bar">
        <!-- rows -->
    </div>
    <input type="submit">
</form>

Opinions?

While (c) is the best solution from a BC point of view, I think it is rather unintuitive. As a newcomer, if I put a "class" attribute on a form, I would expect it to be on the outermost <form> tag, not on some <div> within.

(a) and (b), on the other hand, break BC for those people that use form_widget() to render the complete form. I don't know how many people would be affected by this, this poll tries to gather some data about that. IMO both (a) and (b) are more intuitive and future-proof.

@webmozart
Copy link
Contributor Author

Added the helper form() to prototype a complete form:

{{ form(form) }}

} else {
$default = $form->getConfig()->getCompound() ? array() : null;
$data = isset($_POST[$name]) ? $_POST[$name] : $default;
}
Copy link
Member

Choose a reason for hiding this comment

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

you should also merge `$_FILES`` to handle file uploads in the native processor and be consistent with the Request one

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. Had to copy over some code from FileBag for that.

@Tobion
Copy link
Contributor

Tobion commented Dec 31, 2012

@bschussek I think {{ form_end(form, { 'render_rest': false }) }} is fine.

And to the question about attributes: I think we should not maintain BC here with c) because attributes are better applied to the parent. Using CSS you can easily select the child with #formid > div but not the other way round which would make it impossible the style the form element.
So I would say a) or b) is better than c). And a) is better than b) because in HTML 4 Strict <input> element must be within a block element like <div>. So b) would not produce valid HTML 4 Strict. This restriction is not there in HTML5 anymore but your id validation also uses the HTML4 paradigm.
All in all, IMO the best solution is a).

{% spaceless %}
{{ form_start(form) }}
{{ form_widget(form) }}
<input type="submit" />
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned, inputs must be wrapped with a block level element like <div> to be valid HTML4 Strict. So this would need to be changed too.

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 will be fixed by #6528.

@webmozart
Copy link
Contributor Author

And a) is better than b) because in HTML 4 Strict <input> element must be within a block element like <div>. So b) would not produce valid HTML 4 Strict.

@Tobion This is not correct. With (b), each row would still be wrapped in a <div>.

*/
interface FormProcessorInterface
{
public function processForm(FormInterface $form, $request = null);
Copy link
Member

Choose a reason for hiding this comment

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

you should document the method in the interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed

@webmozart
Copy link
Contributor Author

I propose to postpone the question about the wrapping <div> to a future PR.

@fabpot This PR should be mergeable.

@webmozart
Copy link
Contributor Author

Updated the documentation.

@weaverryan
Copy link
Member

Hi guys!

I certainly don't mind the addition of the new features here, but I'm -1 on the BC breaks and deprecations. I don't love the deprecation of form_enctype, and I wish the form_start and form_end things would have come earlier, as we'll have a mixture on the web (think 2.0 + 2.1 versus 2.2+ docs, blog posts, etc) of things using form_enctype and form_rest versus form_start and form_end. Individually, I like the feature, but I'm not sure having this big change is worth it.

Also, I really really don't like the deprecation of form->bind - this will affect far too many projects and the new form->process is just a "nice" feature, at the cost of so much BC breaking. Again, I'm not sure getting rid of the current way is worth it, but I'm also not sure that having both form->bind and form->process is clear either.

Overall, I'd like to see these "nice" things added in a way where they can co-exist with what we have currently. These are really nice features, but there's so much code that will need to update and "education" that will need to happen because the form framework is already being used in so many places.

Thanks!

@webmozart
Copy link
Contributor Author

@weaverryan For reference, this PR does not contain any (immediate) BC breaks, just deprecations.

We can leave in form_enctype without too much hassle if you feel that this is a problem. It is just not necessary anymore. We can also postpone the version where we remove this feature.

$form->bind($request) was not only deprecated for cleanup purposes, but also for performance reasons. Currently, BindRequestListener is attached to and invoked for each and every form element. Upgrading this is very easy so I don't really see the necessity for leaving this in. Again, we can postpone the removal date here.

*
* @var array
*/
private static $allowedMethods = array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this looks like the allowed methods can be changed, like adding or removing some. But that's not the case.
Why not simply inline the allowed methods in setMethod?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I explained this sufficiently above.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I don't see any valid reason against:

if (!in_array($upperCaseMethod, array('GET', 'PUT', 'POST', 'DELETE', 'PATCH'))) {
    throw new FormException(sprintf(
        'The form method is "%s", but should be one of "GET", "PUT", "POST", "DELETE", "PATCH".',
        $method
    ));
}

@jalliot
Copy link
Contributor

jalliot commented Jan 3, 2013

What is the point of the form_end helper if it only renders a </form> tag?
If it was implemented by a customizable block or if it rendered also form_rest then why not but in the current state it seems really overkill.

@webmozart
Copy link
Contributor Author

@jalliot form_end does also render form_rest

@jalliot
Copy link
Contributor

jalliot commented Jan 3, 2013

@bschussek Oh right sorry... I did not refresh my page and did not see the new commits.

@michelsalib
Copy link

Really nice work @bschussek. I like the way the form component is taking. There is still a lot of work, but hey, forms are a complex thing.
So it is a big 👍 for me.

@webmozart
Copy link
Contributor Author

Postponed to 2.3

@webmozart
Copy link
Contributor Author

Reviewed again and rebased on master.

@webmozart
Copy link
Contributor Author

Again, the test failures on 5.3 seem unrelated to this PR. Reviewed and updated the documentation.

@stof
Copy link
Member

stof commented Apr 13, 2013

@bschussek the fix for 5.3 has just been merged

@webmozart
Copy link
Contributor Author

Rebased again.

parent::compile($compiler);

$compiler->raw(";\n");
$compiler->write('trigger_error(\'The helper form_enctype(form) is deprecated since version 2.3 and will be removed in 3.0. Use form_start(form) instead.\', E_USER_DEPRECATED)');
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC when something will be removed in 3.0, it should not throw error, right @fabpot ?

Copy link
Member

Choose a reason for hiding this comment

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

You are right. We will trigger the error just before 3.0. For now, it should only be documented in the CHANGELOG for 3.0.

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

@mvrhov
Copy link

mvrhov commented Apr 17, 2013

I'm guessing that this still won't handle the case where you have the data for two forms present e.g. the http method is something different that GET, but the query part of the uri contains the data submitted in a search in a previous step. Right now you need to know the form name and get such a data from the request and then bind manually.... This becomes annoying and tedious if you need to persist this through more than one step...

fabpot added a commit that referenced this pull request Apr 19, 2013
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
@fabpot fabpot merged commit 11fee06 into symfony:master Apr 19, 2013
@stof
Copy link
Member

stof commented Apr 19, 2013

@mvrhov the Form component is able to get the right data based on its name (since 2.0)

*/
public function setAction($action)
{
throw new \BadMethodCallException('Buttons do not support actions.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, submit buttons do support actions via "formaction" attribute. Also methods with "formmethod".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Could you open a ticket please?

@mvrhov
Copy link

mvrhov commented Apr 19, 2013

@stof
Imagine the two step wizard. The first step is a search form ($form1) with action set to GET.
Now the second step contains another form ($form2) with action set to POST and the query parameters from the $form1 are part of action uri for $form2.

However in the action that gets executed on second step you need data from both form1 and form2 to complete the action. So calling $form2->bind($request), only the $from2 will get bound.
If you also want to bind the data to $form1 then you have to do.. $form->bind($request->query->get('form_name'));

@webmozart
Copy link
Contributor Author

@mvrhov IMO this is the wrong approach to the problem. You should not bind data that you already successfully submitted again. Instead, store the values somewhere in your session and write them into your persistence layer when you're done with the wizard.

@stof
Copy link
Member

stof commented Apr 19, 2013

@mvrhov if you want to bind $form, call $form->bind($request) too. There is no reason for $form2->bind() to bind $form (it does not even know that you have a second Form instance created in your code)

fabpot added a commit that referenced this pull request Apr 23, 2013
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()
weaverryan added a commit to symfony/symfony-docs that referenced this pull request Oct 19, 2014
….3 (xabbuh)

This PR was merged into the 2.3 branch.

Discussion
----------

[Component][Forms] add missing features introduced in 2.3

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes (symfony/symfony#6522)
| Applies to    | all
| Fixed tickets | #2969

- [x] use ``submit()`` instead of ``bind()`` to manually submit a form
- [x] configure method and action of a form
- [ ] ~~render a form's start and its end separately~~ (see [the comment](#4085 (comment)))
- [ ] configure buttons

Commits
-------

33f6422 [Forms] add missing features being new in 2.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.