Caching and task to update list.

[micaherne]

I've added caching to the disposable emails plugin as promised.

The patch also includes a scheduled task to update the list once per day. This isn't a separate pull request as the way the caching works is quite tightly coupled with the refreshing of the list.

How it works is that the list is updated regularly in the Moodle filestore and the cache is cleared. For each email domain used in a signup attempt the actual list will be checked once and then whether that domain is a disposable one or not will be stored in the cache and used from there until the next refresh of the file. I thought this was a reasonable compromise compared with pre-loading the cache each time the list is refreshed, which is more complex and isn't something that tends to happen much in Moodle.

[daledavies]

Nice! 🙂

[micaherne]

After some fighting with the phpdoc checker this should be ok now :)

[daledavies]

Thanks for this, I'll take a proper look later once the kids are in bed but I'm wondering if we should wait for PR #22 to be merged and rebase your branch first as I made a few changes to this subplugin there. Using it as an example for implementing the new scoring method, catching any issues fetching the list and returning the fallback score etc.

[daledavies]

I can foresee a merge conflict when PR #22 gets merged, also that PR changes the way rule plugins return a rule_check_result , so I think we should wait for that PR first. But otherwise this looks great!

[micaherne]

Thanks Dale, that sounds like a good idea! I'll keep an eye on #22 and rebase once it's merged. It shouldn't be too much bother to resolve any conflicts I don't think. Most of the caching stuff is new files so I can sort out the changes to the ones that already exist.

[phager-at]

Hi Michael,

Great changes! Caching for this expensive tasks is awesome!

I've taken a look at it and wanted to ask, have you also considered moving cache invalidation to the method downloading the new file in lists manager? There it would be possible to dynamically react to download/connection failures, so we could keep the old file and cache if the new one couldn't be downloaded.

Also the cache itself could be moved to list_manager and get_blocked_domains() itself could first query the cache. Then we could also use a method is_domain_blocked($domain) to encapsulate the cache usage currently in the rule itself inside list_manager Then the outside world doesn't need anything to know about the cache itself. It will become an implementation detail.

That way every time the file gets updated we also purge the cache automatically and the scheduled task has to only call the method downloading the file.

[daledavies]

I'm assuming the cache would not get invalidated during the scheduled task if the download failed as list_manager::download_list() throws an exception, but I agree encapsulating all use of the cache in list_manager.

I guess something like move the following to line 135-136 of list+manager.php...

        $cache = \cache::make('registrationrule_disposableemails', 'blockedemaildomains');
        $cache->purge();

And then as you say move the cache::make() and $cache->get($domain) from rule into a new method in list_manager, maybe we call it get_blocked_domain() to fit with the rest of list_manager?