Skip to content

[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

Merged
merged 6 commits into from
Jan 31, 2012
Merged

[Form] Added LazyChoiceList #3218

merged 6 commits into from
Jan 31, 2012

Conversation

webmozart
Copy link
Contributor

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -

Travis Build Status

Adds a ChoiceList implementation that satisfies people who formerly extended ArrayChoiceList and loaded choices lazily in its load method.

* choice indices as keys on the lowest levels and the choice
* group names in the keys of the higher levels.
*/
public function getPreferredViews()
Copy link
Contributor

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

Copy link
Contributor Author

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 ?

Copy link
Contributor

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

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You rock!

@craue
Copy link
Contributor

craue commented Jan 30, 2012

👍

@craue
Copy link
Contributor

craue commented Jan 30, 2012

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 LazyChoiceLists.

@stof
Copy link
Member

stof commented Jan 30, 2012

@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.

@craue
Copy link
Contributor

craue commented Jan 30, 2012

@stof: That would make SimpleChoiceList useless for almost any case. Are you sure?

@craue
Copy link
Contributor

craue commented Jan 30, 2012

The bug even occurs when using

return new ChoiceList(array_keys($choices), array_values($choices), array(), ChoiceList::COPY_CHOICE, ChoiceList::COPY_CHOICE);

@stof
Copy link
Member

stof commented Jan 30, 2012

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.

@craue
Copy link
Contributor

craue commented Jan 30, 2012

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.

@webmozart
Copy link
Contributor Author

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.

@craue
Copy link
Contributor

craue commented Jan 30, 2012

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.

@webmozart
Copy link
Contributor Author

@craue: What's your use case?

@craue
Copy link
Contributor

craue commented Jan 30, 2012

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 ArrayChoiceLists 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

@craue
Copy link
Contributor

craue commented Jan 30, 2012

Anyway, this PR for LazyChoiceList is great, so please merge it. ;)

@craue
Copy link
Contributor

craue commented Jan 31, 2012

@bschussek: Is it ready to be merged?

@webmozart
Copy link
Contributor Author

Yes

fabpot added a commit that referenced this pull request Jan 31, 2012
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: -

![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3156)

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
@fabpot fabpot merged commit 57cc531 into symfony:master Jan 31, 2012
@chmielot
Copy link

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_?

@chmielot
Copy link

Never mind .. after spending about 5 hours on this, debugging like hell with a buggy xdebug, I found out that a combination of
the PRE_BIND Listener and 'value_strategy' => ChoiceList::COPY_CHOICE, actually works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants