Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37676

Grade items restored incorrectly cause recursion, memory limit exceeded error

    Details

    • Testing Instructions:
      Hide

      You'll need a 1.9 backup. Restore the backup into a 2.x site and check that the backup restores without error. There is one attached that you can use.

      Perform a backup and restore within your 2.x site just to make sure that that all still works fine.

      MDL-29877 will fix previouly restored grade items so don't worry about that.

      Show
      You'll need a 1.9 backup. Restore the backup into a 2.x site and check that the backup restores without error. There is one attached that you can use. Perform a backup and restore within your 2.x site just to make sure that that all still works fine. MDL-29877 will fix previouly restored grade items so don't worry about that.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              woolardfa@appstate.edu 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
              woolardfa@appstate.edu 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
              cfulton 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
              cfulton 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
              cfulton 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
              cfulton 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
              andyjdavis Andrew Davis added a comment -

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

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

              No worries.

              Show
              woolardfa@appstate.edu Fred Woolard added a comment - No worries.
              Hide
              andyjdavis 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
              andyjdavis 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
              rajeshtaneja 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
              rajeshtaneja 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
              cfulton Charles Fulton added a comment -

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

              Show
              cfulton Charles Fulton added a comment - Attached a generalized backup. Verified that it crashes current master.
              Hide
              andyjdavis 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
              andyjdavis 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
              poltawski 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
              poltawski 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
              poltawski 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
              poltawski 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
              poltawski Dan Poltawski added a comment -

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

              Show
              poltawski Dan Poltawski added a comment - I've decided i'll fix this up myself in order to keep things moving.
              Hide
              poltawski 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
              poltawski 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
              poltawski 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
              poltawski 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
              woolardfa@appstate.edu Fred Woolard added a comment -

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

              Show
              woolardfa@appstate.edu Fred Woolard added a comment - Dan, sorry for the delay. Rebased each of the branches.
              Hide
              abgreeve 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
              abgreeve 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
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    13/May/13