Moodle

Grade items restored incorrectly cause recursion, memory limit exceeded error

Details

Description

Refer to MDL-37672. Restoring a course backup made in 1.9 which is missing all grade categories, but still has manual grade items that reference those absent categories, will incorrectly insert the grade items with a null category id.

This will cause a recursion/memory exhaustion error when trying to access the course's Grades interface, as the null category id is unanticipated.

Suggested fix is to not allow an import of a manual grade item with a null category id. If the mapping (backup_ids_temp) fails, use the course's grade category as a default.

Issue Links

will be (partly) resolved by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-37672 Backup of course omits grade categories

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
will help resolve

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-29877 Problem in grade category

  • Major - Major loss of function, incorrect output
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Fred Woolard added a comment -

Fixing the 1.9 code will remedy this problem only for the specific case, but adjusting the 2.x code will prevent bad data insertion regardless of the cause/source.

Show
Fred Woolard added a comment - Fixing the 1.9 code will remedy this problem only for the specific case, but adjusting the 2.x code will prevent bad data insertion regardless of the cause/source.
Hide
Permalink
Charles Fulton added a comment -

We just encountered this issue as well; the symptoms are well-reported in MDL-29877 which I think this issue will resolve. We can reproduce it on demand with a 1.9 site and verified that this patch resolves the issue.

Show
Charles Fulton added a comment - We just encountered this issue as well; the symptoms are well-reported in MDL-29877 which I think this issue will resolve. We can reproduce it on demand with a 1.9 site and verified that this patch resolves the issue.
Hide
Permalink
Charles Fulton added a comment -

Added Andrew Davis as a watcher since he looked at MDL-37672. I suppose this is more of a backup/restore issue but it does affect the gradebook.

Show
Charles Fulton added a comment - Added Andrew Davis as a watcher since he looked at MDL-37672 . I suppose this is more of a backup/restore issue but it does affect the gradebook.
Hide
Permalink
Andrew Davis added a comment -

Hi. I've put this in to the next stable team sprint. Sorry to keep you waiting.

Show
Andrew Davis added a comment - Hi. I've put this in to the next stable team sprint. Sorry to keep you waiting.
Hide
Permalink
Fred Woolard added a comment -

No worries.

Show
Fred Woolard added a comment - No worries.
Hide
Permalink
Andrew Davis added a comment -

Adding some testing instructions and putting this up for peer review. The proposed code change looks fine.

I did try to set Fred Woolard as the issue assignee but wasn't able to. I'm guessing there is some tracker permission involved there.

Show
Andrew Davis added a comment - Adding some testing instructions and putting this up for peer review. The proposed code change looks fine. I did try to set Fred Woolard as the issue assignee but wasn't able to. I'm guessing there is some tracker permission involved there.
Hide
Permalink
Rajesh Taneja added a comment -

Thanks Fred and Andrew,

Patch looks good, it will be nice if you could attach 1.9 backup file to make testing easy.
Also, comment at #L0R153 doesn't comply with moodle coding standards.

[y] Syntax
[y] Output
[y] Whitespace
[-] Language
[-] Databases
[-] Testing
[-] Security
[y] Documentation - comment at #L0R153
[y] Git
[y] Sanity check

Show
Rajesh Taneja added a comment - Thanks Fred and Andrew, Patch looks good, it will be nice if you could attach 1.9 backup file to make testing easy. Also, comment at #L0R153 doesn't comply with moodle coding standards. [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [-] Testing [-] Security [y] Documentation - comment at #L0R153 [y] Git [y] Sanity check
Hide
Permalink
Charles Fulton added a comment -

Attached a generalized backup. Verified that it crashes current master.

Show
Charles Fulton added a comment - Attached a generalized backup. Verified that it crashes current master.
Hide
Permalink
Andrew Davis added a comment - - edited

I don't have a 1.9 backup to hand. If anyone has one please attach it. I'll see what I can dig up. EDIT: there is one attached.

Regarding the comments, the new comment is in line with style used throughout that file. I've raised MDL-38860 to go through fix them all in one go.

Putting this up for integration.

Show
Andrew Davis added a comment - - edited I don't have a 1.9 backup to hand. If anyone has one please attach it. I'll see what I can dig up. EDIT: there is one attached. Regarding the comments, the new comment is in line with style used throughout that file. I've raised MDL-38860 to go through fix them all in one go. Putting this up for integration.
Hide
Permalink
Dan Poltawski added a comment -

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Show
Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
Hide
Permalink
Dan Poltawski added a comment -

Hi,

Sorry another minor coding style thing here, the NULL should be lowercase:
http://docs.moodle.org/dev/Coding_style#Booleans_and_the_null_value

Show
Dan Poltawski added a comment - Hi, Sorry another minor coding style thing here, the NULL should be lowercase: http://docs.moodle.org/dev/Coding_style#Booleans_and_the_null_value
Hide
Permalink
Dan Poltawski added a comment -

I've decided i'll fix this up myself in order to keep things moving.

Show
Dan Poltawski added a comment - I've decided i'll fix this up myself in order to keep things moving.
Hide
Permalink
Dan Poltawski added a comment -

I take it all back! I notice from the surrounding code its using NULL all over.

Sorry - I agree with your call to use the same style as the surrounding code.

Show
Dan Poltawski added a comment - I take it all back! I notice from the surrounding code its using NULL all over. Sorry - I agree with your call to use the same style as the surrounding code.
Hide
Permalink
Dan Poltawski added a comment -

Integrated to master, 24 and 23. thanks Fred/Andrew!

Sorry for the mixed messages about coding style - it'd good to stick to the coding style and eventually over time we'll hopefully end up with a consistent coding style, but it makes sense to make the style look good with the surounding code.

Show
Dan Poltawski added a comment - Integrated to master, 24 and 23. thanks Fred/Andrew! Sorry for the mixed messages about coding style - it'd good to stick to the coding style and eventually over time we'll hopefully end up with a consistent coding style, but it makes sense to make the style look good with the surounding code.
Hide
Permalink
Fred Woolard added a comment -

Dan, sorry for the delay. Rebased each of the branches.

Show
Fred Woolard added a comment - Dan, sorry for the delay. Rebased each of the branches.
Hide
Permalink
Adrian Greeve added a comment -

Tested on the 2.2, 2.3, 2.4 and master branches.
Everything seemed to work okay.
No errors encountered.
Test passed.

Show
Adrian Greeve added a comment - Tested on the 2.2, 2.3, 2.4 and master branches. Everything seemed to work okay. No errors encountered. Test passed.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Your awesome contributions are now part of Moodle, your fav LMS out there.

Closing this as fixed.

Many thanks for all the hard work, ciao

Show
Eloy Lafuente (stronk7) added a comment - Your awesome contributions are now part of Moodle, your fav LMS out there. Closing this as fixed. Many thanks for all the hard work, ciao

People

Vote (11)
Watch (9)

Dates

  • Created:
    Updated:
    Resolved: