[Bug]: Fix BC-Break: Default value on multiselect object type introduced by #15871 #16779
Conversation
Review Checklist
|
|
@NiklasBr Thank you for the prompt report of the bc-break related to this topic, could you please have a look and try if this PR could also solve it? |
Co-authored-by: Sebastian Blank <sebastian.bl@gmx.de>
Co-authored-by: Sebastian Blank <sebastian.bl@gmx.de>
| if (!$defaultValue) { | ||
| $defaultValue = null; | ||
| }elseif (is_string($defaultValue)){ | ||
| $defaultValue = [$defaultValue]; | ||
| } |
There was a problem hiding this comment.
Maybe this? When it is null, it must not be changed, Empty Array should also no problem or?
| if (!$defaultValue) { | |
| $defaultValue = null; | |
| }elseif (is_string($defaultValue)){ | |
| $defaultValue = [$defaultValue]; | |
| } | |
| if (is_string($defaultValue)) { | |
| $defaultValue = $defaultValue !== '' ? [$defaultValue] : null; | |
| } |
There was a problem hiding this comment.
My current project has null and "" and somestring values which broke, I would suspect that perhaps some integers could have made it's way into the default value storage as well. Maybe check for scalar values would make it more forgiving?
And for v12.0 create a migration which forces array type for all while making the type stricter again?
There was a problem hiding this comment.
Where did these defaultValues came from?
Was the field type a single select in the past?
So after the convert in the backend the default value from the single select still exists?
If this is the case, I think we could not make it type strict. This can also possible in v12. We should than also check the case when someone convert the multi select in a single select.
There was a problem hiding this comment.
They were created using v6 or v10, some could possibly have been selects or other field types at one point as well.
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: Sebastian Blank <sebastian.bl@gmx.de>
|
@kingjia90 I think we need no if there. The trait is only used on this two classes. So we can remove it. I created a PR to check this: |
* Fix SelectionProviderTrait * Change unneeded getOptions() call by string
|
|
Should be ready for tagging a release, despite i am still having issues when using it with multiselect within classification stores, but opened a new issue |
Changes in this pull request
Resolves #16777
Additional info