Joseph Rézeau added a comment - 31/Jan/13 8:57 AM BUMP

Mary Evans added a comment - 31/Jan/13 9:52 AM @ Joseph Rézeau Did you add me as a watcher as I don't recall seeing this tracker ticket before? I think the problem is in the way the renderer has been written I think it should be written like so... /** * A custom renderer for the mymobile theme to produce snippets of content. * * @package theme * @subpackage mymobile * @copyright John Stabinger * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */   class theme_mymobile_renderer extends mod_choice_renderer {   /** * Produces the settings tree * * @param settings_navigation $navigation * @return string */ public function settings_tree(settings_navigation $navigation) { $content = $this->navigation_node($navigation, array('class' => 'settings')); if (has_capability('moodle/site:config', context_system::instance())) { // TODO: Work out whether something is missing from here. } return $content; }   The fact that John has written a TODO suggests to me that this is experimental or he wants to see if it can be added to, which is fair enough in the circumstances. Let's face it, renderers are pretty much the "new kid on the block" as far as theme designers are concerned, and not easy to understand.

Joseph Rézeau added a comment - 31/Jan/13 9:55 AM Thanks for the reply, Mary. Yes I had added you as watcher. Any idea why John is not responding?

Mary Evans added a comment - 31/Jan/13 10:11 AM - edited Just looking at this and finding it difficult to locate where public function settings_tree has been derived from. If the renderer does not exist then it cannot be created in a theme. So this looks like it's flawed in some way. Although it is more likely I am reading this the wrong way.

Mary Evans added a comment - 31/Jan/13 10:15 AM Not sure where John is at the moment, so can't answer that Joseph. He might not be getting any notifications.

Joseph Rézeau added a comment - 31/Jan/13 10:18 AM It certainly is a flaw for a theme to have a renderer which is an extension of a Moodle module . I expect this was only a provisional attempt and really the myMobile theme should include its own complete renderer.

Mary Evans added a comment - 31/Jan/13 10:20 AM - edited @ Tim Hunt I've just added you as a watcher here to ask for some help with the renderer in question. It sort of similar to what I was asking in Dev Chat yesterday, but not the same renderer I was talking about. This one is pretty new to me. So if you have a minute to spare to take a look, I would be glad of a second opinion. Cheers Mary

Mary Evans added a comment - 31/Jan/13 11:10 AM - edited Just looking at this again and as I suspected I was reading this render wrongly. What I said earlier is wrong. I am not sure about any of this now...totally lost in fact! But what I did wonder is if the word include is wrong and should actually be required instead? So may be should look like this... required_once ($CFG->dirroot. '/mod/choice/renderer.php');

Joseph Rézeau added a comment - 31/Jan/13 11:16 AM @Mary, in my opinion a theme renderer should not require or include a module renderer at all.

Tim Hunt added a comment - 31/Jan/13 11:24 AM - edited There is no bug here. theme_mymobile wants to override the mod_choice renderer: class theme_mymobile_mod_choice_renderer extends mod_choice_renderer { that is a perfectly reasonable thing for a theme to do. In order to do this, it has to load the definition of the mod_choice_renderer class, hence the require once, also reasonable. As Joseph says, the problem comes when you want to un-install the choice module. (Although there is really no reason to do this. Just disable the module in the admin settings.) A better example would be a theme that wants to override mod_forumng_renderer, or something like that. Clearly, it should be possible for a theme to provide an override for the renderer of an add-on plugin, without causing errors if the plug-in is not installed. At least it is include not require, so it is not a fatal error. Acutally, it can be a bit messy putting all the renderres in one file. For the OU theme, we make a renderers folder, with separate files in. Then our renderers.php file looks like // Core renderers (alphabetical) require_once($CFG->dirroot . '/theme/ou/renderers/core_renderer.php'); require_once($CFG->dirroot . '/theme/ou/renderers/core_calendar_renderer.php'); require_once($CFG->dirroot . '/theme/ou/renderers/core_question_renderer.php');   // Module renderers (alphabetical) require_once($CFG->dirroot . '/theme/ou/renderers/mod_oucontent_renderer.php'); require_once($CFG->dirroot . '/theme/ou/renderers/mod_subpage_renderer.php'); require_once($CFG->dirroot . '/theme/ou/renderers/mod_forumng_renderer.php');   // Block renderers (alphabetical) require_once($CFG->dirroot . '/theme/ou/renderers/block_course_resources_renderer.php'); require_once($CFG->dirroot . '/theme/ou/renderers/block_navigation_renderer.php'); require_once($CFG->dirroot . '/theme/ou/renderers/block_settings_renderer.php');   // Other renderers (alphabetical) require_once($CFG->dirroot . '/theme/ou/renderers/format_studyplan_renderer.php'); If you adopt that pattern, then you could change it to $choice = get_plugin_dir('mod', 'choice'); if (file_exists($choice . '/renderer.php')) { require_once($CFG->dirroot . '/theme/mymobile/renderers/mod_choice_renderer.php'); } then /theme/mymobile/renderers/mod_choice_renderer.php would do the require_once of /mod/choice/renderer.php.

Mary Evans added a comment - 31/Jan/13 2:14 PM Thanks Tim for your input. Sorry for going over this all again, it's just that I want to make absolutely sure you are not assuming the code I added earlier was the actual code that was in 'mymobile/renderers.php', whereas in actual fact it was just my interpretation of what I thought it should be written as, at least the top section of that renderers.php. But looking through the whole of mymobile/renderers.php again I'm seeing a different view of things. If you look at theme/mymobile/renderers.php in Moodle_24_STABLE https://github.com/moodle/moodle/blob/MOODLE_24_STABLE/theme/mymobile/renderers.php you will see that it starts off with... /** * A custom renderer for the mymobile theme to produce snippets of content. * * @package theme * @subpackage mymobile * @copyright John Stabinger * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ include_once ($CFG->dirroot. '/mod/choice/renderer.php');   class theme_mymobile_renderer extends plugin_renderer_base {   /** * Produces the settings tree * * @param settings_navigation $navigation * @return string */ public function settings_tree(settings_navigation $navigation) { $content = $this->navigation_node($navigation, array('class' => 'settings')); if (has_capability('moodle/site:config', context_system::instance())) { // TODO: Work out whether something is missing from here. } return $content; }   /** * Produces the navigation tree * * @param global_navigation $navigation * @return string */ public function navigation_tree(global_navigation $navigation) { return $this->navigation_node($navigation, array()); }   /** * Protected method to render a navigaiton node * * @param navigation_node $node * @param array $attrs * @return type */ protected function navigation_node(navigation_node $node, $attrs = array()) { $items = $node->children;   // exit if empty, we don't want an empty ul element if ($items->count() == 0) { return ''; }   // array of nested li elements $lis = array(); foreach ($items as $item) { if (!$item->display) { continue; }   $isbranch = ($item->children->count() > 0 || $item->nodetype == navigation_node::NODETYPE_BRANCH); $item->hideicon = true;   $content = $this->output->render($item); $content .= $this->navigation_node($item);   if ($isbranch && !(is_string($item->action) || empty($item->action))) { $content = html_writer::tag('li', $content, array('data-role' => 'list-divider', 'class' => (string)$item->key)); } else if($isbranch) { $content = html_writer::tag('li', $content, array('data-role' => 'list-divider')); } else { $content = html_writer::tag('li', $content, array('class' => (string)$item->text)); } $lis[] = $content; } if (!count($lis)) { return ''; } return implode("\n", $lis); } } And then continues with more renderers which extend the core renderer... class theme_mymobile_core_renderer extends core_renderer { ... And then goes on to extend the mod/choice/renderer...again! class theme_mymobile_mod_choice_renderer extends mod_choice_renderer {   /** * Returns HTML to display choices of option * @param object $options * @param int $coursemoduleid * @param bool $vertical * @return string */ public function display_options($options, $coursemoduleid, $vertical = false) { $layoutclass = 'horizontal'; if ($vertical) { $layoutclass = 'vertical'; } $target = new moodle_url('/mod/choice/view.php'); //changed below to post from target john $attributes = array('method'=>'POST', 'action'=>$target, 'class'=> $layoutclass);   $html = html_writer::start_tag('form', $attributes); $html .= html_writer::start_tag('ul', array('class'=>'choices', 'data-role'=>'controlgroup' ));   $availableoption = count($options['options']); foreach ($options['options'] as $option) { $html .= html_writer::start_tag('li', array('class'=>'option')); $option->attributes->name = 'answer'; $option->attributes->type = 'radio'; $option->attributes->id = 'answer'.html_writer::random_id();   $labeltext = $option->text; if (!empty($option->attributes->disabled)) { $labeltext .= ' ' . get_string('full', 'choice'); $availableoption--; }   $html .= html_writer::empty_tag('input', (array)$option->attributes); $html .= html_writer::tag('label', $labeltext, array('for'=>$option->attributes->id)); $html .= html_writer::end_tag('li'); } $html .= html_writer::tag('li','', array('class'=>'clearfloat')); $html .= html_writer::end_tag('ul'); $html .= html_writer::tag('div', '', array('class'=>'clearfloat')); $html .= html_writer::empty_tag('input', array('type'=>'hidden', 'name'=>'sesskey', 'value'=>sesskey())); $html .= html_writer::empty_tag('input', array('type'=>'hidden', 'name'=>'id', 'value'=>$coursemoduleid));   if (!empty($options['hascapability']) && ($options['hascapability'])) { if ($availableoption < 1) { $html .= html_writer::tag('label', get_string('choicefull', 'choice')); } else { $html .= html_writer::empty_tag('input', array('type'=>'submit', 'value'=>get_string('savemychoice','choice'), 'class'=>'button')); }   if (!empty($options['allowupdate']) && ($options['allowupdate'])) { $url = new moodle_url('view.php', array('id'=>$coursemoduleid, 'action'=>'delchoice', 'sesskey'=>sesskey())); $html .= html_writer::link($url, get_string('removemychoice','choice')); } } else { $html .= html_writer::tag('label', get_string('havetologin', 'choice')); }   $html .= html_writer::end_tag('ul'); $html .= html_writer::end_tag('form');   return $html; }   /** * Returns HTML to display choices result * * TODO: There are differences between this method and the mod choice renderers function. * This needs to be checked VERY careful as the minor changes look like they * may lead to regressions. * * @param object $choices * @param bool $forcepublish * @return string */ public function display_publish_name_vertical($choices) { $html =''; $html .= html_writer::tag('h2',format_string(get_string("responses", "choice")), array('class'=>'main'));   $attributes = array('method'=>'POST'); $attributes['action'] = new moodle_url('/mod/choice/view.php'); $attributes['id'] = 'attemptsform';   if ($choices->viewresponsecapability) { $html .= html_writer::start_tag('form', $attributes); $html .= html_writer::empty_tag('input', array('type'=>'hidden', 'name'=>'id', 'value'=> $choices->coursemoduleid)); $html .= html_writer::empty_tag('input', array('type'=>'hidden', 'name'=>'sesskey', 'value'=> sesskey())); $html .= html_writer::empty_tag('input', array('type'=>'hidden', 'name'=>'mode', 'value'=>'overview')); }   $table = new html_table(); $table->cellpadding = 0; $table->cellspacing = 0; $table->attributes['class'] = 'results names '; $table->tablealign = 'center'; $table->data = array();   $count = 0; ksort($choices->options);   $columns = array(); foreach ($choices->options as $optionid => $options) { $coldata = ''; if ($choices->showunanswered && $optionid == 0) { $coldata .= html_writer::tag('div', format_string(get_string('notanswered', 'choice')), array('class'=>'option')); } else if ($optionid > 0) { $coldata .= html_writer::tag('div', format_string($choices->options[$optionid]->text), array('class'=>'option')); } $numberofuser = 0; if (!empty($options->user) && count($options->user) > 0) { $numberofuser = count($options->user); }   $coldata .= html_writer::tag('div', ' ('.$numberofuser. ')', array('class'=>'numberofuser', 'title' => get_string('numberofuser', 'choice'))); $columns[] = $coldata; }   $table->head = $columns;   $coldata = ''; $columns = array(); foreach ($choices->options as $optionid => $options) { $coldata = ''; if ($choices->showunanswered || $optionid > 0) { if (!empty($options->user)) { foreach ($options->user as $user) { $data = ''; if (empty($user->imagealt)){ $user->imagealt = ''; }   if ($choices->viewresponsecapability && $choices->deleterepsonsecapability && $optionid > 0) { $attemptaction = html_writer::checkbox('attemptid[]', $user->id,''); $data .= html_writer::tag('div', $attemptaction, array('class'=>'attemptaction')); } $userimage = $this->output->user_picture($user, array('courseid'=>$choices->courseid)); $data .= html_writer::tag('div', $userimage, array('class'=>'image'));   $userlink = new moodle_url('/user/view.php', array('id'=>$user->id,'course'=>$choices->courseid)); $name = html_writer::tag('a', fullname($user, $choices->fullnamecapability), array('href'=>$userlink, 'class'=>'username')); $data .= html_writer::tag('div', $name, array('class'=>'fullname')); $data .= html_writer::tag('div','', array('class'=>'clearfloat')); $coldata .= html_writer::tag('div', $data, array('class'=>'user')); } } }   $columns[] = $coldata; $count++; }   $table->data[] = $columns; foreach ($columns as $d) { $table->colclasses[] = 'data'; } $html .= html_writer::tag('div', html_writer::table($table), array('class'=>'response'));   $actiondata = ''; if ($choices->viewresponsecapability && $choices->deleterepsonsecapability) { $selecturl = new moodle_url('#');   $selectallactions = new component_action('click',"select_all_in", array('div',null,'tablecontainer')); $selectall = new action_link($selecturl, get_string('selectall'), $selectallactions); $actiondata .= $this->output->render($selectall) . ' / ';   $deselectallactions = new component_action('click',"deselect_all_in", array('div',null,'tablecontainer')); $deselectall = new action_link($selecturl, get_string('deselectall'), $deselectallactions); $actiondata .= $this->output->render($deselectall); //below john fixed $actiondata .= html_writer::tag('label', ' ' . get_string('withselected', 'choice') . ' ', array('for'=>'menuaction'));   $actionurl = new moodle_url('/mod/choice/view.php', array('sesskey'=>sesskey(), 'action'=>'delete_confirmation()')); $select = new single_select($actionurl, 'action', array('delete'=>get_string('delete')), null, array(''=>get_string('moveselectedusersto', 'choice')), 'attemptsform');   $actiondata .= $this->output->render($select); } $html .= html_writer::tag('div', $actiondata, array('class'=>'responseaction'));   if ($choices->viewresponsecapability) { $html .= html_writer::end_tag('form'); }   return $html; } } So what is causing the Error that Joseph is getting? If it is not a 'big' is it that the renderers.php is written wrongly?

Tim Hunt added a comment - 31/Jan/13 2:22 PM The bit class theme_mymobile_renderer extends plugin_renderer_base is nothing to do with mod/choice. Only the bit class theme_mymobile_mod_choice_renderer extends mod_choice_renderer { has anything to do with mod/choice. You could move the require_once down to just before that class if you wanted, but it is geenrally better to do all require_once calls at the top of the file. Joseph's error is caused beacuse Joseph deleted mod/choice, but theme/mymoodle assumes that it is present. We could argue about who is wrong there.

Mary Evans added a comment - 31/Jan/13 2:25 PM @ Joseph, it looks like I missed the vital clue to whole of this. The fact you deleted the mod/choice is why you got the ERROR. How did I miss that? The fog is clearing and I can see better now. I'm still confused about the use of 'include' though.

Mary Evans added a comment - 31/Jan/13 2:31 PM Thanks for that Tim. OK...just so I have this correct. If I use require_once instead of include_once (as it is currently written) plus add the condition as suggested will this be OK?

Joseph Rézeau added a comment - 31/Jan/13 2:50 PM Tim: If you adopt that pattern, then you could change it to $choice = get_plugin_dir('mod', 'choice'); if (file_exists($choice . '/renderer.php')) { require_once($CFG->dirroot . '/theme/mymobile/renderers/mod_choice_renderer.php'); } then /theme/mymobile/renderers/mod_choice_renderer.php would do the require_once of /mod/choice/renderer.php. Yes, that would make sense. Much better than rely on the existence of a module renderer outside of the /theme/ folder. To come back to the initial triggering of this "bug", I'm sure there are a number of Moodle sites out there which have opted for a "light" installation, removing a number of core modules that they never use. I mean actually removing those modules folders from their server, not just disabling them in admin. That is what I did.

Mary Evans added a comment - 31/Jan/13 4:11 PM Moodle does not understand get_plugin_dir() ( ! ) Fatal error: Call to undefined function get_plugin_dir() in C:\wamp\www\moodle24\theme\mymobile\renderers.php on line 803 Call Stack # Time Memory Function Location 1 0.0019 43526648 {main}( ) ..\index.php:0 2 1.6568 107431776 theme_config::load( ) ..\index.php:159 3 1.6585 107463256 theme_config->__construct( ) ..\outputlib.php:362 4 1.6622 107907856 include_once( 'C:\wamp\www\moodle24\theme\mymobile\renderers.php' ) ..\outputlib.php:451

Tim Hunt added a comment - 31/Jan/13 4:16 PM Well, there is a function wiht a name like that somewhere in moodlelib.php. get_plugin_directory

Mary Evans added a comment - 31/Jan/13 5:38 PM That did it! Yipee!

Mary Evans added a comment - 03/Feb/13 11:59 PM Hi Tim, I've just managed to find time to do this. Can you peer review the changes I have made then I can submit for integration review. Thanks Mary

Joseph Rézeau added a comment - 04/Feb/13 8:50 AM Hi Mary, I've just tested your changes on master and it works OK for me. Thanks! Joseph

Tim Hunt added a comment - 04/Feb/13 9:11 AM +1 nice work. Please submit for integration when you have written testing instructions and cherry-picked to stable branches.

David Scotson added a comment - 04/Feb/13 10:06 AM I hate making performance related comments without being very sure of myself, but is checking for the existence of a a bunch of files on every page access a good idea? I'm just making suggestions off the top of my head here but perhaps Moodle could enforce the directory layout and naming structure that the OU and myself seem to have settled on independantly? Then moodle would only call mod_forumng_renderer.php if that module was installed and switched on? (I think currently there some stuff in Moodle, like the CSS combiner that includes files even from switched off modules maybe that could be changed to check for somethin being turned on too?.) Or the existance could be checked once and then remembered (so you'd get errors if you removed a mod from a running site)?

Tim Hunt added a comment - 04/Feb/13 10:30 AM David, 1. Actually, with a PHP op-code cache, this is actually very, very fast. We do this sort of thing in lots of other places in Moodle. (Look at the code that build the navigation. It does this for every activity in a course, not just one.) 2. Your idea of changing the core renderer factory (or just making a new one theme_overridden_renderer_factory_using_new_improved_directory_layout - that is not a serious naming suggestion) would be a good idea.

Mary Evans added a comment - 04/Feb/13 10:40 AM Thanks Tim...will do.

Mary Evans added a comment - 04/Feb/13 12:12 PM @ Joseph Rézeau Can you suggest some Test Instructions? How I might write these may not be the best way, as I was thinking of just renaming the mod/choice/renderer.php to renderer-old.php. Would that be OK. It worked for me but not sure it's the correct thing to do? Thanks Mary

Joseph Rézeau added a comment - 04/Feb/13 1:52 PM @Mary, What I did to test your mod is: A. Before applying your mod 1.- Site administration ► Plugins ► Activity modules ► Manage activities: Delete the Choice module 2.- Remove yourmoodle/mod/choice folder 3.- Backup a couple of resources or activities without users from a course. 4.- Restore that backup 5.- You get the error message: Warning: include_once(moodle/mod/choice/renderer.php) [function.include-once] : failed to open stream: No such file or directory in moodle/theme/mymobile/renderers.php on line 35 B.- Apply your mod same scenario: check you do not get error message upon restoring course. Hope that helps, Joseph

Mary Evans added a comment - 04/Feb/13 2:56 PM Many thanks Joseph

Eloy Lafuente (stronk7) added a comment - 04/Feb/13 11:29 PM Integrated (23, 24 & master), thanks!

Rajesh Taneja added a comment - 06/Feb/13 2:08 AM Thanks for fixing this Mary, Works as mentioned. No error was thrown, while selecting/changing to MyMobile theme

Mary Evans added a comment - 06/Feb/13 2:53 AM I learnt something fixing this! Thanks Rajesh for testing it

Damyon Wiese added a comment - 08/Feb/13 4:47 AM Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone! Regards, Damyon