[Form] Form validates although invalid choice is given with expanded=true #5113

frastel · 2012-07-30T12:51:32Z

In Symfony 2.0 a form is not valid when an unknown/invalid choice value is bound to a form. It does not matter if the type option expanded is set to true or false.
However with the latest changes in the Symfony master the form is now valid with this invalid choice. The invalid choice "is lost" somewhere in the form workflow and so the field is validated successfully because its value is empty and not required.
When the expanded=true option is set in the type everything is working as expected.
Besides one can pass anything as invalid choice, a string, an array the result is always the same: the form validates and the resulting field value is null. It also does not matter if the choice type gets the optional Choice constraint assigned.

Here two example test cases (without namespaces) for Symfony 2.0 and 2.1

Test case for Symfony 2.0 (form is not valid: OK):

require_once dirname(__DIR__).'/../../../app/AppKernel.php';

use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\FormBuilder;
use Symfony\Component\Validator\Constraints\Choice;

/**
 * Test case
 */
class InvalidChoiceType20Test extends \PHPUnit_Framework_TestCase
{
    private $container;

    protected function setUp()
    {
        $this->kernel = new \AppKernel('test', true);
        $this->kernel->boot();

        $this->container = $this->kernel->getContainer();
    }

    /**
     * @group invalidchoice
     */
    public function testInvalidChoice()
    {
        $type = new WhatChoiceType();

        $form = $this->container->get('form.factory')->createBuilder($type)->getForm();
        $form->bind(array(
            'what' => 'baz' //invalid choice should fail
        ));

        print_r($form->getErrors());
        //print_r($form->get('something')->getErrors());
        $this->assertFalse($form->isValid());
    }
}

class WhatChoiceType extends AbstractType
{
    public function buildForm(FormBuilder $builder, array $options)
    {
        $choices = array('foo' => 'FOO!', 'bar' => 'BAR?');

        $options = array(
            'label' => 'Your choice',
            'choices' => $choices,
            'multiple' => false,
            'expanded' => true,
        );
        $builder->add('what', 'choice', $options);
    }

    public function getDefaultOptions(array $options)
    {
        return array(
            'csrf_protection' => false
        );
    }

    public function getName()
    {
        return 'what';
    }
}

Test case for Symfony 2 master (form is valid: FAIL!):

require_once dirname(__DIR__).'/../../../../../app/AppKernel.php';

use Symfony\Component\Form\FormFactory;
use Symfony\Component\Form\Form;

use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Validator\Constraints\Choice;

/**
 * Test case
 */
class InvalidChoiceType21Test extends \PHPUnit_Framework_TestCase
{
    private $container;

    protected function setUp()
    {
        $this->kernel = new \AppKernel('test', true);
        $this->kernel->boot();

        $this->container = $this->kernel->getContainer();
    }

    /**
     * @group invalidchoice
     */
    public function testInvalidChoice()
    {
        $whatType = new WhatChoiceType();

        $form = $this->container->get('form.factory')->createBuilder($whatType, null, array('csrf_protection' => false))->getForm();
        $form->bind(array(
            'what' => 'baz' //invalid choice should fail
        ));

        print_r($form->getErrors());
        //print_r($form->get('framework')->getErrors());
        $this->assertFalse($form->isValid());
    }

}

class WhatChoiceType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $choices = array('foo' => 'FOO!', 'bar' => 'BAR?');

        $options = array(
            'label' => 'Your choice',
            'choices' => $choices,
            'multiple' => false,
            'expanded' => true,
            'constraints' => new Choice(array(
                'multiple' => false,
                'choices' => array_keys($choices),
                'message' => 'Plz choose sth'
            ))
        );
        $builder->add('what', 'choice', $options);
    }

    public function getName()
    {
        return 'what';
    }
}

Maybe we are missing something, but we could not get it running.

webmozart · 2012-07-30T14:10:41Z

Hi,

Thanks for taking the time to report this issue. I can confirm the problem for expanded choice fields, both single- and multiple-choice. Nevertheless, having invalid choices in such cases should be rare, i.e. limited to people playing with the POST parameters. As no invalid choices can get through, this is not a security issue either.

This is somewhat tricky to fix, so I'm not sure whether the fix will make it into 2.1.

Bernhard

frastel · 2012-07-30T15:57:57Z

Hi Bernhard,
Yes we already thought about not to worry about this problem due to this "filtering" mechanism.
Currently a developer has to check in a unit test if the form validates correctly and that the according field is null in the resulting data. This is quite confusing as a symfony developer is used another way here.
If this issue will be not fixed for 2.1 is there any chance to add some information about this fact in the documentation? I think several developers will stumble about this case.

We already had a look at the component, but besides some headaches we had no success finding a solution.

Frank

webmozart · 2012-07-30T20:15:03Z

We'd appreciate a lot if you could open a PR on https://github.com/symfony/symfony-doc if you feel that this could help others.

webmozart · 2013-05-05T22:32:43Z

Good news :) This is fixed in the referenced PR.

caponica · 2013-08-28T23:22:39Z

@bschussek note that this does not only happen if somebody hacks a form. There are other ways this can happen, e.g. if a form's options are dynamically set but change between the user loading the form and submitting the data.

If there's anything that I can do to help get this fixed and merged sooner rather than later just let me know!

This PR was merged into the master branch. Discussion ---------- [Form] Fixed expanded choice field to be marked invalid when unknown choices are submitted #7939 must be merged before this PR is merged. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #5113, #5190 | License | MIT | Doc PR | - TODO: - [x] test EntityChoiceList for stricter rules - [ ] test ModelChoiceList for stricter rules - [x] remove/deprecate the ChoiceList::getIndicesFor*() methods Commits ------- 9efdb8e [Form] Deprecated ChoiceList::getIndicesFor*() methods 67ba131 [DoctrineBridge] Improved test coverage of EntityChoiceList 31e5ce5 [Form] Improved test coverage of ChoiceList classes 6283b0e [Form] Fixed expanded choice field to be marked invalid when unknown choices are submitted 79a214f [Form] Fixed ChoiceList::get*By*() methods to preserve order and array keys 62fbed6 [Form] Removed usage of the ChoiceList::getIndicesFor*() methods where they don't offer any performance benefit

webmozart mentioned this issue Aug 16, 2012

[Form] FixCheckboxInputListener can cause an UnexpectedTypeException #5190

Closed

webmozart mentioned this issue Jan 5, 2013

[Form][Choice] If invalid value submitted radio\checkbox fields behavior not consistent with select one. #6352

Closed

webmozart mentioned this issue May 5, 2013

[Form] Fixed expanded choice field to be marked invalid when unknown choices are submitted #7940

Merged

3 tasks

fabpot closed this as completed Sep 10, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Form] Form validates although invalid choice is given with expanded=true #5113

[Form] Form validates although invalid choice is given with expanded=true #5113

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[Form] Form validates although invalid choice is given with expanded=true #5113

[Form] Form validates although invalid choice is given with expanded=true #5113

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!