[#48476] - [5.4] Fix CheckboxesField ignoring option with value '0'
- Pending
- Medium
- Build: 5.4-dev
- # 48476
- Diff
- bhuvan-somisetty:fix-checkboxesfield-zero-value-48475
User tests: Successful: Unsuccessful:
Pull Request resolves #48475.
- I read the Generative AI policy and my contribution is either not created with the help of AI or is compatible with the policy and GNU/GPL 2 or later.
Summary of Changes
In Joomla\CMS\Form\Field\CheckboxesField::getLayoutData(), the field previously checked whether a stored value exists using:
$hasValue = (isset($this->value) && !empty($this->value));In PHP, empty('0') and empty(0) evaluate to true. When a checkboxes field is bound with a scalar/string value '0', integer 0, or comma-separated string "0" (such as from configuration parameters, registry, or $form->bind(['test' => '0'])), !empty($this->value) evaluated to false. This caused the field to:
- Discard the assigned
'0'value and fall back to the defaultcheckedattribute from XML. - In the layout (
layouts/joomla/form/field/checkboxes.php), re-check any XML option marked withchecked="true".
This PR:
- Updates
$hasValueinCheckboxesField::getLayoutData()to$this->value !== null && $this->value !== '' && $this->value !== [], ensuring'0'and0are properly recognized as valid stored values. - Normalizes array values in
$checkedOptionsusingarray_map('strval', $checkedOptions)so strict type comparisons in layout templates match string option values correctly. - Adds comprehensive unit tests in
tests/Unit/Libraries/Cms/Form/Field/CheckboxesFieldTest.php.
Testing Instructions
1. Automated Unit Tests
Run the unit test suite:
libraries/vendor/bin/phpunit tests/Unit/Libraries/Cms/Form/Field/CheckboxesFieldTest.php2. Manual Test via Form Binding
Load a form with a checkboxes field where checked="1" is default and options include 0 and 1, then bind value '0':
$form = new \Joomla\CMS\Form\Form('test');
$form->load('<form><field name="test_field" type="checkboxes" checked="1"><option value="0">Zero</option><option value="1">One</option></field></form>');
$form->bind(['test_field' => '0']);
echo $form->getInput('test_field');Actual result BEFORE applying this Pull Request
Option 0 (Zero) is unchecked and Option 1 (One) is checked (because empty('0') is true, $hasValue is false, so default checked="1" was re-applied).
Expected result AFTER applying this Pull Request
Option 0 (Zero) is checked and Option 1 (One) is unchecked (the bound '0' value is respected).
Link to documentations
Please select:
-
Documentation link for guide.joomla.org:
-
No documentation changes for guide.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
Signed-off-by: bhuvan-somisetty somisettybhuvan5@gmail.com
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries Unit Tests |
@bhuvan-somisetty In your other PR #48282 you were told to not use hard-coded versions but __DEPLOY_VERSION__ for the @since tag of new stuff added by PRs. Now you make the same mistake again here and use @since 5.4.0 for the new test.
Please fix that.
i'm not an expert in writing tests but as this entire PR is about ensuring that a checkbox with a value of 0 is not ignored shouldnt you be testing for that
i'm not an expert in writing tests but as this entire PR is about ensuring that a checkbox with a value of 0 is not ignored shouldnt you be testing for that
Simple black-box testing the unit test before PR:
- Tests: 8, Assertions: 13, Failures: 3.
And after applying the PR:
- OK (8 tests, 15 assertions)
btw. @bhuvan-somisetty with PHP 8.5 there is a deprecation that should be removed:
Deprecated: Method ReflectionMethod::setAccessible() is deprecated since 8.5, as it has no effect since PHP 8.1 in /var/www/html/tests/Unit/Libraries/Cms/Form/Field/CheckboxesFieldTest.php on line 193
| Labels |
Added:
Unit/System Tests
PR-5.4-dev
|
||
Updated:
- Replaced the hardcoded
@sincetags in the test with__DEPLOY_VERSION__. - Removed the deprecated
ReflectionMethod::setAccessible()call for PHP 8.5 compatibility. - Added explicit assertions in the unit test to verify that option
0is marked checked and default option1is excluded.
Thanks for the feedback!
@bhuvan-somisetty Did you actual test this change yourself? As this is Checkboxes field type, if there are options selected, the value will be saved as array. There is no way it is '0' or 0 as you mentioned and trying to fix in this PR
And for real test, I tried to select the 0 option and it is being checked as expected after save without this PR.
@joomdonation When a form is submitted directly from the browser, PHP does send selected checkboxes as an array ['0']. However, form field values are not only populated from POST requests; they can also be bound from database settings, registry parameters, or API calls via $form->setValue(), where values may be a string "0", comma-separated "0,1", or scalar 0.
In those cases:
- In PHP,
empty('0')andempty(0)returntrue. As a result,!empty($this->value)treated'0'/0as having no value, causing$hasValueto befalseand re-applying the XML default checked options. - If values are provided as integers (e.g.
[0]from JSON decoding), strict comparison in the layout (in_array((string) $option->value, $checkedOptions, true)) would fail without string normalization.
This PR ensures both array and scalar/string representations of '0' / 0 are handled consistently.
What you said is more theoretical than how the field is actually uses in Joomla. For example, I don't know if we want to support passing wrong value (string instead of array) when calling $form->setValue, or if the data comes from database, it should be an array like I mentioned earlier
Anyway, you really should test it yourself and give a clear testing instructions for testers to follow to test your PR. Your testing instructions saying submit a form choosing only Option Zero and the result before your PR is Option Zero is unchecked, and Option One is checked is completely wrong meant you might haven't tested it in an actual form yet.
@joomdonation Fair point regarding standard browser POST requests yielding arrays. The problem specifically arises when form data is bound as a scalar string "0" or comma-separated string (e.g. via registry/params or $form->bind(['field' => '0'])), where empty('0') evaluates to true and resets $hasValue to false.
I have updated the PR description and testing instructions with a clear snippet to reproduce and test this directly on a form instance.
In real life, both in the data is submitted from form and also when data coming from database when editing an item contains that field, the value bind to the field would be an array or nothing, not a string as in your demo code. I'm not saying your PR is wrong but it is also not necessary. It does not really fix any bugs in the field, but require testers to test, maintainers to review which takes time. Also, it changes the existing long time behavior (passed '0' as string suddenly makes item selected while it was not before). I will leave this for release managers to decide.
Understood, thanks for your feedback. Happy to leave it for the release managers to decide.
Thanks for sharing the reference and background context!
@muhme PTAL