? Success

User tests: Successful: Unsuccessful:

avatar nikosdion
nikosdion
26 Jul 2014

Joomla! Issue Tracker #33293

This PR is the same as PR 3126 submitted by @itbra It has merely been updated to apply against the current staging branch.

avatar nikosdion nikosdion - open - 26 Jul 2014
avatar infograf768
infograf768 - comment - 26 Jul 2014

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

avatar Bakual
Bakual - comment - 26 Jul 2014

@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.

avatar nikosdion
nikosdion - comment - 26 Jul 2014

@Bakual @infograf768 I have updated the PR with the missing items per your comments.

avatar mbabker
mbabker - comment - 3 Aug 2014

Merged to 3.4-dev

avatar mbabker mbabker - change - 3 Aug 2014
Status New Closed
Closed_Date 0000-00-00 00:00:00 2014-08-03 17:29:25
avatar mbabker mbabker - close - 3 Aug 2014
avatar Hackwar
Hackwar - comment - 4 Nov 2014

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.

avatar infograf768
infograf768 - comment - 4 Nov 2014

@nikosdion
Could you look into this, please.

avatar Hackwar
Hackwar - comment - 4 Nov 2014

Since this is not @nikosdion s code, I would ask a core maintainer to take a look at this.

avatar nikosdion
nikosdion - comment - 4 Nov 2014

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.

avatar Hackwar
Hackwar - comment - 4 Nov 2014

@nikosdion The fact is, that we currently have the fourth solution. ALL ACL is reloaded on each pageload.

avatar nikosdion
nikosdion - comment - 4 Nov 2014

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.

avatar Hackwar
Hackwar - comment - 4 Nov 2014

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.

avatar nikosdion
nikosdion - comment - 4 Nov 2014

Oops! That's a major issue, you're right. The PR should be reverted.

avatar wilsonge
wilsonge - comment - 4 Nov 2014

Is there any way we can fix this without reverting the full PR?

avatar infograf768
infograf768 - comment - 4 Nov 2014

@Committer
if this is reverted, please also revert the administrator/language/en-GB/install.xml where the new plugins ini files have been added

avatar Hackwar
Hackwar - comment - 4 Nov 2014

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.

avatar Hackwar
Hackwar - comment - 4 Nov 2014

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.

avatar wilsonge
wilsonge - comment - 4 Nov 2014

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.

avatar Hackwar
Hackwar - comment - 4 Nov 2014

I'll provide a PR

avatar Hackwar
Hackwar - comment - 4 Nov 2014

The new PR can be found here: #4991

avatar itbra
itbra - comment - 5 Nov 2014

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.

Add a Comment

Login with GitHub to post a comment