Improvement: check validity for (Multi)Select before setting values#17742
Improvement: check validity for (Multi)Select before setting values#17742kingjia90 merged 19 commits intopimcore:12.xfrom
Conversation
When using the Pimcore API, any value can be set on a select or multiselect field. This improvement checks the validity before setting the data. It applies to configured options, SelectOptions and OptionProviders. Resolves: pimcore#17343
Review Checklist
|
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
In case no list of options is defined (yet), don't validate the new input value. This avoids large volume of ValidationErrors when for example a SelectOption or OptionsProvider service gets missing
- use actual values instead of numbers for the MultiSelect fields (as the Country and Language (single) Select already do) - fix the faulty assert for LanguageMultiSelect (which was using the CountryMultiSelect assert)
As the return type is nullable, this should be indicated by the doc block as well.
As the options were already checked for being empty, the array will never need a fallback at this point
Co-authored-by: Sebastian Blank <sebastian.bl@gmx.de>
Co-authored-by: Sebastian Blank <sebastian.bl@gmx.de>
|
There was a problem hiding this comment.
My concern from a bigger picture is that (feels like) the former behavior was kind of intended, at least for existing data:
- Given A,B,C, if one picks A,B,C and save.
- Temporary remove C from available options.
- Try to save object again without any changes
Before PR, C would "disappear" from the UI list, but if one restores the option with the same name, then it would be there again as selected (because the value matches). Overall no any blocks from saving or usual editing.
After PR, C is not removable from UI perspective and the object is stuck forever (can't save, as it doesn't clear the invalid option and keeps throwing exception), until someone overwrite the set value via API.
Maybe we need to refactor to something more limited to API, of ex-novo objects (on setter with empty existing selection) and/or may need some more insights from @fashxp @brusch first
|
@kingjia90 @samynw What do you guys think about making the behavior configurable? WDYT? |
Thank you @brusch for having a look and the feedback, to me it sounds good, a sort of strict mode that also takes care of the redundant/orphanized data (and remove the not visible data and in-existing options), configurable at a single data type field level |
|
@samynw 🏓 Could you please have a look the latest feedbacks and apply the changes? Thank you in advance |
|
Any updates? Please let me know if i should take over and apply the requested changes of making it configurable, TIA |
|
@kingjia90 Thank you for your feedback. You make a valid point and your example topic makes sense. I agree with you, and the suggestion of @brusch to make it configurable is definitely the best approach. However, I'm currently limited in availability to apply the suggested changes, so @kingjia90 feel free to take over if you are still up for it, that would be great! |
|
Tests looks working fine |
|
Changes in this pull request
As described in issue #17343, when using the Pimcore API, any value can be set on a select or multiselect field. This improvement checks the validity before setting the data by matching the value against the allowed options.
If the given data is not allowed, a ValidationException will be thrown.
Additional info
The validity check supports:
Resolves: #17343