Skip to content

[Form] Embedded forms using same property path - error bubbling problems #5578

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
lopsided opened this issue Sep 23, 2012 · 11 comments
Closed

Comments

@lopsided
Copy link

I have a problem that error messages for an embedded form are bubbling to the parent form when used with another embedded form pointing to the same property_path of a related object.

My situation is like this, I have Users and Account entities I have split up the Account into different forms, then for the registration form I am pulling these bits together like so:

class RegistrationFormType extends AbstractType
{

public function buildForm(FormBuilderInterface $builder, array $options)
{
    $builder
        ->add('email', 'email', array(
            'label' => '* Email address:'
        ))
        ->add('account_personal', 'my_personalinfo_form', array(
            'property_path' => 'account'
        ))
        ->add('account_contact', 'my_contactinfo_form', array(
            'property_path' => 'account'
        ))
    ;
}

The problem is that error messages for account_personal are bubbling to the top of the form. For example "Please enter your first name" if first name is left blank in the personal info form. The 'personal' and 'contact' forms work fine in their own forms on their own pages.

The error messages for account_contact are fine and appear next to the correct fields.

HOWEVER, if I swap the two ->add bits about above (so have account_contact first) then the problem reverses; the error messages for account_personal now appear correctly next to their corresponding fields, but now the errors for account_contact get bubbled to the top!

Does this sound like a bug or am I doing something wrong?

Thanks!

---- Other files for reference:

Personal info form:

class PersonalInfoType extends AbstractType
{

public function buildForm(FormBuilderInterface $builder, array $options)
{
    $builder
        ->add('title', 'text')
        ->add('first_name', 'text', array(
            'required' => true,
            'label' => '* First name:'
        ))
        ->add('last_name', 'text', array(
            'required' => true,
            'label' => '* Surname:'
        ))
    ;
}

public function setDefaultOptions(OptionsResolverInterface $resolver)
{
    $resolver->setDefaults(array(
        'data_class' => 'My\UserBundle\Entity\Account',
        'validation_groups' => array('personalInfo')
    ));
}

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

Contact info form:

class ContactInfoType extends AbstractType
{
public function buildForm(FormBuilderInterface $builder, array $options)
{
    $builder
        ->add('postcode', 'text', array(
            'required' => true
        ))
        ->add('address_1', 'text', array(
            'required' => true
        ))
        ->add('address_2', 'text', array(
            'required' => false
        ))
        ->add('address_3', 'text', array(
            'required' => false
        ))
        ->add('town', 'text')
        ->add('phone_daytime', 'text', array(
            'required' => true
        ))
        ->add('phone_mobile', 'text', array(
            'required' => true
        ))
    ;
}

public function setDefaultOptions(OptionsResolverInterface $resolver)
{
    $resolver->setDefaults(array(
        'data_class' => 'My\UserBundle\Entity\Account',
        'validation_groups' => array('contactInfo')
    ));
}

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

and the Account entity for completion:

/**
 * My\UserBundle\Entity\Account
 *
 * @ORM\Entity
 * @ORM\Table( name="accounts" )
 */
class Account
{
/**
 * @ORM\Id
 * @ORM\Column(type="integer")
 * @ORM\GeneratedValue(strategy="AUTO")
 */
protected $id;

/**
 * @ORM\Column(type="string", length=25, nullable=true)
 * @Assert\Choice(choices = {"Mr", "Mrs", "Miss", "Ms", "Dr", "Prof"}, message = "Choose a valid title", groups={"personalInfo"})
 */
protected $title;

/**
 * @ORM\Column(type="string", length=150, nullable=true)
 * @Assert\NotBlank(message="Please enter your first name", groups={"personalInfo"})
 * @Assert\Length(max=150, maxMessage="null|Your first name must have less than {{ limit }} characters", groups={"personalInfo"})
 * @Assert\Regex(
 *     pattern="/\d/",
 *     match=false,
 *     message="Your name cannot contain a number"
 * )
 */
protected $first_name;

/**
 * @ORM\Column(type="string", length=150, nullable=true)
 * @Assert\NotBlank(message="Please enter your last name", groups={"personalInfo"})
 * @Assert\Length(max=150, maxMessage="null|Your last name must have less than {{ limit }} characters", groups={"personalInfo"})
 * @Assert\Regex(
 *     pattern="/\d/",
 *     match=false,
 *     message="Your name cannot contain a number"
 * )
 */
protected $last_name;

/**
 * @ORM\Column(type="string", length=255, nullable=true)
 * @MyAssert\Phone(message="Your daytime phone number is not valid", groups={"contactInfo"})
 */
protected $phone_daytime;

/**
 * @ORM\Column(type="string", length=255, nullable=true)
 * @MyAssert\MobilePhone(message="Your mobile phone number is not valid", groups={"contactInfo"})
 */
protected $phone_mobile;

/**
 * @ORM\Column(type="string", length=255)
 * @Assert\NotBlank(message="Please enter the first line of your address", groups={"contactInfo"}
 * @Assert\Length(max=250, maxMessage="null|The first line of your address must have less than {{ limit }} characters", groups={"contactInfo"})
 */
protected $address_1;

/**
 * @ORM\Column(type="string", length=255, nullable=true)
 * @Assert\Length(max=250, maxMessage="null|The second line of your address must have less than {{ limit }} characters", groups={"contactInfo"})
 */
protected $address_2;

/**
 * @ORM\Column(type="string", length=255, nullable=true)
 * @Assert\Length(max=250, maxMessage="null|The third line of your address must have less than {{ limit }} characters", groups={"contactInfo"})
 */
protected $address_3;

/**
 * @ORM\Column(type="string", length=45, nullable=true)
 * @Assert\NotBlank(message="Please enter your town", groups={"contactInfo"})
 * @Assert\Length(max=45, maxMessage="null|Your town name must have less than {{ limit }} characters", groups={"contactInfo"})
 */
protected $town;

/**
 * @ORM\Column(type="string", length=45, nullable=true)
 * @Assert\NotBlank(message="Please enter your postcode", groups={"contactInfo"})
 * @MyAssert\Postcode(message="Invalid postcode entered", groups={"contactInfo"})
 */
protected $postcode;

 ... etc
@lopsided
Copy link
Author

ping @bschussek - any ideas?

@webmozart
Copy link
Contributor

Hey,

Thank you for taking the time to report this issue.

Adding two fields with the same property path smells fishy to me. Not sure if that could work, I did not try it so far. However, I suggest you use the "virtual" option which was created for solving your use case:

<?php

public function buildForm(FormBuilderInterface $builder, array $options)
{
    $builder
        ->add('email', 'email', array(
            'label' => '* Email address:'
        ))
        ->add('account_personal', 'my_personalinfo_form', array(
            'virtual' => true,
        ))
        ->add('account_contact', 'my_contactinfo_form', array(
            'virtual' => true,
        ))
    ;
}

Within "my_personalinfo_form" and "my_contactinfo_form", set the property paths of the fields explicitly.

I will leave this ticket open for further investigation.

@webmozart
Copy link
Contributor

I added a corresponding feature request.

fabpot added a commit that referenced this issue Oct 3, 2012
This PR was merged into the 2.1 branch.

Commits
-------

2568432 [Form] Hardened code of ViolationMapper against errors

Discussion
----------

[Form] Hardened code of ViolationMapper against errors

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

This ticket improves the code of ViolationMapper to reduce the risk of bugs and in order to make further bug fixing easier. It was implemented while trying to solve #5578 and is semantically equivalent to the previous version.
@lopsided
Copy link
Author

lopsided commented Oct 3, 2012

Thanks for the reply @bschussek!

I still had errors following your suggestion, the first was along the lines of:

Neither property "address_1" nor method "getAddress1()" nor method "isAddress1()" exists in class "My\UserBundle\Entity\User" 

Then I changed the property_path settings of the fields in the sub-forms to account.address_1 etc as you suggested, but the error changed to:

Neither property "account" nor method "getAccount()" nor method "isAccount()" exists in class "My\UserBundle\Entity\Account" 

Couldn't get past this, but then came up with a different tact of creating a new FormType to contain the 2 embedded forms using virtual fields:

class RegistrationAccountFormType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('account_personal', 'my_personalinfo_form', array(
                'data_class' => 'My\UserBundle\Entity\Account',
                'virtual' => true
            ))
            ->add('account_contact', 'my_contactinfo_form', array(
                'data_class' => 'My\UserBundle\Entity\Account',
                'virtual' => true
            ))
        ;
    }

    public function setDefaultOptions(OptionsResolverInterface $resolver)
    {
        $resolver->setDefaults(array(
            'data_class' => 'My\UserBundle\Entity\Account'
        ));
    }

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

and then replaced these fields in the main RegistrationFormType with

->add('account', new RegistrationAccountFormType)

This seems like the 'proper' way of doing this, it keeps the property_paths unique within the form and no property_path changes required in the sub-forms either.

@webmozart
Copy link
Contributor

Embedding "account_personal" and "account_contact" directly within the main form definitly should be possible. Could you post your complete code?

@lopsided
Copy link
Author

lopsided commented Oct 9, 2012

Hi @bschussek Sorry for the slow reply.

So following your suggestions my code looked like this:

Main registration form:

class RegistrationFormType extends AbstractType
{

public function buildForm(FormBuilderInterface $builder, array $options)
{
    $builder
        ->add('email', 'email', array(
            'label' => '* Email address:'
        ))
        ->add('account_personal', 'my_personalinfo_form', array(
            'virtual' => true
        ))
        ->add('account_contact', 'my_contactinfo_form', array(
            'virtual' => true
        ))
    ;
}

Personal info form:

class PersonalInfoType extends AbstractType
{

public function buildForm(FormBuilderInterface $builder, array $options)
{
    $builder
        ->add('title', 'text', array(
            'property_path' => 'account.title'              ///  I had to add these property_paths otherwise was getting errors
        ))
        ->add('first_name', 'text', array(
            'required' => true,
            'label' => '* First name:',
            'property_path' => 'account.first_name'
        ))
        ->add('last_name', 'text', array(
            'required' => true,
            'label' => '* Surname:',
            'property_path' => 'account.last_name'
        ))
    ;
}

public function setDefaultOptions(OptionsResolverInterface $resolver)
{
    $resolver->setDefaults(array(
        'data_class' => 'My\UserBundle\Entity\Account',
        'validation_groups' => array('personalInfo')
    ));
}

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

Contact info form:

class ContactInfoType extends AbstractType
{
public function buildForm(FormBuilderInterface $builder, array $options)
{
    $builder
        ->add('postcode', 'text', array(
            'required' => true,
            'property_path' => 'account.postcode'
        ))
        ->add('address_1', 'text', array(
            'required' => true,
            'property_path' => 'account.address_1'
        ))
        ->add('address_2', 'text', array(
            'required' => false,
            'property_path' => 'account.address_2'
        ))
        ->add('address_3', 'text', array(
            'required' => false
            'property_path' => 'account.address_3'
        ))
        ->add('town', 'text', array(
            'property_path' => 'account.town'
        ))
        ->add('phone_daytime', 'text', array(
            'required' => true,
            'property_path' => 'account.phone_daytime'
        ))
        ->add('phone_mobile', 'text', array(
            'required' => true,
            'property_path' => 'account.phone_mobile'
        ))
    ;
}

public function setDefaultOptions(OptionsResolverInterface $resolver)
{
    $resolver->setDefaults(array(
        'data_class' => 'My\UserBundle\Entity\Account',
        'validation_groups' => array('contactInfo')
    ));
}

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

The error I get with this configuration (I just set this up again to check) is thrown when processing the form (ie, it renders fine):

 Neither property "account" nor method "getAccount()" nor method "isAccount()" exists in class "My\UserBundle\Entity\Account" 

I suspect the data_class is causing the problem. I tried changing these (in PersonalInfoType and ContactType) to My\UserBundle\Entity\User instead, giving:

Expected argument of type "object or array", "NULL" given 

Again, thrown on form submit.

As I say, the other solution I mentioned works well for me anyway as it means I can still use the PersonalInfoType and ContactType forms in isolation without changing any of my other code.

Hope this helps, let me know if I can provide any more information.

@webmozart
Copy link
Contributor

This is weird. I'm reopening this ticket until this is solved.

@webmozart webmozart reopened this Oct 9, 2012
@webmozart
Copy link
Contributor

I just tested your code and, apart from a few mistakes, it works for me. You should not define any custom property paths on PersonalInfoType and ContactInfoType. The code will work with the default property paths.

Also, don't forget to set the "data_class" option on RegistrationType (if you did not already).

@lopsided
Copy link
Author

This doesn't work for me. I think because the data_class option on RegistrationType is set to my User object My\UserBundle\Entity\User, which is where the email and other fields (omitted from the above) are set. The 2 embedded forms are dealing with the related entity My\UserBundle\Entity\Account. If I set these embedded forms without the custom property paths I just get errors like:

Neither property "title" nor method "getTitle()" nor method "isTitle()" exists in class "My\UserBundle\Entity\User"

Does that make sense?

@webmozart
Copy link
Contributor

I correctly reproduced your problem now, I previously made a mistake. I think this may be fixable by solving #5720.

@webmozart
Copy link
Contributor

This is fixed in the referenced PR.

fabpot added a commit that referenced this issue Apr 19, 2013
This PR was merged into the master branch.

Discussion
----------

[2.3] [Form] Renamed option "virtual" to "inherit_data" and improved handling of such forms

Bug fix: yes
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #5899, #5720, #5578
Todo: -
License of the code: MIT
Documentation PR: symfony/symfony-docs#2107

This PR renames the option "virtual" to "inherit_data" for more clarity (the old option is deprecated and usable until 2.3). It also fixes the behavior of forms having that option set.

Forms with that option set will now correctly return their parents' data from `getData()`, `getNormData()` and `getViewData()`. Furthermore, `getPropertyPath()` was fixed for forms that inherit their parent data.

Commits
-------

1290b80 [Form] Fixed the deprecation notes for the "virtual" option
ac2ca44 [Form] Moved parent data inheritance from data mappers to Form
8ea5e1a [Form] Renamed option "virtual" to "inherit_data"
@fabpot fabpot closed this as completed Apr 19, 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

3 participants