Skip to content

[Cookbook][Dynamic Forms] Simplify first example #3312

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

Merged
merged 4 commits into from
Dec 19, 2013
Merged

[Cookbook][Dynamic Forms] Simplify first example #3312

merged 4 commits into from
Dec 19, 2013

Conversation

bicpi
Copy link
Contributor

@bicpi bicpi commented Dec 11, 2013

Q A
Doc fix? yes
New docs? yes
Applies to all
Fixed tickets #2464 (partially)

@weaverryan, I did not forget your note from #2982. If this one is ok I can move on to the Ajax example.

@ggam
Copy link

ggam commented Dec 11, 2013

Nice. But, since it now recommends to use an event listener instead of a subscriber, I think we should add a note about the differences between the two, and some use cases or advise on when to use the subscriber.


.. versionadded:: 2.2
The ability to pass a string into :method:`FormInterface::add <Symfony\\Component\\Form\\FormInterface::add>`
The ability to pass a string into
:method:`FormInterface::add <Symfony\\Component\\Form\\FormInterface::add>`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not simply

:method:`Symfony\\Component\\Form\\FormInterface`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was copy&paste from the existing documentation. But this is about the parameter type for the add() method, so what is wrong with it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems rather complicated to me to use a label here when it is the same to what would be displayed when omitting it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The classname would not be displayed when we remove thelabel text

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stupid me. You're right of course.

@bicpi
Copy link
Contributor Author

bicpi commented Dec 11, 2013

@ggam Shouldn't the differences be explained in EventDispatcher component chapter? I did not found an explanation there what's the advantage of an event subscriber. For the first example in this cookbook I think it's obvious that it is the easiest and fastest way to attach dynamic functionality (as mentioned in #2464). If the closure becomes too complex then put it in its own method. And if you want to get it out of the way completely or reuse the logic elsewhere, then put it into a separate event subscriber class.

@wouterj
Copy link
Member

wouterj commented Dec 11, 2013

@bicpi as soon as you put event handlers in a class, it should become a subscriber imo. I think we can add some information about that (and link to the eventdispatcher docs)

@ggam
Copy link

ggam commented Dec 11, 2013

@bicpi for me the problem is that the showed example is too simple for an event subscriber, so it's difficult to see its benefits. I just wonder if we can provide a more complex use case example for the subscriber, just to illustrate the user, and then link to the EventDispatcher documentation, as you and @wouterj say.

@bicpi
Copy link
Contributor Author

bicpi commented Dec 13, 2013

But the topic here is how to build a form dynamically. It may be confusing to introduce a complex example only to justify the usage of a subscriber. The text says For better reusability or if there is some heavy logic in your event listener, you can also move the logic for creating the name field to an event
subscriber
. Isn't this enough? This PR just aims to add the example for an event listener and as it is the most simple way it should be the first example. The event subscriber example did not change.

I would like to add a link to http://symfony.com/doc/current/components/event_dispatcher/introduction.html#using-event-subscribers, can someone help me with the syntax? I only found ":doc:" links pointing to whole pages, not to anchors. Is this possible?

@xabbuh
Copy link
Member

xabbuh commented Dec 15, 2013

I would like to add a link to http://symfony.com/doc/current/components/event_dispatcher/introduction.html#using-event-subscribers, can someone help me with the syntax? I only found ":doc:" links pointing to whole pages, not to anchors. Is this possible?

You can use this:

:ref:`your label <event_dispatcher-using-event-subscribers>`

@bicpi
Copy link
Contributor Author

bicpi commented Dec 15, 2013

:ref: is what I was looking for, thank you!

@@ -99,26 +99,97 @@ to an event subscriber::
{
$builder->add('price');

$builder->addEventSubscriber(new AddNameFieldSubscriber());
$builder->addEventListener(FormEvents::PRE_SET_DATA, function(FormEvent $event) {
// adding the name field if needed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// ... adding the name field if needed

weaverryan added a commit that referenced this pull request Dec 19, 2013
[Cookbook][Dynamic Forms] Simplify first example
@weaverryan weaverryan merged commit 5047ff6 into symfony:2.3 Dec 19, 2013
@weaverryan
Copy link
Member

Awesome work Philipp - very clean, merged with no changes :)

Thanks!

@bicpi bicpi deleted the simplify_dynamic_form_modification branch December 19, 2013 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants