Skip to content
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

[2.3] [Form] Support buttons in forms #6528

Merged
merged 11 commits into from Apr 17, 2013

Conversation

Projects
None yet
@webmozart
Copy link
Contributor

commented Dec 31, 2012

Bug fix: no
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #5383
Todo: -
License of the code: MIT
Documentation PR: symfony/symfony-docs#2489

The general idea of this PR is to be able to add buttons to forms like so:

$builder->add('clickme', 'submit');

You can then check in the controller whether the button was clicked:

if ($form->get('clickme')->isClicked()) {
   // do stuff
}

Button-specific validation groups are also supported:

$builder->add('clickme', 'submit', array(
    'validation_groups' => 'OnlyClickMe',
));

The validation group will then override the one defined in the form if that button is clicked.

This PR also introduces the disabling of form validation by passing the value false in the option validation_groups:

$builder->add('clickme', 'submit', array(
    'validation_groups' => false,
));

The same can be achieved (already before this PR) by passing an empty array:

$builder->add('clickme', 'submit', array(
    'validation_groups' => array(),
));

See the linked documentation for more information.

@stof

View changes

src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig Outdated
{% if label is empty %}
{% set label = name|humanize %}
{% endif %}
<input type="submit" {{ block('button_attributes') }} value="{{ label }}" />

This comment has been minimized.

Copy link
@stof

stof Dec 31, 2012

Member

why not <button type="submit"> to be consistent with other buttons ?

This comment has been minimized.

Copy link
@norzechowicz

norzechowicz Dec 31, 2012

Contributor

There are problems with button as a submit in old versions of Internet Explorer.

This comment has been minimized.

Copy link
@Ninir

Ninir Dec 31, 2012

@norzechowicz Problems regarding submit buttons are happening only when you don't specify the type of them. There were browsers using "button" as default type whereas others were using "submit".

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 1, 2013

Author Contributor

I changed it to <button type="submit"> now.

@stof

View changes

src/Symfony/Component/Form/Button.php Outdated
*/
public function getIterator()
{
return new \ArrayIterator(array());

This comment has been minimized.

Copy link
@stof

stof Dec 31, 2012

Member

you could return an \EmptyIterator here

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 1, 2013

Author Contributor

Changed

@stof

View changes

src/Symfony/Component/Form/Extension/Core/Type/ButtonType.php Outdated
// https://github.com/symfony/symfony/issues/5038
'cache_key' => $uniqueBlockPrefix . '_' . $form->getConfig()->getType()->getName(),
));
}

This comment has been minimized.

Copy link
@stof

stof Dec 31, 2012

Member

You should probably find a way to avoid the code duplication with FormType here

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 2, 2013

Author Contributor

Yes, but I'm not sure how. Creating yet another base type on top of form has a severe performance impact - I would like to avoid that.

This comment has been minimized.

Copy link
@stof

stof Jan 2, 2013

Member

maybe using PHP inheritance to have move the duplicated logic to a parent abstract class ?

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 2, 2013

Author Contributor

Yes, I did that now.

@webmozart

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2013

Apart from a few missing tests, this PR is pretty much complete now. Feedback welcome.

@stof

View changes

src/Symfony/Component/Form/Extension/Core/Type/BaseType.php Outdated
*/
public function buildView(FormView $view, FormInterface $form, array $options)
{
/* @var \Symfony\Component\Form\ClickableInterface $form */

This comment has been minimized.

Copy link
@stof

stof Jan 2, 2013

Member

this is wrong. It is not always a clickable one here

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 2, 2013

Author Contributor

Thanks, fixed.

@vicb

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2013

@bschussek could be a good nice to have... after the 100+ form issues are tackled !

@webmozart

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2013

Support for validation groups in submit buttons added.

@webmozart

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2013

This PR is now complete. I adapted the description above.

@stof

View changes

src/Symfony/Component/Form/Extension/Validator/Type/FormTypeValidatorExtension.php Outdated
@@ -53,6 +53,8 @@ public function buildForm(FormBuilderInterface $builder, array $options)
*/
public function setDefaultOptions(OptionsResolverInterface $resolver)
{
parent::setDefaultOptions($resolver);

This comment has been minimized.

Copy link
@stof

stof Jan 2, 2013

Member

You forgot to remove the validation group normalizer here when moving it to the base class

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 2, 2013

Author Contributor

Thanks, fixed

@vicb

View changes

src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig Outdated
{% block button_attributes %}
{% spaceless %}
id="{{ id }}" name="{{ full_name }}"{% if disabled %} disabled="disabled"{% endif %}
{% for attrname, attrvalue in attr %}{% if attrname in ['placeholder', 'title'] %}{{ attrname }}="{{ attrvalue|trans({}, translation_domain) }}" {% else %}{{ attrname }}="{{ attrvalue }}" {% endif %}{% endfor %}

This comment has been minimized.

Copy link
@vicb

vicb Jan 2, 2013

Contributor

you can probably remove placeholder here

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

Thanks, fixed

</service>
<service id="form.type.reset" class="Symfony\Component\Form\Extension\Core\Type\ResetType">
<tag name="form.type" alias="reset" />
</service>

This comment has been minimized.

Copy link
@stof

stof Jan 2, 2013

Member

you also need to register the Validator SubmitButtonTypeExtension

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

Fixed

This comment has been minimized.

Copy link
@josephzhao

josephzhao Nov 2, 2013

can I use if $form->get('clickme') == null to check if this exist or not ?

if ($form->get('clickme') != null && $form->get('clickme')->isClicked()) {
// do stuff
}

This comment has been minimized.

Copy link
@stof

stof Dec 3, 2013

Member

@josephzhao $form->get() will throw an exception in case the child does not exist. This is why we have $form->has()

}

/**
* Unsupported method.

This comment has been minimized.

Copy link
@vicb

vicb Jan 2, 2013

Contributor

is it really unsupported (ie it returns false)... trying to be constructive here !

This comment has been minimized.

Copy link
@stof

stof Jan 2, 2013

Member

This one is indeed supported

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

It's not really supported since buttons cannot have children. This method is a dummy implementation, it's value never changes. That's why this and similar methods have the comment "Unsupported".

@vicb

View changes

src/Symfony/Component/Form/Button.php Outdated
*
* @param mixed $offset
*
* @return void

This comment has been minimized.

Copy link
@vicb

vicb Jan 2, 2013

Contributor

is @return useful ?

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

Removed

@vicb

View changes

src/Symfony/Component/Form/Button.php Outdated
*
* @param FormError $error
*
* @return FormInterface The form instance

This comment has been minimized.

Copy link
@vicb

vicb Jan 2, 2013

Contributor

no

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

Fixed

}

/**
* Binds data to the button.

This comment has been minimized.

Copy link
@vicb

vicb Jan 2, 2013

Contributor

What does this actually means ?

Would renaming bound to clicked makes sense ?

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

This, among others, is a reason why I suggested in #5493 to deprecate bind()/isBound() and rename it to submit()/isSubmitted().

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

To clarify, a button that is bound (submitted) is not necessarily clicked. Every submit button in a form will be bound (submitted), but only one will be clicked.

*/
public function add($child, $type = null, array $options = array())
{
throw new \BadMethodCallException('Buttons cannot have children.');

This comment has been minimized.

Copy link
@vicb

vicb Jan 2, 2013

Contributor

I can't remember if all other form exceptions belong to the form ns

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

Cleaning up the exceptions in the Form component is on the TODO list.

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

This comment has been minimized.

Copy link
@vicb

vicb Jan 3, 2013

Contributor

I am -1 to rush feature in a couple of days before the freeze especially when it introduces new issues. Let's fix outstanding issues first, we can live 6 more months without buttons plus an increased sw quality.

----- Reply message -----
De : "Bernhard Schussek" notifications@github.com
Pour : "symfony/symfony" symfony@noreply.github.com
Cc : "Victor Berchet" victor@suumit.com
Objet : [symfony] [Form] Support buttons in forms (#6528)
Date : jeu., janv. 3, 2013 18:30
In src/Symfony/Component/Form/ButtonBuilder.php:

  • /**
  • \* Unsupported method.
    
  • *
    
  • \* This method should not be invoked.
    
  • *
    
  • \* @param string|integer|FormBuilderInterface $child
    
  • \* @param string|FormTypeInterface            $type
    
  • \* @param array                               $options
    
  • *
    
  • \* @return void
    
  • *
    
  • \* @throws \BadMethodCallException
    
  • */
    
  • public function add($child, $type = null, array $options = array())
  • {
  •    throw new \BadMethodCallException('Buttons cannot have children.');
    

Cleaning up the exceptions in the Form component is on the TODO list.

Reply to this email directly or view it on GitHub.

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

@vicb Let's remain professional please. Using \BadMethodCallException instead of Symfony\Component\Form\BadMethodCallException here is nowhere near "an issue" or "bad sw quality".

@vicb

View changes

src/Symfony/Component/Form/ButtonBuilder.php Outdated
* into a form.
*/
public function setParent(FormBuilderInterface $parent = null)
{

This comment has been minimized.

Copy link
@vicb

vicb Jan 2, 2013

Contributor

trigger_error ?

here and below

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

We can't. Even though setParent() is deprecated, it is still being used inside the Form component, otherwise getParent() would suddenly stop working.

This comment has been minimized.

Copy link
@stof

stof Apr 11, 2013

Member

Deprecated methods have been removed from 2.3 in #7227 so you should remove these methods

This comment has been minimized.

Copy link
@fabpot

fabpot Apr 13, 2013

Member

@bschussek Can you take @stof comment into account?

@vicb

View changes

src/Symfony/Component/Form/ButtonBuilder.php Outdated
*/
public function getAttribute($name, $default = null)
{
return isset($this->attributes[$name]) ? $this->attributes[$name] : $default;

This comment has been minimized.

Copy link
@vicb

vicb Jan 2, 2013

Contributor

why isset, speed ? this makes this code look a bit akward

This comment has been minimized.

Copy link
@stof

stof Jan 2, 2013

Member

how would you handle the default otherwise ?

This comment has been minimized.

Copy link
@vicb

vicb Jan 2, 2013

Contributor

I only mean array_key_exist.

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

I think isset($val) ? $val : $default is common enough.

This comment has been minimized.

Copy link
@vicb

vicb Jan 3, 2013

Contributor

The comment also apply to the method above.

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

I checked with ParameterBag, array_key_exists() is used there. I adapted FormConfigBuilder and ButtonBuilder to be consistent with this.

* @param Boolean $disabled Whether the button is disabled
*
* @return ButtonBuilder The button builder.
*/

This comment has been minimized.

Copy link
@vicb

vicb Jan 2, 2013

Contributor

do we really need all those phpdocs ? (inheritDoc ?)

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 2, 2013

Author Contributor

I added them since the term "form" does not make sense at all for buttons.

This comment has been minimized.

Copy link
@vicb

vicb Jan 3, 2013

Contributor

like in FormBuilderInterface ?

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 3, 2013

Author Contributor

We could argue about the ideal class/interface hierarchy from today's POV now, but it won't get us anywhere :)

This comment has been minimized.

Copy link
@vicb

vicb Jan 3, 2013

Contributor

For once I didn't really want to argue but more say that if the interface is Form... maybe that would be also ok in phpdocs.

@marijn

This comment has been minimized.

Copy link

commented Jan 2, 2013

@vicb I understand your frustration but I think your comment about the outstanding issues is uncalled for...

Nice addition @bschussek! I've long wondered why buttons are not a part of the framework 👍

@willdurand

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2013

That's nice, really, it's really useful and well-designed, so thanks @bschussek.

But as @vicb there are (a few) issues related to the Form component, and it would be nice to focus on fixing them (IMO).

@sstok

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2013

Changing validation_group depending on the button is great, but I wonder if its a good idea to allow a event (onclick) when a button is clicked. I have some forms that have a 'cancel' button and depending which button is clicked 'submit' or cancel the form is either processed or the user is redirected to another page.

But as binding/bounding a complete form to determine if cancel was clicked could a performance hit, I wonder if its better to keep using the 'old' method for that.

And the 's label, its not uncommon to use HTML (Twitter Bootstrap for instance) knowing that Twig automatically escapes the label.

@webmozart

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2013

Changing validation_group depending on the button is great, but I wonder if its a good idea to allow a event (onclick) when a button is clicked.

@sstok What do you mean by that? Have you actually looked at the code of this PR?

But as binding/bounding a complete form to determine if cancel was clicked could a performance hit, I wonder if its better to keep using the 'old' method for that.

Performance is not affected (significantly) by this PR.

@sstok

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2013

A was talking in general, in JavaScript you can apply an event handler on a button for when its clicked.
I was wondering if something similar would be useful for the Form component, as choosing the validation_group depending on which button was clicked can be seen as an event.

As an example I have form with a normal submit button for saving and a cancel button which when clicked redirects the user back to the viewing page. Currently I'm checking this with.


if ($this->request->request->has('cancel')) {
    $isCancelled = true;

    return $this->onCancel($items) ?: true;
}

$this->form->bind($this->request);
if ($this->form->isValid()) {
    $this->onConfirm($items);

    return true;
}

The form is only binded when its actually gonna be used, but as binding a form even tho I'm only checking if a button was checked could be performance hit (compared to what I'm using now). I was wondering if supporting events when clicked could be useful. It was just a thought.

@webmozart

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2013

@sstok I see. You can drop your thoughts in #6576 if you like.

@webmozart

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2013

Postponed to 2.3

@webmozart

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2013

I rebased this branch on master now, fixed a few issues and wrote the documentation that is linked above. Unless reviews discover problems, this PR is ready for merging.

@stof

View changes

src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig Outdated
@@ -218,6 +218,36 @@
{% endspaceless %}
{% endblock email_widget %}

{% block button_widget %}
{% spaceless %}
{% if type is not defined or type is empty %}

This comment has been minimized.

Copy link
@stof

stof Apr 11, 2013

Member

There is a simpler way: type="{{ type|default('button') }}"

This comment has been minimized.

Copy link
@webmozart

webmozart Apr 13, 2013

Author Contributor

Fixed

This comment has been minimized.

Copy link
@fabpot

fabpot Apr 13, 2013

Member

not pushed yet?

This comment has been minimized.

Copy link
@webmozart

webmozart Apr 13, 2013

Author Contributor

now it is

@webmozart

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2013

The latest build fail seems to be unrelated to the content of this PR. The build succeeded on 5.4.

@stof

This comment has been minimized.

Copy link
Member

commented Apr 13, 2013

@bschussek see #7662

@webmozart

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2013

Rebased again.

@raziel057

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2013

+1 Usefull to ease the creation of multi step forms. It would be interesting to write a simple usage case sample for multipage forms.

@GromNaN

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2013

👍 Very useful to detect the clicked button (eg: save / preview).

What is the BC break you mention in the description ?

@stof

This comment has been minimized.

Copy link
Member

commented Apr 14, 2013

@GromNaN copied from the changelog: [BC BREAK] setting the option "validation_groups" tofalsenow disables validation instead of assuming group "Default"
@bschussek it should be mentionned in the UPGRADE file at the root as it is a BC break

@webmozart

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2013

@stof Good point, added this.

fabpot added a commit that referenced this pull request Apr 17, 2013

merged branch bschussek/issue5383 (PR #6528)
This PR was merged into the master branch.

Discussion
----------

[2.3] [Form] Support buttons in forms

Bug fix: no
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #5383
Todo: -
License of the code: MIT
Documentation PR: symfony/symfony-docs#2489

The general idea of this PR is to be able to add buttons to forms like so:

```php
$builder->add('clickme', 'submit');
```

You can then check in the controller whether the button was clicked:

```php
if ($form->get('clickme')->isClicked()) {
   // do stuff
}
```

Button-specific validation groups are also supported:

```php
$builder->add('clickme', 'submit', array(
    'validation_groups' => 'OnlyClickMe',
));
```

The validation group will then override the one defined in the form if that button is clicked.

This PR also introduces the disabling of form validation by passing the value `false` in the option `validation_groups`:

```php
$builder->add('clickme', 'submit', array(
    'validation_groups' => false,
));
```

The same can be achieved (already before this PR) by passing an empty array:
```php
$builder->add('clickme', 'submit', array(
    'validation_groups' => array(),
));
```

See the linked documentation for more information.

Commits
-------

faf8d7a [Form] Added upgrade information about setting "validation_groups" => false
d504732 [Form] Added leading backslashes to @exceptionMessage doc blocks
c8afa88 [Form] Removed deprecated code scheduled for removal in 2.3
36ca056 [Form] Simplified Twig code
ce29c70 [Form] Fixed incorrect doc comment
0bc7129 [Form] Fixed invalid use of FormException
600007b [Form] The option "validation_groups" can now be set to false to disable validation. This is identical to setting it to an empty array.
277d6df [Form] Fixed concatenation operator CS (see 7c47e34)
7b07925 [Form] Changed isset() to array_key_exists() to be consistent with ParameterBag
7b438a8 [Form] Made submit buttons able to convey validation groups
cc2118d [Form] Implemented support for buttons

@fabpot fabpot merged commit faf8d7a into symfony:master Apr 17, 2013

1 check failed

default Scrutinizer: Timed out — Travis: Failed
Details

weaverryan added a commit to symfony/symfony-docs that referenced this pull request Oct 3, 2014

feature #4246 [Reference] add description for the `validation_groups`…
… option (xabbuh)

This PR was merged into the 2.3 branch.

Discussion
----------

[Reference] add description for the `validation_groups` option

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

Commits
-------

c41b17c add description for the `validation_groups` option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.