User tests: Successful: Unsuccessful:
PR #3972 implements access cache clearing in a very BAD way. This reverts that and implements a new method named clearAccessRights() to achieve a similar effect.
Labels |
Added:
?
|
That is expected. As I wrote above, the code that was proposed is not acceptable and thus the way that you can test this wont work either. The new method to get those changes into the system needs to be manually triggered by the extension that changes the permissions for that user by calling JUser::clearAccessRights(). This will also only work for that one single user, not for other users. If you change larger parts of the ACL system, all users have to log out and log back in again. There is no way around this.
In that case, I suggest this PR to be limited to a revert.
the actual code that allows extensions to clear the cache is 4 lines of code that a maintainer can simply review by looking at it.
the actual code that allows extensions to clear the cache is 4 lines of code that a maintainer can simply review by looking at it.
I agree with JM here. If there is currently no way to test it, this will not get merged.
So it's better to just revert the previous PR and then create a new PR for the new feature, accompagned with testing instructions.
Merging a new feature just based on review is not something I want to do myself.
This is the code that would have to be reviewed.
$this->_authLevels = null;
$this->_authGroups = null;
$this->isRoot = null;
JAccess::clearStatics();
Apparently it was easy accepting code that breaks a large part of our userbase, but reverting that and reviewing 4 lines of code is absolutely difficult. If you wanna test this, do this:
<?php
var_dump(JFactory::getUser());
if(JRequest::getVar('clear')) JFactory::getUser()->clearAccessRights();
I refuse to change this PR further. Either take it or I'll close this PR.
Apparently it was easy accepting code that breaks a large part of our userbase, but reverting that and reviewing 4 lines of code is absolutely difficult.
Aren't you a bit exaggerating here? From what I see when I did some tests with a standard installation, we're talking about two additional queries run, giving a total of around 30 queries for a page with some article and modules. The queries itself are lightweight actually and run fast (around a half milisecond each).
One is
SELECT b.id
FROM #__user_usergroup_map AS map
LEFT JOIN #__usergroups AS a
ON a.id = map.group_id
LEFT JOIN #__usergroups AS b
ON b.lft <= a.lft
AND b.rgt >= a.rgt
WHERE map.user_id = 123
the other
SELECT id, rules
FROM `#__viewlevels`
I probably just fail to see how that will "break a large part of our userbase".
It slows down the site by some miliseconds and it may not be worth the added gain of instantely refreshing the permissions. It may also be worse if there are a lot of access levels and user groups.
But it will not break anything and not for a large part of the users imho.
So I agree with the revert in principle as the solution also brings troubles and the gain isn't that big to justify that. But I don't agree with the tone and pressure expressed here.
Imho there is no reason why we can't follow our rules of testing things prior to merge.
thing is, that those queries are executed each time those methods are called.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-11-07 10:31:50 |
I merged the revert part of this PR.
Thanks Hannes for the head up on this issue.
Please propose a new PR for your solution.
I used the code proposed on joomlacode;
Logged in frontend with a user assigned to registered, author and Editor (group_ids: 2,3,4 in _user_usergroup_map
Therefore when login with that user and the code above I get:
Then I edited the user in back-end and deleted the group editor and author (2 and 3)
Reloaded the page in frontend: no change