Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Fixes #1279
Problem
A backend form field rendered as
switchorcheckboxcould have its stored value silently overwritten when saving:1in the database).triggerAPI, theattributesoption, or any third-party JavaScript (el.disabled = true).Result: the field saves as
0even though it was checked and disabled - nobody toggled it off.Root cause
The switch/checkbox field partials render a hidden "unchecked" fallback input (
<input type="hidden" name="X" value="0">) sharing the name of the visible control - the standard Rails-style checkbox trick to make "toggling off" submittable.Per WHATWG HTML §4.10.22.4 ("Constructing the entry list"), the browser discards a disabled control from the submitted entry list - but the visible control being disabled does nothing to the hidden fallback, which stayed enabled and kept contributing
value="0"with the same name. The server then processed that0as the field's submitted value (last value wins), overwriting the stored state.FormField::filterAttributes()renders configdisabledandreadOnlyattributes only at thefieldposition (the visible control), and the runtime trigger API appliesprop('disabled')on the control only - the fallback never knew.What this PR does
Two coordinated fixes:
A - render inheritance (PHP): the hidden fallback in
modules/backend/widgets/form/partials/_field_switch.phpand_field_checkbox.phpnow inherits the disabled state of the visible control when it is known at render time (previewMode, configdisabled, or adisabledHTML attribute rendered via theattributesoption), so static scenarios no longer leak the fallback into the entry list.B - runtime deletion (JS): the backend form widget (
winter.form.js) now listens for theformdataevent on its form and, whenever the entry list is constructed, deletes any entry belonging to a field whose visible checkbox is currently disabled - covering the trigger API, third-party JS, and any future runtime source. The event fires regardless of how serialization happens: form submission and theFormDataconstructor used by both the Snowboard request API (Request.js) and the framework request API.formdatais Baseline (widely available) since September 2021; the listener only registers where'FormDataEvent' in window- older browsers skip it and behave exactly as before, and the render-time fix (A) still protects the static cases in those browsers.Field semantics unchanged (by design)
disabled= the value is not submitted and not saved; the stored value is left as-is. This now holds for runtime disables too - that is the fix.readOnly= the field cannot be edited, but its value is still submitted and saved. This stays as before:readonlyis a UI-level promise, not a server-side trust rule - the server must still authorize and validate what it saves (as it does today for every field).0) - that is the hidden fallback's whole purpose; the listener only removes entries of disabled fields.Testing
postForm()pattern inFormFieldSetSaveTest- a config-disabledswitch field is omitted fromgetSaveData()even when a value arrives, and an enabled switch's fallback0is still saved.disabled+ save): before, the request payload containedField=0and the DB flipped to0; after, the field is entirely absent from the payload and the stored value is preserved. Same verified through the core trigger API path (data-trigger action="disable"), and non-regression confirmed for enabled-checked (0,1entry list, last wins), enabled-unchecked (bare0, saves off) andreadOnly(value still saved) fields.wip/1.3tip, no new failures - the two pre-existing PHPUnit test-runner warnings upstream are unchanged).