Skip to content
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

[Autocomplete] Fix bug of not using choice_value #1328

Closed
wants to merge 2 commits into from

Conversation

ytilotti
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
Issues n/a
License MIT

The option choice_value (like EntityType) is not used in Autocomplete ux on AsEntityAutocompleteField class.

@yceruto
Copy link
Member

yceruto commented Dec 11, 2023

It could be simpler for this autocompleter service if it built the form view (i.e., $view = $this->getForm()->createView()) and get the label and value already processed from $view->vars['choices'], so you wouldn't miss any native features that render those options.

@@ -89,7 +89,7 @@ public function getLabel(object $entity): string
return $this->propertyAccessor->getValue($entity, $choiceLabel);
}

if ($choiceLabel instanceof ChoiceLabel) {
if ($choiceLabel instanceof Cache\ChoiceLabel) {
Copy link
Member

Choose a reason for hiding this comment

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

A particular reason for this change ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To have only one use.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this offers a better readability, but let's see what others think

Copy link
Contributor Author

@ytilotti ytilotti Dec 14, 2023

Choose a reason for hiding this comment

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

I'm not sure this offers a better readability, but let's see what others think

Used in https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/Form/ChoiceList/Factory/CachingFactoryDecorator.php.
I have no personal opinion, whatever ;)

$choiceValue = $this->getFormOption('choice_value');

if (\is_string($choiceValue) || $choiceValue instanceof PropertyPathInterface) {
return $this->propertyAccessor->getValue($entity, $choiceValue);
Copy link
Member

Choose a reason for hiding this comment

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

The return type of the method is not compatible with all the new possibilities no ? Integers, floats, nulls, ...

Copy link
Contributor Author

@ytilotti ytilotti Dec 14, 2023

Choose a reason for hiding this comment

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

Historically, there's already a "problem" when we see the return type of getIdentifierValues():

/**
     * Returns the identifier of this object as an array with field name as key.
     *
     * Has to return an empty array if no identifier isset.
     *
     * @return array<string, mixed>
     */
    public function getIdentifierValues(object $object);

But there is no problem because is always cast to string.

By default, PHP will coerce values of the wrong type into the expected scalar type declaration if possible. For example, a function that is given an int for a parameter that expects a string will get a variable of type string.

@weaverryan
Copy link
Member

It could be simpler for this autocompleter service if it built the form view (i.e., $view = $this->getForm()->createView()) and get the label and value already processed from $view->vars['choices'], so you wouldn't miss any native features that render those options.

That's a really interesting idea. After all, our goal is to return the text and value that is rendered on the frontend. So creating the form view, then extracting from there makes a lot of sense. @ytilotti would you mind giving that a shot?

@ytilotti
Copy link
Contributor Author

ytilotti commented Jan 9, 2024

I don't see @yceruto's idea. Maybe he can suggest it?

@weaverryan
Copy link
Member

Sorry for the delay @ytilotti - it's this idea: #1328 (comment)

So instead of getting the choice_value option from the form, then trying to handle the different types that it may be (which is, I admit, what I've done so far in other spots), what if we did something like this?

$view = $this->getForm()->createView();

return $view->vars['value'];

@kbond
Copy link
Member

kbond commented Apr 15, 2024

Thanks for your work here @ytilotti. This is now fixed via #1723.

@kbond kbond closed this Apr 15, 2024
@ytilotti ytilotti deleted the ytilotti-patch-1 branch April 15, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug Fix Status: Needs Work Additional work is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants