More type-safe comparisons in site/templates#13310
More type-safe comparisons in site/templates#13310rdeutz merged 9 commits intojoomla:stagingfrom frankmayer:more-type-safe-comparisons-in-site-templates
Conversation
|
Could you please explain why we need all these type-safe comparisons? |
|
The quick points:
There is lots of info on the web on this matter. |
|
In general, type safety should be preferred, otherwise you run into a lot of funny PHP quirks. Essentially, Newer PHP versions also start spitting errors when you give non-numeric types (only integer and float are numeric) to be processed for arithmetic operations, older versions were a little less restrictive and would try to do math on a string representation on the value. |
| defined('_JEXEC') or die; | ||
| $class = ' class="first"'; | ||
| if (count($this->items[$this->parent->id]) > 0 && $this->maxLevelcat != 0) : | ||
| if (count($this->items[$this->parent->id]) > 0 && $this->maxLevelcat !== 0) : |
There was a problem hiding this comment.
are we sure maxLevelcat and maxLevel (below) are always an int?
There was a problem hiding this comment.
If I am not mistaken, they are both OK. Definitions:
There was a problem hiding this comment.
But we should probably type cast those... to be sure... or revert the strict comparison for those cases. I would prefer the prior though...
There was a problem hiding this comment.
the question is, for instance, $params->get('maxLevelcat', PHP_INT_MAX); could return a string right?
i would prefer the first alternative too
There was a problem hiding this comment.
Yes, unfortunately it could.
I just checked with the JSON data in the menu table and both are strings!!! Why?
There was a problem hiding this comment.
Would that be B/C? Not sure.
Also, IMHO, to preserve consistency across the core that would be need to be done across all core int fields in all xml forms and all probaly db tables.
So that would probably need to go to 4.0., not sure tough.
There was a problem hiding this comment.
I have already done some thinking on that. Will do some prototyping in the next days.
There was a problem hiding this comment.
Hi @andrepereiradasilva, thanks for the code reviews. Hopefully we can get all the remaining improvements merged this time around 😄
As for your question: I never came around to do some more serious thinking and prototyping. But I agree with you, this might be better suited for 4.0. I will probably reverse this change for now. I think i this should be resolved in a new PR.
There was a problem hiding this comment.
@andrepereiradasilva OK, I reversed those strict comparisons for those properties in all related files of this PR. Thanks for pointing that out.
| <?php echo $this->loadTemplate('items'); ?> | ||
|
|
||
| <?php if (!empty($this->children[$this->category->id])&& $this->maxLevel != 0) : ?> | ||
| <?php if (!empty($this->children[$this->category->id])&& $this->maxLevel !== 0) : ?> |
| <?php echo $this->loadTemplate('items'); ?> | ||
|
|
||
| <?php if (!empty($this->children[$this->category->id])&& $this->maxLevel != 0) : ?> | ||
| <?php if (!empty($this->children[$this->category->id])&& $this->maxLevel !== 0) : ?> |
| <?php endif; ?> | ||
| <?php echo $this->loadTemplate('items'); ?> | ||
| <?php if (!empty($this->children[$this->category->id])&& $this->maxLevel != 0) : ?> | ||
| <?php if (!empty($this->children[$this->category->id])&& $this->maxLevel !== 0) : ?> |
templates/beez3/index.php
Outdated
| $showleft = ($this->countModules('position-4') or $this->countModules('position-7') or $this->countModules('position-5')); | ||
|
|
||
| if ($showRightColumn == 0 and $showleft == 0) | ||
| if ($showRightColumn === false and $showleft === false) |
…te-templates' into more-type-safe-comparisons-in-site-templates
Reversed some strict comparisons, as the values compared might not be of same type
Reversed some more strict comparisons, as the values compared might not be of same type
|
I have tested this item ✅ successfully on 2e806ed This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13310. |
|
I have tested this item ✅ successfully on 2e806ed This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13310. |
|
RTC after two successful tests. |
Summary of Changes
The changes in this PR should be fairly easy to review. They are only type safe
intorboolcomparisons. This PR is done, so the batch PR #12233 can become a little lighter and easier to review. In hope that this will get merged quickly so further work can be done without conflicting with other PRs. ;)Testing Instructions
None, should not change behavior
Documentation Changes Required
None.