Type safe comparison in plugins - second iteration #13272
Conversation
- some more string comparisons - some bool - some int
|
Conflicts with the other PR :) |
|
I think you need to review the strict comparations ... some are not correct For instance: https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/language/multilang.php#L27 |
# Conflicts: # plugins/content/vote/vote.php # plugins/system/languagefilter/languagefilter.php
|
@andrepereiradasilva Thanks missed the one you mentioned and also reverted another case that has multiple comparisons. In that second case though, there might be some cleaning up to do in future (false/null is used if no language is given, though maybe it could just be a '') |
frankmayer
changed the title
|
It would be great, if this sub-PR could be reviewed / tested and merged, so that the associated Batch PR can also be easier reviewed / tested and merged. BTW, 👍 to all for putting in the extra work to support my efforts for all these PR with lots and lots of changes. |
plugins/content/vote/vote.php
Outdated
| @@ -128,7 +128,7 @@ private function displayVotingData($context, &$row, &$params, $page) | |||
| include $path; | |||
| $html = ob_get_clean(); | |||
|
|
|||
| if ($this->app->input->getString('view', '') === 'article' && $row->state == 1) | |||
| if ($this->app->input->getString('view', '') === 'article' && $row->state === 1) | |||
There was a problem hiding this comment.
doubt: is row state (and access in changes below) an int for sure in all cases?
There was a problem hiding this comment.
Did you check this?
There was a problem hiding this comment.
No, sorry, it got under. I checked it just now. You were right! Unfortunately it is not guaranteed that it is an int.
So, either we cast it to an int or we revert the change.
There was a problem hiding this comment.
BTW, I don't know if this has come up in the past, but if we want to do strict(er) typing, as we should, then I guess the db driver classes should make sure the right types are returned, also.
There was a problem hiding this comment.
i agree, but a change like that IMHO should be in 4.0 branch
@wilsonge what do you think?
no, thank you for doing this! |
plugins/editors-xtd/image/image.php
Outdated
| { | ||
| $asset = $extension; | ||
| } | ||
|
|
||
| if ($user->authorise('core.edit', $asset) | ||
| || $user->authorise('core.create', $asset) | ||
| || (count($user->getAuthorisedCategories($asset, 'core.create')) > 0) | ||
| || ($user->authorise('core.edit.own', $asset) && $author == $user->id) | ||
| || ($user->authorise('core.edit.own', $asset) && $author === $user->id) | ||
| || (count($user->getAuthorisedCategories($extension, 'core.edit')) > 0) | ||
| || (count($user->getAuthorisedCategories($extension, 'core.edit.own')) > 0 && $author == $user->id)) |
There was a problem hiding this comment.
Ah, yes, missed that one... Thanks!!
…value is not guaranteed.
|
@frankmayer the one i pointed and you corrected is not the only case in this PR. (example: access) |
|
@andrepereiradasilva Yes, I was already checking those out, too... |
|
ok sorry, tought you missed them. ignore the comment them |
… the value is not guaranteed.
|
No, no, my fault. I shouldn't have pushed the commit, before completing my checks. I was multitasking with other things... 😸 |
|
The rest should be OK now. |
|
It would be great, if this sub-PR could be reviewed / tested and merged, so that the referenced Batch PR #12228 can also be easier reviewed / tested and merged. |
# Conflicts: # plugins/editors-xtd/image/image.php
|
Conflicts resolved... |
|
Pls check and merge. |
# Conflicts: # plugins/user/joomla/joomla.php
|
I have tested this item ✅ successfully on 572572c This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13272. |
|
Thanks, one more tester to get this RTC? |
| @@ -53,7 +53,7 @@ class JReCaptcha | |||
| */ | |||
| public function __construct($secret) | |||
| { | |||
| if ($secret == null || $secret == '') | |||
| if ($secret == null || $secret === '') | |||
There was a problem hiding this comment.
@frankmayer Should this be === null?
There was a problem hiding this comment.
I think It could, and I also think I have done this in another PR.
Summary of Changes
Type safe comparison in plugins - second iteration
This PR is part of a set to try to separate some of the changes done in one of my previous batch PR's for the plugins directory, which is still on hold (#12228).
Once the new set is merged completely, it will hopefully reduce the changes in that PR, so it can be reviewed easier and finally be merged.
The changes in this PR should be also be fairly easy to review. In hope that this will get merged quickly. ;)
Note: Don't bother if some possible changes are missing or could be differently written. They are probably in the batch PR , that this one references. As soon as this set of sub PR's is merged, the batch PR will have its conflicts resolved and should be a lot easier to review and finally get merged.
Testing Instructions
None, should not change behavior
Documentation Changes Required
None.