fix(cli): preserve nested params in handler/settings JSON fields#2061
Merged
Conversation
SettingsHandler::sanitizeField() had no `json` case, so json-typed fields
fell through to the default text-sanitization branch. sanitize_text_field()
coerces array inputs to an empty string, which corrupted the SystemTask
`params` field whenever `wp datamachine flow update --handler-config` was
called with a nested-array payload like
`{"task":"dispatch_message","params":{"channel":"x",...}}`.
The corrupt empty string was then stored in flow_step_settings.params,
which FlowStepConfig::getPrimaryHandlerConfig() reads preferentially over
handler_config.params. SystemTaskStep::executeStep() then fataled with
`array_merge(): Argument #1 must be of type array, string given`.
Add a dedicated sanitizeJson() that:
- returns nested arrays verbatim,
- decodes JSON-encoded strings into associative arrays,
- falls back to the schema default (decoded if needed) or an empty
array on invalid input,
- never returns a scalar string.
Closes #2059
Smoke test for the regression fixed in the previous commit. Asserts:
1. SettingsHandler::sanitize() preserves nested-array `params` verbatim
instead of coercing it to an empty string.
2. JSON-string `params` decodes into the same nested-array shape.
3. `params` is never returned as a scalar string across six input
shapes (nested array, JSON string, missing key, empty array,
invalid JSON, empty string), so downstream array_merge() in
SystemTaskStep cannot fatal.
17/17 assertions pass on the fix branch; 0/17 pass without the fix
(documenting the exact pre-fix coercion failure modes).
Run with: php tests/flow-update-handler-config-params-smoke.php
Contributor
Homeboy Results —
|
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.
Summary
wp datamachine flow update <flow_id> --step=<step_id> --handler-config=<json>was corrupting nested-array values in any handler/settings field declared withtype: 'json'(today: the SystemTaskparamsfield). The corruption storedparams == ""(empty string) inflow_step_settings, whichFlowStepConfig::getPrimaryHandlerConfig()reads preferentially overhandler_config, breaking every subsequent flow run witharray_merge(): Argument #1 must be of type array, string givenatSystemTaskStep.php:211.Closes #2059Root cause
SettingsHandler::sanitizeField()(inc/Core/Steps/Settings/SettingsHandler.php) had nocase 'json':branch.json-typed fields fell through todefault: sanitizeText(), which callssanitize_text_field( wp_unslash( $value ) ). WordPress'ssanitize_text_field()coerces array/object input to an empty string, so any nested-arrayparamspayload was silently zeroed out before storage.Live proof of the coercion (run against
data-machine@main):After the fix, the same input returns
paramsas the expected nested array verbatim.File list
inc/Core/Steps/Settings/SettingsHandler.php— addcase 'json':to the type switch and a newsanitizeJson()method that preserves nested arrays, decodes JSON-string payloads, and never returns a scalar string.tests/flow-update-handler-config-params-smoke.php— pure-PHP smoke test pinning the contract across six input shapes.Reproduction (main fails, branch passes)
Smoke test results, same file, two checkouts of the same branch — one with the
SettingsHandler.phpfix stashed away (==main), one with it in place:Regression sweep
Ran every smoke that touches SystemTask, FlowStep config, or the flow-update CLI path:
tests/dispatch-message-task-smoke.php— 46/46 passtests/flow-update-set-user-message-smoke.php— 35/35 passtests/systemtask-passthrough-smoke.php— 29/29 passtests/system-task-config-passthrough-smoke.php— 36/36 passtests/system-run-task-params-smoke.php— 28/28 passtests/flow-step-config-factory-smoke.php— 6/13 pass (pre-existing, fails identically on main; unrelated to this change)phpcsclean on both changed files.Scope discipline
CHANGELOG.mdedits, no version bumps — homeboy owns those.homeboy release, nohomeboy deploy— PR only.SettingsHandler(right layer) instead of patchingSystemTaskSettings, so every future handler that declares ajsonfield gets the same correct behaviour for free.cc <@532385681268408341>