We had a 1s race condition where a user could get their rights loaded
at the exact time an admin is changing roles/caps and see the "old"
data. Or even see a half-updated view of the access controls.
Yuck.
So we fix the race condition backdating the dirtyness. Cheap, but
effective. And then we backdate it some more to cover for minor clock
flutter on clusters (you still need ntp however!).
has_capability() can handle the fake user that forum cron sets up
and will load the appropriate accessdata into $USER->access.
This makes forum cron work again. A test comparison between before
this patchseries yields:
With 1 forum post, sent total 24 times
- Before 11 000 DB queries (approx)
- After 506 DB queries
With 6 forum posts, sent a total of 452 times
- Before 47 876 DB queries
- After 8 256 DB queries
There is a very high variability, but we are going from 100-500
queries per sent email to 18-21 queries per sent email. The
variability probably stems from 2 of the 6 posts being in a 200-user
forum.
Still huge - by the time we are sending the email, we should know
everything we need to know about the user, the forum/thread/post and
the form. The average should be well below 1 DB query per email sent!
print_user_picture() was forcing an unneeded dbquery
if you need an imagealt. And who doesn't need one these days.
So - teach print_user_picture() to take either $userid
_or_ $userobj as the first parameter. If that first
parameter has the fields we need, never touch the db.
In other words, only touch the DB as a last resort.
There is a bit of ugliness in testing whether we have
the fields or not, because these fields are inconsistently
with/without NOT NULL in the DB definitions. So we cannot
use isset() because it barfs on nulls. And we cannot use empty()
because it will match both on "missing key" and ''.
And while at it, silence warnings that we are missing string
for the year(s). Also fixes a missing string bug in really boring
courses that noone's visitied in many years ;-)
Reworked the logic of the visibility checks so that we evaluate in a
chain:
- can view course (visible or user can see hidden)
- can view category (visible or user can see hidden)
Without this patch, users that could see hidden categories could not get
into courses inside of them.
While at it, fix reference to the old $USER->switchrole
Reworked course_parent_visible() to always return in a constant
number of db queries (2 worst case) regardless of nesting depth.
The rewritten version has a small cache, but if you are going to
walk many courses, it's still 1~2 DB queries per category seen,
so the right thing to do is to check it in the caller, as seen
in get_my_courses().
Reworked gmc to perform the course visibility checks. These are
very cheap if $CFG->allowvisiblecoursesinhiddencategories is true.
However, where we have to enforce category visibility, it adds a bit
of work. In simple terms, it adds a DB query to read all the categories,
and extra checks to make sure we are doing the right thing WRT
- course visibility vs the permission to see hidden courses
- category visibility vs the permission to see hidden categories
and still do it quickly.
Costs next to nothing according to testing, and allows us to walk
the categories very cheaply. We'll need this in get_my_courses()...
What a cheapskates I am...
If you are a teacher in course X, you have at least
teacher-in-X + defaultloggedinuser-sitewide. So in the
course you'll have techer+defaultloggedinuser.
We try to mimic that in switchrole.
Thanks to Petr for pointing me to a similar fix in CVS.
Probably related: MDL-10945
We had quite a bit of leftover rdef and ra mangling.
Be more thorough and clear it up.
While at it, make load_user_accessdata() and load_all_capabilities()
more consistent.
There are some exceptions when checking for caps that are inherited
from the default role. Move the check into has_cap_fad() and stop
mangling the data we put in $ad[rdef].
We now also set $ad[dr] to record default roles added.
This will later allow us to share rdef across many users in $ACCESS.
Affects:
load_user_accessdata()
has_cap_fad()
While at it, document has_cap_fad() a bit.
When setting things up for the guest user, the RA entry in accessdata
was not multi-enrol-friendly. Must have glossed it over in the
multi-enrol rework.
The name for new data structure holding access control data
is "accessdata". And we have a new moniker "fad", short for
"from accessdata".
So
- has_cap_fromsess() -> has_cap_fad()
- access_inaccessdata() -> path_inaccessdata()
- aggr_roles_fromsess() -> aggr_roles_fad()
- $sess -> $ad
- $access -> $ad
Consistency, save typing, shorter codelines...
With the new accesslib, moving courses and categories has a major
impact on enrolments and unenrolments.
At _least_ we need to signal accesslib that it has happened. So here
is context_moved() for exactly that.
Open to refactoring later into something along the lines of
- move_course()
- move_category()
However, at this stage the most important of those two: move_course()
does not fit very well with the code in course/edit. So keep it simple
for now.
... and it populates the context cache too.
Unfortunately, it needs an INSERT followed by an UPDATE. Other than
a stored procedure, I don't know how to deal with this better.
(We could save the SELECT though! that's a thought...)
OTOH, we are getting so much mileage out of the path field
that it's probably a hit we have to take in the chin and move on.
Callers _must_ do their homework before calling create_context().
This allows us to save 2/3 queries per call (!!!).
As it stands, callers are all in accesslib anyway.
Manually enrolling and unenrolling self, and other users should
transparently set the context dirty. So walk all callers to
role_assign() and role_unassign() and mark the context dirty
where appropriate.
OTOH, most automated-backend enrol/unenrol mechanisms should not.
The backend lookups that happen when you login are well covered
by the login/enrolment process, and don't need to be marked dirty.
If accessinfo is stale, we need to reload it without losing
out "interesting" state -- transparently for the end user.
That means preserving active role switches, loginas (site and course
level), etc. The logic for that is encapsulated in
reload_all_capabilities().
Also affected:
- has_capability() which now calls reload_all_capabilities()
- role_switch() - minor tidyup
The refactor that created make_context_subobj() triggered a bug.
Smack in the hand to the sloppy programmer using variables outside
of the context they were meant to be used in!
has_capability() now (ab)uses a new global: DIRTYCONTEXTS where we
load the contexts that have changed since since $USER->access[time]
The shallow/easy checks are done in has_capability(), deeper checks
go into the newfangled is_contextpath_clean().
The only complication now is to reload the relevant caps without losing
switches, loginas, etc...
The accessinfo held in $USER->access can easily get out of
sync with reality if and admin has removed our access,
or expanded it after we loaded our accessinfo.
To handle this, we'll use the config_plugins table with an
'accesslib/dirtycontexts' plugin signature to store the paths of
recently changed contexts. To handle those dirrrty entries, here
we introduce
get_dirty_contexts() - for lib/setup
mark_context_dirty()
cleanup_dirty_contexts() - for cron
Now get_role_users() joins with contexts and roles too, so we can
push more work into it, and simplify the callers.
One important change is that the $view flag gets reworked into
$gethidden, pushing the cap check to the caller.
This commit is followed by a cleanup of the callers...
