Sam Hemelryk added a comment - 21/Nov/13 9:28 PM Hi Adrian, Code looks spot on thanks. I suppose we should consider this for backporting as well, I'm not entirely sure when we expect to see the first calendar types arriving with settings but perhaps as we've only just released 2.6 this would be a candidate for backport (it is down as a bug). I do have one suggestion, but it is only a suggestion. The core_component class has a method get_plugin_list_with_file which would be perfect for your needs here. Feel free to push to integration when you are ready. Cheers Sam

Adrian Greeve added a comment - 22/Nov/13 3:05 AM Hello Sam, Thank you for the review. Yes core_component::get_plugin_list_with_file is exactly what I required. With the call to that method, I know that you can set the file to be automatically included, but I ran into trouble trying to get my settings to show properly when set to true. As for calendar types that will arrive with settings, my Japanese calendar type will be the first one. I want to allow the administrator to decide which year format to use when displaying the date and I have a feeling that there will be more settings to come in the future. I'll add a branch for 2.6 (When the multi-calendar support was introduced). Thanks,

Damyon Wiese added a comment - 26/Nov/13 3:22 AM Noting that for 2.6.0 this won't be supported - so if you write a calendar plugin with a settings page - and the plugin is broken unless those settings are set - you will have to set the requires version of your plugin to a moodle version with this support added. This will need documenting in the dev docs and in lib/upgrade.txt. Adrian - can you please add this to the lib/upgrade.txt in this patch?

Adrian Greeve added a comment - 26/Nov/13 6:19 AM Thanks Damyon, I've added documentation to lib/upgrade.txt and also updated http://docs.moodle.org/dev/Multiple_calendar_support#Settings_page with information about this fix.

Damyon Wiese added a comment - 26/Nov/13 7:14 AM - edited Thanks Adrian, Looking again - this seems to be missing the required functions in lib/classes/plugininfo/calendartype.php to show the links to the settings from the plugin overview page. I copy/pasted this and it seemed to work - can you check/clean this and amend your branch? +++ lib/classes/plugininfo/calendartype.php + + public function get_settings_section_name() { + return 'calendartype_' . $this->name . '_settings'; + } + + public function load_settings(part_of_admin_tree $adminroot, $parentnodename, $hassiteconfig) { + global $CFG, $USER, $DB, $OUTPUT, $PAGE; // In case settings.php wants to refer to them. + $ADMIN = $adminroot; // May be used in settings.php. + $plugininfo = $this; // Also can be used inside settings.php. + $qtype = $this; // Also can be used inside settings.php. + + if (!$this->is_installed_and_upgraded()) { + return; + } + + $section = $this->get_settings_section_name(); + + $settings = null; + $systemcontext = \context_system::instance(); + if (($hassiteconfig) && + file_exists($this->full_path('settings.php'))) { + $settings = new admin_settingpage($section, $this->displayname, + 'moodle/site:config', $this->is_enabled() === false); + include($this->full_path('settings.php')); // This may also set $settings to null. + } + if ($settings) { + $ADMIN->add($parentnodename, $settings); + } + }

Adrian Greeve added a comment - 26/Nov/13 7:52 AM Oh! Sorry about that. I've now included that code as well. Thanks for spotting that.

Damyon Wiese added a comment - 26/Nov/13 9:02 AM Thanks Adrian, Looks fine now. Integrated to 26 and master.

David Monllaó added a comment - 27/Nov/13 2:03 AM It passes. Tested in integration master

Dan Poltawski added a comment - 29/Nov/13 6:34 AM Congratulations, this change has now made its way upstream. Thanks for your contribution! “ Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live. ” - Rick Osborne

Adrian Greeve added a comment - 28/Oct/14 11:38 AM As the comment above that I posted says. I have already included dev docs for this change.

Marina Glancy added a comment - 27/Nov/15 8:48 AM This issue caused a regression MDL-52332 Function core\plugininfo\calendartype::load_settings() has a wrong definition but it did not show up before PHP7 tests because it is never used