Moodle

ML have you any time to think about this?

Argh. I don't think we can do this without some refactoring that touches the callers. The good news is that there are only around 40 callers in the code (git grep get_users_by_capability tells me there are 46, but at least 4 of those are comments).

I thought we'd be able to wing it for 1.9 – this is one of the few places where I did not change the 1.7/1.8 API and hoped for the best. Bad judgement I guess.

Forgetting for a second the impending 1.9 release, my best fix for this (and a few similar cases in accesslib) is as follows --:

1 - change how we do recordsets across moodle – and use an OOP recordset approach. So get_recordset() and friends return a recordset object, and instead of rs_fetch_next_record($rs) we do $rs->fetch_next() or just $rs->next()

2 - make functions like gubc() return an object that offers the same methods that a recordset. So in this case, we'd say return get_recordset() and for the other cases we'd return an trivial object wrapped around the array of results we have

If we forget for a second the massive change I propose in #1, we can probably use that strategy for this. Declare a trivial object that we return from this function, one that implements at least count() and next() methods, masking whether it's working over a rs or an array. So we can change the callers to do

$users = get_users_by_capability();
if ($users->count()) {
while ($u = $users->next()) {

and not care if they got an array we computed in memory, or a $rs that doesn't actually fit in memory. I think that no callers to gubc() try to access the returned set by key (needs to be checked!).

This approach could be used in many places in accesslib and core Moodle, and lets the core team improve the memory handling "behind the scenes" as we get better at answering hard questions with pure SQL, without ever loading everying into mem.

There are acouple of other alternatives, like having a special return value that means 'all', but it goes back to changing all callers. I can't see any alternative to that, so if we have to do it, let's do something like this – I think it is the right fix. And for Moodle 2.0, we are already talking about a more OOPish DMLlib, so making recordsets OOP fits right in...

Let's forget for a 3rd second... nah.

A couple more notes:

> just return those people who not only have the capabilty for that activity but also moodle/course:view for the relevant course context.

this makes some sense (but should we extend it to all cap checks at courselevel? I don't think it is a good general assumption) - but it could still be that everyone has moodle/course:view – normal for the sitecourse!

And I cannot say if I have time to sort this out soon. Seems hi priority, so I'll treat it as such, but I'm between jobs/projects...

I'm not sure now how urgent this is. We seem to be getting along OK as things are, and the new roles stuff in 2.0 will probably replace this logic.

That's the problem of assigning permissions to those "default" roles (guest, authenticated...). Just one more example of how "automatic" role assignment can lead to problems IMO. Perhaps we should be able to define some roles as not-editable or not-considered or not able to handle perms below the course level or whatever.

-1 to add harcoded exceptions in perms calculation (both in 1.9 and HEAD).
+1 to enforce correct configuration (by forbidding some things that shouldn't be modified ever).

I think we have to delay this a bit, because there is not enough time to test and code, sorry

I found this issue whilst looking for a fix to CONTRIB-1931 - it appears to be related, but potentially more of a problem.

As soon as the 'get_users_by_capability' call includes a group as well as a capability that is included in the default role, then the following SQL is generated:

SELECT u.id FROM mdl_user u WHERE u.deleted = 0 AND (ra.userid IN (SELECT userid FROM mdl_groups_members gm WHERE gm.groupid = 96)) ORDER BY u.lastaccess

This fails completely as 'ra.userid' is undefined.

Finally solve in 2.0, thanks!