Skip to content
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] Form Events documentation incorrectly states fields cannot be removed/added during SUBMIT event #5990

Closed
bkosborne opened this issue Dec 9, 2015 · 6 comments
Labels
actionable Clear and specific issues ready for anyone to take them. Form good first issue Ideal for your first contribution! (some Symfony experience may be required) hasPR A Pull Request has already been submitted for this issue.

Comments

@bkosborne
Copy link
Contributor

The form events page indicate

"At this point, you cannot add or remove fields to the form."

when describing the SUBMIT event. It then follows up with an example listener that the Form component provides:

The Symfony\Component\Form\Extension\Core\EventListener\ResizeFormListener subscribes to the FormEvents::SUBMIT event in order to remove the fields that need to be removed whenever manipulating a collection of forms for which allow_delete has been enabled.

Indeed, when looking at the code for that listener, you can see that it is removing fields from the form, which the docs indicate cannot be done.

If someone confirms this is indeed an error, I'll create a PR.

@xabbuh xabbuh added the Form label Dec 9, 2015
@ogizanagi
Copy link
Contributor

Even if it is actually possible, I doubt it is useful, because it won't change data at this point. The removal of a field at this point for instance won't influence the data you'll get from your form. However, if a validation error happens, the form will be rendered without the removed field. Which would be weird, in most cases.

I guess the ResizeFormListener matches this exact particular usage: As this event is planned for, it mainly modifies normalized data. The field removal here is not much important IMO: it only removes empty fields in the collection so that they won't appear back in the form in case of a validation error for instance.

So, I'm not sure it should be fixed. I think it won't help discerning which event is the most appropriated from another.

@xabbuh
Copy link
Member

xabbuh commented Dec 9, 2015

I tend to agree with @ogizanagi here. Even if it is technically possible to do that here without getting an error message, removing the warning may cause people to get confused and end up with hard to debug errors.

@bkosborne
Copy link
Contributor Author

The jarring and confusing part is that immediately after the docs say you cannot remove fields to the form, it provides an example where fields are removed.

@xabbuh xabbuh added good first issue Ideal for your first contribution! (some Symfony experience may be required) actionable Clear and specific issues ready for anyone to take them. labels Dec 9, 2015
@ogizanagi
Copy link
Contributor

@bkosborne: You're absolutely right ! I misinterpreted your intentions here, sorry.

We should probably not mention the field removal, and only talk about the modified normalized data instead, or even find another & simpler example like the FixUrlProtocolListener for instance, in order to avoid any confusion.

@xabbuh
Copy link
Member

xabbuh commented Dec 9, 2015

You are right about this. I suggest that we replace the existing example with the FixUrlProtocolListener which ensure that the value entered in a url type always has a protocol.

@bkosborne
Copy link
Contributor Author

PR opened.

@xabbuh xabbuh added the hasPR A Pull Request has already been submitted for this issue. label Dec 10, 2015
wouterj added a commit that referenced this issue Dec 12, 2015
This PR was merged into the 2.3 branch.

Discussion
----------

Clarify example for SUBMIT form event

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.3+
| Fixed tickets | #5990

It's indicated in the docs that form field cannot be added/removed by listeners of the SUBMIT form event, but the example provided was removing fields. While it's possible to do so, it's not recommended since it has little effect. Replaced with a different example implementation.

Commits
-------

4794492 Clarify example for SUBMIT form event
@xabbuh xabbuh closed this as completed Dec 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actionable Clear and specific issues ready for anyone to take them. Form good first issue Ideal for your first contribution! (some Symfony experience may be required) hasPR A Pull Request has already been submitted for this issue.
Projects
None yet
Development

No branches or pull requests

3 participants