Handle options fields in groups missing from $_POST#811
Merged
Conversation
mogmarsh
approved these changes
Nov 8, 2021
Contributor
mogmarsh
left a comment
There was a problem hiding this comment.
👍 looks good. happy to have my bug resolved, although the need is long gone
mslinnea
approved these changes
Nov 9, 2021
Member
mslinnea
left a comment
There was a problem hiding this comment.
Looks good and solid reasoning 👍 🐴
|
Looks great 👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refines #686, the original Pull Request addressing the original report in #685, and expands it to address the same issue with checkboxes as reported in #764.
The differences between this and #686:
Apply the fix only when
false === $serialize_data, since both of the reports specify that the bug applies to those fields. Also, otherwise, we could inadvertently save a value for$datathat only contained a few of the fields in a group.Check whether the field exists in
$datawitharray_key_exists()rather thanisset(). The idea is that this expands backwards compatibility by contracting the number of cases when the new fallback value will be added to$data.Define
$data = array()only when the field is a group, again to expand backwards compatibility by contracting the number of cases when$datadefaults to something new. Arguably, repeatable single fields could also default toarray(), but they don't currently, and it doesn't seem necessary to change that to fix this issue.Don't provide a fallback for
Fieldmanager_Radios. First, I would think the fallback should be scalar, not an array, since radios don't save multiple values. However, whereas checkboxes or multi-selects missing from$_POSTcould be interpreted as an empty set, a radio input is missing from$_POSTmeans that not one of the options was selected — wouldn't it be incorrect to supply a default?''might well have been one of the available values that wasn't selected by the user. And since Fieldmanager doesn't include a way to un-select a radio option, it seems like the bug in Bugfix for saving empty multi selects or radio fields. #686 would apply only before the user makes a selection and that that period of time should be respected.Don't extend the new behavior to subclasses, which could already supply their own workarounds or alternate behavior.
Closes #685.
Closes #764.