[4.0] Type-safe string comparisons in libraries #27194
Conversation
| @@ -698,7 +698,7 @@ public function load($extension = 'joomla', $basePath = JPATH_BASE, $lang = null | |||
|
|
|||
| $path = LanguageHelper::getLanguagePath($basePath, $lang); | |||
|
|
|||
| $internal = $extension == 'joomla' || $extension == ''; | |||
| $internal = $extension === 'joomla' || $extension == ''; | |||
There was a problem hiding this comment.
| $internal = $extension === 'joomla' || $extension == ''; | |
| $internal = $extension === 'joomla' || $extension === ''; |
There was a problem hiding this comment.
Not sure about this as this concerns a function argument. Although it is documented as string, some may say it's a B/C break (because currently null works). Maintainers decide.
Co-Authored-By: Quy <quy@fluxbb.org>
|
I have tested this item ✅ successfully on ca295ab This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27194. |
|
I'm afraid - maybe tested it wrong? This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27194. |
|
@ChristineWk This needs to be tested by code review only. If you're familiar with PHP code, review the changes here https://github.com/joomla/joomla-cms/pull/27194/files. |
|
Thanks @SharkyKZ In the meantime, checked apprx. 20 files incld. given lines :-) successfully. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27194. |
| @@ -49,7 +49,7 @@ public function filter(\SimpleXMLElement $element, $value, $group = null, Regist | |||
| { | |||
| if ($p !== '') | |||
| { | |||
| $return[$action][$id] = ($p == '1' || $p == 'true') ? true : false; | |||
| $return[$action][$id] = ($p == '1' || $p === 'true') ? true : false; | |||
There was a problem hiding this comment.
| $return[$action][$id] = ($p == '1' || $p === 'true') ? true : false; | |
| $return[$action][$id] = ($p === '1' || $p === 'true') ? true : false; |
|
Thanks! @Quy are you ok to make that last change you proposed? Just in the interests of getting this merged |
|
Let me double check and will submit a PR if necessary. |
|
Sorry. I forgot to respond to suggested change. I haven't looked at it properly so not sure if it's right. It seems to concern a form feld value, in which case I'd leave it as is. Just in case we might start storing those values as integers. |
Summary of Changes
Adds type-safe comparisons where appropriate.
Testing Instructions
Code review.
Documentation Changes Required
No.