Moodle

Uhm...

it seems that ordering by ctx.depth ASC, ctx.path ASC, ra.roleid ASC solves the problem in get_user_access_sitewide(), because there is code included to prevent duplicates (the $lastseen checker).

But AFAIK load_subcontext() doesn't include such code to prevent duplicates, so the ordering solution won't cause any effect.

So I've added that prevention of duplicates in load_subcontext() and seems to be working here. Patch attached, can you please test it?

Ciao

Eloy,
I just tested the patch. Unfortunately I get the same ra as before. When you say "it is working here" how can you be sure that your example is one that would produce duplicates? I frequently see duplicates, but not always.

Well, because I went to my user profile page (with debug=developer enabled) and I saw the duplicates there (in some contexts).

Then I applied the patch, logged-out, logged-in and bump, the duplicates weren't there.

Ciao

My fault, I forgot to logout. Tried it again and it works. You're a genius! Thanks.

Thanks 4 feedback, John!

Eloy,
I hate to report bad news, but I just ran a scenario on my site (which has your patch) and got yet another duplicate ra. Screenshot attached.

This can be a big performance problem. I recently helped a user (who did not have your patch), and he had SIXTY duplicates in a single USER->access data structure.

Screenshot showing duplicate ra after Eloy's patch was applied.

Hi John,

I've been reviewing all the places where data is added to that "ra" array, and apart from the two functions fixed by the first patch:

• get_user_access_sitewide()
• load_subcontext()

There are some more additions to the "ra" array in:

• load_user_accessdata()
• load_all_capabilities()
• load_temp_role()

and these last three functions don't check if the "ra" exists previously, agree, but they seems to be used only for SYSTEM and FRONTPAGE and GUEST role assignments, and that doesn't seem the case of your screenshot.

Anyway.. perhaps we should, simply, change all those:

array_push($ra, roleid);

by:

$ra[$roleid] = $roleid;

with this, we get:

1) Speed, because assigning is faster than array_push()
2) Avoid duplicates, because, we are using the keys of the array.
3) The ORDER BY clause currently in code can be avoided, and that's more speed, for sure.

Please, comment. Ciao

Adding Petr and Martin Langhoff here to see if the last proposal can be good enough.

Ciao

Eloy,
I am away attending back-to-back Moodle Moots and will test your changes one week from today!

my +1 for HEAD commit, and later backport if everything works file during the next 3 weeks

Petr, I will test your changes today.

On 20/Jun/08 08:20 AM Eloy wrote...."

Anyway.. perhaps we should, simply change all those:

array_push($ra, roleid);

by:

$ra[$roleid] = $roleid;

...." but I don't see any such lines in lib/accesslib.php!

If I grep for array_push I get

$ubu[/../www/play/lib]: grep -n 'array_push(' accesslib.php
170: array_push($searchcontexts, $context->id);
1294: array_push($accessdata['ra'][$ra->path], $ra->roleid);
1456: // array_push($accessdata['ra'][$ra->path], $ra->roleid);
1457: // array_push($localroles, $ra->roleid);
1463: array_push($accessdata['ra'][$ra->path], $ra->roleid);
1464: array_push($localroles, $ra->roleid);
1614: array_push($accessdata['ra'][$base], $CFG->defaultuserroleid);
1628: array_push($accessdata['ra'][$base], $CFG->defaultfrontpageroleid);
1694: array_push($accessdata['ra'][$base], $CFG->defaultuserroleid);
1710: array_push($accessdata['ra'][$base], CFG->defaultfrontpageroleid);
1807: array_push($accessdata['loaded'], $context->path);
1813: array_push($accessdata['ra'][$context->path], $roleid);
3283: array_push($storedcaps, $cachedcap->name);
3615: array_push($records, $record);

Which of these do I change, and how exactly do I change them?

I've committed the patch above (accesslib.patch.txt ) to 19_STABLE and HEAD.

As commented by John, it fixes "some" duplicates but not all. In order to get this fixed forever I'd suggest to change the 'ra' array to be roleid indexed (instead of current array_push() approach, as commented in http://tracker.moodle.org/browse/MDL-14817?focusedCommentId=50927#action_50927

But need confirmation about that from somebody knowing internals better than me (not sure if that indexing will afect anything else).

Ciao

With all the changes performed both in 1.9.x and 2.0.x branches related to accesslib... I think it's better not to change this right now, so I'm moving this to Moodle 2.0, whatever it will be the new incarnation of "ra's" and friends we'll ensure there that there aren't duplicates anymore.

Also raising it to Blocker (although it isn't really a blocker, to have it present there).

Ciao

Petr, I've added you here as QA, feel free to switch to assignee if you think this is one thing that will fit in your roles/enrolments/permission clean for 2.0

Ciao

I'm assuming this either isn't that important any more or was fixed in recent role code updates.

Should be finally fixed, the 'ra' is containing (roleid1=>roleid1, roleid2=>roleid2) array now.
Thanks everybody!

Petr Skoda

Hi. Sorry Petr but I'm not getting an error which I think was introduced by this fix. If I view a course (ie /course/view.php?id=2) while logged in as guest I get the following error. The line producing the error was last modified as part of this issue. Accessing the course as an admin, teacher or students works fine.

Warning: array_combine() [function.array-combine]: Both parameters should have at least 1 element in /home/andrew/Desktop/repos/moodle/lib/accesslib.php on line 1566
Call Stack

1. Time Memory Function Location
   1 0.0011 245040 {main}( ) ../view.php:0
   2 0.1464 30360624 require_login( ) ../view.php:50
   3 0.1467 30363488 remove_temp_roles( ) ../moodlelib.php:2466
   4 0.1469 30365032 array_combine ( ) ../accesslib.php:1566

hmm, diagnosing, thanks!

should be fixed now, thanks for the report!