Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
clarify the signature of the choice_value callable#6297
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
backbone87 commentedFeb 22, 2016
| Q | A |
|---|---|
| Doc fix? | yes |
| New docs? | no |
| Applies to | 2.7+ |
| Fixed tickets |
| about this, but it might be handy when processing an API request (since you can | ||
| configure the value that will be sent in the API request). | ||
| This can be a callable or a property path. See `choice_label`_ for similar usage. |
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.
Maybe it's better to remove"Seechoice_label_ for similar usage. completely. This would avoid talking about "differences" with the other options.
HeahDude commentedFeb 22, 2016
👍 to be more explicit with this option. |
backbone87 commentedFeb 22, 2016
HeahDude commentedFeb 22, 2016
Alright then ;) But at least, to quickly avoid errors, I hardly recommend to remove that line as I said in inline comment. |
| *If you use a callable for this option, it gets passed the choice as its **only** | ||
| argument*. This is a difference compared to the callables of `choice_label`_ and | ||
| `choice_name`_, which receive additional arguments. If ``null`` is used, an | ||
| incrementing integer is used as the name. |
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.
I agree that it would be best to have the section overhauled in general, but I also agree that this is a nice short-term fix. However, I suggest to change this a bit to something like this:
..note:: In contrast to the ``choice_label`` and ``choice_name`` option when using a callable for the ``choice_value`` option, the actual choice is the **only** argument passed to it.
What do you think?
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.
👍
This is a detail but I would say :
..note:: In contrast to other choice options (e.g ``choice_label``), the callable for the ``choice_value`` option takes as **only** argument the actual choice.
Because the actual enumeration misseschoice_attr (and maybe futureother(s) ;).
Anyway please removeSeechoice_label_ for similar usage., as I really think it can confuse new comers.
backbone87 commentedFeb 23, 2016
i have applied your feedback, can be autosquashed for clean history |
| In contrast to other choice options (e.g. `choice_label`_), the callable for | ||
| the ``choice_value`` option takes as **only** argument the actual choice, which | ||
| is also the value that gets passed to your model after submitting the form. |
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.
I would replaceafter bywhen.
Beside that 👍
backbone87 commentedFeb 23, 2016
done |
| In contrast to other choice options (e.g. `choice_label`_), the callable for | ||
| the ``choice_value`` option takes as **only** argument the actual choice, which | ||
| is also the value that gets passed to your model when submitting the form. |
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.
Sorry, but now I'm re-reading it and I think that", which is also the value that gets passed to your model when submitting the form" is too confusing. It better should be defined outside of this note, when clarifying the responsibility of this option to return a unique string representation of the choice.
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.
@backbone87 Can you reword this a bit?
…one87)This PR was squashed before being merged into the 2.7 branch (closes#6297).Discussion----------clarify the signature of the choice_value callable| Q | A || --- | --- || Doc fix? | yes || New docs? | no || Applies to | 2.7+ || Fixed tickets | |Commits-------5cf83f6 clarify the signature of the choice_value callable
weaverryan commentedSep 27, 2017
Thanks! I've just merged this, and tried to clarify further at sha:6365c01 |
This PR was merged into the 3.4 branch.Discussion----------[Form] Fixed some phpdocs| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | ~| License | MIT| Doc PR |symfony/symfony-docs#6393refsymfony/symfony-docs#6144,symfony/symfony-docs#6297,#14050Commits-------b9162e8 [Form] Fixed some phpdocs