Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[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

Merged
merged 1 commit into from
Oct 15, 2019

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Oct 12, 2019

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.

@OskarStark
Copy link
Contributor

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.

@javiereguiluz javiereguiluz added this to the 3.4 milestone Oct 14, 2019
@javiereguiluz
Copy link
Member

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 🤔

@javiereguiluz javiereguiluz changed the title [Form] Removed sidebar about Statefull data mappers [Form] Removed sidebar about Stateful data mappers Oct 14, 2019
// ...

// Initialize the data mapper class and e.g. pass some state
->setDataMapper(new ColorMapper($options['opacity']))
Copy link
Member

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.

@wouterj
Copy link
Member Author

wouterj commented Oct 14, 2019

@javiereguiluz accessing services isn't the dealbreaker here: Form types are services, so a form type that implements DataMapperInterface can also access services while data mapping.

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.

@javiereguiluz
Copy link
Member

After reading the new comments now I think that removing this is the best thing to do. Merging it. Thanks!

javiereguiluz added a commit that referenced this pull request Oct 15, 2019
…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
@javiereguiluz javiereguiluz merged commit 60b7dd8 into symfony:3.4 Oct 15, 2019
@wouterj wouterj deleted the issue-11401/data-mapper-fix branch October 15, 2019 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.