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] by_reference when embedding a single object #6965

Closed
pvanliefland opened this Issue Feb 4, 2013 · 3 comments

Comments

Projects
None yet
3 participants
@pvanliefland
Contributor

pvanliefland commented Feb 4, 2013

When setting data on an embedded form that has the option 'by_reference' set to false, the provided model data is cloned before being converted to norm data / view data.

I understand that it is the desirable behavior when dealing with collections, as only the Collection instance is cloned, and not its items.

When embedding a single object, the behavior seems problematic : if the form's underlying object is a Doctrine entity previously inserted in the database, cloning it will prevent Doctrine from being able to update it - it will be considered as a new entity.

Of course, I could set 'by_reference' to true but I need the actual setter to be called on the "parent" object.

A simple example : a user entity, with an optional extended profile entity. My user form type embeds the extended profile form type. I need setExtendedProfile() to be called on the user entity, so 'by_reference' needs to be set to false. However, if I use the form to update an existing user entity, the cloning operation on the extendedProfile entity will trick Doctrine into thinking that this extendedProfile entity is a new entity.

Not sure whether it is an actual bug or if I am missing something. My first idea would be to change the Form class so that it only clones \Traversable instances, and not every single object. @bschussek, if it is indeed a reasonable solution I can provide a patch along with adapted unit tests.


class User
{
    /**
     * @var ExtendedProfile
     * @ORM\OneToOne(targetEntity="ExtendedProfile", mappedBy="user", cascade={"all"}, orphanRemoval=true)
     */
    private $extendedProfile;

    public function getExtendedProfile()
    {
        return $this->extendedProfile;
    }

    public function setExtendedProfile($extendedProfile)
    {
        // Do important stuff here
        $this->extendedProfile = $extendedProfile;
    }
    ...
}

class ExtendedProfile
{
     /**
     * @var User
     * @ORM\OneToOne(targetEntity="User", inversedBy="extendedProfile")
     * @ORM\JoinColumn(name="user_id", referencedColumnName="id", nullable=false)
     */
    private $user;
    ...
}

class UserType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('extendedProfile', 'extended_profile', array('by_reference' => false))
            ...
    }
}
@webmozart

This comment has been minimized.

Show comment
Hide comment
@webmozart

webmozart Feb 22, 2013

Contributor

You are right that the "by_reference" option currently controls two things:

  • whether to write the data back into the data of the parent form by calling set*()
  • whether to edit the actual data or a clone of it

I see your use case, and probably the only reasonable solution is to split that option into two separate options.

The name "by_reference" made sense, because it was inspired by parameter passing strategies in programming languages:

  • pass-by-reference passes an actual value and automatically changes its origin
  • pass-by-value passes a copy, the origin must be overwritten manually

If we split these behaviors, the name "by_reference" probably doesn't make sense anymore. How should we name the options then?

$builder->add('extendedProfile', 'extended_profile', array(
    // call the setter in the originating form
    'write_back' => true,
    // edit the data directly, not a clone
    // we could also name this 'clone_data'
    'by_reference' => true,
));

We should pick the option names carefully, I want to avoid option renames in the future.

Contributor

webmozart commented Feb 22, 2013

You are right that the "by_reference" option currently controls two things:

  • whether to write the data back into the data of the parent form by calling set*()
  • whether to edit the actual data or a clone of it

I see your use case, and probably the only reasonable solution is to split that option into two separate options.

The name "by_reference" made sense, because it was inspired by parameter passing strategies in programming languages:

  • pass-by-reference passes an actual value and automatically changes its origin
  • pass-by-value passes a copy, the origin must be overwritten manually

If we split these behaviors, the name "by_reference" probably doesn't make sense anymore. How should we name the options then?

$builder->add('extendedProfile', 'extended_profile', array(
    // call the setter in the originating form
    'write_back' => true,
    // edit the data directly, not a clone
    // we could also name this 'clone_data'
    'by_reference' => true,
));

We should pick the option names carefully, I want to avoid option renames in the future.

@leevigraham

This comment has been minimized.

Show comment
Hide comment
@leevigraham

leevigraham Nov 26, 2013

Contributor

@bschussek Was there a solution for this situation? I couldn't find anything in the referenced ticket.

Contributor

leevigraham commented Nov 26, 2013

@bschussek Was there a solution for this situation? I couldn't find anything in the referenced ticket.

@webmozart

This comment has been minimized.

Show comment
Hide comment
@webmozart

webmozart Oct 16, 2014

Contributor

Closing in favor of #5013.

Contributor

webmozart commented Oct 16, 2014

Closing in favor of #5013.

@webmozart webmozart closed this Oct 16, 2014

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