[3.x] Form handler refactor to allow more flexible and accurate status reporting in the manager UI - #17023
Draft
smg6511 wants to merge 11 commits into
Draft
[3.x] Form handler refactor to allow more flexible and accurate status reporting in the manager UI#17023smg6511 wants to merge 11 commits into
smg6511 wants to merge 11 commits into
Conversation
Revert to unchanged file
Remove remaining test logging and add more inline documentation
Remove errant space
Remove stray log
Add utils as a testsuite
Ensure plain text msg source is stripped and make dialog box consistent between html-formatted and plain message boxes.
smg6511
marked this pull request as ready for review
September 10, 2026 04:36
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Status routing currently causes compatibility regressions, incomplete package handling, and state leakage across processor responses.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Refactors manager form handling to support typed status dialogs, optional sanitized HTML, and improved package update reporting.
Changes:
- Adds configurable status responses and dialog routing.
- Introduces string sanitization/conversion services.
- Reports “package up to date” as informational and adds sanitizer tests.
File summaries
| File | Description |
|---|---|
manager/assets/modext/workspace/package.grid.js |
Handles empty update results. |
manager/assets/modext/core/modx.js |
Adds status-dialog routing and formatting. |
core/src/Revolution/Utilities/Sanitizers/modStringSanitizer.php |
Adds HTML sanitization. |
core/src/Revolution/Utilities/Converters/modStringConverter.php |
Adds HTML-to-JSON conversion. |
core/src/Revolution/Processors/Workspace/Packages/CheckForUpdates.php |
Returns informational up-to-date status. |
core/src/Revolution/Processors/Processor.php |
Adds status response API. |
core/src/Revolution/modX.php |
Registers utilities and sanitization configuration. |
core/src/Revolution/modConnectorResponse.php |
Exposes message configuration. |
core/src/Revolution/Error/modError.php |
Stores message configuration in responses. |
core/lexicon/en/workspace.inc.php |
Adds package status title. |
core/lexicon/en/default.inc.php |
Adds dialog-related messages. |
_build/test/Tests/Utilities/modStringSanitizerTest.php |
Tests HTML sanitization. |
_build/test/phpunit.xml |
Registers utility tests. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 11
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+73
to
+74
| $this->stringSanitizers = $this->modx->services->get(modStringSanitizer::class); | ||
| $this->stringConverters = $this->modx->services->get(modStringConverter::class); |
| 'object' => $objarray, | ||
| ]; | ||
| 'object' => $objarray | ||
| ], $this->messageConfig); |
Comment on lines
+34
to
+35
| /** Status message type: Success */ | ||
| public const STATUS_TYPE_SUCCESS = 'success'; |
Comment on lines
+413
to
+414
| case !empty($windowTitle): | ||
| $options['messageConfig']['messageWindowTitle'] = $windowTitle; |
Comment on lines
+98
to
+102
| return $this->status( | ||
| $msg, | ||
| $this->modx->lexicon('package_status'), | ||
| Processor::STATUS_TYPE_INFO | ||
| ); |
| and info messages will typically be sent via a success | ||
| rather than failure response. | ||
| */ | ||
| if (r.message || !Ext.isEmpty(r.messageConfig)) { |
Comment on lines
+978
to
+982
| const data = {}; | ||
| data.title = Object.hasOwn(response.messageConfig, 'messageWindowTitle') | ||
| ? Ext.util.Format.stripTags(response.messageConfig.messageWindowTitle) | ||
| : _('error') | ||
| ; |
| * @param string $string The unencoded html string | ||
| * @return string The JSON-encoded string | ||
| */ | ||
| public function htmlToJSON(string $string): string |
smg6511
marked this pull request as draft
September 11, 2026 14:43
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
Rewires the
MODx.form.Handler(js) methods and associated backend methods to:MODx.form.Handler.showFormattedMessagemethod, which is opt-in and does not affect any of the current success/failure messaging unless explicitly changed in the back end logic by using the newProcessor::statusmethod [e.g., instead of$this->failure(...), one would use$this->status(...) with themessageIsFormattedparam set to true]; andIn support of the HTML dialog option, two new php utilities were added as services (
modStringSanitizerandmodStringConverter). Note that:modXbase class, theProcessorclass, and probably elsewhere that can be more appropriately located in these utility classes.There are many instances where a failure is not a failure per se, but we've only had an unrealistic binary success or failure path for reporting the state of a process in the UI. That, along with the inability to format messages when needed was the motivation for this change.
How to test
This update touches many areas, so I advise a general run through of creating objects and purposely causing failures. One easy way is via any object emits a dialog on validation errors (Resource, Element, etc.).
A specific case in included in this PR to see how the change is applied and to verify it works. See the changes in the final commit of this PR for reference (Packages
CheckForUpdates).** To see the effects of the new behavior under different scenarios, make changes in
CheckForUpdates~L97-98+; play with the message itself and the $this->status (new) vs $this->failure (existing) statement. An example of what I used to test:For checking support of html in message, and full stripping of it when not in formatted mode:
Related issue(s)/PR(s)
Although not specifically created for this, the included verification case (
CheckForUpdates) illustrates this PR's way of handling what's addressed in #17000.Compatibility notes
n/a
Breaking change assessment
No BC anticipated, as current behavior is preserved and handler methods marked as deprecated pass through to new methods at least until 3.4 when removal is suggested.
Test coverage
New UT for
stripHTMLmethod located in Tests/Utilities/Sanitizers.Contributors
n/a
AI tool use
n/a