Details
Bug
Minor
Description
The new calendar types don't currently allow the addition of settings pages.
For the standard Gregorian calendar this may be fine, but other calendar types may require certain settings to be configurable.
Gliffy Diagrams
Activity
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,
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?
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.
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);
|
+ }
|
+ }
|
Oh!
Sorry about that. I've now included that code as well. Thanks for spotting that.
Thanks Adrian,
Looks fine now.
Integrated to 26 and master.
It passes. Tested in integration master
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
As the comment above that I posted says. I have already included dev docs for this change.
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