Moodle

Uncontrolled echoes in restorelib.php. breaks remote silent restore

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Won't Fix
  • Affects Version/s: 1.9.3
  • Fix Version/s: None
  • Component/s: Feedback
  • Labels:
    None
  • Difficulty:
    Easy
  • Affected Branches:
    MOODLE_19_STABLE

Description

Two identified locations with echoes that are not checked about RESTORE_SILENTLY.

may breaks XML-RPC transaction when remote restoring on a remote moodle.

Activity

Hide
Mihai Sucan added a comment -

I looked into the code of backup/restorelib.php file and I found uncontrolled output on line 4322 in the restore_log_course() function and on line 4401 in the restore_log_user() function. For both, the echo was noted as a 'Debug' one, as such I took the liberty of commenting the entire echo, such that it won't cause any problems. (many other 'Debug' echoes are already commented-out)

In function startElement() on line 5202 there's a debug echo as well. Another debug echo is in the endElement() function. Both functions seem to be unused. The XML parsing which starts at line 7442 does not associate as a tag parser those two functions. I chose to comment out the debug echoes in both functions (startElement and endElement), just in case.

The attached patch file is made against the current cvs MOODLE_19_WEEKLY. To apply it:

cd moodle
patch p0 < patchMDL-17254.diff

Show
Mihai Sucan added a comment - I looked into the code of backup/restorelib.php file and I found uncontrolled output on line 4322 in the restore_log_course() function and on line 4401 in the restore_log_user() function. For both, the echo was noted as a 'Debug' one, as such I took the liberty of commenting the entire echo, such that it won't cause any problems. (many other 'Debug' echoes are already commented-out) In function startElement() on line 5202 there's a debug echo as well. Another debug echo is in the endElement() function. Both functions seem to be unused. The XML parsing which starts at line 7442 does not associate as a tag parser those two functions. I chose to comment out the debug echoes in both functions (startElement and endElement), just in case. The attached patch file is made against the current cvs MOODLE_19_WEEKLY. To apply it: cd moodle patch p0 < patchMDL-17254.diff
Hide
Mihai Sucan added a comment -

I would also like to note that question/restorelib.php does output error messages, regardless of the RESTORE_SILENTLY constant. I believe that a silent restore should not output any errors at all. All errors should go into some log that the user can check when desired. All the other restorelib.php files do not output any errors.

Show
Mihai Sucan added a comment - I would also like to note that question/restorelib.php does output error messages, regardless of the RESTORE_SILENTLY constant. I believe that a silent restore should not output any errors at all. All errors should go into some log that the user can check when desired. All the other restorelib.php files do not output any errors.
Hide
Dan Marsden added a comment -

Hi Mihai,

commenting the lines out isn't quite the right fix, check out how the other error messages are printed in the other restorelib.php files - they should check if not running RESTORE_SILENTLY and print to the screen.

great point about the error log! - the backup logging in 2.0 has improved slightly so that all errors can be saved to the backup_log table - only a small amount of the errors have been fixed to write to the backup_log table at this stage, but we should add the others as well! - we can't make this improvement to 1.9 as it requires an extra db column in the backup_log table (already in 2.0) as no database changes can be made to Stable versions of Moodle - all database schema changes can only be done in new releases.

So Ideally - there should be 2 patches for this bug.

1st patch for 1.9 checking if restore_silently isn't used and printing the error to the screen

2nd patch for 2.0 doing the same check but always writing to the backup_log table - from memory there's a function in backup/lib.php to facilitate this.

Dan

Show
Dan Marsden added a comment - Hi Mihai, commenting the lines out isn't quite the right fix, check out how the other error messages are printed in the other restorelib.php files - they should check if not running RESTORE_SILENTLY and print to the screen. great point about the error log! - the backup logging in 2.0 has improved slightly so that all errors can be saved to the backup_log table - only a small amount of the errors have been fixed to write to the backup_log table at this stage, but we should add the others as well! - we can't make this improvement to 1.9 as it requires an extra db column in the backup_log table (already in 2.0) as no database changes can be made to Stable versions of Moodle - all database schema changes can only be done in new releases. So Ideally - there should be 2 patches for this bug. 1st patch for 1.9 checking if restore_silently isn't used and printing the error to the screen 2nd patch for 2.0 doing the same check but always writing to the backup_log table - from memory there's a function in backup/lib.php to facilitate this. Dan
Hide
Mihai Sucan added a comment -

New patch for MOODLE_19_WEEKLY as suggested.

Show
Mihai Sucan added a comment - New patch for MOODLE_19_WEEKLY as suggested.
Hide
Mihai Sucan added a comment -

Thanks for your feedback and sorry for the delay. I've been working on my GSoC project proposal.

I have looked into Moodle 2 from CVS HEAD. I see that backup/lib.php has the add_to_backup_log() function. This is the one you are referring to, I believe.

I have some questions before I proceed:

1. Where do I get the course ID from? I see there's $SESSION->restore->course_id and the global clone $restore->course_id (which I presume is deprecated, since it has a comment "ick..."). Is this correct?

2. What do I do with the error message? I only add it to the backup log or do i additionally output the error to the user if RESTORE_SILENTLY is not defined?

3. If I also output the error to the user (when RESTORE_SILENTLY is undefined), which function do I use to output the error message? I see print_error() and notify(). Or I simply use echo as now? I am asking this because I saw in other places that these functions are sometimes used instead of echo.

4. What string should I provide for the $backuptype argument? I presume 'restore' should be fine.

Please let me know how you believe it's best to proceed, and I'll prepare a patch for Moodle 2.

Show
Mihai Sucan added a comment - Thanks for your feedback and sorry for the delay. I've been working on my GSoC project proposal. I have looked into Moodle 2 from CVS HEAD. I see that backup/lib.php has the add_to_backup_log() function. This is the one you are referring to, I believe. I have some questions before I proceed: 1. Where do I get the course ID from? I see there's $SESSION->restore->course_id and the global clone $restore->course_id (which I presume is deprecated, since it has a comment "ick..."). Is this correct? 2. What do I do with the error message? I only add it to the backup log or do i additionally output the error to the user if RESTORE_SILENTLY is not defined? 3. If I also output the error to the user (when RESTORE_SILENTLY is undefined), which function do I use to output the error message? I see print_error() and notify(). Or I simply use echo as now? I am asking this because I saw in other places that these functions are sometimes used instead of echo. 4. What string should I provide for the $backuptype argument? I presume 'restore' should be fine. Please let me know how you believe it's best to proceed, and I'll prepare a patch for Moodle 2.
Hide
Michael de Raadt added a comment -

Thanks for reporting this issue.

We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.

If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

Michael d;

lqjjLKA0p6

Show
Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d; lqjjLKA0p6
Hide
Michael de Raadt added a comment -

I'm closing this issue as it appears to have become inactive and is probably not relevant to a current supported version. If you are encountering this problem or one similar, please launch a new issue.

Show
Michael de Raadt added a comment - I'm closing this issue as it appears to have become inactive and is probably not relevant to a current supported version. If you are encountering this problem or one similar, please launch a new issue.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: