[6.x] Preserve hidden collection settings on update - #15446
wakqasahmed wants to merge 1 commit into
Conversation
Only call revisionsEnabled() when 'revisions' is present in the submitted form values. When STATAMIC_REVISIONS_ENABLED is false, the revisions field is hidden from the collection config screen, so its key is absent from the request - but the controller still applied a `?? false` fallback, silently disabling revisions on unrelated saves.
wakqasahmed
left a comment
There was a problem hiding this comment.
Reviewed cold against #15430. Verified each claim by reading editFormBlueprint() and Fields::values(), not just the diff. (Note: posting as a plain review comment since GitHub blocks self-approval on your own PR — this account is the PR author.)
1. Root cause/call site — correct. The revisions field is excluded from the blueprint definition itself in PHP (if (Statamic::pro() && config('statamic.revisions.enabled')), CollectionsController.php ~line 586), not merely hidden client-side via an if visibility rule. Fields::values() (src/Fields/Fields.php:178) only ever emits keys for fields that exist in the processed Fields collection, so when the field is excluded, $values structurally cannot contain a 'revisions' key — this isn't a form bug to work around, it's a genuinely server-driven exclusion, so the controller is the right place for the guard.
2. array_key_exists correctness — sound, and safe against tampering. Because $values is built from the blueprint's own Fields collection (not the raw request array), even a crafted request that replays a revisions key when the field is server-excluded gets dropped before reaching $values. When the field is shown and unchecked, the toggle submits an explicit false that lands in $values with the key present, so it's correctly disabled. No absent-vs-null ambiguity to worry about here.
3. Scope check — no other field shares this exact bug. Of all fields in editFormBlueprint(), only two groups are conditionally excluded from the blueprint at the PHP level (as opposed to just conditionally displayed via if): revisions (this fix) and the whole sites group (gated on Site::multiEnabled()). The sites-group consumers are already defensively guarded (Arr::get($values, 'sites') inside an if ($sites = ...) block covering propagate/origin_behavior too). Fields with client-side if visibility (past_date_behavior, max_depth, etc.) stay in the blueprint always and are already accessed via Arr::get() in this same method. So the PR's "kept scoped to this field" reasoning holds up.
4. Regression test — traced through, it's correct and non-trivial. Without the fix: config off → field excluded from blueprint → $values['revisions'] ?? false → false → persisted, so the reload fails assertTrue. With the fix: array_key_exists is false → call skipped → the route-bound $collection (freshly loaded from disk per RouteServiceProvider::bind('collection', ...), not the test's in-memory variable) keeps its original true and re-saves it. One good catch in the test: Collection::revisionsEnabled()'s getter (src/Entries/Collection.php:692-704) forces false whenever config is off, so the test deliberately flips config back to true before the final assertion — without that it'd fail even with a correct fix.
5. Other notes. Nothing blocking. Pre-existing (not introduced here): ->mount($values['mount'] ?? null) in the main chain is followed later by ->mount(Arr::get($values, 'mount')) — a redundant duplicate call, but it predates this PR and is out of scope.
Solid, well-scoped fix with a test that actually exercises the failure mode.
|
Duplicate of #15431 |
Closes #15430
Saving a collection's config always applied
revisionsEnabled($values['revisions'] ?? false), so ifrevisionsisn't in the submitted request it gets forced tofalse. Therevisionsfield is only shown on the config screen whenSTATAMIC_REVISIONS_ENABLEDistrue, so on an instance/environment where that'sfalse, saving a collection's config silently wipes outrevisions: truein its YAML even though nothing related was touched.Fixed by only calling
revisionsEnabled()whenrevisionsis actually present in the request, so the existing value survives when the field is hidden. Added a regression test that saves a collection with revisions enabled while the config flag (and therefore the field) is disabled, then confirms the setting wasn't overwritten.Kept the fix scoped to this field since other conditionally-hidden collection settings have different defaults/semantics and widening this would need its own look.