Skip to content

Update groups.rst #11223

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

Closed
wants to merge 1 commit into from
Closed

Update groups.rst #11223

wants to merge 1 commit into from

Conversation

ThomasLandauer
Copy link
Contributor

Integrating the findings of EasyCorp/EasyAdminBundle#2673 (comment)

Please fix the link to [guess your form types] - I don't know how to link to an anchor in the docs!

Integrating the findings of EasyCorp/EasyAdminBundle#2673 (comment)

Please fix the link to [guess your form types] - I don't know how to link to an anchor in the docs!
@javiereguiluz
Copy link
Member

Thanks! Personally I find this article a bit complex ... and we keep adding more and more complexity to it ... but maybe it's the way it should be because the underlying is confusing/complex/full of edge cases.

@ThomasLandauer
Copy link
Contributor Author

Well, I've seen many Symfony pages that are worse than this one ;-) It took me hours to figure the details out yesterday - just wanted to share that.

And whenever anything doesn't work like one could expect, it's always better to explain it...

@wouterj
Copy link
Member

wouterj commented Apr 7, 2019

First of all: Thank you for your pull request! It's definitely appreciated that you put time into finding out internal logic and make it available to the rest of the community through documentation!

However, I can also see Javier's point. Can't we reduce the note to "Validation groups are ignored while guessing form types and options"? That would be a short warning and will give enough information I think to understand the "weird" behaviour. What do you think?

@wouterj wouterj added actionable Clear and specific issues ready for anyone to take them. ⭐️ EU-FOSSA Hackathon https://symfony.com/blog/the-symfony-and-api-platform-hackathon-is-coming and removed actionable Clear and specific issues ready for anyone to take them. labels Apr 7, 2019
@xabbuh
Copy link
Member

xabbuh commented Apr 7, 2019

I agree with @wouterj.

@ThomasLandauer
Copy link
Contributor Author

First of all: #11222 is by far more important! This one here is only a side product.

Sorry guys, I can't see why you're always so hesitant to include more details. Do you really think that it's easier for anybody to figure it out themselves?? Compared to spending another minute to read it?

"Validation groups are ignored while guessing form types and options"

"ignored" is ambiguous here: Will it apply all constraints (i.e. ignore the groups setting), or will it ignore the constraints that are in a group? Besides, it only covers my first list point, not the second:

If you don't let Symfony guess your form types, no constraints will be considered at all in the form's view.

@wouterj
Copy link
Member

wouterj commented Oct 3, 2020

Yes, less verbose docs is better. Nobody likes to (or will actually) read very verbose documentation. People will skip sentences, meaning they'll almost certainly miss the important bit. If you only type the important information and leave out any verbose details that aren't needed, you "force" the reader to read the important bits.

Anyway, I'm going to close this PR as there hasn't been any activity for over a year. Besides, I just discovered that this information is already inside the documentation (for the past 9 years) on the form type guessing section, which also makes more sense to me: 7a82a89

@wouterj wouterj closed this Oct 3, 2020
@ThomasLandauer ThomasLandauer deleted the patch-7 branch October 3, 2020 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⭐️ EU-FOSSA Hackathon https://symfony.com/blog/the-symfony-and-api-platform-hackathon-is-coming Status: Needs Review Validator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants