User tests: Successful: Unsuccessful:
@nikosdion I think this needs also SQL commands so the new plugin is installed automatically during installation and update.
Will have to go into Joomla 3.4.
@Bakual @infograf768 I have updated the PR with the missing items per your comments.
Merged to 3.4-dev
Status | New | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-08-03 17:29:25 |
This PR unfortunately is the worst possible solution for this issue. I understand what you are trying to do, but first of all, the 2 plugins that you are introducing are useless and second of all, this will introduce so much performance problems that it might bring down several sites.
What you want to achieve is the reloading of the usergroups and viewlevels of a user in the user object when they might have changed. That is simply achieved by removing the ifs around line 430 and 450 in /libraries/joomla/user/user.php. This means that it will load that data on each pageload on each call to those respective methods. I haven't checked how far JAccess caches the data per pageload. The respective methods could be replaced by return JAccess::getAuthorisedViewLevels($this->_id).
So we are introducing a lot of additional load.
At the same time, the whole stuff of the plugins to load the session data and somehow modify it and all that, is completely unnecessary.
I'm sorry, but this change does not fix the issue in an acceptable way. I humbly ask you to revert this.
@nikosdion
Could you look into this, please.
Since this is not @nikosdion s code, I would ask a core maintainer to take a look at this.
Please note that I am NOT the author of the PR. I was asked to edit the code to make sure it can be applied to the then current branch.
If you ask me, there in no proper solution which doesn't cause significant performance issues due to the way Joomla!'s ACL system works. The only change you can catch without a major performance impact is when the assigned usergroups to a user have changed: load the list of the assigned user groups for the current user on each page load; if it's different than the cached list, invalidate the ACL privileges cache and repopulate it.
However, this doesn't help when you add/remove user groups to access levels. The list of groups assigned to each user is the same, but the authorised access levels have changed. If you are on a site with a reasonable number (less than ~100) of View Access Levels you can reasonable reload them on each page load, exactly as @hackwar said. But if you have a ton of access levels and / or user groups this process takes forever and a significant amount of memory. That's why there is the caching currently in place.
It all comes down to the ACL system design. We can't change it. So we have to decide which compromise solution we want. Do we want a hack which can be disabled (plugins)? Do we want a partial solution which has a small performance impact (check list of user groups per user)? Or do we go for a proper fix with a potentially catastrophic performance impact (reload ACLs from scratch on every single page load)?
If anyone has a fourth solution which is all inclusive and has no performance impact, please do share.
@nikosdion The fact is, that we currently have the fourth solution. ALL ACL is reloaded on each pageload.
That's the third solution :) I agree it's bad for performance on big sites. As I understand it (without going deep into the code – sorry, no time today) having it as a plugin is a tad slower but it does let you disable this feature if your site can't cope with the extra load. I might be missing something important. If so, feel free to tell me where I'm wrong. As I said, I have no time to dive into the code today, I'm neck-deep into refactoring something.
As I wrote earlier: You are loading viewlevels and usergroups each and every time that the methods are called. This introduces at least 2 additional queries per pageload. That part is NOT possible to disable via the plugin. I don't have a different solution right now, but this solution is not acceptable.
Oops! That's a major issue, you're right. The PR should be reverted.
Is there any way we can fix this without reverting the full PR?
@Committer
if this is reverted, please also revert the administrator/language/en-GB/install.xml where the new plugins ini files have been added
No. The basic idea on how this PR tries to fix this is not going to work. If we want to do it, we have to do this a completely different way.
For situations like with subscriptions that @nikosdion is aiming at, we could include a method named "invalidateUserACL" or something that clears all caches. But that will only work for the currently logged in user. For changes in the groups, ACL or viewlevels, you wont get around logging out all current users.
I think that's acceptable. I think the major issue is to make sure if you add a user group to a user on some kind of subscription/payment etc. then currently they need to log out and log back in again. Which is not something that a user would expect generally.
I'll provide a PR
As i stated in my initial post of the initial PR i felt
unsure if this solution is stable or might pull up new issues.
and
Ideas and impressions are highly welcome
I was aware of the introduced repetitive extra db-queries on every page load. But i couldn't imagine another solution to bypass the caching and force the user state refreshing and thus the detection of the state change. But i didn't consider the impact on the overall page performance would be that hard as described by @Hackwar. The much i'm glad i could pull the awareness of other, more experienced Joomla developers onto this topic and see the discussion and coding for this topic going on.
Thanks Hannes for taking over. I'm exitedly looking forward to see the final solution for this essential topic.
Please, when you make a change in one of the lib_joomla.ini, always do it also in the other one.
Here the same changes have to be done in administrator/en-GB/en-GB.lib_joomla.ini