Details
-
Type:
Bug
-
Status: Closed
-
Priority:
Blocker
-
Resolution: Fixed
-
Affects Version/s: 1.8, 1.9
-
Component/s: Database activity module, Libraries
-
Labels:None
-
Environment:PHP5
-
Affected Branches:MOODLE_18_STABLE, MOODLE_19_STABLE
-
Fixed Branches:MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE
Description
Moodle config settings:
cachetype: internal
rcache: true
The test (I added this block to the top of index.php):
// Get course, print name, change name
$course = get_record('course', 'id', SITEID);
notify($course->fullname); // Prints: TESTING SITE
$course->fullname = 'newname';
// Get course again, print name
$course = get_record('course', 'id', SITEID);
notify($course->fullname); // Prints: newname
die;
From the above, the first time we get the course, we modify it, which in turn modifies the object in the cache. The cache should return a clone of the object stored in the cache to prevent this from happening.
This doesn't seem to be a problem with the other caching types, but I wasn't able to test eaccelerator.
Gliffy Diagrams
Attachments
Activity
- All
- Comments
- Work Log
- History
- Activity
- Links Hierarchy
I agree - it should return a cloned obj. All the other caching schemes I'm aware of (memcached, eaccelerator, apc, turck) will return a different object, not a reference.
It is fairly constrained...
- The impact is short-lived as the cache is discarded at the end of the request
- If you grab a record from the cache and change it, it's usually to update the DB...
and probably easy to fix 
Reproduced on 1.9 also.
Modified the set_rcache method so that it saves a clone of the record object. This solves this issue.
Reopening. This patch is wrong on 2 counts
- It does not fix the problem! it only clones when saving into the cache, not when retrieving. 2 independent retrievals will get the same reference.
- It uses fullclone() – the record cache is for performance, the cost of serialize/unserialize is high, and it is not needed, as we know records from the DB won't carry references. They are 'final'.
Patched to
- only use clone
- do it on set and get
Under PHP4 I'm getting the following error:
Parse error: parse error, unexpected T_VARIABLE in /var/www/cvs/19/moodle/lib/dmllib.php on line 2586
This happens after a fresh install of MOODLE_19_STABLE
Causes syntax error in PHP4 causing new installation to fail
Thanks for spotting this - eagle eyes, finding the right bug to comment this. Just fixed it so if you update in the next hour or so you should get the fix.
Else, just search for clone $bla and replace it with clone($bla) - we need the parenthesis for it to work correctly in PHP4. I'm off home for the day, so it'd be great to hear feedback on the fix I just committed to cvs.
I just updated/tested and it's working fine for me now.
Thanks.
Thanks for all of the great work on this. Checked out the code and it looks like Martin's changes have not been merged to HEAD or to MOODLE_18_STABLE. MOODLE_18_STABLE is still using fullclone and HEAD is using clone without the open/close parenthesis.
Good spotting. The commit to HEAD had errored out and I didn't realise. Fixed in HEAD and 18_STABLE now.
Thanks Martin for your patch, and sorry about my botch-up
Looks like we are not done. Get the following error:
Fatal error: __clone method called on non-object in path/to/lib/dmllib.php on line 2327
Looks like we are caching boolean false as well (least in this case). Need to check to see if it is an object before cloning.
Attaching a diff with a proposed solution.
Mark, Dan Poltawski already spotted and fixed it as MDL-12882, perhaps you are testing with an older codebase? Have a look at his patch, anyway.
Closing again since the reason why it was opened is now in MDL-12882
Have just reproduced this on 1.9.. Adding some watchers and raising priority.