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

[LiveComponent] Handle loose comparison with empty placeholder #2426

Merged

Conversation

FoxCie
Copy link

@FoxCie FoxCie commented Dec 5, 2024

Fix for a bug that arises when using an empty placeholder in a required select field, that is wrongly treated as an absence of placeholder by LiveComponents.

Q A
Bug fix? yes
New feature? no
Issues Fix #2425
License MIT

LiveComponents test placeholders of required selects to detect when to set a default value from the choices (i.e. if the select is required and does not have a placeholder, among other conditions) by just casting the placeholder to a boolean. This leads to empty placeholders (placeholders that are the empty string) to be treated as an absence of placeholders when they should not. This MR fixes this by replacing the condition on placeholders with a more appropriate one.
According to the docs https://symfony.com/doc/current/reference/forms/types/choice.html#placeholder and https://symfony.com/doc/current/reference/forms/types/choice.html#field-variables, the placeholder can be a boolean (false indicates that there should not be a placeholder), a string (the text to display for the empty value) or a TranslatableMessage (same as string, but goes through a translator before being displayed). The vars property is either the value of the field, or null if there is no placeholder specified. From this, I considered that there is no placeholder either if the placeholder holds false or null.

I added a simple test that checks the behaviour when there is a required select with an empty placeholder ; we expect the value of the field to be empty in such cases when we do not modify it.

#SymfonyHackday :)

@carsonbot carsonbot added Bug Bug Fix Status: Needs Review Needs to be reviewed labels Dec 5, 2024
Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

Nice catch @FoxCie, and thanks for the PR :)

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Dec 5, 2024
@Kocal Kocal changed the title Fix for #2425 Bug with empty placeholders [LiveComponent] Fix for #2425 Bug with empty placeholders Dec 7, 2024
@Kocal Kocal changed the title [LiveComponent] Fix for #2425 Bug with empty placeholders [LiveComponent] Handle loose comparaison with empty placeholder Dec 7, 2024
@Kocal Kocal changed the title [LiveComponent] Handle loose comparaison with empty placeholder [LiveComponent] Handle loose comparison with empty placeholder Dec 7, 2024
@Kocal Kocal force-pushed the issue2425_empty_placeholder_required_select branch from c72be99 to 3965302 Compare December 7, 2024 10:13
@Kocal
Copy link
Member

Kocal commented Dec 7, 2024

Thank you @FoxCie.

@Kocal Kocal merged commit 6008eed into symfony:2.x Dec 7, 2024
6 checks passed
@FoxCie
Copy link
Author

FoxCie commented Dec 9, 2024

Thank you for the quick merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug Fix LiveComponent Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LiveComponents] Bug with required select with empty placeholders
3 participants