caching_optional_param() ?

caching_optional_param() ?

by Dan Poltawski -
Number of replies: 6

I was going through some old bugs and came across MDL-1915, which is basically about not loosing location in pagination/sortorder of user browse list when using it. Which is really one of those small things which would make the list feel much more usable - (I know i've got irritated by before).

The sort orders and pageination on these kinda of pages are usually GET params which could be cached to make things much more usable, so I quickly hacked up a wrapper function to optional_param which caches the value in our session:

function caching_optional_param($paramname, $default, $type, $cachename){ global $SESSION; if( isset($SESSION->{$cachename}) ){ $default = clean_param($SESSION->{$cachename}, $type); } $SESSION->{$cachename} = optional_param($paramname, $default, $type); return $SESSION->{$cachename}; }

Slightly modify the param defintions and it works as planned, we can browse through users with filters, edit our user and return where we left off:

$sort = caching_optional_param('sort', 'name', PARAM_ALPHA, 'userlist_sort'); $dir = caching_optional_param('dir', 'ASC', PARAM_ALPHA, 'userlist_dir'); $page = caching_optional_param('page', 0, PARAM_INT, 'userlist_page' );

So what do you think about that?

Presumably the new gradeboook has faced lots of issues like this for improving the usability, how has it been achieved there?

I know MartinL is going to mention about the performance problems of serialize with lots of $SESSION abuse. So how should we improve things like this without polluting $SESSSION?

Average of ratings: -
In reply to Dan Poltawski

Re: caching_optional_param() ?

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
I think this is a horrible solution. A much better solution is to use the moodle_url class that Jamie Pratt made in Moodle 1.9.
Average of ratings: -
In reply to Tim Hunt

Re: caching_optional_param() ?

by sam marshall -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers
Although I do agree with Tim's opinion, he was a bit harsh there. smile

(Normally it is my role to be nasty and sarcastic, not Tim's. However, I pretend to be nice in public! Er, sometimes.)

I think in general, anything that you can possibly NOT store in the session should not be stored in the session. Remember that sessions play havoc with tabbed browsing and generally break various web assumptions - e.g. you bookmark the page, come back to it later, and whoops! it's different.

If it can easily be passed using ordinary URL parameters (i.e. it doesn't absolutely need to be preserved when you go to a completely different page and then follow another link and arrive at the first feature again), it should be. There are many parts of Moodle where things are stored in the session which shouldn't be, so I don't think it should be made easier.

I didn't know about the class Tim mentioned so I got him to explain it to me smile It basically lets you store a bunch of parameters and then convert this to a string or to form hidden fields in other parts of the code. It sounds very useful - I have code (but not a nice class) which does exactly the same thing, only specialised, in ouwiki.

--sam
Average of ratings: -
In reply to sam marshall

Re: caching_optional_param() ?

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Yes, sorry. I was having a bad day. I shouldn't have let it out in the forums. The moodle_url class is defined in weblib, and I think that the code it pretty self-explanatory, and also there are PHPdoc comments. The only place it is used so far is in the question bank code, so look there for examples.

Where you have settings that you want to persist for longer than what you can do by keeping things in the URL, then there are the get/set_user_preferences methods (I hope I remembered those names right), which actually store per-user settings in the database. These are used in the new gradebook code for some of the settings. (Also, in formslib, to remember whether you like advanced mode on or off for each form.)
Average of ratings: -
In reply to Tim Hunt

Re: caching_optional_param() ?

by Dan Poltawski -

For what its worth I didn't think it was a particularly good solution myself, which was why I was interested in how the gradebook had dealt with it ;) Having just looking around the new grader report it seems like sessions have been [ab]used there too ;)


Where you have settings that you want to persist for longer than what you can do by keeping things in the URL, then there are the get/set_user_preferences methods (I hope I remembered those names right), which actually store per-user settings in the database.

Trouble is, pagination/sorting and such like is not really long-term enough to store in a user preference, but some operations will send you 'quite far away' from the original page, and the return url could have to be sent here there and everywhere.

But its also probably too short-lived for the session also (you can imagine users being confused by starting to browse at page 6 of their userlist and wondering where the rest have gone).

Average of ratings: -
In reply to Dan Poltawski

Re: caching_optional_param() ?

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
It think number of records per page is a good candidate for user_prefs. The database module does this. Notice the way the Modules and Plugins database remember remember the option you set for this between logins.

The current page number definitely belongs in the URL. If you go a long way away and come back, it is quite reasonable to end up back on the first page.

In between these two, you can probalby construct nasty border-line cases where the best option is not clear.
Average of ratings: -
In reply to Dan Poltawski

Re: caching_optional_param() ?

by Martín Langhoff -
/me: session abuse is bad, bad, bad! wink

Not just performance - we can perhaps use hidden form vars for this, and that means that if you open multiple tabs with the same "browsing" page they won't overwrite eachother.

Session-based tricks have this problem - and I think it results in a bigger bug than the one you are trying to solve.

(Haven't seen yet the fix Tim mentions, though.)
Average of ratings: -