[Form] Forms now call addXxx() and removeXxx() in your model #3239

webmozart · 2012-02-01T18:06:58Z

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1540
Todo: adapt documentation

Adds functionality for calling addXxx and removeXxx method in your model. All types returning a collection of values are affected: "collection", "choice" (with multiple selection) and "entity" (with multiple selection).

Example:

class Article
{
    public function addTag($tag) { ... }
    public function removeTag($tag) { ... }
    public function getTags($tag) { ... }
}

And the controller:

$form = $this->createFormBuilder($article)
    ->add('tags')
    ->getForm();

Upon modifying the form, addTag() and removeTag() are now called appropiately.

henrikbjorn · 2012-02-01T18:10:52Z

src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php

@@ -36,7 +36,7 @@ public function buildForm(FormBuilder $builder, array $options)
    {
        if ($options['multiple']) {
            $builder
-                ->addEventSubscriber(new MergeCollectionListener())
+//                 ->addEventSubscriber(new MergeDoctrineCollectionListener())


Oops, silly me. Fixed.

stof · 2012-02-01T18:23:49Z

Really great

vicb · 2012-02-01T18:24:04Z

Great !!

Two suggestions:

make "add" and "remove" configurable,
introduce a base class for the remove listeners with (final?) ::getSubscribedEvents() and ::getEventPriorities()

stof · 2012-02-01T18:36:56Z

src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php

@@ -11,6 +11,8 @@

 namespace Symfony\Component\Form\Extension\Core\Type;

+use Symfony\Component\Form\Extension\Core\EventListener\MergeCollectionListener;
+


same here than for the choice type

haswalt · 2012-02-01T18:57:46Z

+1 this

Tobion · 2012-02-01T18:58:14Z

tests/Symfony/Tests/Bridge/Doctrine/Form/Type/EntityTypeTest.php

@@ -350,7 +350,7 @@ public function testSubmitMultipleNonExpandedSingleIdentifier_existingData()
        $field->bind(array('1', '3'));

        // entry with index 0 was removed


leftover, can be removed?

daFish · 2012-02-01T19:54:46Z

+1

michelsalib · 2012-02-01T20:55:37Z

Can wait to have it!
It will save lots of time trying to solve WTF effects and making workarounds.

webmozart · 2012-02-02T09:37:12Z

@vicb: Your first point is done. The second, I don't understand.

stof · 2012-02-02T09:40:50Z

@bschussek your branch conflicts with master according to github

vicb · 2012-02-02T09:52:40Z

@bschussek my point is that I can stand hard-coded priorities which are error prone. A better solution might be to introduce constants (in FormEvents / FormEventPriorities ?) with meaningful names.

…emoveXxx() in your model if found The listener is used by the Collection type as well as the Choice and Entity type (with multiple selection). The effect is that you can have for example this model: class Article { public function addTag($tag) { ... } public function removeTag($tag) { ... } public function getTags($tag) { ... } } You can create a form for the article with a field "tags" of either type "collection" or "choice" (or "entity"). The field will correctly use the three methods of the model for displaying and editing tags.

…nd removers are not called if "by_reference" is disabled.

webmozart · 2012-02-02T10:21:52Z

@stof Rebased

@vicb I know, but who is responsible for managing priorities? There is no central entitty that can do this. (btw this is a general problem of the priority system of the EventDispatcher)

@fabpot Ready to merge.

vicb · 2012-02-02T10:23:28Z

@bschussek doesn't each form has is own dispatcher so there is no need for a global registry here, something local to the form could be good enough.

@vicb

Commits ------- 9b0245b [Form] Made prefix of adder and remover method configurable. Adders and removers are not called if "by_reference" is disabled. 49d1464 [Form] Implemented MergeCollectionListener which calls addXxx() and removeXxx() in your model if found 7837f50 [Form] Added FormUtil::singularify() Discussion ---------- [Form] Forms now call addXxx() and removeXxx() in your model Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: #1540 Todo: adapt documentation ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue1540) Adds functionality for calling `addXxx` and `removeXxx` method in your model. All types returning a collection of values are affected: "collection", "choice" (with multiple selection) and "entity" (with multiple selection). Example: class Article { public function addTag($tag) { ... } public function removeTag($tag) { ... } public function getTags($tag) { ... } } And the controller: $form = $this->createFormBuilder($article) ->add('tags') ->getForm(); Upon modifying the form, addTag() and removeTag() are now called appropiately. --------------------------------------------------------------------------- by stof at 2012-02-01T18:23:49Z Really great --------------------------------------------------------------------------- by vicb at 2012-02-01T18:24:04Z Great !! Two suggestions: * make "add" and "remove" configurable, * introduce a base class for the remove listeners with (final?) `::getSubscribedEvents()` and `::getEventPriorities()` --------------------------------------------------------------------------- by haswalt at 2012-02-01T18:57:46Z +1 this --------------------------------------------------------------------------- by daFish at 2012-02-01T19:54:46Z +1 --------------------------------------------------------------------------- by michelsalib at 2012-02-01T20:55:37Z Can wait to have it! It will save lots of time trying to solve WTF effects and making workarounds. --------------------------------------------------------------------------- by bschussek at 2012-02-02T09:37:12Z @vicb: Your first point is done. The second, I don't understand. --------------------------------------------------------------------------- by stof at 2012-02-02T09:40:50Z @bschussek your branch conflicts with master according to github --------------------------------------------------------------------------- by vicb at 2012-02-02T09:52:40Z @bschussek my point is that I can stand hard-coded priorities which are error prone. A better solution might be to introduce constants (in `FormEvents` / `FormEventPriorities` ?) with meaningful names. --------------------------------------------------------------------------- by bschussek at 2012-02-02T10:21:52Z @stof Rebased @vicb I know, but who is responsible for managing priorities? There is no central entitty that can do this. (btw this is a general problem of the priority system of the EventDispatcher) @fabpot Ready to merge. --------------------------------------------------------------------------- by vicb at 2012-02-02T10:23:28Z @bschussek doesn't each form has is own dispatcher so there is no need for a global registry here, something local to the form could be good enough.

webmozart · 2012-02-02T10:27:03Z

@vicb Sure, but even then we can only register the priorities of the core lsiteners. What if my bundle A and your bundle B add listeners? Who manages the priorities of those?

vicb · 2012-02-02T10:34:09Z

Constants.

I agree it is still not perfect but it could be a little more future proof for those bundles if they rely on well named constants.

webmozart · 2012-02-02T10:35:08Z

@fapbot How are listener priorities managed in the rest of the framework?

vicb · 2012-02-02T10:35:46Z

"@fapbot" ;-)

webmozart · 2012-02-02T10:36:30Z

Oops :) ping @fabpot

stof · 2012-02-02T10:52:40Z

@bschussek currently, they are handled same way you did it there: comments explaining the reason for the value

acasademont · 2012-02-02T11:01:52Z

Most awaited PR in sf2 for years. Thanks!!

vicb · 2012-02-02T11:21:19Z

@bschussek Do you think there is an use case for $allowDelete = false; with only an adder on the object (and the opposite) ?

Tobion · 2012-02-02T11:59:57Z

src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php

@@ -77,6 +90,8 @@ public function getDefaultOptions(array $options)
        return array(
            'allow_add'      => false,
            'allow_delete'   => false,
+            'adder_prefix'   => 'add',
+            'remover_prefix' => 'remove',


I don't think configuring only the prefix is a good solution. Instead I would let the users specify the complete adder and remover method name.
If not specified fallback to the current addSinglurarified approach. Singularify only works with English terms anyway so configuring the prefix only doesn't make sense in other languages.
This would also allow performance improvement when the system doesn't have to infer the name anymore.

Done. See #3256

webmozart · 2012-02-02T12:47:28Z

@vicb Yes I think so. See #3256

vicb · 2012-02-02T12:54:22Z

@bschussek (Y)

Tobion · 2012-02-02T13:33:20Z

One of the dogged bugs (nice animalistic pun ^^) has been tackled by Bernhard, well done.

@fabpot

Commits ------- 8714d79 [Form] Simplified code in MergeCollectionListener 8ab982a [Form] Fixed: Custom add and remove method are not invoked if disallowed 02f61ad [Form] Renamed choice and collection options "adder_prefix" and "remover_prefix" to "add_method" and "remove_method" and allowed to specify full method names b393774 [Form] Used direct method access in MergeCollectionListener instead of Reflection to avoid problems when using class hierarchies d208f4e [Form] Made it possible to use models with only either addXxx() or removeXxx() Discussion ---------- [Form] Fixed edge cases in MergeCollectionListener Bug fix: yes Feature addition: no Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: - Todo: - ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3239) Fixes an issue mentioned in the comments of #3239 see #3239 (comment) --------------------------------------------------------------------------- by bschussek at 2012-02-02T12:12:17Z Wait a minute before merging this. --------------------------------------------------------------------------- by bschussek at 2012-02-02T13:01:55Z @fabpot Ready to merge

henrikbjorn reviewed Feb 1, 2012
View reviewed changes

stof reviewed Feb 1, 2012
View reviewed changes

Tobion reviewed Feb 1, 2012
View reviewed changes

webmozart added 3 commits February 2, 2012 11:15

[Form] Added FormUtil::singularify()

7837f50

[Form] Made prefix of adder and remover method configurable. Adders a…

9b0245b

…nd removers are not called if "by_reference" is disabled.

fabpot merged commit 9b0245b into symfony:master Feb 2, 2012

webmozart mentioned this pull request Feb 2, 2012

[Form] Document usage of adders and removers symfony/symfony-docs#1057

Closed

webmozart mentioned this pull request Feb 2, 2012

[Form] Fixed edge cases in MergeCollectionListener #3256

Merged

Tobion reviewed Feb 2, 2012
View reviewed changes

webmozart mentioned this pull request May 2, 2012

Owner of collection is not set on collection items by collection form field #4184

Closed

		@@ -11,6 +11,8 @@

		namespace Symfony\Component\Form\Extension\Core\Type;

		use Symfony\Component\Form\Extension\Core\EventListener\MergeCollectionListener;

		@@ -350,7 +350,7 @@ public function testSubmitMultipleNonExpandedSingleIdentifier_existingData()
		$field->bind(array('1', '3'));

		// entry with index 0 was removed

Uh oh!

[Form] Forms now call addXxx() and removeXxx() in your model #3239

[Form] Forms now call addXxx() and removeXxx() in your model #3239

Uh oh!

Conversation

webmozart commented Feb 1, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Feb 1, 2012

Uh oh!

vicb commented Feb 1, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haswalt commented Feb 1, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daFish commented Feb 1, 2012

Uh oh!

michelsalib commented Feb 1, 2012

Uh oh!

webmozart commented Feb 2, 2012

Uh oh!

stof commented Feb 2, 2012

Uh oh!

vicb commented Feb 2, 2012

Uh oh!

webmozart commented Feb 2, 2012

Uh oh!

vicb commented Feb 2, 2012

Uh oh!

webmozart commented Feb 2, 2012

Uh oh!

vicb commented Feb 2, 2012

Uh oh!

webmozart commented Feb 2, 2012

Uh oh!

vicb commented Feb 2, 2012

Uh oh!

webmozart commented Feb 2, 2012

Uh oh!

stof commented Feb 2, 2012

Uh oh!

acasademont commented Feb 2, 2012

Uh oh!

vicb commented Feb 2, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmozart commented Feb 2, 2012

Uh oh!

vicb commented Feb 2, 2012

Uh oh!

Tobion commented Feb 2, 2012

Uh oh!

Uh oh!