Issue Details (XML | Word | Printable)

Key: MDL-8209
Type: Bug Bug
Status: Resolved Resolved
Resolution: Fixed
Priority: Major Major
Assignee: Dan Poltawski
Reporter: Dan Poltawski
Votes: 0
Watchers: 3
Operations

Add/Edit UI Mockup to this issue
If you were logged in you would be able to see more operations.
Moodle

Incorrectly Displaying New Admin Settings If Not Set

Created: 17/Jan/07 06:22 PM   Updated: 06/May/07 05:21 PM
Return to search
Component/s: Administration
Affects Version/s: 1.7, 1.7.1, 1.8
Fix Version/s: 1.9

File Attachments: 1. Text File admin_setting_redirect.patch (2 kB)


Database: MySQL
Participants: Dan Poltawski and Petr Skoda
Security Level: None
Resolved date: 06/May/07
Affected Branches: MOODLE_17_STABLE, MOODLE_18_STABLE
Fixed Branches: MOODLE_19_STABLE


 Description  « Hide
Settings using the new admin interface will incorrectly appear as though they are set to their default value even if they have not been created.

This could be reproduced in a number of ways, from deleting one of the values for the config table to accidentally clicking away during the upgrade:

* Upgrade a moodle
* upgrade core takes place
* prompted to upgrade settings
* click away from the settings page (for example the admin link, rather than save settings)
* module upgrade continues
* upgrade finished
* View a setting which should've been set (e.g. enablerecordcache in performance, gradebookroles in appearance)
* the settings will look like its set to the default, but the setting doesn't exist.

A way round this would perhaps be more agreessive redirection to the uprgrade settings screen if any settings don't exist? Or perhaps something which indicates is showing the default rather than what it is set to?

 All   Comments   Change History   Version Control      Sort Order: Ascending order - Click to sort in descending order
Dan Poltawski added a comment - 17/Apr/07 04:25 AM
Attaching a patch which will look check for admin settings for any which aren't initialised from admin/index.php, and redirect to the new settings screen if any aren't found. Being slighly more agressive at redirecting than the previous one-time chance it had on upgrades.

I don't know if searching for new settings every time we hit admin/index.php is a bit heavyweight, though..


Dan Poltawski added a comment - 17/Apr/07 04:30 AM
Hi Petr,

I wonder if you could review this patch i've created. Basically the idea is to try and prevent the condition where by a site admin doesn't properly setup new admin settings.

This causes a problem as it means the settings will just display their default values when examined (despite not actually being set).

In order to combat this i've added a redirect to the new settings screen if any admin settings aren't set..

Dan


Petr Skoda added a comment - 18/Apr/07 06:00 AM
I would move the new function to lib/adminlib,php

My +1 to commit it into HEAD, and maybe backporting to 1.8.x after some more testing


Petr Skoda added a comment - 18/Apr/07 06:03 AM
I do not think we need the
unset($setting)
there

Dan Poltawski added a comment - 18/Apr/07 05:42 PM
I've commited this to HEAD