Skip to content

[Form] use a choice label normalizer instead of violating ChoiceListFactoryInterface::createView #32955

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

Closed
wants to merge 1 commit into from
Closed

[Form] use a choice label normalizer instead of violating ChoiceListFactoryInterface::createView #32955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ private static function addChoiceView($choice, $value, $label, $keys, &$index, $
if (null === $label) {
// If the labels are null, use the original choice key by default
$label = (string) $key;
} elseif (false !== $label) {
} else {
// If "choice_label" is set to false and "expanded" is true, the value false
// should be passed on to the "label" option of the checkboxes/radio buttons
$dynamicLabel = \call_user_func($label, $choice, $key, $value);
Expand Down
11 changes: 11 additions & 0 deletions src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,16 @@ public function configureOptions(OptionsResolver $resolver)
return $choiceTranslationDomain;
};

$choiceLabelNormalizer = function (Options $options, $choiceLabel) {
if (false === $choiceLabel) {
return function () {
Copy link
Member

@yceruto yceruto Aug 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I afraid we can't do this here, because you'll break things for people relying on the false value, e.g. false === $options['choice_label'].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that counts otherwise we would not be able to add or change any form option. But I'm fine to merge this PR to 4.4 only.

Copy link
Member

@yceruto yceruto Aug 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

we have (since 4.2) the ->setDeprecated() method to deprecate any form option before removing it or changing its value.

Tecnically this would look like a BC break to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you propose?

Copy link
Member

@yceruto yceruto Aug 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I guess deprecating to pass false to the createView method and make the normalization to callable before calling it, I only found one occurence: ChoiceType::createChoiceListView(), but likely must be done for all current implementations of ChoiceListFactoryInterface. Also that should be targeted to 4.4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make the normalization to callable before calling it

Then there is no point in adding a deprecation as false will never be passed.

Copy link
Member

Choose a reason for hiding this comment

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

The point is, it's a public method, hence we can't add the callable type-hint to the $label argument without adding first a deprecation warning about not passing false.

Make the normalization to callable before calling to createView is the intended operation to get rid of the deprecation warning.

Copy link
Member

@yceruto yceruto Aug 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We're talking about this change https://github.com/symfony/symfony/pull/32237/files#diff-ca1123eff9e0aa207aaec667aa16c8e8R48 right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It will be nice to have ?callable $label for consistency with other arguments.
I agree with @yceruto — we should deprecate false. I can help with the deprecation if you wish.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to take over

return false;
};
}

return $choiceLabel;
};

$resolver->setDefaults([
'multiple' => false,
'expanded' => false,
Expand Down Expand Up @@ -339,6 +349,7 @@ public function configureOptions(OptionsResolver $resolver)
$resolver->setNormalizer('placeholder', $placeholderNormalizer);
$resolver->setNormalizer('choice_translation_domain', $choiceTranslationDomainNormalizer);
$resolver->setNormalizer('choices_as_values', $choicesAsValuesNormalizer);
$resolver->setNormalizer('choice_label', $choiceLabelNormalizer);

$resolver->setAllowedTypes('choices', ['null', 'array', '\Traversable']);
$resolver->setAllowedTypes('choice_translation_domain', ['null', 'bool', 'string']);
Expand Down