Design of a pluggable plugin container and a basic report plugin

Hi Ankit,

have had a really quick look at this - good start!

it's missing the code from mod/scorm/db/upgrade.php to create the new table - install.xml is only used for fresh installs.

the mod.php file isn't really needed - all SCORM reports should use a standard method for display - by calling: /mod/scorm/report.php with the mode set to the report plugin name - like you've done for the basic plugin.

If we want to allow plugins to override this and use a different url we could add a function to the class that allows a plugin to override the location - but lets leave that out for now.

in report.php:
foreach ($reportlist as $reportobj)

should be:
foreach ($reportlist as $reportobj) {

if($mode!=NULL)

should be:
if (!empty($mode)) {

few more comments to come later.

Hi Dan,
Mod.php is also responsible for the display of the Anchor text..and also it can allow plugins to be displayed as per their custom capability.For example a plugin can provide its own "capability" and based on that the tab representing that plugin might be visible to some users and invisible to others while scorm/report.php is loaded..So will it be okay to keep the mod.php ? or still its better to remove it?

I will also look on the other stuff you mentioned and make changes accordingly.

Thanks

I thought the capability was going to be managed in the scorm_report table? - like done in quiz_reports - I think it uses the scorm/reports/plugin/install.php to manage that in quiz - see quiz/report/statistics for an example.

my vote - remove mod.php - thanks!

NOTES FOR INTEGRATORS:
MASTER only please - this should not go in stable branches.
The basic plugin is basically a copy of the existing report in Moodle - any bugs with the code structure that are present in existing code can be classed as "new" bugs - (feel free to create them if you spot issues)

I'd like to move the plugin away from using "echo" style statements and use it's own renderer functions but this can come later and shouldn't hold up this patch from being integrated.

Hi Dan, Ankit,

nice to have proper report subplugins here, super!

Anyway, I think this needs a bit more of work before landing, hence I'm reopening:

0) One recommendation: Please don't use quiz as reference, lol, it was one of the very first implementations and I'm not sure it's the best one. I'd look to workshop instead.

1) the subplugin name. "scorm" is NOT a good name at all (sure it's because you copied from quiz, where that name (the module one) was picked. Instead use "scormreport" or something unique. Imagine tomorrow you'll want to create another subplugin for displaying one scorm, it will be "scormdisplay" (just one silly example). Simply don't use the name of the module to name on subplugin. See workshop.

2) The get_reports_list() function. Please don't transverse directories there at all, instead use the get_plugin_list('subpluginame') function that will return you a better list. Then apply for cap checks or whatever you need.

3) Capabilities needed for each report?

4) Add them in the list of "core" plugins, so they will show properly (and not as 3rd part) on upgrade.

These are the most important points I've detected looking to this. Please fix them, I'll be happy to take a more deeper review then.

Great work, ciao

Thanks Eloy -
1 good point about the plugin name
2. whoops - we were using get_plugin_list at some point but that function has been through a few changes and I forgot about it.
3. yeah - each report should be able to define it's own capabilities and restrict access if required. (quiz reports do this using the nasty mod.php file.) - but, I've just taken another look and /scorm/report.php requires mod/scorm:viewreport so we don't need to implement that check in the initial "Basic" plugin - Ankit we should just remove the canview function from the basic plugin class - the default class returns true and we know the user already has scorm:viewreport - hope that makes sense, ping me on jabber if not.
4. isn't this what the subplugins.php file is for? - or do we need to put them into the main core list?

Ankit - can you please fix points 1-3 above (make sure you grab the latest code from my repo) - thanks!

There is a tiny cosmetic problem with phpdocs - your new plugins should use
@package scormreport
@subpackage basic

I am going to fix it myself.

4/ yes, the subplugins.php is the right place to add module subplugins - if you are not sure always look into mod/workshop...

Integrated, thanks!

I was getting some noise from the userreport.php:
Invalid get_string() identifier: 'cmi.core.lesson_location' or component 'scorm'
line 5949 of /lib/moodlelib.php: call to debugging()
line 6539 of /lib/moodlelib.php: call to core_string_manager->get_string()
line 322 of /mod/scorm/userreport.php: call to get_string()
Invalid get_string() identifier: 'cmi.interactions_0.id' or component 'scorm'
line 5949 of /lib/moodlelib.php: call to debugging()
line 6539 of /lib/moodlelib.php: call to core_string_manager->get_string()
line 322 of /mod/scorm/userreport.php: call to get_string()
Invalid get_string() identifier: 'cmi.interactions_1.id' or component 'scorm'
line 5949 of /lib/moodlelib.php: call to debugging()
line 6539 of /lib/moodlelib.php: call to core_string_manager->get_string()
line 322 of /mod/scorm/userreport.php: call to get_string()

but I guess that would be a separate issue.

Tested, works as described. Thanks.

Hi Skodak,
Thanks for the integrations and comments.
About the errors you mentioned..I have already created MDL-27624 for that.
Will be patching that soon.
Thanks

wow, that was fast! thanks

There is one more issue, the new basic report is not included in the list of standard plugins yet (it is displayed as non-standard plugin during install/upgrade). There is a list of standard plugins somewhere in moodellib.php (or is it admin lib?)

I guess you mean standard_plugins_list in pluginlib.php - I'll create a new bug for that and push through a fix - thanks.

have created MDL-28441 to track this - probably won't get to it in time for this weeks review.

Ah, right, that one. It is only a cosmetic issue in dev branch, no worries.