8000 [Form] `required` attribute is set on all form fields regardless of whether they are required or not · Issue #3661 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Form] required attribute is set on all form fields regardless of whether they are required or not #3661

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
oscherler opened this issue Mar 21, 2012 · 37 comments

Comments

@oscherler
Copy link
Contributor

Build a form from a form type class:

public function buildForm( FormBuilder $builder, array $options )
{
    $builder
        ->add( 'text', 'text', array(
            'label' => 'search.form.text.label'
        ) )
        ->add( 'date', 'date', array(
            'widget' => 'single_text',
            'format' => 'dd.MM.yyyy',
            'label' => 'search.form.date.label'
        ) );
}

Add basic validation rules where none of the fields are required:

Me\MyBundle\Form\Model\TextSearch:
  properties:
    date:
      - Type: \DateTime

Render the form:

<form method="post" {{ form_enctype( form ) }}>
    {{ form_widget( form ) }}
    <input type="submit" value="Search" />
</form>

All the fields get a required attribute:

<label for="text_search_text" class=" required">Text</label>
<input type="text" id="text_search_text" name="text_search[text]" required="required" value="" />

This is incorrect, since the validation rules state that they are not required.

Under Forms (2.0) – Form Validation, the documentation states:

HTML5 Validation
As of HTML5, many browsers can natively enforce certain validation constraints on the client side. The most common validation is activated by rendering a required attribute on fields that are required. […]
Generated forms take full advantage of this new feature by adding sensible HTML attributes that trigger the validation.

This is clearly not the case: all the fields get the required attribute.

Also, under Built-in Field Types – Field Type Options (i.e. not under Form Validation), one can read:

The required option
The most common option is the required option, which can be applied to any field. By default, the required option is set to true, meaning that HTML5-ready browsers will apply client-side validation if the field is left blank.

In other words, Symfony is artificially introducing a discrepancy between the client- and the server-side validation criteria, and this by default.

Moreover, there is a conflict between the two cited paragraphs of the documentation. The first one states that the required attribute is rendered on fields that are required, which is confirmed by the following paragraph under Field Type Guessing – Field Type Options Guessing:

required: The required option can be guessed based off of the validation rules (i.e. is the field NotBlank or NotNull) or the Doctrine metadata (i.e. is the field nullable). This is very useful, as your client-side validation will automatically match your validation rules.

However, the second paragraph states that the required option is set to true by default.

Resolution

  1. The documentation should be corrected for consistency: under Built-in Field Types – Field Type Options:

    By default, the required option is set to true on the fields that are guessed to be required based on the validation rules, meaning that HTML5-ready browsers will apply client-side validation if the field is left blank.

  2. Symfony’s behaviour should be changed to match the documentation: the required option should be set based on the guessed value and not to true by default.

@stof
Copy link
Member
stof commented Mar 21, 2012

There is no inconsistency in the doc: the first place tells that the HTML5 required attribute is rendered when the field is required (i.e. when the required option is set to true) and the second place tells you that the default value of this option is true. I don't see how it could be conflict as it concerns 2 different things.

@oscherler
Copy link
Contributor Author

OK, then let’s focus on fixing the behaviour, and then update the documentation to reflect it. required should not be true by default because it introcuces the aforementioned client-side/server-side discrepancy, whereas setting it to false doesn’t break anything. From the documentation:

In other words, the required option is "nice", but true server-side validation should always be used.

@mvrhov
Copy link
mvrhov commented Mar 21, 2012

setting required to false so late would be a huge and by huge I mean giant BC break.

@webmozart
Copy link
Contributor

@mvrhov The change is not that giant. @oscherler only talks about cases were a class was annotated with validation constraints and fields that did not have a Not[Null|Blank] constraints were still set to required by default, leaving the "required" attribute in the browser. Server-side validation did not happen, so on browsers < HTML5 you don't even notice the difference after fixing this.

fabpot added a commit that referenced this issue Apr 7, 2012
Commits
-------

e0ce6b4 [Form] Fixed required value guessed by ValidatorTypeGuesser

Discussion
----------

[Form] Fixed required value guessed by ValidatorTypeGuesser

Bug fix: yes
Feature addition: no
Backwards compatibility break: no*
Symfony2 tests pass: yes
Fixes the following tickets: #3661
Todo: -

![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3661)

The documentation states that a field is set to required if constrained with NotBlank or NotNull and to not required otherwise. Due to a bug this was not the case and fields were always required. This is now fixed.

The consequence is that some fields that were required before by default are not required anymore. The only difference is that the HTML5 "required" attribute disappears, because server-side validation did not occur before anyway.
@fabpot fabpot closed this as completed Apr 7, 2012
@kevou84
Copy link
kevou84 commented Nov 3, 2012

I fear this behaviour is back... I'm having required field by default EVEN with required => false set in some case (ie repeated password). Again, I have all my fields set to required even if no assert annotation exists. See my e-mail this morning on SF2 ML called "Forms&validators / Capricious required option ?". thanks for your reply.

@mlwacosmos
Copy link

I am using symfony 2.2 and it looks like bschussek solution has been implemented in this version but the problem remains !!

@tayhimself
Copy link

@mlwacosmos I'm having the same issue as well on 2.2 using Propel (and therefore not using annotations). All fields are required by default.

@dnohales
Copy link
Contributor

Actually, the problem still exist. Symfony does not try to guess the form options when does not need to guess the form type, check FormBuilder::create. As you can see, if Symfony knows the type, doesn't create the form using FormFactory::createBuilderForProperty, that, as you also can see, is the only method that call the guesser chain to guess not only the type but some form options, including the required option.

You can easily reproduce the bug, just create an entity with this field like this:

...
/**
 * @ORM\Column(type="string", length=64, nullable=true)
 */
private $name;
...

Now let's create a form for this entity, if you create the field without specifying the type:

$builder->add('name', null);

Symfony guess that the required options must be false and maxlength must be 64. But if you create the field specifying the type:

$builder->add('name', 'text');

Symfony doesn't guess the required nor maxlength values and it just uses the default values.

This also apply if we change the constraints for the field since {Propel,DoctrineOrm}TypeGuesser and ValidatorTypeGuesser are both in the guesser chain.

@Tobion
Copy link
Contributor
Tobion commented Jun 28, 2014

@eagleoneraptor This is on purpose, so guessing can be deactivated. Otherwise guessing might add things suddenly you don't want.
The bigger problem is that required defaults to true which I already said in #3821 (comment)

@Tobion
Copy link
Contributor
Tobion commented Jun 28, 2014

See also #5365

@InternalServerError
Copy link

Hi there,
I'm using Symfony 2.5 and I've the same issue.
Indeed, I'm trying to setting the "required" value from true to false when I'm on an edit form with an addEventListener.
Unfortunately, I do not any success with it :

class UserType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('firstname', null, array('label' => 'form.user.firstname', 'required' => true, 'trim' => true) )
            ->add('lastname', null, array('label' => 'form.user.lastname', 'required' => true, 'trim' => true) )
            ->add('username', null, array('label' => 'form.user.username', 'required' => true, 'trim' => true) )
            ->add('email', null, array('label' => 'form.user.email', 'required' => true, 'trim' => true))
            ->add('roles', 'collection', array(
                    'type' => 'choice',
                    'options' => array(
                        'label' => false,
                        'choices' => array('ROLE_USER' => 'User', 'ROLE_ADMIN' => 'Administrator')
                    ),
            ));

        $builder->addEventListener(FormEvents::PRE_SET_DATA, function(FormEvent $event) {
            $user = $event->getData();
            $form = $event->getForm();

            if (!$user || null === $user->getId()) {
                $form->add('password', 'repeated', array(
                    'type' => 'password',
                    'invalid_message' => 'form.user.password.repeat',
                    'options' => array('required' => true),
                    'first_options'  => array('label' => 'form.user.password'),
                    'second_options' => array('label' => 'form.user.password.confirm'),
                ));
            } else {
                $form->add('password', 'repeated', array(
                    'type' => 'password',
                    'invalid_message' => 'form.user.password.repeat',
                    'options' => array('required' => false),
                    'first_options'  => array('label' => 'form.user.password', 'required' => false),
                    'second_options' => array('label' => 'form.user.password.confirm', 'required' => false),
                ));
            }
        });
    }

The executed code is well the "else" case, but the fields are still required.

Is something which can be done for fix it please ?

Thanks.

@Tobion
Copy link
Contributor
Tobion commented Nov 3, 2014

Your code is wrong. It must be

$form->add('password', 'repeated', array(
                    'type' => 'password',
                    'invalid_message' => 'form.user.password.repeat',
                    'required' => false
...

@InternalServerError
Copy link

Hi Tobion,

Thanks a lot for your quick answer, that was the trick !
You made my day !

See you.

@Tobion
Copy link
Contributor
Tobion commented Nov 3, 2014

You are welcome.

@felds
Copy link
Contributor
felds commented Nov 3, 2014

This problem still occurs when using the form builder (without any data class).

I think this is the minimal setup:

<?php
// Controller
public function indexAction()
{
    $form = $this->createFormBuilder()
        ->add('q', 'text')
        ->getForm()
    ;

    return [
        'form' => $form->createView(),
    ];
}
{# Template #}
{{ form_widget(form.q) }}
<!-- Output -->
<input type="text" id="form_q" name="form[q]" required="required" class=" form-control" />

@stof
Copy link
Member
stof commented Nov 21, 2014
<?php
// Controller
public function indexAction()
{
    $form = $this->createFormBuilder()
        ->add('q', 'text', ['required' => false])
        ->getForm()
    ;

    return [
        'form' => $form->createView(),
    ];
}

The required option defaults to true

@felds
Copy link
Contributor
felds commented Nov 21, 2014

@stof this still holds true (although this is maybe a DX matter):

In other words, Symfony is artificially introducing a discrepancy between the
client- and the server-side validation criteria, and this by default.

@felds
Copy link
Contributor
felds commented Nov 21, 2014

@stof complementing my last comment:
I can't think of many cases when I would require a field without saying why it's been required.
Can you think of such a situation?

I would probably do something like this:

<?php
// Controller
public function indexAction()
{
    $form = $this->createFormBuilder()
        ->add('q', 'text', [
            'constraints' => [ new NotBlank ],
        ])
        ->getForm()
    ;

    return [
        'form' => $form->createView(),
    ];
}

@rufinus
Copy link
rufinus commented Dec 28, 2014

i faced the same problem, and after digging around i made a Extension which solves the UI problem for me.
i'm sure there is a lot of room for improvements but maybe it gives some folks a starting point, and saves the time i was wasting finding the proper information :-)
http://stackoverflow.com/questions/27671839/required-based-on-asset-from-validation-groups-in-forms/27673607#27673607

@waltercruz
Copy link

So, the validation_groups can't control the html5 required?

@felds
Copy link
Contributor
felds commented Feb 4, 2015

@waltercruz My point exactly. Some validation groups require the field, others do not. Hardcoding a 'required' => false would just flip the problem upside down (not requiring the field even when the validation rules tell otherwise).

@murilolobato
Copy link

I'm also facing this problem, with validation groups and required in the interface. I solved this, making a small change to how things work in the system flow. So I ended up with 2 forms, this way I've avoided the validation groups. But, it's annoying.

@fleskalebas
Copy link

Still a 'problem' in 2.7.3. I can't let Symfony guess the field type because I need textarea instead of normal text. So I have to hardcode the required -> false, instead of using nullable=true.

@felds
Copy link
Contributor
felds commented Aug 28, 2015

@fabpot @webmozart Can we get a little love in here? I think we have a pretty compelling case. Can we, at least, know why this won't be fixed? Or even better, is there a way so we can implement it ourselves in a per-project basis? (something like changing the default guesser)

@stof
Copy link
Member
stof commented Aug 28, 2015

@felds the guesser is fine. when guessing is used, it will guess it properly.

The issue is with the default value when guessing is not used. and this default value cannot be changed in 2.x as it is a BC break. However, the form component has tons of extension points, so it is totally possible to change the default (be aware that you may miss the HTML5 validation in forms provided in third-party bundles if they rely on the existing default):

namespace AppBundle\Form\TypeExtension;

use Symfony\Component\Form\AbstractTypeExtension;

class ChangeRequiredDefaultTypeExtension extends AbstractTypeExtension
{
    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults(array('required' => false));
    }

    public function getExtendedType()
    {
        return 'form';
    }
}
# app/config/services.yml
services:
    # ...
    app.form.type_extension.change_required_default:
        class: AppBundle\Form\TypeExtension\ChangeRequiredDefaultTypeExtension
        tags:
            - { name: form.type_extension, alias: form }

@felds
Copy link
Contributor
felds commented Aug 28, 2015

@stof It still doesn't fix this #3661 (comment), but it is surely a better solution than putting a novalidate attr on every form like I was doing until now. Thank you for that. 😃

To condense the current state of this issue (as a tl;dr for us in the future):

The current form...

$form = $this->createFormBuilder()
    ->add('x', 'text')
    ->add('y', 'text', [
        'constraints' => [
            new Assert\NotBlank,
        ],
    ])
    ->getForm()
;

... returns without the extension:

<input type="text" id="form_x" name="form[x]" required="required" />
<input type="text" id="form_y" name="form[y]" required="required" />

... and with the extension:

<input type="text" id="form_x" name="form[x]" />
<input type="text" id="form_y" name="form[y]" />

What me and others in this issue would like it to return:

<input type="text" id="form_x" name="form[x]" />
<input type="text" id="form_y" name="form[y]" required="required" />

(without hardcoding the 'required' => false/true so we can manage this state from the validation groups)

@drmjo
Copy link
drmjo commented May 17, 2016

@felds were you able to accomplish what you were trying to do

<input type="text" id="form_x" name="form[x]" />
<input type="text" id="form_y" name="form[y]" required="required" /> 

by using validation_groups

@felds
Copy link
Contributor
felds commented May 17, 2016

@drmjo Not really. I've been doing all by hand.

Sent from my George Foreman Grill

On May 17, 2016, at 15:53, Coyote notifications@github.com wrote:

@felds were you able to accomplish what you were trying to do

by using validation_groups


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@drmjo
Copy link
drmjo commented May 17, 2016

@felds Same Here... the logic involved gets really complicated and involves hard coded group names...

@ruskid
Copy link
ruskid commented Jan 26, 2018

Have the same issue. with checkboxes.

->add('published', Type\CheckboxType::class); // this will generate required HTML5
->add('published'); //this will guess it as not required.

The problem with checkboxes is that you can't submit a FALSE value x'd
So user must check it first than send, pretty stupid

Is it possible to set required = false for all the checkboxes only by default?

@Kruczkowski
Copy link

In your class of form you must implement method configureOptions, like this:

public function configureOptions(OptionsResolver $resolver) {
	$resolver->setDefaults([
		'required' => false,
	]);
}

@yyaremenko
Copy link

8 years and still not fixed. Amazing.

@yyaremenko
Copy link
yyaremenko commented Jun 27, 2020

for those who may be interested, this

public function configureOptions(OptionsResolver $resolver) {
	$resolver->setDefaults([
		'required' => false,
	]);
}

WILL NOT WORK (smf v 5.1); even more - none of your fields will have required='required' anymore, even if you explicitly set 'required' => true,

example

class CompanyType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('name', TextType::class, [
                'required' => true,
            ])
            ->add('description', TextareaType::class, [
                'required' => false,
            ])
        ;
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults([
            'required' => false,
        ]);
    }
}

you expect name to have 'required' html, right? nope. only when you remove default to false in configureOptions(), the name becomes required.

so the only option you are left is to do is
first, don't set required to false in configureOptions()
second, manually set 'required' => false to those fields which you want non-required

@xabbuh
Copy link
Member
xabbuh commented Jun 27, 2020

As it seems that there is still the need by developers to only guess the options for a form I have decided to start to work on a new guess_options option which allows to opt-in to only guess the form type options while still allowing to choose which form type to use: #37437

@xabbuh
Copy link
Member
xabbuh commented Jun 27, 2020

@yyaremenko I am not sure if I completely understand the issue you describe in #3661 (comment). It may be worth to open a new issue with it and some example application that allows to reproduce it.

@jsfgreen
Copy link
jsfgreen commented Mar 8, 2022

This should be a simple fix, what's going on?

@quasipickle
Copy link

2024, using 7.3, I am encountering exactly the situation @yyaremenko described.

While there may be technical reasons for this, as a new Symfony user, the behaviour of setDefaults() is completely non-obvious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

0