Moodle
  1. Moodle
  2. MDL-37676

Grade items restored incorrectly cause recursion, memory limit exceeded error

    Details

    • Rank:
      47381

      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

          Activity

          Hide
          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
          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
          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
          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
          Fred Woolard added a comment -

          No worries.

          Show
          Fred Woolard added a comment - No worries.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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

            • Votes:
              11 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: