search

catalyst / moodle-tool_objectfs

Object file storage system for Moodle
https://moodle.org/plugins/tool_objectfs
88 stars 72 forks source link

Saving settings without selecting a filesystem during install results in warning on plugin settings page #530

Open marxjohnson opened 1 year ago

marxjohnson commented 1 year ago

The following warnings are displayed on the plugin settings page:

Warning: Trying to access array offset on value of type null in /var/www/moodlecore/lib/adminlib.php on line 4053 Warning: Trying to access array offset on value of type null in /var/www/moodlecore/lib/adminlib.php on line 4059

Steps to reproduce:

  1. Install objectFS
  2. On the "New plugin settings" page, leave all the plugin settings as default and click "Save changes"
  3. Navigate to "Site Adminstration > Plugins > Admin tools > Object storage file system > Plugin settings"
  4. Set "Storage File System" to \tool_objectfs\s3_file_system and click "Save changes"
  5. Several PHP Warnings will be output to the top of the page (or the log, if display_errors is off). The "Pre-Signed URL expiration time" field will be empty, even though the default is 2 hours.

This is because unless you select a file system during installation, the expiration time field is never displayed, and so no value is saved for the setting. The admin_setting_configduration class assumes the setting was saved during installation, and so doesn't cope well when it is empty.

danmarsden commented 1 year ago

@brendanheywood - will leave this to you or someone else in AU to review but... I'd be more inclined to fix it where the expirationtime var is used eg something like:

$this->expirationtime = !isset($config->expirationtime) ? 0 : $config->expirationtime;
marxjohnson commented 1 year ago

I think where it's actually used is already taken care of in manager::get_objectfs_config(). This is just an issue on the plugin settings page. #531 should resolve it.

danmarsden commented 1 year ago

any chance of adding in the actual php warning errors that appear here in the github issue? - will also help with others doing a google search for an error.

marxjohnson commented 1 year ago

Done!

danmarsden commented 1 year ago

thanks Mark - this isn't something I'd merge in myself - using set_config() in the place you've added it just seems wrong - I wonder if there's a bug in the admin_setting_configduration class if it's not coping when the var isn't set, otherwise it might be somewhere else that needs fixing.

but - @brendanheywood may think otherwise and just merge it.... :-)