-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[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
[Cookbook][Dynamic Forms] Simplify first example #3312
Conversation
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>` |
There was a problem hiding this comment.
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`
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@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. |
@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) |
@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. |
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 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>` |
|
@@ -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 |
There was a problem hiding this comment.
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
[Cookbook][Dynamic Forms] Simplify first example
Awesome work Philipp - very clean, merged with no changes :) Thanks! |
@weaverryan, I did not forget your note from #2982. If this one is ok I can move on to the Ajax example.