Use option labels instead of values in summary tokens#101
Conversation
Toflar
left a comment
Toflar
left a comment
There was a problem hiding this comment.
Thanks for trying to fix this issue. I agree that we should try to improve the token values out-of-the box. However, unfortunately your logic falls short with for example missing support for option_callbacks. Can you try to use the Haste Formatter for that? https://github.com/codefog/contao-haste/blob/main/docs/Formatter.md
| return $formFieldValue; | ||
| } | ||
|
|
||
| $field = FormFieldModel::findBy( |
There was a problem hiding this comment.
This should be grouped into one query for all fields instead of potentially hundreds of queries for big forms. Should be able to use findPublishedByPid() maybe?
| } | ||
|
|
||
| $fft = FormFieldModel::getTable(); | ||
| $palettesWithOpts = preg_grep('/([,;]|^)options([,;]|$)/', array_filter($GLOBALS['TL_DCA'][$fft]['palettes'], 'is_string')); |
There was a problem hiding this comment.
This won't work for widgets not using the options field. I don't see how this guard would make sense, maybe you can elaborate?
| $formFieldOptLabels = \is_array($formFieldValue) | ||
| ? implode(', ', array_intersect_key($optsAssoc, array_flip($formFieldValue))) | ||
| : $optsAssoc[$formFieldValue]; | ||
| $formFieldOptLabels = System::getContainer()->get('contao.insert_tag.parser')->replace($formFieldOptLabels); |
There was a problem hiding this comment.
If insert tags are supported we should support them in the haste formatter as well - that would be the place to add that then.
|
Thank you for the very quick feedback!
Can you elaborate on that? How does Parts of your review might to be downstream from a non Form Generator viewpoint as well, so I'd like to understand this first before trying to work on that. |
|
I thought the whole point of your PR is to show the option labels instead of the option raw values in a selection? Which should work in general. For all widgets. |
|
No, this PR has nothing to do with the options that are rendered in the actual form field widgets (input/select) themselves. There's no problem there that I know of. This PR is referring to the summary tokens in the Placeholder widget. |
|
Yes, and the summary token should take the values from the form field options? (which can be provided via options_callback in case you did not specify the options yourself)? |
You cannot provide form field options via the |
|
@fritzmg Thank you, I didn't know what else to say other than, "uh, misunderstanding of unclear provenance?!" 😅 @Toflar You might be thinking of modifying the value of the |
|
(awkwardly) modifying the options via the hook shouldn't make a difference 👍 |
3 participants
At the moment, the summary tokens for any field type that uses the
optionsfield (natively: select, radio, checkbox) display the values of those options, not the labels. E. g., if you have a checkbox field with name "accessibility", label "Barrierefreit", and the following options:and you select those options in the form, then the output in the summary will be something like:
This is technically consistent with other fields, because what is shown is always the field value, but I can't think of a use case in which someone would actually want the output to be like this for options fields. It's quite the opposite: The question of how one can display the option labels instead has been asked many times for many years. The user can technically work around the problem field by field, but it cannot be solved when the token
##mp_forms_summary##is used.This PR makes it so that, for fields that use options, the option labels are assembled and used as the token values.
I was initially concerned about multiple form fields that have the same name, but this is actually irrelevant because the Core only outputs the last one of these fields anyway. So all we have to do is make sure we fetch the last field as well so that the option labels match those shown in the form.
I'm also a little concerned about having placed the new
getOptionLabels()method in the Widget rather than somewhere else. It's really a much more general method than just something for this context, but an obvious better place within this bundle didn't stand out to me.