fix(manager): keep settings grid usable after delete - #17002
Open
Ibochkarev wants to merge 5 commits into
Open
Conversation
Cloning saveParams on remove prevents the deleted key/action from leaking into later UpdateFromGrid calls. Settings grids also skip the extra store.remove after refreshFilterOptions already reloads.
smg6511
requested changes
Aug 18, 2026
smg6511
left a comment
Collaborator
There was a problem hiding this comment.
My testing verifies that the cloning of the saveParams fixes the issue. The other changes weren't really needed from my observations, although I did suggest an update to the afterRemoveRow method, as the original implementation didn't make sense.
Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
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.
What changed and why
After you delete a context setting from the grid, the next inline edit or delete often returned “Setting not found”.
MODx.grid.Grid.remove()wroteactionand the deletedkeystraight onto sharedconfig.saveParams. Later autosave (saveRecord→UpdateFromGrid) merged those polluted params into another row and sent the wrong key.remove()now clonessaveParamsbefore mutating. Settings grids also returnfalsefromafterRemoveRowafterrefreshFilterOptions()(which already reloads the store), soremoveActiveRowdoes not race with a secondstore.remove(selection).How to test
web→ Context Settings.test_a,test_b,test_c) with any values.test_afrom the grid menu.test_bvalue, change it, blur/save — should succeed (no “Setting not found”).test_c— should succeed without a page reload.Browser smoke on revolution.test: after remove,
config.saveParamsstays{context_key:"web"}; UpdateFromGrid for the next row returns success.Related issue(s)/PR(s)
Resolves #14280
Compatibility notes
Manager ExtJS grids that use
config.saveParamswithremove()(context / system / user / usergroup settings). No PHP or connector API change.Breaking change assessment
No public API change. Behavior fix only: remove no longer mutates the shared
saveParamsobject.Test coverage
No ExtJS unit harness in-repo. Verified with
node --checkand a manager smoke that deletes one context setting then UpdateFromGrid on another.Contributors
@GulomovCreative reported the original bug.
AI tool use
Cursor (Composer) traced the saveParams leak, applied the clone + afterRemoveRow veto, and drafted this PR body. Browser checks on revolution.test confirmed the polluted params before the fix and clean autosave after.