Type safe comparison in plugins - second iteration by frankmayer · Pull Request #13272 · joomla/joomla-cms

frankmayer · 2016-12-18T22:53:01Z

Summary of Changes

Type safe comparison in plugins - second iteration

some more string comparisons
some bool
some int

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.

- some more string comparisons - some bool - some int

wilsonge · 2016-12-19T15:16:49Z

Conflicts with the other PR :)

andrepereiradasilva · 2016-12-19T15:32:06Z

I think you need to review the strict comparations ... some are not correct

For instance:
https://github.com/joomla/joomla-cms/pull/13272/files#diff-72a6846b2b3372956e666bfffccd2ed7R99

https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/language/multilang.php#L27

…comparisons)

# Conflicts: # plugins/content/vote/vote.php # plugins/system/languagefilter/languagefilter.php

frankmayer · 2016-12-19T16:52:00Z

@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 · 2016-12-19T23:02:00Z

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.

andrepereiradasilva · 2016-12-19T23:00:37Z

plugins/content/vote/vote.php

 		$html = ob_get_clean();

-		if ($this->app->input->getString('view', '') === 'article' && $row->state == 1)
+		if ($this->app->input->getString('view', '') === 'article' && $row->state === 1)


doubt: is row state (and access in changes below) an int for sure in all cases?

Did you check this?

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.

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.

i agree, but a change like that IMHO should be in 4.0 branch
@wilsonge what do you think?

andrepereiradasilva · 2016-12-19T23:12:59Z

BTW, 👍 to all for putting in the extra work to support my efforts for all these PR with lots and lots of changes.

no, thank you for doing this!

shur · 2016-12-24T09:18:12Z

plugins/editors-xtd/image/image.php

-			|| ($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))


and what about line 53 ?

Ah, yes, missed that one... Thanks!!

…value is not guaranteed.

andrepereiradasilva · 2016-12-28T16:27:17Z

@frankmayer the one i pointed and you corrected is not the only case in this PR. (example: access)
Please review the PR changes according to that. IMO is important to make sure this (and the others) PR don't break anything by making incorrect assumptions reggarding the variable types.

frankmayer · 2016-12-28T16:45:27Z

@andrepereiradasilva Yes, I was already checking those out, too...
Commit will follow soon.

andrepereiradasilva · 2016-12-28T16:51:25Z

ok sorry, tought you missed them. ignore the comment them

… the value is not guaranteed.

frankmayer · 2016-12-28T16:54:40Z

No, no, my fault. I shouldn't have pushed the commit, before completing my checks. I was multitasking with other things... 😸

frankmayer · 2016-12-28T16:55:43Z

The rest should be OK now.

frankmayer · 2016-12-29T16:11:27Z

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

frankmayer · 2017-01-12T17:26:57Z

Conflicts resolved...

frankmayer · 2017-04-26T10:57:13Z

Pls check and merge.

# Conflicts: # plugins/user/joomla/joomla.php

andrepereiradasilva · 2017-05-31T19:09:58Z

I have tested this item ✅ successfully on 572572c

on code review

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13272.}

frankmayer · 2017-05-31T19:41:36Z

Thanks, one more tester to get this RTC?

Quy · 2017-05-31T21:24:47Z

plugins/captcha/recaptcha/recaptchalib.php

 	public function __construct($secret)
 	{
-		if ($secret == null || $secret == '')
+		if ($secret == null || $secret === '')


@frankmayer Should this be === null?

I think It could, and I also think I have done this in another PR.

Type safe string comparison of strings in plugins - second iteration

39a2e07

- some more string comparisons - some bool - some int

joomla-cms-bot added the PR-staging label Dec 18, 2016

frankmayer added 2 commits December 19, 2016 18:42

Fixed two bugs introduced in previous commit (one involving multiple …

d8e1eb7

…comparisons)

Merge branch 'staging' into type-safety-in-plugins-2

e058414

# Conflicts: # plugins/content/vote/vote.php # plugins/system/languagefilter/languagefilter.php

frankmayer changed the title ~~Type safe string comparison of strings in plugins - second iteration~~ Type safe comparison in plugins - second iteration Dec 19, 2016

andrepereiradasilva reviewed Dec 19, 2016

View reviewed changes

shur reviewed Dec 24, 2016

View reviewed changes

Missed a comparison. Thanks @shur!

cfaca22

frankmayer mentioned this pull request Dec 27, 2016

Performance 4 (plugins) #12228

Merged

5 tasks

Reversed type safe comparison, as it turns out, that the type of the …

90055f6

…value is not guaranteed.

Reversed type safe comparison (#2), as it turns out, that the type of…

a52d3be

… the value is not guaranteed.

Reverted this one to not clash with @laoneo's refactoring efforts

669da6f

Merge branch 'staging' into type-safety-in-plugins-2

4d9479e

# Conflicts: # plugins/editors-xtd/image/image.php

Merge branch 'staging' into type-safety-in-plugins-2

572572c

# Conflicts: # plugins/user/joomla/joomla.php

wilsonge merged commit 9ba22b7 into joomla:staging May 31, 2017

wilsonge added this to the Joomla 3.7.3 milestone May 31, 2017

frankmayer deleted the type-safety-in-plugins-2 branch May 31, 2017 20:34

Quy reviewed May 31, 2017

View reviewed changes

Uh oh!

Conversation

frankmayer commented Dec 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Documentation Changes Required

Uh oh!

wilsonge commented Dec 19, 2016

Uh oh!

andrepereiradasilva commented Dec 19, 2016

Uh oh!

frankmayer commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankmayer commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Dec 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankmayer commented Dec 28, 2016

Uh oh!

andrepereiradasilva commented Dec 28, 2016

Uh oh!

frankmayer commented Dec 28, 2016

Uh oh!

frankmayer commented Dec 28, 2016

Uh oh!

frankmayer commented Dec 29, 2016

Uh oh!

frankmayer commented Jan 12, 2017

Uh oh!

frankmayer commented Apr 26, 2017

Uh oh!

andrepereiradasilva commented May 31, 2017

Uh oh!

frankmayer commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

frankmayer commented Dec 18, 2016 •

edited

Loading

frankmayer commented Dec 19, 2016 •

edited

Loading

andrepereiradasilva commented Dec 28, 2016 •

edited

Loading