[#24574] - Fix allowAdd attribute of the JFormField categoryedit
- Closed
- 20 Jul 2019
- Medium
- Build: staging
- # 24574
- Diff
- hardik-codes:Fix#24566
- Success Hound No violations found. Woof! Details
- Failure continuous-integration/drone/pr the build failed Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #24566
Summary of Changes
Changed
$this->allowAdd = isset($this->element['allowAdd']) ? $this->element['allowAdd'] : '';to
$this->allowAdd = isset($this->element['allowAdd']) ? (bool)filter_var((((array)$this->element->attributes())['@attributes']['allowAdd']), FILTER_VALIDATE_BOOLEAN) : '';in administrator/components/com_categories/models/fields/categoryedit.php
Testing Instructions
Code Inspection
Documentation Changes Required
None
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_categories |
| Labels |
Added:
?
|
||
Maybe adding 1 or 2 helper function in JForm getting attribute values, they should work for "all" get attribute use cases. At least for boolean and string or a combination of this.
Maybe adding 1 or 2 helper function in JForm getting attribute values, they should work for "all" get attribute use cases. At least for boolean and string or a combination of this.
Yes at minimum for boolean-like attributes, would be nice
This has come up before, the check of boolean-like attributes is not consistent, at every place that such an attribute is used it may be checked a different way, some examples:
-
Some are just compared to == 'true'
e.g.$hidden = $element['hidden'] == 'true';in menuhelper -
Some are checked if their are not equal to 'false' and not '0' and not zero length string (mean ="foo" is evaluated as true)
e.g. Joomla\CMS\Form;
$showTime = (string) $element['showtime'];
$showTime = ($showTime && $showTime != 'false');This is pr will also allow values "on" "off" and "yes", "no", etc, ??
which i think is a new case of compare if true
I would like to reduce possible values as less as possible to have a consistent codebase, at least for the future...
so adding new possible values makes it more confusing.
Actualy it's a bug, when the allowAdd attribute is present, it's allways consider as true, and you cannot remove an attribute.
And i think that the soluce proposed solve the problem (and i have the problem).
After it's perhaps a good soluce to have a common and more simple function to test a simpleXMLElement value and more specificaly a boolean value.
| Title |
|
||||||
| Title |
|
||||||
@hardik-codes can you please comment?
@hardik-codes whats the Status of this Pull Request?
closed due the lack of response.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-07-20 09:51:21 |
| Closed_By | ⇒ | franz-wohlkoenig |
I think thats the wrong approach, it should be solved more generic.
Also there is missing that the attribute name as value is also true.