Moodle
  1. Moodle
  2. MDL-25432

i have a row in course_module for a deleted forum

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Note: You need database access to test this.

      1. Log in as admin
      2. Add a wiki activity to course
      3. Go back to course and move mouse over wiki and note module id (in my case 58)
      4. In course_module table look for id (58) and note instance id (in my case 21)
      5. In wiki table delete record with id = instance id (21)
      6. Backup this course (Settings-> Course administration-> Backup) and keep and eye on error log.
      7. Error should be logged in error logs
      8. Go though the whole process and make sure you can backup a course.
      9. Try backing up another (normal) course.
      Show
      Note: You need database access to test this. Log in as admin Add a wiki activity to course Go back to course and move mouse over wiki and note module id (in my case 58) In course_module table look for id (58) and note instance id (in my case 21) In wiki table delete record with id = instance id (21) Backup this course (Settings-> Course administration-> Backup) and keep and eye on error log. Error should be logged in error logs Go though the whole process and make sure you can backup a course. Try backing up another (normal) course.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-mdl-25432
    • Rank:
      473

      Description

      If I go into a course and go to course administration -> backup I get the following error.

      error/activity_task_coursemodule_not_found

      More information about this error
      Stack trace:

      • line 104 of /backup/util/factories/backup_factory.class.php: backup_task_exception thrown
      • line 120 of /backup/moodle2/backup_plan_builder.class.php: call to backup_factory::get_backup_activity_task()
      • line 144 of /backup/moodle2/backup_plan_builder.class.php: call to backup_plan_builder::build_activity_plan()
      • line 165 of /backup/moodle2/backup_plan_builder.class.php: call to backup_plan_builder::build_section_plan()
      • line 89 of /backup/moodle2/backup_plan_builder.class.php: call to backup_plan_builder::build_course_plan()
      • line 163 of /backup/util/factories/backup_factory.class.php: call to backup_plan_builder::build_plan()
      • line 61 of /backup/util/plan/backup_plan.class.php: call to backup_factory::build_plan()
      • line 333 of /backup/controller/backup_controller.class.php: call to backup_plan->build()
      • line 126 of /backup/controller/backup_controller.class.php: call to backup_controller->load_plan()
      • line 82 of /backup/backup.php: call to backup_controller->__construct()

      The query that is failing to get a row is in get_coursemodule_from_id():
      SELECT cm.*, m.name, md.name AS modname
      FROM mdl_course_modules cm
      JOIN mdl_modules md ON md.id = cm.module
      JOIN mdl_forum m ON m.id = cm.instance
      WHERE cm.id = 1119 AND md.name = 'forum'

      I rewrote the query to find out that cm.instance == 247 but there is no forum with that ID. There are higher IDs indicating that the forum did exist but was deleted. Petr has suggested this may be caused by double clicking the link to add a forum.

      Im now not able to backup my course. Ideally we'd prevent this occurring in the first place but at a minimum we need to be able to recover from this once it occurs.

      1. duplicatedcontent.jpg
        22 kB

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment -

          Perhaps get_backup_activity_task() instead of throwing an exception when it detects this situation could instead delete the orphan records. I'm a bit wary of automated deletion however. Maybe we could provide a page within Site Administration where the admin can check the db for orphans and optionally remove them.

          Show
          Andrew Davis added a comment - Perhaps get_backup_activity_task() instead of throwing an exception when it detects this situation could instead delete the orphan records. I'm a bit wary of automated deletion however. Maybe we could provide a page within Site Administration where the admin can check the db for orphans and optionally remove them.
          Hide
          Petr Škoda added a comment -

          +1 for extra page, we need to add tons of consistency tests and cleanup queries for similar problems elsewhere.

          Show
          Petr Škoda added a comment - +1 for extra page, we need to add tons of consistency tests and cleanup queries for similar problems elsewhere.
          Hide
          Petr Škoda added a comment -

          the current approach is to ignore these orphaned records usually by doing JOIN in sql.

          Show
          Petr Škoda added a comment - the current approach is to ignore these orphaned records usually by doing JOIN in sql.
          Hide
          Antonio Espinosa added a comment -

          A workaround to find orphans modules (and delete them manually) is:
          1. Active edition mode in the affected course
          2. Look at the sections id, moving mouse over section edit icon
          3. Run this SQL sintaxt for each section: SELECT cm.id, m.name AS modname FROM mdl_course_modules cm JOIN mdl_modules m ON m.id = cm.module WHERE cm.course = <course_id> AND cm.section = <section_id>
          4. Compare the modules of the query result to the module_id of all resources and activities of that section (moving mouse over resource or activity link)
          5. If find a missing module_id in the query, you find the orphan
          6. Go to mdl_course_modules and delete the row with that id.

          This help me when the course backup is urgent.
          Waiting for a bugfix ...

          Show
          Antonio Espinosa added a comment - A workaround to find orphans modules (and delete them manually) is: 1. Active edition mode in the affected course 2. Look at the sections id, moving mouse over section edit icon 3. Run this SQL sintaxt for each section: SELECT cm.id, m.name AS modname FROM mdl_course_modules cm JOIN mdl_modules m ON m.id = cm.module WHERE cm.course = <course_id> AND cm.section = <section_id> 4. Compare the modules of the query result to the module_id of all resources and activities of that section (moving mouse over resource or activity link) 5. If find a missing module_id in the query, you find the orphan 6. Go to mdl_course_modules and delete the row with that id. This help me when the course backup is urgent. Waiting for a bugfix ...
          Hide
          Adam Olley added a comment -

          Whats the status on this? Given the only way to resolve the conflict is to manually delete the offending record from the course_modules table, this can be frustrating for users.

          An admin page/function that presents a list of orphaned course_module records sounds like a good solution to me.

          Show
          Adam Olley added a comment - Whats the status on this? Given the only way to resolve the conflict is to manually delete the offending record from the course_modules table, this can be frustrating for users. An admin page/function that presents a list of orphaned course_module records sounds like a good solution to me.
          Hide
          James Yale added a comment -

          Could anyone clarify the process to manually fix this error? I'm getting the reported problem on the front page (course 1) of a large site but the page is almost completely baron of content apart from the normal Navigation, Course Category and Settings boxes - do I get the section IDs to search for from the edit boxes on these parts of the page?

          Show
          James Yale added a comment - Could anyone clarify the process to manually fix this error? I'm getting the reported problem on the front page (course 1) of a large site but the page is almost completely baron of content apart from the normal Navigation, Course Category and Settings boxes - do I get the section IDs to search for from the edit boxes on these parts of the page?
          Hide
          Paul Vaughan added a comment -

          We have this error on a course (initially noticed in the backup log, but reproducible via clicking on course administration -> backup), which was restored from an earlier backup on a different server (1.9 or 2.1, I'm not sure), but which appears to have been restored twice into the same course (definately a mistake). Please see screenshot.

          I came across this Moodle Forum post http://moodle.org/mod/forum/discuss.php?d=175880 which references an alpha-version of ForumNG, and admittedly we are using one alpha-level module, the OUWiki, but not in this course.

          As James suggests, a workaround would be very useful, so at least we can run backups.

          Show
          Paul Vaughan added a comment - We have this error on a course (initially noticed in the backup log, but reproducible via clicking on course administration -> backup), which was restored from an earlier backup on a different server (1.9 or 2.1, I'm not sure), but which appears to have been restored twice into the same course (definately a mistake). Please see screenshot. I came across this Moodle Forum post http://moodle.org/mod/forum/discuss.php?d=175880 which references an alpha-version of ForumNG, and admittedly we are using one alpha-level module, the OUWiki, but not in this course. As James suggests, a workaround would be very useful, so at least we can run backups.
          Hide
          Paul Vaughan added a comment -

          Example of a possible duplicated course restore creating this issue.

          Show
          Paul Vaughan added a comment - Example of a possible duplicated course restore creating this issue.
          Hide
          James Yale added a comment - - edited

          To update this I've fixed (backups on) a couple of courses using the following method:

          My process was this:

          • Get the course ID (from the URL when you click the course in Moodle)
          • Go to to the database to the mdl_course_modules table
          • Select/search for records where the value in the course column matches the course ID
            SELECT * FROM `mdl_course_modules` WHERE `course` =<courseid>
          • Find all the entries where idnumber was NULL and delete them:
            SELECT `idnumber` FROM `mdl_course_modules` WHERE `course` =<courseid> AND `idnumber` IS NULL

          I guess you could put this together and do a:

          DELETE FROM `mdl_course_modules` WHERE `course` =<courseid> AND `idnumber` IS NULL

          But make sure you take a backup first..

          Show
          James Yale added a comment - - edited To update this I've fixed (backups on) a couple of courses using the following method: My process was this: Get the course ID (from the URL when you click the course in Moodle) Go to to the database to the mdl_course_modules table Select/search for records where the value in the course column matches the course ID SELECT * FROM `mdl_course_modules` WHERE `course` =<courseid> Find all the entries where idnumber was NULL and delete them: SELECT `idnumber` FROM `mdl_course_modules` WHERE `course` =<courseid> AND `idnumber` IS NULL I guess you could put this together and do a: DELETE FROM `mdl_course_modules` WHERE `course` =<courseid> AND `idnumber` IS NULL But make sure you take a backup first..
          Hide
          Miroslav Fikar added a comment -

          To fix this issue I have tried both approaches suggested here: by James (25/Aug/11) and Antonio (10/Feb/11). Only Antonio's method worked for me. The suggestion by James deleted many more items and corrupted the database. Just to add, in my case all courses had duplicate entries for forums only.

          Show
          Miroslav Fikar added a comment - To fix this issue I have tried both approaches suggested here: by James (25/Aug/11) and Antonio (10/Feb/11). Only Antonio's method worked for me. The suggestion by James deleted many more items and corrupted the database. Just to add, in my case all courses had duplicate entries for forums only.
          Hide
          Chris Meyer added a comment -

          Ditto for Miroslav Fikar. The tedious way Antonio suggested is the only safe one since it seems many things can cause this problem. I'm losing a lot of time fixing courses. The null for idnumber is sexy but unreliable. Looking forward to a fix. Does this problem keep coming up or was it a result of the upgrade from 1.9?

          Show
          Chris Meyer added a comment - Ditto for Miroslav Fikar. The tedious way Antonio suggested is the only safe one since it seems many things can cause this problem. I'm losing a lot of time fixing courses. The null for idnumber is sexy but unreliable. Looking forward to a fix. Does this problem keep coming up or was it a result of the upgrade from 1.9?
          Hide
          Paul Vaughan added a comment - - edited

          My +1 for Antonio's method of removal which worked perfectly for me. Thanks!!

          Also, shouldn't this be raised to a blocker, as (at least for me) it's causing the backups to fail?

          Show
          Paul Vaughan added a comment - - edited My +1 for Antonio's method of removal which worked perfectly for me. Thanks!! Also, shouldn't this be raised to a blocker, as (at least for me) it's causing the backups to fail?
          Hide
          Brent Lee added a comment -

          Where are we at for a fix on this issue?

          Show
          Brent Lee added a comment - Where are we at for a fix on this issue?
          Hide
          Brent Lee added a comment -

          I agree with Paul, this is a blocker as backups are failing.

          Show
          Brent Lee added a comment - I agree with Paul, this is a blocker as backups are failing.
          Hide
          Michael de Raadt added a comment -

          It looks like this issue has been a little neglected. Bumping it...

          Show
          Michael de Raadt added a comment - It looks like this issue has been a little neglected. Bumping it...
          Hide
          Brent Lee added a comment -

          Any more information on this bug?

          Show
          Brent Lee added a comment - Any more information on this bug?
          Hide
          Bart Kamphuis added a comment -

          This is becoming a very urgent issue for us, with our End-Of-Year rollover procedure dependent on course-backups.

          Show
          Bart Kamphuis added a comment - This is becoming a very urgent issue for us, with our End-Of-Year rollover procedure dependent on course-backups.
          Hide
          Brent Lee added a comment -

          To update this I've fixed (backups on) a couple of courses using the following method:

          My process was this:

          "Get the course ID (from the URL when you click the course in Moodle)
          Go to to the database to the mdl_course_modules table
          Select/search for records where the value in the course column matches the course ID
          SELECT * FROM `mdl_course_modules` WHERE `course` =<courseid>
          Find all the entries where idnumber was NULL and delete them:
          SELECT `idnumber` FROM `mdl_course_modules` WHERE `course` =<courseid> AND `idnumber` IS NULL
          I guess you could put this together and do a:

          DELETE FROM `mdl_course_modules` WHERE `course` =<courseid> AND `idnumber` IS NULL

          But make sure you take a backup first.."

          <strong>I am unable to do this as I cannot take a backup in the first place to ensure I can rollback.</strong>

          Show
          Brent Lee added a comment - To update this I've fixed (backups on) a couple of courses using the following method: My process was this: "Get the course ID (from the URL when you click the course in Moodle) Go to to the database to the mdl_course_modules table Select/search for records where the value in the course column matches the course ID SELECT * FROM `mdl_course_modules` WHERE `course` =<courseid> Find all the entries where idnumber was NULL and delete them: SELECT `idnumber` FROM `mdl_course_modules` WHERE `course` =<courseid> AND `idnumber` IS NULL I guess you could put this together and do a: DELETE FROM `mdl_course_modules` WHERE `course` =<courseid> AND `idnumber` IS NULL But make sure you take a backup first.." <strong>I am unable to do this as I cannot take a backup in the first place to ensure I can rollback.</strong>
          Hide
          James Yale added a comment -

          @Brent don't follow my method, it's overly destructive - Antonio's comment (http://tracker.moodle.org/browse/MDL-25432?focusedCommentId=103700&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-103700) works far better even if it is somewhat more labour intensive.

          Show
          James Yale added a comment - @Brent don't follow my method, it's overly destructive - Antonio's comment ( http://tracker.moodle.org/browse/MDL-25432?focusedCommentId=103700&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-103700 ) works far better even if it is somewhat more labour intensive.
          Hide
          Bart Kamphuis added a comment -

          I was able to solve the issue using Antonio's work-around. There were 4 orphan records in the mdl_course_modules table, and I note that for each of them instance = 0.

          Show
          Bart Kamphuis added a comment - I was able to solve the issue using Antonio's work-around. There were 4 orphan records in the mdl_course_modules table, and I note that for each of them instance = 0.
          Hide
          Helen Foster added a comment -

          Just noting that I encountered this bug when attempting to publish or backup a course on the demo site, http://demo.moodle.net/course/view.php?id=3.

          Show
          Helen Foster added a comment - Just noting that I encountered this bug when attempting to publish or backup a course on the demo site, http://demo.moodle.net/course/view.php?id=3 .
          Hide
          Mark Ward added a comment -

          I've written a PHP script which automates Antonio's process, checking through course_modules and reporting all orphaned entries. The script can also delete all such entries if you wish. Remember that this is not a supported file, so use it at your own risk.

          I'd highlight that the script identifies more of these orphaned entries than I expected, this is because it is looking for resources as well as activities. I had 4 courses which weren't backing up, but this script identified 17 orphaned entries across 12 courses.

          Hope it helps people with this issue!

          Show
          Mark Ward added a comment - I've written a PHP script which automates Antonio's process, checking through course_modules and reporting all orphaned entries. The script can also delete all such entries if you wish. Remember that this is not a supported file, so use it at your own risk. I'd highlight that the script identifies more of these orphaned entries than I expected, this is because it is looking for resources as well as activities. I had 4 courses which weren't backing up, but this script identified 17 orphaned entries across 12 courses. Hope it helps people with this issue!
          Hide
          Rajesh Taneja added a comment -

          As discussed in Scrum, for orphaned entry, rather then throwing exception, moodle will delete the entry and notify user about this (During backup process).

          Please feel free to put your thoughts, if you think there can be a better way to deal with this.

          Show
          Rajesh Taneja added a comment - As discussed in Scrum, for orphaned entry, rather then throwing exception, moodle will delete the entry and notify user about this (During backup process). Please feel free to put your thoughts, if you think there can be a better way to deal with this.
          Hide
          Aparup Banerjee added a comment -

          also noting here (as discuesed in scrum) that the 'extra page/info' for exceptional notifications shouldn't be part of the process normally, it should only appear when there are exceptions/problems.

          Show
          Aparup Banerjee added a comment - also noting here (as discuesed in scrum) that the 'extra page/info' for exceptional notifications shouldn't be part of the process normally, it should only appear when there are exceptions/problems.
          Hide
          Rajesh Taneja added a comment -

          I was implementing this, and it seems we are adding extra functionality to backup process, which is not a responsibility of backup process.
          I think solution for this bug should be:

          1. If orphaned entry exists, then notify user that there are some orphan entries and will not be backed-up, rather then throwing error.
          2. In addition to that, we should point them to orphan entry cleaner (should be handled as different issue).

          Any suggestion?

          Show
          Rajesh Taneja added a comment - I was implementing this, and it seems we are adding extra functionality to backup process, which is not a responsibility of backup process. I think solution for this bug should be: If orphaned entry exists, then notify user that there are some orphan entries and will not be backed-up, rather then throwing error. In addition to that, we should point them to orphan entry cleaner (should be handled as different issue). Any suggestion?
          Hide
          James Yale added a comment -

          It seems reasonable to not automagically do anything as part of the backup process, but I'd worry that if this is only mentioned in the output of the cron.php run it might be missed a long time by admins assuming everything was OK (the cron output can be quite lengthy).

          Could it also flag the problem in the Notifications area in Site Administration in a similar way to how wwwroot errors are flagged?

          Show
          James Yale added a comment - It seems reasonable to not automagically do anything as part of the backup process, but I'd worry that if this is only mentioned in the output of the cron.php run it might be missed a long time by admins assuming everything was OK (the cron output can be quite lengthy). Could it also flag the problem in the Notifications area in Site Administration in a similar way to how wwwroot errors are flagged?
          Hide
          Chris Follin added a comment -

          I, too, have concerns if the orphans aren't fixed and the only message about it is found in cron output. That kind of output will not be seen or heeded by the majority of our admin users and course backups will continue to fail.

          Other than simply not being a responsibility of backup, is there a problem with automatically removing the orphans? I suppose what I'm actually asking is how reliable is the method of identifying the orphans? If there is no problem with it but backup is just not the place to do it, can that functionality be placed elsewhere? If it can be handled somewhere without requiring a user to take action, then it should be. People are prone to not seeing a message, ignoring a message, thinking the message will go away on its own, delaying taking action on the message, etc. and the orphans never get removed. Backups are failing and the time when someone needs a backup is not when they want to discover that they don't have a backup because of these orphans that could have been removed but weren't.

          Show
          Chris Follin added a comment - I, too, have concerns if the orphans aren't fixed and the only message about it is found in cron output. That kind of output will not be seen or heeded by the majority of our admin users and course backups will continue to fail. Other than simply not being a responsibility of backup, is there a problem with automatically removing the orphans? I suppose what I'm actually asking is how reliable is the method of identifying the orphans? If there is no problem with it but backup is just not the place to do it, can that functionality be placed elsewhere? If it can be handled somewhere without requiring a user to take action, then it should be. People are prone to not seeing a message, ignoring a message, thinking the message will go away on its own, delaying taking action on the message, etc. and the orphans never get removed. Backups are failing and the time when someone needs a backup is not when they want to discover that they don't have a backup because of these orphans that could have been removed but weren't.
          Hide
          Rajesh Taneja added a comment -

          There is no problem in automatically removing orphans (IMO), but it is highly discouraged to delete something without user intervention. In saying so, we can always send message about detected orphans or add them to log for admin to look at.
          My proposal is:

          1. Backup should not throw exception, but should notify user about orphan modules (either by logs or mail)
          2. Orphan cleaner should be implemented, which can be configured to delete modules or require user input. Should also be run as part of cron.
          Show
          Rajesh Taneja added a comment - There is no problem in automatically removing orphans (IMO), but it is highly discouraged to delete something without user intervention. In saying so, we can always send message about detected orphans or add them to log for admin to look at. My proposal is: Backup should not throw exception, but should notify user about orphan modules (either by logs or mail) Orphan cleaner should be implemented, which can be configured to delete modules or require user input. Should also be run as part of cron.
          Hide
          Rajesh Taneja added a comment -

          After taking to Eloy, I will be be just providing warning and skipping orphan modules.

          As we don't have any standard way of handling/highlighting errors in backup (except putting debug/throwing exception), I will be creating two improvement issues:

          1. Implement graceful handling/highlighting of errors (especially in backup) and use the same in backup_factory::get_backup_activity_task()
          2. Implement orphan module cleaner (under /admin/tool/), which should be verbose and can be run unattended via cron(if configured to do so)
          Show
          Rajesh Taneja added a comment - After taking to Eloy, I will be be just providing warning and skipping orphan modules. As we don't have any standard way of handling/highlighting errors in backup (except putting debug/throwing exception), I will be creating two improvement issues: Implement graceful handling/highlighting of errors (especially in backup) and use the same in backup_factory::get_backup_activity_task() Implement orphan module cleaner (under /admin/tool/), which should be verbose and can be run unattended via cron(if configured to do so)
          Hide
          Chris Follin added a comment -

          I'm not trying to be difficult but I am trying to understand. This has been a hot button issue for us due to the failing backups and I will need to explain this resolution to other people who will undoubtedly have questions.

          1. How or where will admin users see this warning that is being added in place of the exception?
          2. When the orphaned items are skipped, the rest of the course backs up successfully, correct?
          3. When will the orphan module cleaner be implemented (ballpark estimate)?
          4. Under what circumstances might someone not want the orphaned records to be removed? My understanding is that these records are garbage that can't be used in any way. So, might the records actually not always be garbage? If the orphan is not garbage but it's also not working because it's orphaned, how would someone fix it without deleting it?

          Show
          Chris Follin added a comment - I'm not trying to be difficult but I am trying to understand. This has been a hot button issue for us due to the failing backups and I will need to explain this resolution to other people who will undoubtedly have questions. 1. How or where will admin users see this warning that is being added in place of the exception? 2. When the orphaned items are skipped, the rest of the course backs up successfully, correct? 3. When will the orphan module cleaner be implemented (ballpark estimate)? 4. Under what circumstances might someone not want the orphaned records to be removed? My understanding is that these records are garbage that can't be used in any way. So, might the records actually not always be garbage? If the orphan is not garbage but it's also not working because it's orphaned, how would someone fix it without deleting it?
          Hide
          Rajesh Taneja added a comment -

          Hello Chris,

          1. Depending on developer debugging level (set in site administration), this can be viewed on site. Otherwise it will always be logged in php error logs.
          2. Yes, course backup will successfully run, and will just log errors about skipping orphan modules.
          3. I am not sure when orphan module cleaner will be implemented, will request Michael to push the priority of MDL-31571
          4. I am not aware of any condition, when user might not want to clean orphan module records. But reason for my current approach is:
            • Backup is not meant to clean any bad records. Backup should only report for bad records. Deletion should be handled by responsible framework.
            • Before deleting any record (bad/good), it's always good to report to user, that this information cannot be retrieved after this action. Moodle being a generic framework, we should provide flexibility to let user choose, what is good for them.
          Show
          Rajesh Taneja added a comment - Hello Chris, Depending on developer debugging level (set in site administration), this can be viewed on site. Otherwise it will always be logged in php error logs. Yes, course backup will successfully run, and will just log errors about skipping orphan modules. I am not sure when orphan module cleaner will be implemented, will request Michael to push the priority of MDL-31571 I am not aware of any condition, when user might not want to clean orphan module records. But reason for my current approach is: Backup is not meant to clean any bad records. Backup should only report for bad records. Deletion should be handled by responsible framework. Before deleting any record (bad/good), it's always good to report to user, that this information cannot be retrieved after this action. Moodle being a generic framework, we should provide flexibility to let user choose, what is good for them.
          Hide
          Adi Tedjasaputra added a comment -

          With regards to orphan modules, I think the backup would be more 'friendly' if a warning of orphan module existence (and skipping/deleting the orphan modules) is given. An additional option to proceed or abandon would be nice. As an admin may not be aware about the existence of orphan module(s) in a course when doing the backup, with the warning, the admin will be fully aware of what his/her doing and also inform the course creator if they are not the same person. The orphan module cleaner may tackle this issue, abandoning the requirement for the warning and an additional option, but that would be another discussion.

          Show
          Adi Tedjasaputra added a comment - With regards to orphan modules, I think the backup would be more 'friendly' if a warning of orphan module existence (and skipping/deleting the orphan modules) is given. An additional option to proceed or abandon would be nice. As an admin may not be aware about the existence of orphan module(s) in a course when doing the backup, with the warning, the admin will be fully aware of what his/her doing and also inform the course creator if they are not the same person. The orphan module cleaner may tackle this issue, abandoning the requirement for the warning and an additional option, but that would be another discussion.
          Hide
          Rajesh Taneja added a comment -

          Hello Adi,
          I agree with your argument and we should add a hook for interactive backup, so user can take decision on how they want to proceed with orphan module cleanup.
          Saying so, cleanup should be handled by orphan module cleaner and not backup. Backup should just notify user and point user to orphan module cleaner.

          Will add a note to MDL-31571

          Show
          Rajesh Taneja added a comment - Hello Adi, I agree with your argument and we should add a hook for interactive backup, so user can take decision on how they want to proceed with orphan module cleanup. Saying so, cleanup should be handled by orphan module cleaner and not backup. Backup should just notify user and point user to orphan module cleaner. Will add a note to MDL-31571
          Hide
          Chris Follin added a comment -

          Thank you for the answers, Rajesh. The backups completing successfully minus the orphans will resolve about 95% of our issue. The other 5% is in how users are notified of these warnings or prompted to make a decision. There seems to be an assumption that backups are being run manually by a human user. That does happen but it is far less common in our environment than making backups via the overnight automated backups. I'm not sure that those warnings would ever be seen. Our clients also don't have access to PHP error logs, so those won't be seen either. This is why something automated is important to us and if the orphaned module cleaner can be configured to run via cron then that will meet that need. Of course this does depend on the orphaned module cleaner being developed so I'll follow up with Michael about 31571.

          Thanks again.

          Show
          Chris Follin added a comment - Thank you for the answers, Rajesh. The backups completing successfully minus the orphans will resolve about 95% of our issue. The other 5% is in how users are notified of these warnings or prompted to make a decision. There seems to be an assumption that backups are being run manually by a human user. That does happen but it is far less common in our environment than making backups via the overnight automated backups. I'm not sure that those warnings would ever be seen. Our clients also don't have access to PHP error logs, so those won't be seen either. This is why something automated is important to us and if the orphaned module cleaner can be configured to run via cron then that will meet that need. Of course this does depend on the orphaned module cleaner being developed so I'll follow up with Michael about 31571. Thanks again.
          Hide
          Ankit Agarwal added a comment -

          Hi Raj,
          Code looks good to me.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Raj, Code looks good to me. Thanks
          Hide
          Rajesh Taneja added a comment -

          Thanks Ankit,
          Pushing if for integration review.

          Show
          Rajesh Taneja added a comment - Thanks Ankit, Pushing if for integration review.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've just been looking at this now.
          Code to me looks fine (haven't tested but I can't see anything wrong with it) however I think it is probably best that Eloy looks at this.
          I'm not sure about the approach myself, this will fix backups of course, but surely backups are only the start of the problems if you have a widowed cm record?
          I think part of the solution for this issue should be the development of a tool to identify and clean up orphaned information that can be run across courses on a site.
          Anyway food for thought, leaving this for Eloy.

          Actually also worth thinking about do we want to continue on with a backup if there is a cm without matching module record? What if there are other records still relating to the cm do we risk trying to include further information later? Perhaps Eloy is the best person to answer that.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've just been looking at this now. Code to me looks fine (haven't tested but I can't see anything wrong with it) however I think it is probably best that Eloy looks at this. I'm not sure about the approach myself, this will fix backups of course, but surely backups are only the start of the problems if you have a widowed cm record? I think part of the solution for this issue should be the development of a tool to identify and clean up orphaned information that can be run across courses on a site. Anyway food for thought, leaving this for Eloy. Actually also worth thinking about do we want to continue on with a backup if there is a cm without matching module record? What if there are other records still relating to the cm do we risk trying to include further information later? Perhaps Eloy is the best person to answer that. Cheers Sam
          Hide
          Adam Olley added a comment -

          Sam, such a tool is already in development and appears to be linked MDL-31571

          Show
          Adam Olley added a comment - Sam, such a tool is already in development and appears to be linked MDL-31571
          Hide
          Sam Hemelryk added a comment -

          Ok great, thanks for pointing that out Adam, glad to see something is already planned there!

          Show
          Sam Hemelryk added a comment - Ok great, thanks for pointing that out Adam, glad to see something is already planned there!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I think it's the correct place to intercept the problems, both with missing modules and block instances and log&skip them. Cannot imagine any regression caused by that as far as it's where the plan is built so, if no task is added, nothing related to that missing activity/block is processed.

          My only concern... is if we should be translating those strings or no. The rest of backup have all the exceptions/errors (but the UI ones) untranslated, and some ideas about how to format all them into backup_exception are ongoing... so perhaps it would be better to, simply, log the string key and done? Not 100% sure, though, grrr.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I think it's the correct place to intercept the problems, both with missing modules and block instances and log&skip them. Cannot imagine any regression caused by that as far as it's where the plan is built so, if no task is added, nothing related to that missing activity/block is processed. My only concern... is if we should be translating those strings or no. The rest of backup have all the exceptions/errors (but the UI ones) untranslated, and some ideas about how to format all them into backup_exception are ongoing... so perhaps it would be better to, simply, log the string key and done? Not 100% sure, though, grrr. Ciao
          Hide
          Rajesh Taneja added a comment -

          Thanks Eloy,
          Reason for descriptive string was to log module id, so admin can easily look at records (if required).
          Please advice, if I should change the strings, and it will be helpful if you can please advice why it's not recommended to have translated strings in backup.

          Show
          Rajesh Taneja added a comment - Thanks Eloy, Reason for descriptive string was to log module id, so admin can easily look at records (if required). Please advice, if I should change the strings, and it will be helpful if you can please advice why it's not recommended to have translated strings in backup.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Raj,

          really the error-strings handling @ backup is a looong story, with several changes since backup began to be built, caused by changes in string managing, first not allowing "underscored" string keys, later allowing them... That has caused current error/exception messages to be really heterogeneous in backup right now (some have underscores, others don't. Some have translations, others don't.

          So, my current idea, to implement is something like:

          0) use "underscored" strings for all errors / logs.
          1) not require to translate them at all.
          2) throw backup/restore exceptions normally, by passing the string key, the $a and the $debuginfo.
          3) provide a custom formatter to the base backup_exception (or perhaps to the moodle_exception or to the default exception handler), about to be able to show $a, no matter if the string has translation or no. Surely if the exception has translation it will be used too. But the string key will be shown too, always.
          4) use the same formatter when exceptions are logged into backup logs.

          So, as you can see, the idea is to make the translation of those error/exception messages not necessary, mainly for easier identification of the problem (the string key). And also show the $a information always, no matter if the string has translation or no (I think this is the biggest problem right now, because moodle_exception simply discards the $a if no translation is available.

          Said that, here are my doubts, about adding those new strings if later all we agree that they are not going to be necessary. Although, in the other side, we can make the "formatter" to work both with translated and untranslated strings.

          Sorry if all the explanations above are totally unrelated to this issue, I am just trying to explain how I'm seeing the future of those error string keys in backup.

          It is about the task of improving output/logging of errors, that has a lot of related issues @ the backup & restore 2.0 meta: MDL-21432 (like MDL-22143, MDL-22149, MDL-23686, MDL-25313... and others here and there).

          So surely... and in summary, I think that it would be perfect if you:

          0) keep everything as is now, but
          1) change the string keys to "underscored" ones.

          that would be enough for the future, no matter if finally we decide to require or ignore the translation of backup error strings.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Raj, really the error-strings handling @ backup is a looong story, with several changes since backup began to be built, caused by changes in string managing, first not allowing "underscored" string keys, later allowing them... That has caused current error/exception messages to be really heterogeneous in backup right now (some have underscores, others don't. Some have translations, others don't. So, my current idea, to implement is something like: 0) use "underscored" strings for all errors / logs. 1) not require to translate them at all. 2) throw backup/restore exceptions normally, by passing the string key, the $a and the $debuginfo. 3) provide a custom formatter to the base backup_exception (or perhaps to the moodle_exception or to the default exception handler), about to be able to show $a, no matter if the string has translation or no. Surely if the exception has translation it will be used too. But the string key will be shown too, always. 4) use the same formatter when exceptions are logged into backup logs. So, as you can see, the idea is to make the translation of those error/exception messages not necessary, mainly for easier identification of the problem (the string key). And also show the $a information always, no matter if the string has translation or no (I think this is the biggest problem right now, because moodle_exception simply discards the $a if no translation is available. Said that, here are my doubts, about adding those new strings if later all we agree that they are not going to be necessary. Although, in the other side, we can make the "formatter" to work both with translated and untranslated strings. Sorry if all the explanations above are totally unrelated to this issue, I am just trying to explain how I'm seeing the future of those error string keys in backup. It is about the task of improving output/logging of errors, that has a lot of related issues @ the backup & restore 2.0 meta: MDL-21432 (like MDL-22143 , MDL-22149 , MDL-23686 , MDL-25313 ... and others here and there). So surely... and in summary, I think that it would be perfect if you: 0) keep everything as is now, but 1) change the string keys to "underscored" ones. that would be enough for the future, no matter if finally we decide to require or ignore the translation of backup error strings. Ciao
          Hide
          Aparup Banerjee added a comment -

          maybe there is shared need for a directory within lang when translations are not required? ie: the need to mark string that don't need translation, see:

          http://tracker.moodle.org/browse/MDL-31567?focusedCommentId=143676&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-143676

          Show
          Aparup Banerjee added a comment - maybe there is shared need for a directory within lang when translations are not required? ie: the need to mark string that don't need translation, see: http://tracker.moodle.org/browse/MDL-31567?focusedCommentId=143676&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-143676
          Hide
          Rajesh Taneja added a comment -

          Thanks Eloy and Aparup,

          I have changed string keys to "underscored" ones (as suggested), not sure if there is any naming convention which I missed.
          Now, the strings are getting translated, but string keys are with underscores.

          Show
          Rajesh Taneja added a comment - Thanks Eloy and Aparup, I have changed string keys to "underscored" ones (as suggested), not sure if there is any naming convention which I missed. Now, the strings are getting translated, but string keys are with underscores.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks, Rajesh. I think that's enough for any future we finally implement related with backup output/logging/exception. So +1.

          Re: yup, Apu. It's somehow similar to the currencies one (MDL-31567), but at least in this case, it's under our control to decide if those are really "string keys" or no. I mean, we assume that they are keys just because moodle_exception uses them that way. If finally we intercept such usage in backup_exception, changing it by an alternative formatter... then they won't be string keys anymore. Let's see how it goes.

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks, Rajesh. I think that's enough for any future we finally implement related with backup output/logging/exception. So +1. Re: yup, Apu. It's somehow similar to the currencies one ( MDL-31567 ), but at least in this case, it's under our control to decide if those are really "string keys" or no. I mean, we assume that they are keys just because moodle_exception uses them that way. If finally we intercept such usage in backup_exception, changing it by an alternative formatter... then they won't be string keys anymore. Let's see how it goes.
          Hide
          Rajesh Taneja added a comment -

          Branches re-based

          Show
          Rajesh Taneja added a comment - Branches re-based
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (21, 22 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (21, 22 & master), thanks!
          Hide
          Gerard Caulfield added a comment -

          All worked well on master, 22 and 21 including the "Orphan course module (id: X) found. This module will not be backed up." error mentioned in step 7 of the testing instructions. Passed.

          Show
          Gerard Caulfield added a comment - All worked well on master, 22 and 21 including the "Orphan course module (id: X) found. This module will not be backed up." error mentioned in step 7 of the testing instructions. Passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some changes to Moodle should be milestones in the project by themselves.

          This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Some changes to Moodle should be milestones in the project by themselves. This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks! Closing as fixed, ciao

            People

            • Votes:
              24 Vote for this issue
              Watchers:
              26 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: