Skip to content

Handle options fields in groups missing from $_POST#811

Merged
dlh01 merged 1 commit intomainfrom
fix/GH-685/multioptions
Nov 11, 2021
Merged

Handle options fields in groups missing from $_POST#811
dlh01 merged 1 commit intomainfrom
fix/GH-685/multioptions

Conversation

@dlh01
Copy link
Copy Markdown
Member

@dlh01 dlh01 commented Nov 7, 2021

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 $data that only contained a few of the fields in a group.

  • Check whether the field exists in $data with array_key_exists() rather than isset(). 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 $data defaults to something new. Arguably, repeatable single fields could also default to array(), 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 $_POST could be interpreted as an empty set, a radio input is missing from $_POST means 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.

@dlh01 dlh01 requested review from mboynes and mslinnea November 7, 2021 22:19
Copy link
Copy Markdown
Contributor

@mogmarsh mogmarsh left a comment

Choose a reason for hiding this comment

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

👍 looks good. happy to have my bug resolved, although the need is long gone

Copy link
Copy Markdown
Member

@mslinnea mslinnea left a comment

Choose a reason for hiding this comment

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

Looks good and solid reasoning 👍 🐴

@discoinfiltrator
Copy link
Copy Markdown

Looks great 👍

@dlh01 dlh01 merged commit 1af82d1 into main Nov 11, 2021
@dlh01 dlh01 deleted the fix/GH-685/multioptions branch November 11, 2021 06:01
@dlh01 dlh01 added this to the 1.4.0 milestone Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't uncheck all checkboxes if serialize_data set to false Cannot save empty multi-select when serialize_data is false

4 participants