Moodle

adding watchers spam ping

Martin is happy for me to add this to the end of my Moodle 2.0 todo list.

Just linking this to MDL-16487 - which is a bug I created mentioning that the plugins check during install/upgrade is outdated. I'm pretty sure we'd need to improve that page a bit to make it more useful with all our plugin types though!

Note get_db_directories(MDL-19346) is using, since some weeks ago () get_plugin_types() and that should be considered the standard way to detects allowed paths everywhere.

Ciao

Note, get_plugin_types() uses different prefixes than string_manager for a few plugin types (e.g. mod/quiz/report) which is potentially confusing. Also, the lists in get_plugin_types() and string_manager include different types. This worries me.

I also think it should be moved out of adminlib.php, since It is probably needed in places where you don't need the rest of adminlib.php. I will probably move it to moodlelib.php if not one objects.

mod/quiz/plugin is not a good example - it is hardcoded "module subplugin" in one specific module, hopefully similar uses in resource and asignment will be eliminated soon.

anyway I agree this code needs a bit of cleanup and since it is me who introduced it working on solution now...

Petr, you asked for it

Also, can I make an additional request for a get_plugin_dir function that does

quiz -> mod/quiz
qtype_shortanswer -> question/type/shortanswer

and so on. Thanks.

key changes are:

• function get_plugin_types($fullpaths=true) now returns full paths by default
• function get_plugin_types() supports module subtypes - no more hardcoding of quiz reports and friends
• new function get_plugin_directory($plugintype, $name, $fullpaths=true)
• new function get_plugin_list($plugintype, $fullpaths=true)
• normalized component values (aka full plugin names) in capabilities, events and messaging tables
• fixed assignment and quiz plugin names
• fixed savepoint version bug
• get_string() separation of full reports and course reports
• plugin support everywhere
• local/ support completely replaced by plugins in /local/xxxx/*

This code is not tested much,going to test it more today and fix regressions

More info retated to local customisations at http://moodle.org/mod/forum/discuss.php?d=126017 and http://docs.moodle.org/en/Development:Local_customisation#Local_plugins_in_2.0_proposal

Code review comments from looking at the patch

• http://docs.moodle.org/en/Development:Coding_style#Foreach shows space around => in foreach loops. (foreach ($modules as $module=>$moduledir)
  Unknown macro: { should be foreach ($modules as $module => $moduledir) {) * Instead of changing {code} $mods = get_list_of_plugins('mod'); if (!in_array(strtolower($modulename), $mods)) { {code}
  to
  

  $mods = get_plugin_list('mod');
  
          $mods = array_keys($mods);
  
           if (!in_array(strtolower($modulename), $mods)) {

  
  you could do
  

  
  $mods = get_list_of_plugins('mod');
  if (!array_key_exists(strtolower($modulename), $mods)) {{code}}

• Or, is it worth making a plugin_exists($type, $modulename) function? (No need to get a list of all plugins if you just want to know whether one exists.)

• Similarly, you could write function get_plugins_with_file($type, $filename), so you can say things like get_plugins_with_file('local', 'cron.php'); get_plugins_with_file('gradeimport', 'lib.php'); Would let you delete if(file_exists(...)) code from lots of other places.

• case FEATURE_MOD_SUBPLUGINS: return array('quiz'=>'mod/quiz/report'); is really nice!

• Surely we don't need the if in

  if ($mods = get_plugin_list('mod')) {
  
          foreach ($mods as $mod=>$moddir) {

  I hope get_plugin_list('mod') returns an empty array when there are none.

• @param string $component full plugin name, examples: 'moodle', 'mod_forum'

'mod_forum' is not consistent with how get_string has always worked.

• get_cached_capabilities, the new logic here looks broken. Oh, I see, will you force a complete reload of the table, so the contents of the component field is updated for all rows?

... OK, am up to line 815 of the patch, and need to take a break. More later.

• coding style - oki, going to change my style
• mod - I did not want to do major refactoring at the same time, sometimes I resorted to array_keys - lazy and greater danger of regressions - but in general I agree
• plugin_exists() - yes good idea
• get_plugin_with_file() - yes, good idea
• mod always exist - the same as $mod above
• component string were previously "mod/forum", I personally hate the missing "mod_" prefix in string, tables and everything because it may cause collisions
• get_cached_capabilities was buggy in fact because it did not use component string, you could not define mod/forum:something outside of forum - local hooks

hmm, I think we could standardize the "component name" - moodle, mod_forum, block_something (we could add automatic conversion of forum to mod_forum for BC reasons)
so instead of get_plugin_directory($type, $plugin, $fullpath) would be get_component_directory($component, $fullpath);

get_plugin_types($fullpaths=true)
get_plugin_list($plugintype, $fullpaths=true, $withfile=null)
get_component_directory($component, $fullpaths=true)

hmm, we could have both - get_plugin_directory() and get_component_directory() which accepts 'moodle' and unittest hooks as extra features

• It seems that the old get_list_of_plugins is still there. Why? (Backwards compatibility, I guess.)

• Wow! locallib.php is completely gone. Is the information from all the useful comments preserved somewhere (in updated form)? I guess it should be on moodle docs.

• $plugs = get_plugin_list($type); I would prefer the variable name $plugins, but I guess the above comment about refactoring applies.

• javascript-mod.php is a bit of a nasty hack. I am hoping to phase it out and let people use $PAGE->requires instead. (Should be better performance than doing is_readable on a file in about a 100 different folders. Therefore, can we leave this doing only mod, block and filter for backwards compatibility.

• Diffstat says:
  48 files changed, 467 insertions, 763 deletions
  300 lines of code saved. Very nice.

In summary, this is brillant. Please get it finishded, tested, and checked in soon

Of the above comments, the only one I feel strongly about is the javascript-mod.php one. Thank you.

• get_list_of_plugins() is there because it is used for lang and otehr custom types, we need it for BC too - those are not true plugins
• yes locallib info needs to be stored somewhere, locallib.txt ?
• yes, longer names are better

1. Ah, so now get_list_of_plugins is needed, but has a confusing name. Please make sure the PHPdoc comment explains.
2. Martin suggested local/readme.txt. Seems sensible to me.

You did not comment on the javascript-mod.php bit. I hope that means you agree.

1. sure - doing
2. adding local might cause problems in CVS updates, could it? I intentionally did not put anything into "/local/", I guess we should ask GIT people too

javascript - sure, I trust you completely :-D

I think becuase of the way CVS only tracks files, adding local/readme.txt will only cause problems for people who already have a file called exactly that.

ok, great, going to add it there, we can revert it anytime later if really needed, thanks

I prefer when all up-to-date info is in cvs including this local customisation info

committed into cvs, thanks a lot everybody
please test and report any problems

thanks again

Yay! thanks Petr.

Petr, this doesn't seem to work with the assignment module.

In mod/assignment/lib.php line 3140, you use

$assignmenttypes = get_plugin_list('assignment');

This always returns an empty array, no matter what is in mod/assignment/type

Delving a bit deeper, in lib/moodlelib.php lines 6841-6847, we see that there is possible way that get_plugin_list('assignment') could return anything, no matter was exists in mod/assignment.

Nico, there is a possible way, but it is commented out. Lines 6791-6801.

I wonder why Petr commented that out?

testin get_plugin_list('assignment'); ...

In relation to MDL-20152, I wonder if we should add something like gformat for glossary formats to the get_plugin_types function. I am thinking along the lines of:

'gformat' => 'mod/glossary/formats',

Peace - Anthony

Anthony - I would suggest using "glossaryformat" plugin type. What would you guess a "wgrading" means? IMO "workshopgrading" is better (self-explanation), even longer.

moodle_needs_upgrading() ignores subplugin requirements. IMO the subplugin's version.php should specify the minimal (and optionally maximal) supported parent plugin version. So the logic is: subplugin declares the required plugin's version, the plugin declares the required Moodle version.

David - I agree I like glossary_format better but was trying to be 'consistent' with qformat which I am not a fan of. Generally speaking we do a very good job of keeping the code readable. I thought gformat was closest to qformat (question) but I believe I mentioned this previously to someone (perhaps Tim) and was told that it would be difficult to change. In any case, I just wanted to make a plug for making the glossary formats as plugin friendly as possible. Peace - Anthony

Anthony - use glossaryformat with no underscore. The underscore is used as a separator between plugintype_pluginname. So for example mod/glossary/formats/plain can be refered as glossaryformat_plain.

David - Yep, I see why glossaryformat works better - good catch. Thanks and congrats on working with HQ! Peace - Anthony

This current implementation of FEATURE_MOD_SUBPLUGINS is causing major problems in my new get_string() implementation, there are also other place where the code might get into infinite loop.

Going to add new mod/xx/db/subplugins.php file which is used in stead of lib.php - this should solve all current and potential problems...

reimplemented the subplugin support, closing
please file separate issues for glossary if necessary

oops, my problems were caused by my new code, but I still like the changes I did and it could save some memory too because we do not have to load all the lengthy lib.php's on every page.