8000 [Form] Form validates although invalid choice is given with expanded=true · Issue #5113 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

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

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
frastel opened this issue Jul 30, 2012 · 5 comments
Closed

Comments

@frastel
Copy link
frastel commented Jul 30, 2012

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
Copy link
Contributor

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
Copy link
Author
frastel commented Jul 30, 2012

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
Copy link
Contributor

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
Copy link
Contributor

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

@caponica
Copy link
Contributor

@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!

fabpot added a commit that referenced this issue Sep 10, 2013
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
@fabpot fabpot closed this as completed Sep 10, 2013
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

4 participants
0