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

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

Ciao

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.

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

Ciao

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?

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

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

Ciao

Ciao