-
-
Notifications
You must be signed in to change notification settings - Fork 341
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[LiveComponent] fix required select not initialized #2403
Conversation
dfbcd61
to
6a24416
Compare
If someone knows why ux.symfony.com tests failed... it's gives me headache.. Nothing was changed in the ux.symfony.com directory and in local the tests are ok. |
ok great, thank you ! I will take a look on it. Thanks again |
You did not run the php bin/link-locally i suppose ? I let you look at the suggestion, i found the source of the problem. I let you check locally and tell me if that seems ok for you ? |
continue; | ||
} | ||
|
||
if (\array_key_exists('choices', $child->vars) | ||
&& $child->vars['required'] | ||
&& !$child->vars['multiple'] | ||
&& !$child->vars['expanded'] | ||
&& null === ($values[$name] ?? null) | ||
) { | ||
if (0 === \count($choices = $child->vars['choices'])) { | ||
throw new UnprocessableEntityHttpException(\sprintf('The required field "%s" is missing and has no default value', $name)); | ||
} | ||
|
||
$values[$name] = reset($choices)->value; | ||
} else { |
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.
continue; | |
} | |
if (\array_key_exists('choices', $child->vars) | |
&& $child->vars['required'] | |
&& !$child->vars['multiple'] | |
&& !$child->vars['expanded'] | |
&& null === ($values[$name] ?? null) | |
) { | |
if (0 === \count($choices = $child->vars['choices'])) { | |
throw new UnprocessableEntityHttpException(\sprintf('The required field "%s" is missing and has no default value', $name)); | |
} | |
$values[$name] = reset($choices)->value; | |
} else { | |
continue; | |
} | |
// Special handling: required <select> fields with no value, no | |
// placeholder and no multiple attribute should have their value set | |
// to the first choice, if it exists. | |
if ( | |
\array_key_exists('choices', $child->vars) | |
&& $child->vars['required'] | |
&& !$child->vars['disabled'] | |
&& !$child->vars['value'] | |
&& !$child->vars['placeholder'] | |
&& !$child->vars['multiple'] | |
&& !$child->vars['expanded'] | |
&& $child->vars['choices'] | |
) { | |
if (null !== $firstKey = array_key_first($child->vars['choices'])) { | |
$values[$name] = $child->vars['choices'][$firstKey]->value ?? null; | |
continue; | |
} | |
} | |
There were some pre-condition checks missing. This one seems more robust, i'll let you check.
'required' => false, | ||
]) | ||
->add('choice_required', ChoiceType::class, [ | ||
'choices' => [ | ||
'foo' => 1, | ||
'bar' => 2, | ||
], | ||
]) | ||
->add('choice_expanded', ChoiceType::class, [ |
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.
'required' => false, | |
]) | |
->add('choice_required', ChoiceType::class, [ | |
'choices' => [ | |
'foo' => 1, | |
'bar' => 2, | |
], | |
]) | |
->add('choice_expanded', ChoiceType::class, [ | |
'required' => false, | |
]) | |
->add('choice_required_with_placeholder', ChoiceType::class, [ | |
'choices' => [ | |
'bar' => 2, | |
'foo' => 1, | |
], | |
'required' => true, | |
'placeholder' => 'foo', | |
]) | |
->add('choice_required_without_placeholder', ChoiceType::class, [ | |
'choices' => [ | |
'bar' => 2, | |
'foo' => 1, | |
], | |
'required' => true, | |
'placeholder' => false, | |
]) | |
->add('choice_expanded', ChoiceType::class, [ |
I added a test to distinct the two scenarios
'choice_required' => '1', | ||
'choice_expanded' => '', | ||
'choice_multiple' => ['2'], | ||
'select_multiple' => ['2'], | ||
'entity' => (string) $id, |
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.
'choice_required' => '1', | |
'choice_expanded' => '', | |
'choice_multiple' => ['2'], | |
'select_multiple' => ['2'], | |
'entity' => (string) $id, | |
'choice_required_with_placeholder' => '', | |
'choice_required_without_placeholder' => '2', | |
'choice_expanded' => '', | |
'choice_multiple' => ['2'], | |
'select_multiple' => ['2'], | |
'entity' => (string) $id, |
@smnandre indeed I missed the link-locally, thanks for your help. Suggestions was good, all is ok now. Many thanks ! |
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.
Thank you very much @dsoriano!
891bf54
to
9d1dada
Compare
Thank you Denis and Simon :) |
…ues()` with edge cases (smnandre) This PR was squashed before being merged into the 2.x branch. Discussion ---------- [LiveComponent] Fix `ComponentWithFormTrait::extractFormValues()` with edge cases | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Issues | Fix #2487 | License | MIT #2403 fixed an old bug to simulate browser behaviour when a select field has no option selected, no placeholder, and is required (it then uses the first option) #2425 and #2426 fixed the way we handled empty strings as placeholders In #2487 `@maciazek` explained us how a custom type with a "choices" option became unusable since the last changes. Also.. while I was reading all this I realized we returned wrong values here when "option groups" were used.. leading to other problems. I updated the code of `extractFormValues()` method to: * check if most of the caracteristic keys added in `ChoiceType::buildView` were defined in the `$view->vars`, to ensure we are dealing with a `<select>` field built by a `ChoiceType` * fetch recursively the value of the first displayed option (even with groups) Commits ------- b7f1ba4 [LiveComponent] Fix `ComponentWithFormTrait::extractFormValues()` with edge cases
Initialize required select(ChoiceType or EntityType) with the first entry. Not applied to multiple or expanded.