Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Form] label path in ObjectChoiceList can also be callable#3479
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
I don't see any reason to add this. there is already a |
IMHO this doesn't belong to the entity class |
why not ? it is about getting some display name. Adding a getter in the entity makes this reusable |
i could imagine the case where the label needs to be rendered using a service, e.g.: $builder->add('product','entity',array(// ... //'label_builder' =>function($entity)use ($priceManager) {returnsprintf('%s (%.2f)',$entity->getName(),$priceManager->calculateDiscountPrice($entity)); }))); This is definitely not something that the entity should handle. |
pvanliefland commentedMar 8, 2012
Just asked myself the same question today... I would like to display a dropdown list of hierarchical entity data, something like Europe
To do that, I could add a method getChoiceText() in my entity : publicfunctiongetChoiceText() {returnltrim(str_repeat('-',$this->level) .'' .$this->name);} It works, but it feels a bit odd to add purely "form/presentation" logic in my entity. This method will only be used in my form. The callback idea makes sense, although there's no doubt we can live without. Wouldn't it be possible to use the 'property' option to achieve what@mvrhov is suggesting ? This option could accept either a string, to be used by PropertyPath, or a closure that would be provided with an entity ? |
Would be nice to have something like this. Enhancing the @stof: I'd need to call the method of a service to build each label, which is not possible currently. |
+1 |
+1, it's not going to be used a lot, but still a useful feature |
Nice. I'd love to have this. |
| @@ -64,6 +64,9 @@ class EntityChoiceList extends ObjectChoiceList | |||
| */ | |||
| private $loaded = false; | |||
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.
extra empty line here
This should probably be implemented in the ObjectChoiceList instead of the EntityChoiceList |
Added test, moved the implementation to base class (ObjectChoiceList) as suggested by@stof. and extended $labelPath parameter as suggested by@pvanliefland. I'd really like to see some comments about changed phpdoc from someone more fluent in English. |
| $labels[$i] = $this->labelPath->getValue($choice); | ||
| } elseif (is_callable($this->labelPath)) { | ||
| $labels[$i] = $callable($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 is wrong. It won't work for callable likearray($object, $method) on PHP 5.3
@bschussek is it ok for you? |
| * is used instead. | ||
| * by calling the getter on the object. You can | ||
| * also pass any callable. When callable will be | ||
| * executed it will get the choice parameter passed. |
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 say :
Alternatively, $labelPath can be a PHP callable
taking an element from the ObjectChoiceList as
parameter and returning a string.
But then $labelPath should be documented as a mixed parameter ?
@fabpot I'm not yet satisfied with this PR. I think this problem should be solved on a more general scope than just ObjectChoiceList, but I need to give this some more thought. |
Closing this PR in favor of#4067 |
ping @bschussek |
@bschussek ok, thanks! |
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
This PR adds a callback support as additional option to label path in ObjectChoiceList class.
Right now you either have to set a property path to a property or a getter or implement __toString method in the objects you pass as choices to get the choice label. Usually this is ok, but sometimes you need to have different labels depending on where the choice filed is displayed, or __toString is used elsewhere for different purposes.
Usage example: