Skip to content

Conversation

martineiber
Copy link
Contributor

Changes in this pull request

Resolves pimcore/studio-backend-bundle#1409

Additional info

@martineiber martineiber added this to the 2.3.0 milestone Oct 2, 2025
@martineiber martineiber marked this pull request as ready for review October 2, 2025 13:49
@martineiber
Copy link
Contributor Author

@lukmzig Do you know if there is a reason why we only allow string on multiselect?

@martineiber martineiber requested a review from lukmzig October 2, 2025 13:50
@lukmzig
Copy link
Contributor

lukmzig commented Oct 2, 2025

@lukmzig Do you know if there is a reason why we only allow string on multiselect?

hmmm I think it was just to have a better controller over that modifier itself. I we allow any values, then we also do not need https://github.com/pimcore/generic-data-index-bundle/blob/2.x/src/Model/Search/Modifier/Filter/FieldType/BooleanMultiSelectFilter.php right?
But then should we consider this as a bug?

Another option is to introduce NumberMultiselectFilter and allow integer and floats? Maybe we should discuss the approach here

@martineiber
Copy link
Contributor Author

martineiber commented Oct 3, 2025

@lukmzig
I’d go with that solution and validate against the allowed types (string, int, float):
https://github.com/pimcore/generic-data-index-bundle/blob/2.x/src/Model/Search/Modifier/Filter/FieldType/BooleanMultiSelectFilter.php#L46

This should be considered as a bug? I would say it is an Improvement, since we now allow additional types.

Introducing a NumberMultiSelectFilter would be unnecessary overhead, since the existing MultiSelectFilter already supports numbers. Otherwise, we’d also have to distinguish in Studio whether a value is a string or a number.

WDYT?

@lukmzig
Copy link
Contributor

lukmzig commented Oct 3, 2025

@lukmzig I’d go with that solution and validate against the allowed types (string, int, float): https://github.com/pimcore/generic-data-index-bundle/blob/2.x/src/Model/Search/Modifier/Filter/FieldType/BooleanMultiSelectFilter.php#L46

Alright, sounds good :) Could you add the validation for the types?

Copy link

sonarqubecloud bot commented Oct 3, 2025

@martineiber martineiber requested review from lukmzig and removed request for lukmzig October 3, 2025 07:27
Copy link
Contributor

@lukmzig lukmzig left a comment

Choose a reason for hiding this comment

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

LGTM :)

@martineiber martineiber merged commit 141bdc8 into 2.x Oct 3, 2025
10 checks passed
@martineiber martineiber deleted the 1409-not-only-allow-string-multiselect branch October 3, 2025 07:58
@github-actions github-actions bot locked and limited conversation to collaborators Oct 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Grid][Filter] - create filter that allows numbers in the array (specific case: "User")

2 participants