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] FixChoiceType options#6393
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
HeahDude commentedMar 23, 2016
| Q | A |
|---|---|
| Doc fix? | yes |
| New docs? | no |
| Applies to | all |
| Fixed tickets | #6144 |
| 'choices_as_values' => true, | ||
| 'data' => true, // pre selected choice | ||
| 'empty_data' => '1', // default submitted value | ||
| )); |
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.
Should we detail that whenmultiple it should be:
'data' =>array(true),'empty_data' =>array('1'),
??
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 clearly needs to explained.
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.
Man, this note basically doesn't make any sense to me :). I don't think it's your fault - I just find this choice stuff really hard. What exactly is the problem that we're trying to warn the user about? The only issue I can understand is thatempty_data (if you want to set that) would need to equal1 to simulatetrue (I think, because apparently these values will render as 0, 1, 2 because they can't be cast into strings?).
I'm normally under the impression that the user will rarely care about the HTML value attribute that is rendered/submitted - as long as if I select "Yes", I end up withtrue in my code.
6da211f tob49bb5aCompareThis PR was merged into the 2.7 branch.Discussion----------[Form] remove useless copy in ChoiceType| Q | A| ------------- | ---| Branch? | 2.7+| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR |symfony/symfony-docs#6393`ChoiceListFactoryInterface` expected `$groupBy` to be a callable or null, not an array ([ref](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/Factory/ChoiceListFactoryInterface.php#L111)).The factory defaults `groupBy` to `ChoiceListInterface::getStructuredValues()` ([ref](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php#L122)).Hence, the copy of the choices array and the recursive flip are useless and may be slowing down performances imho (especially with `EntityType`).Commits-------562f5e4 [Form] remove useless code in ChoiceType
This PR was merged into the 2.7 branch.Discussion----------[Form] remove useless copy in ChoiceType| Q | A| ------------- | ---| Branch? | 2.7+| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR |symfony/symfony-docs#6393`ChoiceListFactoryInterface` expected `$groupBy` to be a callable or null, not an array ([ref](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/Factory/ChoiceListFactoryInterface.php#L111)).The factory defaults `groupBy` to `ChoiceListInterface::getStructuredValues()` ([ref](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php#L122)).Hence, the copy of the choices array and the recursive flip are useless and may be slowing down performances imho (especially with `EntityType`).Commits-------562f5e4 [Form] remove useless code in ChoiceType
HeahDude commentedApr 25, 2016
This one needs reviews :) |
reference/forms/types/choice.rst Outdated
| ..note:: | ||
| Pre selected choices will depend on the **data** passed to field and the values of |
alexislefebvreMay 10, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
@HeahDude Should it bePreselected?
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.
Je ne sais pas :) I would natively say "pré-selectionné" with a dash.
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.
In case of doubt, our official reference is the "American English Oxford Dictionary":preselected is correct.
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.
Thank you very much@javiereguiluz! Reference added in my favorites :)
b49bb5a to5430cecCompareHeahDude commentedJun 25, 2016
Comments addressed. Status: Needs Review |
| Finally, if your values are objects, you can also specify a property path string | ||
| on the object that will return true or false. | ||
| See the `choice_label`_ for details about using a property path or a callable. |
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 reference doesn't work.
| Also, the:class:``Symfony\\Component\\Form\\ChoiceList\\Factory\\ChoiceListFactoryInterface`` will cache the choice list | ||
| so the same:class:``Symfony\\Component\\Form\\ChoiceList\\Loader\\ChoiceLoaderInterface`` can be used in different fields with more performance | ||
| (reducing N queries to 1). |
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: a few of these lines got real long :)
HeahDude commentedJul 10, 2016
@weaverryan thanks for the review! Status: Needs Work |
| Pre selected choices will depend on the **data** passed to the field and | ||
| the values of the ``choices`` option. However submitted choices will depend | ||
| on the **string** matching the **choice**. In the example above, the default | ||
| values are incrementing integers because ``null`` cannot be casted to string. |
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.
becausenull is not a scalar value.
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.
In fact, I think it's a bug ;) we can show"", "1", "0" values forarray(null, true, false) choices and be more consistent.
Trying to fix that insymfony/symfony#21160
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.
As said in your PR you should close it, this is why this needs a better doc :)
| 'No' => false, | ||
| ), | ||
| 'choices_as_values' => true, | ||
| 'data' => true, // pre selected 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.
This should be a bad practice. The reader could interpret that is a way to set default value and it isn't. As you know, if an object is bound to the form, thedata option overrides this default value.
I suggest you to use$this->createFormBuilder(array('isAttending' => true)) from a controller for this sample. WDYT?
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.
weaverryan commentedOct 6, 2017
Ping@HeahDude! Could you go through my review and finish this? Thx! |
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
javiereguiluz commentedSep 23, 2019
It's sad but this pull request is no longer mergeable. Things have changed too much and there are too many conflicts. We can open a new pull request in the future and get some inspiration from this one. I'm sorry for having let this pull request stale and thanks anyway for the contribution. |