[#17120] - Janitorial Duty
- Fixed in Code Base
- 16 Jan 2018
- Medium
- Build: staging
- # 17120
- Diff
- Paladin:patch-1
- Success hound No violations found. Woof! Details
- 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
- Error continuous-integration/jenkins/pr-merge This commit cannot be built Details
User tests: Successful: Unsuccessful:
(And I mean that literally.)
Cleans up ModArchiveHelper so it conforms to Joomla coding conventions and is ready for PHP7.
Summary of Changes
Let's cover the "biggest" change in here first:
Line 59 returned a void while line 88 (still in the same method) returned an Array. Returns from methods should be consistent (PHP7 supports specifying method return types -- it's an essential part of Design By Contract) and given the fact the docblock claims the method returns an array, we should ensure the method always returns an array, like it promises. That way any future developer writing code that uses it knows what to expect.
B/C exposure on this exists, but is minimal, and as far as I can see entirely hypothetical. First it's in an exception handling routine, so there's already a problem happening; normal processing may already be off the table when it gets executed. Also, the way this is handled in the output routines is by testing the return for empty (empty($lists)) and emtpy(NULL) and empty(array()) both return true. I've looked, and not run into a single instance of B/C breakage from of this change, and I think the change is worth doing because it makes the method perform as the documentation says it does, which I think is important. It's hard to see how making software behave as advertised is breaking B/C; if we grant that, then isn't pretty much any bug fix a B/C break? And in this case I have seen literally no instances in the wild this would break. (It's hard to even conceive of one, as the list is typically processed as a foreach, which works without error for empty arrays.)
The rest of the changes are routine. The docblock change in line 24 reflects the fact the '&' is not part of the parameter; it's meta-data about how the parameter is passed (Side note: Look deep enough, it may even have been me that put it in there, back in the day. I recall doing that several times before I caught myself.) Zero impact.
The rest of the changes involve changing local variables from serpentine case to camel case. Zero potential impact outside of the method itself.
Testing Instructions
Running the normal tests for the module, including simply loading a website with archived articles that publishes this module, will prove the correctness/failure of the variable name changes. As for the return value, I have been unable to find anything that breaks B/C, but if there's a template out there that might, either load it up (or pass me a copy of the template and I will) and test it. The result from a B/C break will be something appearing on the site where that module should be that shouldn't be there.
Expected result
Should pass every test it passed before. If testing by displaying on website, should display the exact same after the change as it did before the change (I can't specify the exact code it will display, because it's output through a template, so the output code is malleable).
Actual result
For me, I saw no results changes in testing between before and after. That was expected, as this does not fix a user issue; it cleans up the code so other developers can work with it easier.
There was one difference, however: It now passes phpcs with the Joomla coding standards selected, whereas before it cited 11 problems with the code.
Documentation Changes Required
None.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Modules Front End |
I can see why grep would think they were the same. But as far as PHP is concerned, the '&' merely means "pass the accompanying argument by reference instead of by value;" it's not a part of the parameter itself (it's like pretty much any other operator, in this context meaning "address of" rather than, for example "negation of" as ! would mean). Calls to a method defined with that don't use the "&," they use "$fred" rather than "&$fred" (and if they try to use the latter they will cause a deprecated warning in anything past php5.3 or so). PHP has no trouble telling the difference between a method signature and a parameter.
But coding standards all boil down to convention rather than correctness, so if you want to have it there, that's fine with me; by all means pull that change out, or tell me and I'll do it. For the sake of consistency, though, you should pass that info on to those doing the Joomla coding standard for phpcs (joomla/coding-standards repo)\ because the current CodeSniffer (using phpcs 2.8+) using that standard recognizes it and actually flagged that docblock as being in error in its report:
21 | ERROR | Doc comment for parameter "$params" missing
Irrelevant Trivia: phpcs is actually why I did this file. It happened to be the file I pointed the sniffer at when I was testing the connection of phpcs to VisualStudio Code, and it flagged errors, so I fixed them.
you should pass that info on to those doing the Joomla coding standard for phpcs (joomla/coding-standards repo)
The standard in use on this repo is still using our old PHPCS 1.5 definitions. We're finally getting PHPCS 2.x definitions stabilized and rolling across the project. So, expect some groaning between the two definitions.
1.5 -- And I thought requiring phpcs 2.x was quaint. ;{>} Back in 2012/3 when phpcs 1.5 was coming out, PHP required the "&" on the call as well as the signature. PHP was in the process of fixing that defect when sniffer 1.5 shipped, but the future hadn't been evenly distributed by then. These days Joomla doesn't support a version of PHP that would accept that construct without at least muttering under its breath. (Dang, I feel old today!)
So, do we leave this as a beacon for the future, or harken back? You want to make the necessary change or should I? (Or would it be easier just closing this PR and redoing it?)
For now I'd just put the doc block back how it was and as we work on getting PHPCS 2 implemented we can fix it at the same time the sniffer that groans about it lands in the repo.
| Labels |
Added:
?
|
||
Awww, can't we stop being the slow kids in class and skip phpcs 2 and go straight to 3? It's not hard. Pretty much anything that works in 2 will work in 3 after about 15 minutes of surgery (main change is the classes are all namespaced in 3; when I did a quick pass at it, the only real entanglement came with extending one of the generic sniffs b/c when you're using just a class name from one namespace PHP doesn't like having a class in the current namespace with the same name).
drone be happy now.
| Status | Pending | ⇒ | Needs Review |
Status is set on "Needs Review".
@franz-wohlkoenig why do you think this needs to be set to Needs Review? Just two tests and merge is missing ;)
This is not what the needs review status is for ;) This is a general rule that before we merge something we review it ;) Just move it back to pending so we can get tests
The needs review is some kind on-hold status until someone review the discussion general idea. That is not a problem here. Just two tests and merge do the trick here
| Status | Needs Review | ⇒ | Pending |
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17120.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17120.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17120.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-01-16 22:52:05 |
| Closed_By | ⇒ | mbabker | |
| Labels |
Added:
?
|
||
Oh, that was interesting. http://213.160.72.75/joomla/joomla-cms/6872 thinks the '&' is part of the variable name? Really? In that case the CodeSniffer standard needs looked at.