FIX ListboxField discarding mised-type values - CMS 6.1#11972
FIX ListboxField discarding mised-type values - CMS 6.1#11972emteknetnz wants to merge 1 commit intosilverstripe:6.1from
Conversation
emteknetnz
force-pushed
the
pulls/6.1/merge-up
branch
2 times, most recently
from
99284c3 to
9afeebb
Compare
emteknetnz
changed the title
emteknetnz
force-pushed
the
pulls/6.1/merge-up
branch
from
9afeebb to
7f1b28d
Compare
GuySartorelli
left a comment
GuySartorelli
left a comment
There was a problem hiding this comment.
This code is almost identical to the original PR, except it has some new conditions added and a bunch of new test coverage.
I'm guessing the new conditions are related to "Added some extra stuff to for edge cases"? If so, please indicate what the edge cases were that necessitated each new condition - and why they aren't being handled separately from the merge-up.
src/Forms/ListboxField.php
Outdated
| if ($trimmed !== '') { | ||
| $firstChar = substr($trimmed, 0, 1); | ||
| if ($firstChar === '{' || $firstChar === '[') { | ||
| if (strpos($trimmed, '"Value"') !== false || strpos($trimmed, '"value"') !== false) { |
There was a problem hiding this comment.
What is this new condition here for? It's not in the original code on this branch nor in the code being merged up.
There was a problem hiding this comment.
The if (strpos($trimmed, '"Value"') !== false) { condition is still there - it's not in the original code on this branch nor in the code being merged up.
src/Forms/ListboxField.php
Outdated
| if (strpos($trimmed, '"Value"') !== false || strpos($trimmed, '"value"') !== false) { | ||
| $decoded = json_decode($item, true); | ||
| if (is_array($decoded) | ||
| && (array_key_exists('Value', $decoded) || array_key_exists('value', $decoded)) |
There was a problem hiding this comment.
&& array_key_exists('Value', $decoded) is still there
emteknetnz
force-pushed
the
pulls/6.1/merge-up
branch
from
7f1b28d to
da4951f
Compare
GuySartorelli
left a comment
GuySartorelli
left a comment
There was a problem hiding this comment.
Also needs a rebase to resolve conflicts
src/Forms/ListboxField.php
Outdated
| if ($trimmed !== '') { | ||
| $firstChar = substr($trimmed, 0, 1); | ||
| if ($firstChar === '{' || $firstChar === '[') { | ||
| if (strpos($trimmed, '"Value"') !== false || strpos($trimmed, '"value"') !== false) { |
There was a problem hiding this comment.
The if (strpos($trimmed, '"Value"') !== false) { condition is still there - it's not in the original code on this branch nor in the code being merged up.
src/Forms/ListboxField.php
Outdated
| if (strpos($trimmed, '"Value"') !== false || strpos($trimmed, '"value"') !== false) { | ||
| $decoded = json_decode($item, true); | ||
| if (is_array($decoded) | ||
| && (array_key_exists('Value', $decoded) || array_key_exists('value', $decoded)) |
There was a problem hiding this comment.
&& array_key_exists('Value', $decoded) is still there
emteknetnz
force-pushed
the
pulls/6.1/merge-up
branch
from
da4951f to
cd111ec
Compare
emteknetnz
force-pushed
the
pulls/6.1/merge-up
branch
from
cd111ec to
e605991
Compare
|
Rebased |
|
Looks like you've removed all of the actual source code changes? |
|
Funny, it's because I just manually resolved in a merge-up yesterday i.e. the thing I originally raised this PR to avoid doing 😆 5fc3f2d - my changes on this PR mirrored what I did yesterday. I'll just close this PR |
2 participants
Issue #11956
Tricky merge-up, lots of conflict, so redoing solution for CMS 6.1 (different PR from 5.4).
Added some extra stuff to for edge casesGuard ListboxField getValueArray type coercion for mixed-type source values and handle zero scalar inputsTighten ListboxField schema value decoding to only accept Value or value payloads while preserving mixed-type selectionsAdd regression coverage for numeric-string-first choices and JSON schema values in ListboxFieldTestCMS 5.4 PR
After merged assign back to Steve to handle the merge-up