Type safe comparison in libraries/cms - second iteration #13276

frankmayer · 2016-12-19T00:51:54Z

Summary of Changes

Type safe comparison in libraries/cms - second iteration

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 libraries/cms directory, which is still on hold (#12171).
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.

Type safe comparison in libraries/cms - second iteration - some bool - some int

andrepereiradasilva · 2016-12-19T19:27:06Z

libraries/cms/html/string.php

 	{
 		// Assume a lone open tag is invalid HTML.
-		if ($length == 1 && substr($text, 0, 1) == '<')
+		if ($length === 1 && substr($text, 0, 1) === '<')


Are we sure lenght is an int?

Thanks to not having the ability to do scalar typehinting, no, we aren't. That said, any dev passing a string into a method declaring a parameter as an integer is just asking for trouble as newer PHP releases come out.

Yes. I agree. Still to keep b/c maybe is better to (int) the var....

It's not a B/C break if the code enforces correct typing of a parameter. Otherwise either we bump to PHP 7 and typehint everything or remove all typehints and we're stuck running gettype() and is_a() or instanceof checks on everything because the documentation is unreliable.

Sooner or later the mentality of "well just pass anything in any ol' format" has to be broken. There are far too many issues dealing with something like 0 == '0' == false == null.

OK. If is not considered a b/c break by the project fine. Just raising the question. For the rest obvisualy i agree.

I totally agree with @mbabker. 👍

andrepereiradasilva · 2016-12-19T19:27:58Z

libraries/cms/html/string.php


 		// If the original HTML string is shorter than the $maxLength do nothing and return that.
-		if ($baseLength <= $maxLength || $maxLength == 0)
+		if ($baseLength <= $maxLength || $maxLength === 0)


Are we sure maxlenght is an int?

Leaving as is, because the variable is defined as a parameter to the function, with type integer.

andrepereiradasilva · 2016-12-19T19:28:19Z

libraries/cms/html/string.php


 		// Deal with maximum length of 1 where the string starts with a tag.
-		if ($maxLength == 1 && substr($html, 0, 1) == '<')
+		if ($maxLength === 1 && substr($html, 0, 1) === '<')


Same as above

Leaving as is, because the variable is defined as a parameter to the function, with type integer.

andrepereiradasilva · 2016-12-19T19:29:18Z

libraries/cms/language/associations.php

 				{
 					// Do not return itself as result
-					if ((int) $item->{$pk} != $id)
+					if ((int) $item->{$pk} !== $id)


Are we sure id is an int?

Leaving as is, because the variable is defined as a parameter to the function, with type integer.

andrepereiradasilva · 2016-12-19T19:30:13Z

libraries/cms/plugin/helper.php

 		$defaults = false;

-		if (is_null($plugin) && $autocreate == true && is_null($dispatcher))
+		if (is_null($plugin) && $autocreate === true && is_null($dispatcher))


Are we sure autocreate is a boolean?

Leaving as is, because the variable is defined as a parameter to the function, with type boolean.

andrepereiradasilva · 2016-12-19T19:32:42Z

libraries/cms/table/corecontent.php


 		// If checkin is supported and all rows were adjusted, check them in.
-		if ($checkin && (count($pks) == $this->_db->getAffectedRows()))
+		if ($checkin && (count($pks) === $this->_db->getAffectedRows()))


Are we sure affected rows return an int in all db drivers?

That's a good question. But normally int is supported in every common db and so should the drivers. Will check, though, to be sure. Or we just cast to int, to be on the safe side

@andrepereiradasilva Yes, we are sure now. I just checked. :)

On, to review & merge?

andrepereiradasilva

according to comment above

# Conflicts: # libraries/cms/component/router/rules/standard.php # libraries/cms/html/string.php

frankmayer · 2017-04-26T10:45:30Z

Conflicts resolved. Pls check and merge.

…nto tye-safety-in-libraries-cms-2

frankmayer · 2017-06-02T09:22:38Z

@andrepereiradasilva & @Quy Would you be so nice to code review this, too. So we can RTC this and after merging, tackle the parent PR (when all its Sub PRs are merged)? Again, thank you for your efforts!!

Quy · 2017-06-11T23:46:53Z

libraries/cms/html/html.php


-		$prefix = (count($parts) == 3 ? array_shift($parts) : 'JHtml');
-		$file = (count($parts) == 2 ? array_shift($parts) : '');
+		$prefix = (count($parts) === 3 ? array_shift($parts) : 'JHtml');


Remove parentheses (both lines).

Quy · 2017-06-11T23:48:29Z

libraries/cms/table/corecontent.php


 		// If checkin is supported and all rows were adjusted, check them in.
-		if ($checkin && (count($pks) == $this->_db->getAffectedRows()))
+		if ($checkin && (count($pks) === $this->_db->getAffectedRows()))


Remove parentheses

…de style

andrepereiradasilva · 2017-06-12T21:09:42Z

I have tested this item ✅ successfully on f5fe9bf

code review and according to comments above

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

Quy · 2017-06-12T21:54:41Z

I have tested this item ✅ successfully on f5fe9bf

Code review.

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

ghost · 2017-06-13T04:19:59Z

RTC after two successful tests.

### Summary of Changes

579bacd

Type safe comparison in libraries/cms - second iteration - some bool - some int

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

andrepereiradasilva reviewed Dec 19, 2016

View reviewed changes

andrepereiradasilva approved these changes Dec 28, 2016

View reviewed changes

frankmayer mentioned this pull request Jan 5, 2017

Performance optimizations #1 #12171

Merged

6 tasks

Merge branch 'staging' into tye-safety-in-libraries-cms-2

06b1743

# Conflicts: # libraries/cms/component/router/rules/standard.php # libraries/cms/html/string.php

rdeutz and others added 4 commits May 27, 2017 19:13

Merge branch 'staging' into tye-safety-in-libraries-cms-2

15ff27e

Merge branch 'staging' into tye-safety-in-libraries-cms-2

15a6212

Merge remote-tracking branch 'origin/tye-safety-in-libraries-cms-2' i…

76a417a

…nto tye-safety-in-libraries-cms-2

Merge branch 'staging' into tye-safety-in-libraries-cms-2

6bca581

Quy reviewed Jun 11, 2017

View reviewed changes

frankmayer added 2 commits June 12, 2017 10:38

Changes according to reviewer's comments + more type safety + some co…

8e5d494

…de style

Merge branch 'staging' into tye-safety-in-libraries-cms-2

f5fe9bf

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 13, 2017

rdeutz merged commit 128e175 into joomla:staging Jun 13, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 13, 2017

rdeutz added this to the Joomla 3.7.3 milestone Jun 13, 2017

frankmayer deleted the tye-safety-in-libraries-cms-2 branch June 13, 2017 21:56

Uh oh!

Type safe comparison in libraries/cms - second iteration #13276

Type safe comparison in libraries/cms - second iteration #13276

Uh oh!

Conversation

frankmayer commented Dec 19, 2016 • edited by joomla-cms-bot 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!

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 Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 left a comment

Choose a reason for hiding this comment

Uh oh!

frankmayer commented Apr 26, 2017

Uh oh!

frankmayer commented Jun 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Jun 12, 2017

Uh oh!

Quy commented Jun 12, 2017

Uh oh!

ghost commented Jun 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

frankmayer commented Dec 19, 2016 •

edited by joomla-cms-bot

Loading

andrepereiradasilva Dec 19, 2016 •

edited

Loading

andrepereiradasilva Dec 19, 2016 •

edited

Loading