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

[Form][DelegatingValidator] errors related to input[type=radio] rendered in wrong place #1917

Closed
brikou opened this issue Aug 7, 2011 · 18 comments
Milestone

Comments

@brikou
Copy link
Contributor

brikou commented Aug 7, 2011

When using expanded choice list (input[type=radio] tags), error is bubbling in top of the form and not next to the errornous field, when expanded is set to false (select tag), everything workds as expected...

@ajfranzoia
Copy link

I'm having the same problem. The issue lies in the DelegatingValidator class:

foreach ($violations as $violation) {
        $propertyPath = $violation->getPropertyPath();
        $template = $violation->getMessageTemplate();
        $parameters = $violation->getMessageParameters();
        $error = new FormError($template, $parameters);

        foreach ($mapping as $mappedPath => $child) {
              if (preg_match($mappedPath, $propertyPath)) {
                   $child->addError($error);
                   continue 2;
              }
        }

       $form->addError($error);
}

The mapping never matches the radio or select id and therefore the error is appended to the form

@ajfranzoia
Copy link

Situation example:

I have one form called "user", with a subform called "profile".

Fields in subform that aren't expanded choices have mappings like "[/^children[profile].data.name_of_field(?!\w)]"
Fields in subform that are expanded choices have mappings like "[/^children[profile][name_of_field].data.(?!\w)]"

The target property_path of the constraint violation for these fields is "children[profile].data.name_of_field"

This way, preg_match($mappedPath, $propertyPath) for expanded fields will not match

@fabpot
Copy link
Member

fabpot commented Sep 14, 2011

I'm not able to reproduce this issue. Furthermore, when using radio buttons, I don't see what kind of errors you can have (except if you mess up with the HTML source code of course)? Anyway, can you provide a simple example that exhibits the problem? Thanks.

@ajfranzoia
Copy link

I'll try to build up an example in these days and I'll submit as soon as possible...

@pkruithof
Copy link
Contributor

@fabpot I'm having a similar issue, although not with radio buttons but with an integer field.

The constraint:

build_year:
    - Type: { type: integer }

The (partial) form:

$builder->add('build_year', 'integer', array('required' => false));

When I change the form field type to text, the error message is displayed in the field row, but with integer it's displayed above the form (as a general form error).

Should I open a new issue for this?

@fabpot
Copy link
Member

fabpot commented Sep 28, 2011

@pkruithof: yes please.

@ryancastle
Copy link

We're having similar issues with EntityType fields. When the underlying entity selected isn't valid the errors generated are attached to the root form object (and then usually don't have any meaning). It would be good if 'choice' fields similar had an option not to validate the selected entity.

I think there is a case for making this validation optional. There are scenarios with legacy data (or even just changing requirements) where you are willing to have "invalid" data in the system, but want to make sure that next time it's update it is made valid by the user.

@spantaleev
Copy link

I'm also experiencing the same problem.

Here's how my form is structured:

TopFormType
    InnerFormType
        Choice(expanded = true)

Validation errors for that choice field get attached not to the field or to it's enclosing form, but to the top.

It seems like the changes made by @tamirvs resolve it.

@stof
Copy link
Member

stof commented Apr 4, 2012

@spantaleev does it still occur in the master branch with the refactoring ?

@spantaleev
Copy link

Strange.. I couldn't reproduce this on 2.0.12, 2.0.12 + the Form component from master, and the 2.0.10 that the project was using at the time.

I don't have the exact piece of code that created that form structure before, so I had to recreate it again now. Maybe I had something slightly different then, or something else changed..

In any case, things seem to be fine for me now.

@stof stof closed this as completed Apr 4, 2012
@tamirvs
Copy link
Contributor

tamirvs commented Apr 6, 2012

@stof I was able to reproduce this error on 2.0.12.
It occurs on expanded choice fields of nested forms, for example:

class UserType extends AbstractType
{
    public function buildForm(FormBuilder $builder, array $options)
    {
        $builder->add('username', 'text')
                ->add('gender', 'choice', array('choices' => array('0' => 'male',
                                                                   '1' => 'female'),
                                                'expanded' => true));
    }
}

class RegistrationType extends AbstractType
{
    public function buildForm(FormBuilder $builder, array $options)
    {
        $builder->add('user', new UserType());
    }
}

In this case, username will render it's error in the right place, while gender will bubble up.

I was able to fix it Here, please have a look :)

@webspin
Copy link

webspin commented Jun 16, 2012

This is still an issue, please don't close it if it hasn't been fixed yet...

@stof
Copy link
Member

stof commented Jun 17, 2012

@webspin please try with Symfony master as many things changed in the Form component (and some changes have been made for the mapping of errors)

@kevindew
Copy link

I just came across this error (expanded embedded forms) with 2.0.15 and the changes @webspin posted did fix it. Be good if we could get this fixed in the 2.0 branch as it can leave some pretty nasty bugs

@stof
Copy link
Member

stof commented Jun 19, 2012

@kevindew which changes ? the only comment posted by @webspin in the issue is to say that it is not fixed.

@kevindew
Copy link

@stof the changes linked to here: tamirvs@cd136d0

@tamirvs
Copy link
Contributor

tamirvs commented Jun 19, 2012

@kevindew, @stof these changes were posted by me, but I think that @bschussek was working on a fix (#4341, #3903)

@webspin
Copy link

webspin commented Jun 20, 2012

So, is this fix coming or do I really need to upgrade to 2.1? (which sucks, being the ***** kohana style of doing stuff)

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

10 participants