Skip to content

[Form] [DX] choice_attr and choice_label_attr options in ChoiceType #18318

Open
@HeahDude

Description

@HeahDude

Previous works and discussions

The famous #14050.

But also #17019, symfony/symfony-docs#6144, symfony/symfony-docs#6393 and #16834.

Current definition

Since #14050, choice_value, choice_label, choice_name, choice_attr, preferred_choices and group_by options are dynamics.

Here's the current allowed types defined in ChoiceType:

choice_value => null, callable, string (or PropertyPath)
choice_name  => null, callable, string (or PropertyPath)

choice_label => null, callable, string (or PropertyPath), bool (false to discard label)
choice_attr  => null, callable, string (or PropertyPath), array

preferred_choices  => callable, string (or PropertyPath), array or \Traversable
group_by     => null, callable, string (or PropertyPath), (use nested choices by default)

The current signatures of the callables are the same for all: function ($choice, $key, $value)
Excepted for choice_value: function ($choice).

Now considering the needed returned types from the callables or property paths:

choice_label      => string
choice_value      => string (unique AND not null or falls back on i++)
choice_name       => string (unique AND valid html "id" or falls back on i++)

choice_attr       => array

preferred_choices => bool (preferred)
group_by          => string (group name) | null (no group)

Problem

  1. It's actually impossible to add a choice_label_attr option in the core following this model without breaking BC (ref discussion in [Form] added "choice_label_attr" option and deprecated "choice_attr" as multi-arrays or property path #16834 and 616fc8a for the currently "consistent" implementation).
    Worse, it can't be easily implemented in user land.
    For example, in one of my first project with symfony last year (2.7 first betas), I found how to "hack" choice_attr (see here).
    (When I updated this project last week on 3.0, I figured that it can be done in a BC way but without support for property path, which lead me to:)

  2. Of courses it makes much sense to use property paths to return string or bool, especially when they are relevant properties of a choice object:

    $builder->add('tags', ChoiceType::class, array(
        'choices' => $tags,
        'choice_value'      => 'id',            // (string) $tag->getId()
        'choice_label'      => 'displayName',   // (string) $tag->getDisplayName()
        'choice_name'       => 'name',          // (string) $tag->getName()
        'group_by'          => 'category.name', // (string) $tag->getCategory()->getName()
        'preferred_choices' => 'favorite',      // (bool)   $tag->isFavorite()
    
        'choice_attr' => ? // (array) $tag->getHtmlAttributes() ??
    );

    That's very great for DX, whereas this might be irrelevant for choice_attr IMHO, unless using special classes for html form options (is there a use case ?).

  3. Using choice_attr as an array may lead to confusion (ref [Form] choice_attr option in ChoiceType generates options attributes wrong #17019), currently even the doc is wrong about how to use it (see here).
    Indeed, when using an array, one has to pass nested arrays mapped by their key to the corresponding choice key:

    $builder->add('tags', ChoiceType::class, array(
        'choices' => array(
            'Choice 1' => 1,
            'Choice 2' => 2,
        ),
        'choice_attr' => array(
            'Choice 1' => array('class' => 'class_choice_one'),
            'Choice 2' => array('class' => 'class_choice_two'),
        ),
    );

    Although:

    • In 2.x versions if choices_as_values is false one must rely on the numerical index instead.
    • Worse, when choices_as_values is true (even in 3.x), if a choice key is not a valid html "id" attribute (e.g 'Tom & Jerry'), keys fall back on an incrementing integer, so the attributes arrays are not mapped anymore.
    • It may be impossible to know the initial choice keys when choices are loaded.

    Even worse (from a DX perspective), if one wants to use the same attributes for all the choices, he would expect to use a single array and that must be why the doc currently says in a total contradiction:

    This can be an array of attributes (if they are the same for each choice)

    Which is wrong, instead, it must be:

    'choice_attr' => function () { return array('class' => 'choice_option'); }

    That's exactly what I needed to do in a form with 5 choice fields needing the same choice_attr option as this example. I could not keep it in a variable as some fields need some extra classes.
    (Btw, this is the same form I use in which I need choice_label_attr with this exact same behavior and "hacking" this option allowed me to divide by four the lengh of my template, by preventing me to loop over the fields, test variables etc.
    One thing I've learned, the more accurate is your FormType the nearest you are to just write form(form) in your template without needing to override any block.
    Hum, hum... "Bref":)

    • If one need a "static" array (same for all), it must be wrapped in a callable without using any argument.
    • If one need it "dynamic", he needs to be sure about what keys to set and that they won't change in the future... Hence he'd better use a callable instead.

    IMHO we should expect the same behavior as entry_options of CollectionType here, one array for all.

Solution

  1. We can deprecate property path for choice_attr (point 2)
  2. We can deprecate using nested arrays for choice_attr (point 3)
  3. Then we could introduce a choice_label_attr with the same "new" behavior without breaking BC while keeping it all constistent (point 1)
  4. Eventually move the logic of choice_label_attr resolution to DefaultChoiceListFactory in 3.4 when we can change the interface for more consistency.

So we get:

choice_value => null, callable, string (or PropertyPath)
choice_name  => null, callable, string (or PropertyPath)
choice_label => null, callable, string (or PropertyPath), bool (false to discard label)

preferred_choices  => callable, string (or PropertyPath), array or \Traversable
group_by     => null, callable, string (or PropertyPath),

choice_attr       => array, callable
choice_label_attr => array, callable

I've totally refactored and updated #16834 to fix this issue accordingly.
(See 616fc8a for the previous implementation breaking BC).

Metadata

Metadata

Assignees

No one assigned

    Labels

    DXDX = Developer eXperience (anything that improves the experience of using Symfony)FeatureFormKeep open

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions