-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[Form] Removed sidebar about Stateful data mappers #12468
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] Removed sidebar about Stateful data mappers #12468
Conversation
Not sure if we should remove it if it’s available yet. And to say the will find out in its own is another thing than having a well written (short) doc. |
I'm pretty ignorant about "form data mappers" ... but instead of removing this ... I'd remove the "sidebar" and turn this into regular text ... or even maybe a small section? Then, I'd change the "Stateful Data Mappers" title by something more focused around "Accessing Services in Data Mappers", which would be the most useful feature of doing this ... but you can access services from regular forms too, right? ... so maybe we should indeed remove this sidebar entirely 🤔 |
// ... | ||
|
||
// Initialize the data mapper class and e.g. pass some state | ||
->setDataMapper(new ColorMapper($options['opacity'])) |
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.
And also it isn't the best example, you can access to any form option from the data mapper methods through the parent form of the children.
@javiereguiluz accessing services isn't the dealbreaker here: Form types are services, so a form type that implements Accessing a FormType option is also easily achievable when implementing data mapper in the form type, like @yceruto also commented. The only case were this is not possible is when you have a "state" per Form (not form type!). In this case, state is not maintained by form types, but by the parent form. As this is such an edge-case imho, I think removing it is the best thing. |
After reading the new comments now I think that removing this is the best thing to do. Merging it. Thanks! |
…erj) This PR was merged into the 3.4 branch. Discussion ---------- [Form] Removed sidebar about Stateful data mappers Fixes #11401 As you can see in the referenced comment by @webmozart, having to implement the data mapper in a separate class is a real edge-case: It would have to depend on an option configured on the Form. I would say it's better to just completely remove the sidebar. If a user needs such advanced usage of the form component, they'll probably find out themselves that you can also pass instances of another class to this method. Commits ------- 60b7dd8 Removed sidebar about Statefull data mappers
Fixes #11401
As you can see in the referenced comment by @webmozart, having to implement the data mapper in a separate class is a real edge-case: It would have to depend on an option configured on the Form. I would say it's better to just completely remove the sidebar. If a user needs such advanced usage of the form component, they'll probably find out themselves that you can also pass instances of another class to this method.