Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
[Form] fixed EntityType choice options#6444
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Form] fixed EntityType choice options#6444
Uh oh!
There was an error while loading.Please reload this page.
Conversation
reference/forms/types/entity.rst Outdated
| 2.7, it was called ``property`` (which has the same functionality). | ||
| **type**: ``string``or``callable`` | ||
| **type**: ``string``, ``callable``or:class:``Symfony\\Component\\PropertyAccess\\PropertyPath`` |
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.
Minor syntax issue:
:class:``Symfony\\Component\\PropertyAccess\\PropertyPath``should be:
:class:`Symfony\\Component\\PropertyAccess\\PropertyPath`HeahDude commentedApr 8, 2016
Thanks@javiereguiluz for those feedbacks, I'll work on it :) |
f7d73a7 tocd6fe11CompareHeahDude commentedApr 9, 2016
Comments addressed. Needs comments, thanks! |
cd6fe11 tobf13394Comparereference/forms/types/entity.rst Outdated
| **type**: ``string``, ``callable`` or:class:`Symfony\\Component\\PropertyAccess\\PropertyPath` **default**: id | ||
| By default the name of each field is the id of the entity, if it can be get in |
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.
Reading this paragraph I wonder: what will be the default value if the "id reader" can't read the id of the object?
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.
@javiereguiluz The loader just doesn't use it, and falls back on the default process (casting objects to string or using incremental integer). Do you think we should add this ?
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.
"[...] if it can be get in the class metadata [...]" => "[...] if it can be read from the class metadata [...]" ?
bf13394 to1d058a5CompareHeahDude commentedMay 1, 2016
Updated! |
reference/forms/types/entity.rst Outdated
| By default the name of each field is the id of the entity, if it can be read | ||
| from the class metadata by an internal id reader. Otherwise the process will | ||
| fall back on an incremental integer. |
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.
[...] fall back to using increasing integers?
HeahDude commentedMay 4, 2016
Thanks@xabbuh for the review, I addressed your comments in the last pushed commit. |
xabbuh commentedMay 4, 2016
👍 |
xabbuh commentedMay 19, 2016
javiereguiluz commentedMay 19, 2016
I'm sorry but I can't vote on this because I don't understand the Form component well enough. |
c389bc9 to302afe5CompareHeahDude commentedMay 21, 2016
Squashed, should be ready to be merged. Thanks! |
This PR was merged into the 2.7 branch.Discussion----------[Form] fixed EntityType choice options| Q | A ||-----------|-------|| branch | 2.7+ || doc fix | yesWe should point these overridden options from the `ChoiceType`, so users keep the benefit of the optimization made in the `DoctrineChoiceLoader`.Commits-------302afe5 [Form] fixed EntityType choice options
weaverryan commentedMay 22, 2016
This PR was merged into the 2.7 branch.Discussion----------Removing some extra details from#6444See#6444 - I wanted to accomplish 2 things:1) Ideally remove needing to duplicate the option - i.e. *try* to re-use the existing `.inc` file, as long as things remain clear. Also, the original `.inc` actually explain what the point of the option is.2) Removing some extra low-level details that I'm not sure are understandable.The only thing I wasn't sure about was related to `choice_values`. If I'm using an API, and I want the user to be able to submit some string (e.g. the `username` for a User instead of the id), is this possible by setting this to `username`? Or will that mess up how the entities are queried?Commits-------2336e88 Trying to remove some duplication and some extra details
We should point these overridden options from the
ChoiceType, so users keep the benefit of the optimization made in theDoctrineChoiceLoader.