[MDL-36381] Problems editing memcache configuration - Moodle Tracker

Details

Type: Bug
Status: Closed
Priority: Blocker
Resolution: Fixed
Affects Version/s: 2.4
Fix Version/s: 2.4
Component/s: Caching
Labels:
- triaged

Testing Instructions:
Hide

Log in as a admin.

Browse to Settings > Plugins > Caching configuration.

Repeat these steps for each of the following: file, memcache, memcached, and mongodb.

Add an instance of the store

Edit the instance of the store. Make sure the values you entered when you created it are still there.

Change something and save the form.

Edit it again and make sure the change you made is shown.

Worth noting is that you don't actually need to use the cache instances.
In order to add instances you need the PHP extensions loaded, installation should be simple for memcache and mongodb.
For those on Ubuntu it'll be something like this, depending upon how you installed things.

sudo aptitude install mongodb memcached php5-memcache php5-memcached

sudo pecl install mongo
Show
Log in as a admin. Browse to Settings > Plugins > Caching configuration. Repeat these steps for each of the following: file, memcache, memcached, and mongodb. Add an instance of the store Edit the instance of the store. Make sure the values you entered when you created it are still there. Change something and save the form. Edit it again and make sure the change you made is shown. Worth noting is that you don't actually need to use the cache instances. In order to add instances you need the PHP extensions loaded, installation should be simple for memcache and mongodb. For those on Ubuntu it'll be something like this, depending upon how you installed things. sudo aptitude install mongodb memcached php5-memcache php5-memcached sudo pecl install mongo
Affected Branches:
MOODLE_24_STABLE
Fixed Branches:
MOODLE_24_STABLE
Pull from Repository:
git://github.com/samhemelryk/moodle.git
Pull Master Branch:
wip-~~MDL-36381~~-m24
Pull Master Diff URL:
https://github.com/samhemelryk/moodle/compare/086f5f9...wip-MDL-36381-m24

Description

Go to Home ▶ Site administration ▶ Plugins ▶ Caching ▶ Configuration
Add a memcache instance with some config
Click edit store on the added instance

Two problems:

The configuration for 'Servers' doesn't seem to be present
When submitting get:
A required parameter (store) was missing

More information about this error

Debug info:
Error code: missingparam
Stack trace:
line 467 of /lib/setuplib.php: moodle_exception thrown
line 523 of /lib/moodlelib.php: call to print_error()
line 79 of /cache/admin.php: call to required_param()

Gliffy Diagrams

Attachments

Issue Links

will help resolve

MDL-36361 On editing a store, the configuration is not loaded into the form

Closed

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Sam Hemelryk added a comment - 07/Nov/12 9:48 AM

Hmmm, this affects all plugins that allow instances to be added, not just memcache.
I forgot in the process to add a means for stores being edited to set data against the form.

I've created a patch that adds this functionality.

Many thanks
Sam

Show

Sam Hemelryk added a comment - 07/Nov/12 9:48 AM Hmmm, this affects all plugins that allow instances to be added, not just memcache. I forgot in the process to add a means for stores being edited to set data against the form. I've created a patch that adds this functionality. Many thanks Sam

Hide

Permalink

Sam Hemelryk added a comment - 07/Nov/12 9:49 AM

Making a blocker because this needs to be fixed before 2.4

Show

Sam Hemelryk added a comment - 07/Nov/12 9:49 AM Making a blocker because this needs to be fixed before 2.4

Hide

Permalink

Rajesh Taneja added a comment - 07/Nov/12 5:26 PM

Thanks Sam,

Patch looks great. Although you might want to consider few minor things:

config for usesafevalue is not considered in mongodb https://github.com/samhemelryk/moodle/compare/086f5f9...wip-MDL-36381-m24#L4R433
mongodb/lib.php $server says "The server connection string. Comma separated values.". Just wondering how multiple servers will be handled. (Not related)
renaming $class and $form might help readability.
https://github.com/samhemelryk/moodle/compare/086f5f9...wip-MDL-36381-m24#L0R783

Please add test Instructions and probably add instructions for testing on all supported cache-stores.

Show

Rajesh Taneja added a comment - 07/Nov/12 5:26 PM Thanks Sam, Patch looks great. Although you might want to consider few minor things: config for usesafevalue is not considered in mongodb https://github.com/samhemelryk/moodle/compare/086f5f9...wip-MDL-36381-m24#L4R433 mongodb/lib.php $server says "The server connection string. Comma separated values.". Just wondering how multiple servers will be handled. (Not related) renaming $class and $form might help readability. https://github.com/samhemelryk/moodle/compare/086f5f9...wip-MDL-36381-m24#L0R783 Please add test Instructions and probably add instructions for testing on all supported cache-stores.

Hide

Permalink

Sam Hemelryk added a comment - 08/Nov/12 5:54 AM

Thanks for looking at this Raj.

Regrading the points you raised:

usesafevalue is now handled correctly.
We should get Helen to look over the strings at some point I think would be best.
$class is used frequently throughout the cache and cache stores, at this point I'd rather not be renaming it. I have however renamed $form to $editform now.

Putting up for integration review now

Many thanks
Sam

Show

Sam Hemelryk added a comment - 08/Nov/12 5:54 AM Thanks for looking at this Raj. Regrading the points you raised: usesafevalue is now handled correctly. We should get Helen to look over the strings at some point I think would be best. $class is used frequently throughout the cache and cache stores, at this point I'd rather not be renaming it. I have however renamed $form to $editform now. Putting up for integration review now Many thanks Sam

Hide

Permalink

Dan Poltawski added a comment - 08/Nov/12 9:41 AM

Thanks Sam, i've integrated that now.

Show

Dan Poltawski added a comment - 08/Nov/12 9:41 AM Thanks Sam, i've integrated that now.

Hide

Permalink

David Monllaó added a comment - 08/Nov/12 2:14 PM

All works as expected except memcached "Use compression" parameter, if I change the value to 'No' it continues showing 'Yes' when opening the form again. Failing it.

Show

David Monllaó added a comment - 08/Nov/12 2:14 PM All works as expected except memcached "Use compression" parameter, if I change the value to 'No' it continues showing 'Yes' when opening the form again. Failing it.

Hide

Permalink

Dan Poltawski added a comment - 08/Nov/12 6:41 PM

I tried to save Sam some time and fix this, but actually I can't work it out quickly!

Show

Dan Poltawski added a comment - 08/Nov/12 6:41 PM I tried to save Sam some time and fix this, but actually I can't work it out quickly!

Hide

Permalink

Sam Hemelryk added a comment - 09/Nov/12 4:38 AM

Aha, thanks for picking that up David.
The problem is the use of empty for fields where an empty value is a valid value, checkboxes/bools etc.

Dan I've pushed up a commit on top of my branch now to address this issue. It fixes not just the Use compression setting but all other bool settings that would be affected in the same way.

// Diff:

https://github.com/samhemelryk/moodle/commit/e0c11b38e2c6f402ec2e51e9954c66b4d85f7e8c

// To pull:

git fetch git://github.com/samhemelryk/moodle.git wip-MDL-36381-m24 && git cherry-pick FETCH_HEAD

Cheers
Sam

Show

Sam Hemelryk added a comment - 09/Nov/12 4:38 AM Aha, thanks for picking that up David. The problem is the use of empty for fields where an empty value is a valid value, checkboxes/bools etc. Dan I've pushed up a commit on top of my branch now to address this issue. It fixes not just the Use compression setting but all other bool settings that would be affected in the same way. // Diff: https://github.com/samhemelryk/moodle/commit/e0c11b38e2c6f402ec2e51e9954c66b4d85f7e8c // To pull: git fetch git://github.com/samhemelryk/moodle.git wip-MDL-36381-m24 && git cherry-pick FETCH_HEAD Cheers Sam

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 09/Nov/12 7:02 AM

(new commit applied and reset to testing)

Show

Eloy Lafuente (stronk7) added a comment - 09/Nov/12 7:02 AM (new commit applied and reset to testing)

Hide

Permalink

Sam Hemelryk added a comment - 09/Nov/12 7:09 AM

Thanks Eloy

Show

Sam Hemelryk added a comment - 09/Nov/12 7:09 AM Thanks Eloy

Hide

Permalink

David Monllaó added a comment - 09/Nov/12 3:01 PM

It passes.

I've tried to set a '!!!' username in a mongo store and it's cleaned, but it works as expected with regular alphanumericext caracters.

Show

David Monllaó added a comment - 09/Nov/12 3:01 PM It passes. I've tried to set a '!!!' username in a mongo store and it's cleaned, but it works as expected with regular alphanumericext caracters.

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 10/Nov/12 1:11 AM

Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

(not really)

Closing, thanks!

Show

Eloy Lafuente (stronk7) added a comment - 10/Nov/12 1:11 AM Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!

People

Assignee:

Sam Hemelryk

Reporter:

Dan Poltawski

Peer reviewer:

Rajesh Taneja

Integrator:

Dan Poltawski

Tester:

David Monllaó

Participants:

Dan Poltawski, David Monllaó, Eloy Lafuente (stronk7), Rajesh Taneja, Sam Hemelryk

Votes:

0 Vote for this issue

Watchers:

2 Start watching this issue

Dates

Created:

06/Nov/12 10:56 AM

Updated:

11/Nov/12 9:06 PM

Resolved:

10/Nov/12 1:11 AM

Fix Release Date:

3/Dec/12