[#13272] - Type safe comparison in plugins - second iteration
- Fixed in Code Base
- 31 May 2017
- Medium
- Build: staging
- # 13272
- Diff
- frankmayer:type-safety-in-plugins-2
- Success continuous-integration/drone/pr the build was successful Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
- Pending JTracker/HumanTestResults Human Test Results: 1 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
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.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Front End Plugins |
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
| Labels |
Added:
?
|
||
@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 '')
| Title |
|
||||||
| 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,
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!
@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.
@andrepereiradasilva Yes, I was already checking those out, too...
Commit will follow soon.
ok sorry, tought you missed them. ignore the comment them
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.
Conflicts resolved...
Pls check and merge.
I have tested this item
on code review
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?
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-05-31 20:16:46 |
| Closed_By | ⇒ | wilsonge |
Conflicts with the other PR :)