Skip to content

[6.x] Preserve hidden collection settings on update - #15446

Closed
wakqasahmed wants to merge 1 commit into
statamic:6.xfrom
wakqasahmed:fix/issue-15430-collection-revisions-overwrite
Closed

wakqasahmed wants to merge 1 commit into
statamic:6.xfrom
wakqasahmed:fix/issue-15430-collection-revisions-overwrite

Conversation

@wakqasahmed

Copy link
Copy Markdown
Contributor

Closes #15430

Saving a collection's config always applied revisionsEnabled($values['revisions'] ?? false), so if revisions isn't in the submitted request it gets forced to false. The revisions field is only shown on the config screen when STATAMIC_REVISIONS_ENABLED is true, so on an instance/environment where that's false, saving a collection's config silently wipes out revisions: true in its YAML even though nothing related was touched.

Fixed by only calling revisionsEnabled() when revisions is 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.

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 wakqasahmed left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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'] ?? falsefalse → 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.

@jasonvarga jasonvarga changed the title Preserve hidden collection settings on update (#15430) Preserve hidden collection settings on update Sep 11, 2026
@duncanmcclean duncanmcclean changed the title Preserve hidden collection settings on update [6.x] Preserve hidden collection settings on update Sep 11, 2026
@jasonvarga
jasonvarga enabled auto-merge (squash) September 11, 2026 19:48
@jasonvarga
jasonvarga disabled auto-merge September 11, 2026 19:48
@jasonvarga

Copy link
Copy Markdown
Member

Duplicate of #15431

@jasonvarga jasonvarga marked this as a duplicate of #15431 Sep 11, 2026
@jasonvarga jasonvarga closed this Sep 11, 2026
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.

Collection Config Overwrites Revisions Setting if They're Turned Off

2 participants