-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] Added LazyChoiceList #3218
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
Conversation
* choice indices as keys on the lowest levels and the choice | ||
* group names in the keys of the higher levels. | ||
*/ | ||
public function getPreferredViews() |
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.
please use {@inheritdoc} for all ChoiceListInterface methods
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.
AFAIK {@inheritdoc} doesn't work with the API viewer. @fabpot ?
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.
its already used in many places
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.
There is a bug in the API viewer that I will fix soon. So, you can use {@inheritdoc} if you want.
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.
fixed
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.
The {@inheritdoc} support has been fixed in the API generator.
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.
You rock!
👍 |
Not sure if it's a bug in this PR or in #3156, but the labels get replaced by their keys: <?php
use Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceList;
use Symfony\Component\Form\Extension\Core\ChoiceList\LazyChoiceList;
use Symfony\Component\Form\Extension\Core\ChoiceList\SimpleChoiceList;
class MyChoiceList extends LazyChoiceList {
protected function loadChoiceList() {
$choices = array(
'bla' => 'blaaaaaahhhh',
);
return new SimpleChoiceList($choices, array(), ChoiceList::COPY_CHOICE, ChoiceList::COPY_CHOICE);
}
public function getChoices() {
$choices = parent::getChoices();
// $choices is array('bla' => 'bla')
return $choices;
}
} If it's not this PR, I can of course open a new ticket for that. But I'm only working with |
@craue the |
@stof: That would make |
The bug even occurs when using
|
well, the SimpleChoiceList is for simple cases (thus its naming) where you want the same for the label and the values. And if you look at the class, you will see it extends the ChoiceList implementation which is the generic one. |
For me, "simple" would mean that it just takes the array given, using keys as indices and values as labels. No fancy stuff messing around with anything. :D But, is there anything wrong in this code or in mine? @bschussek: Please enlighten me. |
You are both wrong :) The difference between simple and complex choice lists is, that simple choice lists can only contain scalar values as choices, while other choice lists (such as ObjectChoiceList, EntityChoiceList) may contain objects as choices. Choice labels are now stored in ChoiceView objects, which are returned by the various |
It's pretty annoying having provided an array with keys and values when initializing the |
@craue: What's your use case? |
I just used choice lists extensively, even for not directly form-related stuff. In one case, I'm using two of them (which are also used individually) to build up a third one. That went well using the old |
Anyway, this PR for |
@bschussek: Is it ready to be merged? |
Yes |
Commits ------- 57cc531 [Form] Improved PHPDocs of choice lists 9e7e2af [Form] Fixed PHPDoc: Used {@inheritdoc} where applicable 2c530cc Fixed typos in UPGRADE file 7899bea Added examples to UPGRADE d346ae6 Improved choice list sections of UPGRADE and CHANGELOG a676598 [Form] Added class LazyChoiceList Discussion ---------- [Form] Added LazyChoiceList Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: - Todo: -  Adds a ChoiceList implementation that satisfies people who formerly extended ArrayChoiceList and loaded choices lazily in its `load` method. --------------------------------------------------------------------------- by craue at 2012-01-30T12:56:49Z :+1: --------------------------------------------------------------------------- by craue at 2012-01-30T14:55:38Z Not sure if it's a bug in this PR or in #3156, but the labels get replaced by their keys: ```php <?php use Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceList; use Symfony\Component\Form\Extension\Core\ChoiceList\LazyChoiceList; use Symfony\Component\Form\Extension\Core\ChoiceList\SimpleChoiceList; class MyChoiceList extends LazyChoiceList { protected function loadChoiceList() { $choices = array( 'bla' => 'blaaaaaahhhh', ); return new SimpleChoiceList($choices, array(), ChoiceList::COPY_CHOICE, ChoiceList::COPY_CHOICE); } public function getChoices() { $choices = parent::getChoices(); // $choices is array('bla' => 'bla') return $choices; } } ``` If it's not this PR, I can of course open a new ticket for that. But I'm only working with `LazyChoiceList`s. --------------------------------------------------------------------------- by stof at 2012-01-30T16:07:41Z @craue the ``SimpleChoiceList`` is an implementation using the same string as label and value. If you need different ones, you need to use the ``ChoiceList`` implementation. --------------------------------------------------------------------------- by craue at 2012-01-30T16:22:06Z @stof: That would make `SimpleChoiceList` useless for almost any case. Are you sure? --------------------------------------------------------------------------- by craue at 2012-01-30T16:33:31Z The bug even occurs when using ``` return new ChoiceList(array_keys($choices), array_values($choices), array(), ChoiceList::COPY_CHOICE, ChoiceList::COPY_CHOICE); ``` --------------------------------------------------------------------------- by stof at 2012-01-30T16:40:19Z well, the SimpleChoiceList is for simple cases (thus its naming) where you want the same for the label and the values. And if you look at the class, you will see it extends the ChoiceList implementation which is the generic one. --------------------------------------------------------------------------- by craue at 2012-01-30T16:53:36Z For me, "simple" would mean that it just takes the array given, using keys as indices and values as labels. No fancy stuff messing around with anything. :D But, is there anything wrong in this code or in mine? @bschussek: Please enlighten me. --------------------------------------------------------------------------- by bschussek at 2012-01-30T17:16:58Z You are both wrong :) `getChoices` does not return the choices with their associated labels anymore. What you get now are the choice indices as array keys and the choice values as array values. How both are determined depends on the index and value generation strategy, which, in your case, are both COPY_CHOICE. The difference between simple and complex choice lists is, that simple choice lists can only contain scalar values as choices, while other choice lists (such as ObjectChoiceList, EntityChoiceList) may contain objects as choices. Choice labels are now stored in ChoiceView objects, which are returned by the various `get*Views` methods. --------------------------------------------------------------------------- by craue at 2012-01-30T18:07:43Z It's pretty annoying having provided an array with keys and values when initializing the `ChoiceList` but being unable to retrieve it again. Guess I just over-used or even abused those choice lists as kind of (not only form related) lookup tables. --------------------------------------------------------------------------- by bschussek at 2012-01-30T19:27:21Z @craue: What's your use case? --------------------------------------------------------------------------- by craue at 2012-01-30T20:10:16Z I just used choice lists extensively, even for not directly form-related stuff. In one case, I'm using two of them (which are also used individually) to build up a third one. That went well using the old `ArrayChoiceList`s and their `getChoices` method. Just thinking about creating another set of model classes which just contain my lists. So for only one select field in a form it'll take three classes then: (A) a list, (B) a choice list based on A, (C) a choice form type based on B. Oh well ... :D --------------------------------------------------------------------------- by craue at 2012-01-30T21:31:32Z Anyway, this PR for `LazyChoiceList` is great, so please merge it. ;) --------------------------------------------------------------------------- by craue at 2012-01-31T14:00:46Z @bschussek: Is it ready to be merged? --------------------------------------------------------------------------- by bschussek at 2012-01-31T16:59:17Z Yes
Since the modifications on the choice list implementation it is not possible to have dynamic multiple choice lists. My use case: User types something into input field and presses "add". This adds the value to a multiple select prepared by the jquery chosen plugin. When submitted, I want to iterate over all these values and process them. This worked before, now it sais, that the value is not valid. I tried to go through the code step by step, but there doesn't seem to be a parameter to allow this, like "allow_add". I also tried to experiment with Form event listeners, with no success.. I think I'm just doing it wrong. Can someone please point me to the right direction how to accomplish this_? |
Never mind .. after spending about 5 hours on this, debugging like hell with a buggy xdebug, I found out that a combination of |
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Adds a ChoiceList implementation that satisfies people who formerly extended ArrayChoiceList and loaded choices lazily in its
load
method.