Moodle
  1. Moodle
  2. MDL-30434

Define how we should handle restore exceptions like error_question_answers_missing_in_db

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.2
    • Fix Version/s: 2.3
    • Component/s: Backup, Language
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Create a quiz.

      Add a multiple choice question with at least two answers.

      perform a full backup of the course.

      modify the code to get an exception to be thrown.
      in backup/moodle2/restore_qtype_plugin.class.php find process_question_answer()
      change if ($questioncreated) { to if (false) {
      comment out if (!$newitemid)

      { and the matching }

      Restore the backup into a new course. The exception error message should include some debug information.

      Finally, revert your code changes and perform another restore. This one should complete without any errors.

      Show
      Create a quiz. Add a multiple choice question with at least two answers. perform a full backup of the course. modify the code to get an exception to be thrown. in backup/moodle2/restore_qtype_plugin.class.php find process_question_answer() change if ($questioncreated) { to if (false) { comment out if (!$newitemid) { and the matching } Restore the backup into a new course. The exception error message should include some debug information. Finally, revert your code changes and perform another restore. This one should complete without any errors.
    • Workaround:
      Hide

      Add a line to lang/en/error.php such as:

      $string['error_question_answers_missing_in_db'] = 'Unable to import question {$a->filequestionid} into database as {$a->dbquestionid} because answer "{$a->answer}" did not import correctly.';

      Being able to tell which question and caused the exception would make it much easier to troubleshoot this problem in the future.

      Show
      Add a line to lang/en/error.php such as: $string ['error_question_answers_missing_in_db'] = 'Unable to import question {$a->filequestionid} into database as {$a->dbquestionid} because answer "{$a->answer}" did not import correctly.'; Being able to tell which question and caused the exception would make it much easier to troubleshoot this problem in the future.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-30434_debug_exception
    • Rank:
      33093

      Description

      The language string for the backup restore question error: error_question_answers_missing_in_db is missing. The error has shown up in at least 3 different bugs in the tracker.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for spotting that and providing a solution.

          Show
          Michael de Raadt added a comment - Thanks for spotting that and providing a solution.
          Hide
          Andrew Davis added a comment -

          I've created a branch that includes this new string. I've also added a very similar string for another exception I came across while doing some grepping.

          The exception name (which is used to find the exception string) does not match what seems to be the naming convention in /lang/en/errors.php I'm not sure whether its a good idea to change the name to match the string naming convention as I'm unsure of what else it is used for.

          I'm putting this up for peer review by Eloy as this is his area of expertise so hopefully he has some wise words.

          Show
          Andrew Davis added a comment - I've created a branch that includes this new string. I've also added a very similar string for another exception I came across while doing some grepping. The exception name (which is used to find the exception string) does not match what seems to be the naming convention in /lang/en/errors.php I'm not sure whether its a good idea to change the name to match the string naming convention as I'm unsure of what else it is used for. I'm putting this up for peer review by Eloy as this is his area of expertise so hopefully he has some wise words.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Andrew,

          I think your patch is ok, but let me explain which was the idea with all the possible backup/restore execution exceptions.

          1) I created them with underscores on purpose because in the main backup/restore_exception all I wanted to do is to perform one "string_exists" and, if not present, show the error_code and one string version of the $a param. So exceptions would look like:

          An exception has happened: "error_question_answers_missing_in_db", info: [questionid => 38, anwer => 346]

          Because I was not sure if it has sense to translate all those error messages or no, so aiming to readability I decided to put the underscores. Note that, at the same time, David was developing the new string manager and underscores were initially not allowed, so I raised MDL-22149, but finally they became perfectly valid.

          But, after that... I got the initial idea of tidying the output completely out of the radar (flooded with the rest of backup/restore issues).

          So perhaps this could be a good moment to implement it so, instead of beginning to translate backup/restore execution exceptions, we provide the information by converting the information and passing it as $debug to moodle_exception or something like that. Or make the backup/restore exceptions to inherit from coding_exception, to avoid translation, or override as much as possible from moodle_exception to make them work as desired.

          How does that alternative sound? It has been in my mind since ages ago, but I always find one excuse (other tasks, pain to start discussing about...) to avoid it.

          So perhaps it could be a good moment to worth discussing about that to finally do it in the better way before adding any translation. In fact I think there is some issue about that somewhere, but it seems that my search abilities are not working today, grrrr.

          +1 to introduce the issue in next HQ meeting, to decide about "untranslated + customized" or "translated". Surely translated means that we can take out the underscores if we want to.

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Andrew, I think your patch is ok, but let me explain which was the idea with all the possible backup/restore execution exceptions. 1) I created them with underscores on purpose because in the main backup/restore_exception all I wanted to do is to perform one "string_exists" and, if not present, show the error_code and one string version of the $a param. So exceptions would look like: An exception has happened: "error_question_answers_missing_in_db", info: [questionid => 38, anwer => 346] Because I was not sure if it has sense to translate all those error messages or no, so aiming to readability I decided to put the underscores. Note that, at the same time, David was developing the new string manager and underscores were initially not allowed, so I raised MDL-22149 , but finally they became perfectly valid. But, after that... I got the initial idea of tidying the output completely out of the radar (flooded with the rest of backup/restore issues). So perhaps this could be a good moment to implement it so, instead of beginning to translate backup/restore execution exceptions, we provide the information by converting the information and passing it as $debug to moodle_exception or something like that. Or make the backup/restore exceptions to inherit from coding_exception, to avoid translation, or override as much as possible from moodle_exception to make them work as desired. How does that alternative sound? It has been in my mind since ages ago, but I always find one excuse (other tasks, pain to start discussing about...) to avoid it. So perhaps it could be a good moment to worth discussing about that to finally do it in the better way before adding any translation. In fact I think there is some issue about that somewhere, but it seems that my search abilities are not working today, grrrr. +1 to introduce the issue in next HQ meeting, to decide about "untranslated + customized" or "translated". Surely translated means that we can take out the underscores if we want to. Thanks and ciao
          Hide
          Andrew Davis added a comment -

          Ok, Ill park this issue for the time being and we can work out what we're doing later.

          Show
          Andrew Davis added a comment - Ok, Ill park this issue for the time being and we can work out what we're doing later.
          Hide
          Andrew Davis added a comment -

          Improved testing instructions.

          Show
          Andrew Davis added a comment - Improved testing instructions.
          Hide
          Andrew Davis added a comment -

          Two possible alternative fixes.

          1) add smarts to the exception and/or exception handling to allow it to produce debug info even without a string. This implementation of this idea may or may not be the best way to go about this.
          https://github.com/andyjdavis/moodle/compare/master...MDL-30434_restore_exception_alt

          2) add strings.
          https://github.com/andyjdavis/moodle/compare/master...MDL-30434_error_string

          Show
          Andrew Davis added a comment - Two possible alternative fixes. 1) add smarts to the exception and/or exception handling to allow it to produce debug info even without a string. This implementation of this idea may or may not be the best way to go about this. https://github.com/andyjdavis/moodle/compare/master...MDL-30434_restore_exception_alt 2) add strings. https://github.com/andyjdavis/moodle/compare/master...MDL-30434_error_string
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I really would prefer to follow the 1) route, avoiding to introduce translations for those errors.

          Pasted from chat, for reference:

          Ho... I had ideas to something similar to that, but following this approach:

          1) Find all the top-level (direct child of moodle_exception) exceptions in /backup
          2) Surely at some time in the future they will be reorganized, making them descendant of backup_exception but not now.
          3) To all those top-level exceptions:

          A) Look if string exists.
          B) If exists, normal use of $errorcode, $a
          C) If not exists, make: $e->debuginfo = $errorcode + $a (into a readable way)+ $debuginfo (to preserve it if existed). So any backup-related exception will, always show something like (unreal example):

          • If the string exists:

          Exception: Cannot restore answer 18 because question 22 does not exist
          Debuginfo: Verify if the water is hot.

          • if the string does not exist:

          "Exception: error/question_db_missing_answer
          Debuginfo: Error "question_db_missing_answer" with data

          Unknown macro: {a->answerid = 18, a->questionid = 22}

          found. Verify if the water is hot.

          That was my idea, more or less. Basically, if the string does not exists, then accumulate everythig in the $debuginfo (keeping the original $debuginfo at the end) and done.

          I really prefer not to get those errors translated.

          Also, we should have all exceptions inheriting from backup_exception (so that code only needed to be implemented once but, for now, I guess we'll need to replicate it into all top-level backup exceptions.

          Also, there is one alternative, it's about to implement that code, not only for backup exceptions but, into the main moodle_exception, so every exception without translation continues showing the $a and $debuginfo. Surely this should be implemented in the get_exception_info() function. Or in the default exception handler.

          So I think that implementing the behavior described above (A, B, C) is a correct way to continue showing all the information of the error, no matter if the translation exists or no.

          The only point is to decide if we want this implemented in backup_exception_xxx classes (that causes our hacking possibilitites to be reduced to the ->debuginfo) or if we want that in globally, in core get_exception_info/default_exception_hadler, so we can hack the message instead.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I really would prefer to follow the 1) route, avoiding to introduce translations for those errors. Pasted from chat, for reference: Ho... I had ideas to something similar to that, but following this approach: 1) Find all the top-level (direct child of moodle_exception) exceptions in /backup 2) Surely at some time in the future they will be reorganized, making them descendant of backup_exception but not now. 3) To all those top-level exceptions: A) Look if string exists. B) If exists, normal use of $errorcode, $a C) If not exists, make: $e->debuginfo = $errorcode + $a (into a readable way)+ $debuginfo (to preserve it if existed). So any backup-related exception will, always show something like (unreal example): If the string exists: Exception: Cannot restore answer 18 because question 22 does not exist Debuginfo: Verify if the water is hot. if the string does not exist: "Exception: error/question_db_missing_answer Debuginfo: Error "question_db_missing_answer" with data Unknown macro: {a->answerid = 18, a->questionid = 22} found. Verify if the water is hot. That was my idea, more or less. Basically, if the string does not exists, then accumulate everythig in the $debuginfo (keeping the original $debuginfo at the end) and done. I really prefer not to get those errors translated. Also, we should have all exceptions inheriting from backup_exception (so that code only needed to be implemented once but, for now, I guess we'll need to replicate it into all top-level backup exceptions. Also, there is one alternative, it's about to implement that code, not only for backup exceptions but, into the main moodle_exception, so every exception without translation continues showing the $a and $debuginfo. Surely this should be implemented in the get_exception_info() function. Or in the default exception handler. So I think that implementing the behavior described above (A, B, C) is a correct way to continue showing all the information of the error, no matter if the translation exists or no. The only point is to decide if we want this implemented in backup_exception_xxx classes (that causes our hacking possibilitites to be reduced to the ->debuginfo) or if we want that in globally, in core get_exception_info/default_exception_hadler, so we can hack the message instead. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Adding Petr here to see his opinion about to implement the ABC behavior into core exceptions handling, for any errorcode missing translation.

          Show
          Eloy Lafuente (stronk7) added a comment - Adding Petr here to see his opinion about to implement the ABC behavior into core exceptions handling, for any errorcode missing translation.
          Hide
          Petr Škoda added a comment -

          Hmm, I do not like any current error strings much, either the exception is expected (wrong) and we need a good string explaining it or it is a weird problem that even the dev does not know what is going on (usually you need debug trace to find the source of the problem). I do not believe that ordinary users need to know the problem details, localising complex error messages makes resolution/bugfixing even harder because you can not even google it.

          I would personally use very general error messages together with error codes that can be looked up easily both in code (developer looking for the place where it is happening) and in docs wiki (for users/admins trying to find solution). This would also eliminate the error translation madness, devs could add tons of error codes easily.

          Example:

          • all dml exceptions could just say "Database error (ME-xxxxxxx)" where ME-00511 is more detailed error code (not just read/write/execute)

          Benefits:

          • less end user confusion
          • admins can search for solutions easier
          • developers would know more precise location without debug stack trace
          • a lot less work for translators
          • it would be easier to search logs

          Please do not try to expose error details to ordinary users and simply add the "error_question_answers_missing_in_db" to debuginfo and use general "Question error" error string. The point is we intentionally hide debug info from users for security reasons, either use debug mode or go through the logs to find the problem details later.

          Show
          Petr Škoda added a comment - Hmm, I do not like any current error strings much, either the exception is expected (wrong) and we need a good string explaining it or it is a weird problem that even the dev does not know what is going on (usually you need debug trace to find the source of the problem). I do not believe that ordinary users need to know the problem details, localising complex error messages makes resolution/bugfixing even harder because you can not even google it. I would personally use very general error messages together with error codes that can be looked up easily both in code (developer looking for the place where it is happening) and in docs wiki (for users/admins trying to find solution). This would also eliminate the error translation madness, devs could add tons of error codes easily. Example: all dml exceptions could just say "Database error (ME-xxxxxxx)" where ME-00511 is more detailed error code (not just read/write/execute) Benefits: less end user confusion admins can search for solutions easier developers would know more precise location without debug stack trace a lot less work for translators it would be easier to search logs Please do not try to expose error details to ordinary users and simply add the "error_question_answers_missing_in_db" to debuginfo and use general "Question error" error string. The point is we intentionally hide debug info from users for security reasons, either use debug mode or go through the logs to find the problem details later.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Well I basically agree with you 100%:

          • It's better not to translate exception errorcodes.
          • Unless it is one "expected exception" that ideally should have its own output instead of relying on default_exception_handler output.
          • Extra information must go to the $debuginfo member.

          Said that, I also think that:

          • current $errorcode is already a good error code, we don't need to invent any ME-XXXXX that would need extra documentation / new pages / listing somewhere. Current one is already both 1) one code and 2) one short description (1-long-word).

          So, the question is, simply if, for developers (DEBUG_DEV enabled) we want to change how the default_exception_handler() behaves - in fact I think the hack should happen @ get_exception_info() - so $debuginfo would contain also the human_readable($a) contents to give devs more information.

          Note that, right now it does:

                  ....
                  ....
                  } elseif ($module == 'error' && get_string_manager()->string_exists($errorcode, 'moodle')) {
                      // Search in moodle file if error specified - needed for backwards compatibility
                      $message = get_string($errorcode, 'moodle', $a);
                  } else {
                      $message = $module . '/' . $errorcode;   <<<==== no translation, $a is lost
                  }
                  ....
                  ....
          

          So, if there is no translation, $a is 100% lost. I'm simply proposing to add that lost $a to $debugging, so at the end that information will be shown to developers. As commented, that can be achieved in the get_exception_info(), where we already have performed the string_exists() or later by the different outputs called by the handler.

          I assume that, from a backup POV, all those exceptions are "unexpected" ones, like "error_question_answers_missing_in_db" because it's something impossible to pre-detect until you arrive to it @ process time. And it would be great to have $a contents included in output (as part of the $debuginfo) while keeping the errorcode untranslated.

          Hence my question/proposal if that behavior (adding $a to $debuginfo in case of missing translation) is a general acceptable idea to implement in core, as explained above, or if I should keep it only inside backup_exceptions. Just that.

          That apart from any other exceptions consideration, that really needs to be discussed and fixed, because I think we are relying too much on exceptions to output expected errors and I'm sure that is not a good thing. But this issue is not about that.

          My +1 goes to apply the "$a goes to $debuginfo for untranslated" globally at core, so we (devs) won't miss $a information anymore.

          Ciao

          PS: More yet, I would add also always the $errorcode to $debuginfo, so, for translated $errorcodes we also get the original $errorcode in output, for easier detection.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Well I basically agree with you 100%: It's better not to translate exception errorcodes. Unless it is one "expected exception" that ideally should have its own output instead of relying on default_exception_handler output. Extra information must go to the $debuginfo member. Said that, I also think that: current $errorcode is already a good error code, we don't need to invent any ME-XXXXX that would need extra documentation / new pages / listing somewhere. Current one is already both 1) one code and 2) one short description (1-long-word). So, the question is, simply if, for developers (DEBUG_DEV enabled) we want to change how the default_exception_handler() behaves - in fact I think the hack should happen @ get_exception_info() - so $debuginfo would contain also the human_readable($a) contents to give devs more information. Note that, right now it does: .... .... } elseif ($module == 'error' && get_string_manager()->string_exists($errorcode, 'moodle')) { // Search in moodle file if error specified - needed for backwards compatibility $message = get_string($errorcode, 'moodle', $a); } else { $message = $module . '/' . $errorcode; <<<==== no translation, $a is lost } .... .... So, if there is no translation, $a is 100% lost. I'm simply proposing to add that lost $a to $debugging, so at the end that information will be shown to developers. As commented, that can be achieved in the get_exception_info(), where we already have performed the string_exists() or later by the different outputs called by the handler. I assume that, from a backup POV, all those exceptions are "unexpected" ones, like "error_question_answers_missing_in_db" because it's something impossible to pre-detect until you arrive to it @ process time. And it would be great to have $a contents included in output (as part of the $debuginfo) while keeping the errorcode untranslated. Hence my question/proposal if that behavior (adding $a to $debuginfo in case of missing translation) is a general acceptable idea to implement in core, as explained above, or if I should keep it only inside backup_exceptions. Just that. That apart from any other exceptions consideration, that really needs to be discussed and fixed, because I think we are relying too much on exceptions to output expected errors and I'm sure that is not a good thing. But this issue is not about that. My +1 goes to apply the "$a goes to $debuginfo for untranslated" globally at core, so we (devs) won't miss $a information anymore. Ciao PS: More yet, I would add also always the $errorcode to $debuginfo, so, for translated $errorcodes we also get the original $errorcode in output, for easier detection.
          Hide
          Andrew Davis added a comment -

          Here is a diff that shows my implementation of this idea (I believe). https://github.com/andyjdavis/moodle/compare/master...MDL-30434_debug_exception

          Im also attaching 2 screenshots. One with debugging on, one with it off. Just so you guys can see what the output looks like with this change.

          Show
          Andrew Davis added a comment - Here is a diff that shows my implementation of this idea (I believe). https://github.com/andyjdavis/moodle/compare/master...MDL-30434_debug_exception Im also attaching 2 screenshots. One with debugging on, one with it off. Just so you guys can see what the output looks like with this change.
          Hide
          Andrew Davis added a comment -

          Changing the title of the issue to reflect what is going on.

          Show
          Andrew Davis added a comment - Changing the title of the issue to reflect what is going on.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          I think that, basically that's the idea. To add the error_code and the $a to debugging always, so it's shown and never lost.

          Not sure if print_r() is the best, or we need some other formatter (and also some label for the $a). And also there is one missing whitespace after "Error code:".

          But the idea is, simply that, to be shown with debug developer enabled.

          Any comment, Petr? Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited I think that, basically that's the idea. To add the error_code and the $a to debugging always, so it's shown and never lost. Not sure if print_r() is the best, or we need some other formatter (and also some label for the $a). And also there is one missing whitespace after "Error code:". But the idea is, simply that, to be shown with debug developer enabled. Any comment, Petr? Ciao
          Hide
          Michael de Raadt added a comment -

          Carrying over to the new sprint.

          Show
          Michael de Raadt added a comment - Carrying over to the new sprint.
          Hide
          Andrew Davis added a comment -

          Added the missing white space. Will think more about this tomorrow. Any further feedback is welcome.

          Show
          Andrew Davis added a comment - Added the missing white space. Will think more about this tomorrow. Any further feedback is welcome.
          Hide
          Andrew Davis added a comment -

          Added a label for the contents of $a. I also did some experimenting with various ways to output that stuff. None were significantly better than each other. I could just have this loop through and output keys and values but not sure its worth the bother.

          Show
          Andrew Davis added a comment - Added a label for the contents of $a. I also did some experimenting with various ways to output that stuff. None were significantly better than each other. I could just have this loop through and output keys and values but not sure its worth the bother.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Two little comments...

          1) I would not add any harcoded \r\n, but PHP_EOL instead. And also I would not add any trailing one, but only leading ones both to errorcode (like it has now) and $a contents (missing now).

          2) I continue not really happy with the print_r() display (especially because all that "Array {..." nesting (looks ugly), but it's better than nothing so, unless anybody has anything against it... np here.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Two little comments... 1) I would not add any harcoded \r\n, but PHP_EOL instead. And also I would not add any trailing one, but only leading ones both to errorcode (like it has now) and $a contents (missing now). 2) I continue not really happy with the print_r() display (especially because all that "Array {..." nesting (looks ugly), but it's better than nothing so, unless anybody has anything against it... np here. Ciao
          Hide
          Andrew Davis added a comment -

          I believed I've fixed that all best I can. Rebased and putting up for integration.

          Show
          Andrew Davis added a comment - I believed I've fixed that all best I can. Rebased and putting up for integration.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Sam Hemelryk added a comment -

          Thanks Andrew, this has been integrated now. As this really does look like an improvement to exception handling it has been integrated to master only.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now. As this really does look like an improvement to exception handling it has been integrated to master only. Cheers Sam
          Hide
          Frédéric Massart added a comment -

          Successfully tested on master

          Show
          Frédéric Massart added a comment - Successfully tested on master
          Hide
          Eloy Lafuente (stronk7) added a comment -

          U P S T R E A M I Z E D !

          Many thanks for the hard work, closing this as fixed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks for the hard work, closing this as fixed. Ciao
          Hide
          Miguel Santos added a comment -

          I found that the affected question answer rows typically have non ascii characters.

          I found the affected rows by running this query

          select id from mdl_question_answers where answer <> CONVERT(answer USING ASCII);

          And I found a great stored function that can clean them out here - http://www.shayanderson.com/mysql/remove-invalid-non-ascii-characters-in-mysql-query-using-stored-function.htm

          Show
          Miguel Santos added a comment - I found that the affected question answer rows typically have non ascii characters. I found the affected rows by running this query select id from mdl_question_answers where answer <> CONVERT(answer USING ASCII); And I found a great stored function that can clean them out here - http://www.shayanderson.com/mysql/remove-invalid-non-ascii-characters-in-mysql-query-using-stored-function.htm

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: