Skip to content

[Form] Deprecated choice_attr as nested arrays mapped by indexes #36272

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Form] Deprecated choice_attr as nested arrays mapped by indexes #36272

wants to merge 1 commit into from

Conversation

HeahDude
Copy link
Contributor

@HeahDude HeahDude commented Mar 30, 2020 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? yes
Tickets Part of #18318
License MIT
Doc PR TODO

Extracted from #16834, which I'll split in 3 to ease the review/merge process, hopefully in time for 5.1.
This PR deprecates using the choice_attr option as array of arrays of attributes per choice index, in favor of a new (more intuitive) support for a global array of attributes for all choices, or already implemented callable usage to define attributes per choice (or index or value).

Before:

// Single array for all choices using callable
'choice_attr' => function ($choice, $index, $value) {
    return ['class' => 'choice-options'];
},

// Different arrays per choice using array
'choices' => [
    'Yes' => true,
    'No' => false,
    'Maybe' => null,
],
'choice_attr' => [
    'Yes' => ['class' => 'option-green'],
    'No' => ['class' => 'option-red'],
],

After:

// Single array for all choices using array
'choice_attr' => ['class' => 'choice-options'],

// Different arrays per choice using callable
'choices' => [
    'Yes' => true,
    'No' => false,
    'Maybe' => null,
],
'choice_attr' => function ($choice, $index, $value) {
    if ('Yes' === $index) {
        return ['class' => 'option-green'];
    }
    if ('No' === $index) {
        return ['class' => 'option-red'];
    }

    return [];
},

There has been many issues in the past on the docs and Stack Overflow (ref #symfony/symfony-docs#6443), because the choice indexes are not always known, and with both using a callable to define global attributes, current ways are error prone. In consequence, this will also simplify the documentation of the option.

This PR also have a side effect on a potential new choice_label_attr (branch pending), because its behavior (and doc) should be the same.

@HeahDude HeahDude requested a review from xabbuh as a code owner March 30, 2020 17:42
@HeahDude HeahDude force-pushed the form-deprecate-choice_attr-multi_array branch 6 times, most recently from 93e3467 to f05f575 Compare March 30, 2020 18:04
@HeahDude HeahDude force-pushed the form-deprecate-choice_attr-multi_array branch from f05f575 to 42c36ca Compare March 30, 2020 18:27
@nicolas-grekas nicolas-grekas added this to the next milestone Mar 31, 2020
@nicolas-grekas
Copy link
Member

By "unique array", would you mean "flat array" maybe? Otherwise, I don't get what it means.

Then: why?

What's the issue with the current way? Why do we need to deprecate it?

@HeahDude
Copy link
Contributor Author

@nicolas-grekas, thanks for the review! I've updated the description and changed unique to global, is this ok for you?

@nicolas-grekas
Copy link
Member

I'm far from being an expert on the component, but the before/after doesn't look like an improvement to me.

A declarative structure ("before") looks actually better than any imperative one ("after"). The closure can contain logic bugs. The array cannot.

An idea: reject keys that are in choice_attr but not in choices. We could do this with a deprecation first if needed.

Another idea: allow passing a list of ChoiceView objects to the choices option (and its corollary: allow a choice_loader to do the same). This would fix this strange definition where the same item (a single choice) has to be described in different options.

Overall, I'm not convinced this is really an improvement, thus I'm not convinced it's worth the cost of a deprecation.

But again, I don't know all this very well.

@HeahDude HeahDude mentioned this pull request Apr 8, 2020
Open
@xabbuh
Copy link
Member

xabbuh commented Apr 25, 2020

I agree with Nicolas. If we want to improve the experience for the case where you want to set the attributes to be the same for all choices, I think introducing a new option for that would be more appropriate than having to use closures for the other case.

@fabpot
Copy link
Member

fabpot commented Aug 17, 2020

Any interest in finishing this PR based on the feedback?

@fabpot
Copy link
Member

fabpot commented Sep 13, 2020

Closing as there is no more activity.

@fabpot fabpot closed this Sep 13, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Deprecation Feature Form Status: Needs Review
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
5 participants
@HeahDude @nicolas-grekas @xabbuh @fabpot @carsonbot