[#4638] - [fix] permissions not checked to change state of a redirect
- Closed
- 16 May 2015
- Medium
- Build: staging
- # 4638
- Diff
- phproberto:redirect-issues
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
This PRs tries to fix some issues found after merging #4609 (comment)
- I'm deprecating
JHtmlRedirect::published()as the fix we applied is a bit tricky - I'm creating a new function
JHtmlRedirect::status()as replacement of the previous method - I'm replacing the only existing call to
JHtml::_('redirect.published'to use nowJHtml::_('redirect.status' - When replacing the call I realised that the
$canChangeis not checked to display the icon. That + having a default value of true made it always clickable by the user.
cc / @roland-d
jissues-bot
- | Labels |
Added:
?
|
||
It's a change in the ordering of the parameters as well as making one param required that was previously optional, so either there would need to be code added into the method to put the params in the right order or you just create a totally new method. This is the right move for this instance.
The $i has never been optional and the only reason we're moving it to put it as the first param for codestyle as I understand (it was made to be null to satisfy codestyle in #4609 this weekend at the sprint). And in that case I'd just much rather see us wait for v4 before doing this kind of stuff imo.
It's still the correct move. Having required params in the middle of a method declaration inherently makes everything before it required too. The fix is either make the param truly optional or make preceding params required, and the latter is not B/C.
By the B/C policy, if we deprecated the signature that's in today's 3.3.6 production release at 4.0, then the old way would have to be supported until 5.0, meaning the code would have to check and correctly order the params within the function and the expected behavior is no longer as clear because of this (see JHtml in the 2.5 branch, there's a few methods doing this for support with 1.5).
Creating a new method now with the correct signature provides a compatibility layer today. With this merged, anything from 3.4 up and into the 4.x branch (assuming no major restructuring happened between now and then) would work just fine with this call.
brianteeman
- | Category | ⇒ | Components |
I understand why the codestyle rule is what it is and why we're changing to implement it. It's having this principle of needing to do this every time is just stupid imo...
Should I update this PR or directly close it? Remember that there is a missing permission check there
Update it. We need to fix things. And we've done this elsewhere now.... so I guess precedent set :(
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-05-16 12:25:44 |
| Closed_By | ⇒ | phproberto |
I think you guys are just trying to break my hard work :P
On a serious note I'm not a fan of this new method at all. I think it's a bad idea to set a precedent that if we create a new parameter for a method we have to create a full new function just to satisfy code style. Imagine how new features multiply our code base so quickly!!!
Obviously the stuff with the permissions needs fixing though.