fix(processors): normalize string 'true'/'false' to boolean in getProperty#16888
fix(processors): normalize string 'true'/'false' to boolean in getProperty#16888Ibochkarev wants to merge 5 commits intomodxcms:3.xfrom
Conversation
…perty JavaScript sends boolean params as strings. getProperty now converts 'true'/'false' (case-insensitive) to actual booleans so processor logic works correctly without explicit type casting.
|
I'm mildly worried this may have unintentional far-reaching consequences in processors that have been doing its own normalisation based on strings that is now going to break. |
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix an issue where JavaScript/ExtJS sends boolean parameters as strings ("true"/"false") to processors, causing them to be treated as truthy strings instead of proper booleans. The solution modifies Processor::getProperty() to automatically normalize case-insensitive string "true"/"false" (with trimming) to boolean values.
Changes:
- Modified
getProperty()method inProcessor.phpto convert string "true"/"false" (case-insensitive, trimmed) to boolean - Added inline documentation explaining the normalization behavior
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $value = array_key_exists($k, $this->properties) ? $this->properties[$k] : $default; | ||
| if (is_string($value)) { | ||
| $normalized = strtolower(trim($value)); | ||
| return match ($normalized) { | ||
| 'true' => true, | ||
| 'false' => false, | ||
| default => $value, | ||
| }; | ||
| } | ||
| return $value; |
There was a problem hiding this comment.
The automatic normalization changes the data type globally for all processors without any opt-in mechanism. This is a breaking API change that affects the return type of a widely-used method. Consider that third-party code and extensions may rely on the current behavior where string values are returned as-is.
A safer approach would be to:
- Introduce a new method like
getBooleanProperty()that performs normalization - Or add an optional parameter to control normalization:
getProperty($k, $default = null, $normalizeBoolean = false) - Or use a trait/mixin that processors can opt into for boolean normalization
This would allow new code to benefit from the normalization while preserving backward compatibility for existing code.
| $value = array_key_exists($k, $this->properties) ? $this->properties[$k] : $default; | ||
| if (is_string($value)) { | ||
| $normalized = strtolower(trim($value)); | ||
| return match ($normalized) { | ||
| 'true' => true, | ||
| 'false' => false, | ||
| default => $value, | ||
| }; | ||
| } | ||
| return $value; |
There was a problem hiding this comment.
This change lacks test coverage to verify:
- That string 'true' and 'false' (in various cases) are correctly normalized to boolean
- That the normalization handles whitespace correctly
- That non-string values are not affected
- That default values work correctly with normalization
- That existing code comparing with string 'false' is not broken
Given the widespread use of getProperty() throughout the codebase (hundreds of calls), comprehensive tests are critical to ensure this change doesn't introduce regressions. Tests should cover both the new normalization behavior and compatibility with existing usage patterns.
| $value = array_key_exists($k, $this->properties) ? $this->properties[$k] : $default; | ||
| if (is_string($value)) { | ||
| $normalized = strtolower(trim($value)); | ||
| return match ($normalized) { | ||
| 'true' => true, | ||
| 'false' => false, | ||
| default => $value, | ||
| }; | ||
| } | ||
| return $value; |
There was a problem hiding this comment.
This automatic normalization of string "true"/"false" to boolean will break existing code that explicitly checks for the string "false". For example, in Security/Profile/Update.php (lines 76 and 111), there's code that checks if ($this->getProperty('newpassword') !== 'false'). With this change, getProperty('newpassword') will return boolean false instead of string "false", breaking the logic since false !== 'false' is always true.
Similarly, in Resource/Get.php (line 34), there's a check $this->getProperty('skipFormatDates') == 'false' which will no longer work as expected.
The setCheckbox method (line 261) also relies on comparing with string 'false': $this->properties[$k] === 'false'. Since this method accesses $this->properties directly instead of using getProperty(), it would still work, but this creates an inconsistency where the same property value behaves differently depending on how it's accessed.
Consider these alternatives:
- Only normalize in specific scenarios where you know the value should be boolean
- Add a separate method like
getBooleanProperty()that performs this normalization - Fix all existing code that compares with string 'false' before implementing this change
- Add a parameter to
getProperty()to opt-in to normalization:getProperty($k, $default = null, $normalizeBoolean = false)
| return match ($normalized) { | ||
| 'true' => true, | ||
| 'false' => false, | ||
| default => $value, | ||
| }; |
There was a problem hiding this comment.
This change will affect the behavior of code that uses string comparisons with 'on' values. In Context/Duplicate.php, there are comparisons like $this->getProperty('preserve_resources') == 'on'. Since 'on' is not normalized, this will continue to work, but it creates an inconsistency where boolean-like string values are handled differently ('true'/'false' vs 'on'/'off').
ExtJS checkboxes can send 'on' as their checked value, and there's existing code that handles this. The PR description mentions JavaScript sends "true"/"false", but doesn't address the 'on' pattern which is also used in the codebase. This inconsistency could be confusing for developers.
I think this suggestion makes a lot of sense given the potential impact this may have on core and extras. Copilot already highlighted a few cases in the codebase that break. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Revert getProperty() to original behavior (no boolean normalization)
- Add getBooleanProperty($k, $default) for explicit bool parsing
- Support 'true'/'false'/'on'/'off'/'1'/'0'/'yes'/'no' (case-insensitive)
- Resource/Get: use getBooleanProperty('skipFormatDates', false)
- formatDates: use !empty($resourceArray['publishedon']) for consistency
- Add ProcessorTest for getBooleanProperty() coverage
- Fix PSR12 file header spacing in Resource/Get.php
Addresses PR modxcms#16888 review (Mark-H, Copilot): avoid changing getProperty()
to prevent impact on core and extras.
|
@Mark-H please re-run review copilot. Thanks |
Code ReviewSummary Adds Assessment Clean, non-breaking addition. The Verdict: Approve |
What does it do?
Adds a new method
Processor::getBooleanProperty($k, $default = false)that normalizes string values like"true","false","on","off","1","0","yes","no"(case-insensitive, trimmed) to boolean.Processor::getProperty()is unchanged and still returns raw values. When JavaScript sends boolean parameters to processors (e.g. via ExtJS or fetch), they arrive as strings; processors that need a real boolean can callgetBooleanProperty()so logic works without manual casting.Why is it needed?
JavaScript/ExtJS sends boolean values as strings (
"true"/"false"). Processors usinggetProperty('someFlag')receive strings, soif ($this->getProperty('someFlag'))is always truthy for non-empty strings and breaks when the intended value isfalse. Introducing a dedicatedgetBooleanProperty()avoids changing existinggetProperty()behavior (and potential impact on core and extras) while giving processors an explicit way to read boolean params.How to test
$this->getBooleanProperty('someBooleanParam', false).someBooleanParam=false(e.g. via AJAX or connector).getBooleanProperty('someBooleanParam', false)returnsfalse(boolean).someBooleanParam=trueand assert it returnstrue(boolean).someBooleanParam=TRUE,True, ortrueand assert case-insensitive, trimmed handling.someBooleanParam=on,yes,1(true) andoff,no,0(false) and assert normalization._build/test/Tests/Processors/ProcessorTest.phpfor fullgetBooleanProperty()coverage.Related issue(s)/PR(s)
Fixes #14016