Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.9, 2.0
    • Fix Version/s: 2.0
    • Component/s: General
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Gliffy Diagrams

        Attachments

          Issue Links

            Activity

            Hide
            dongsheng Dongsheng Cai added a comment -

            All the error() calls are changed to print_error() now, creating error strings may take a while, tooooo many strings need to be created.

            Show
            dongsheng Dongsheng Cai added a comment - All the error() calls are changed to print_error() now, creating error strings may take a while, tooooo many strings need to be created.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Wow,

            I thought your were going to do this "gradually", adding new strings and changing some calls each day.

            In fact, now that all them are "correct" print_error() calls it's more difficult to find them, when it was trivial to do before your commit. Also be prevented about those BIG commits because they use to make more difficult to merge things (sometimes, not now, I guess) and to revert and so on. Just a recommendation.

            And yes, next step is about to creating error strings and replace them in current calls to print_error(), that's a huge task (I'd recommend to make that "gradually" )

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Wow, I thought your were going to do this "gradually", adding new strings and changing some calls each day. In fact, now that all them are "correct" print_error() calls it's more difficult to find them, when it was trivial to do before your commit. Also be prevented about those BIG commits because they use to make more difficult to merge things (sometimes, not now, I guess) and to revert and so on. Just a recommendation. And yes, next step is about to creating error strings and replace them in current calls to print_error(), that's a huge task (I'd recommend to make that "gradually" ) Ciao
            Hide
            dongsheng Dongsheng Cai added a comment -

            Oops, sorry, I didn't realize these problems before I commited changes, I will be careful about these changes from now on, thanks for your recommendation, Eloy.

            Show
            dongsheng Dongsheng Cai added a comment - Oops, sorry, I didn't realize these problems before I commited changes, I will be careful about these changes from now on, thanks for your recommendation, Eloy.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Dongsheng,

            just saw you latest gradual changes about this issue and have found that here:

            http://cvs.moodle.org/moodle/backup/backup.php?r1=1.48&r2=1.49

            You've used the same error string "cannotusepage" in THREE places. If you look at this carefully you'll see that the second replacement is incorrect, only admins can use that page (whereas the rest are admins and teachers).

            I'd suggesto you to fix this by:

            1) Adding some different lang strings like:

            cannotuseadmin
            cannotuseadminadminorteacher
            ...

            (i.e. all them will start by "cannotuse" followed by the list of roles allowed to use the page). Just a example proposal, feel free to improve it.

            2) Apply then where necessary.

            That way, always you find one error message, you can resuse those langs strings safely in future changes of this gradual task.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Dongsheng, just saw you latest gradual changes about this issue and have found that here: http://cvs.moodle.org/moodle/backup/backup.php?r1=1.48&r2=1.49 You've used the same error string "cannotusepage" in THREE places. If you look at this carefully you'll see that the second replacement is incorrect, only admins can use that page (whereas the rest are admins and teachers). I'd suggesto you to fix this by: 1) Adding some different lang strings like: cannotuseadmin cannotuseadminadminorteacher ... (i.e. all them will start by "cannotuse" followed by the list of roles allowed to use the page). Just a example proposal, feel free to improve it. 2) Apply then where necessary. That way, always you find one error message, you can resuse those langs strings safely in future changes of this gradual task. Ciao
            Hide
            dongsheng Dongsheng Cai added a comment -

            Thanks you advices, fixed

            Show
            dongsheng Dongsheng Cai added a comment - Thanks you advices, fixed
            Hide
            roal Robert Allerstorfer added a comment -

            As it seems, the transition from error() to print_error() does now not only go to HEAD, but to MOODLE_19_STABLE as well. Thus, should error() no more be used when creating new error checks in the STABLE branch?

            Why using the error string
            cannotuseadminadminorteacher (cannotuse admin admin or teacher)
            at http://cvs.moodle.org/moodle/backup/backup.php?r1=1.50&r2=1.48 ?

            Show
            roal Robert Allerstorfer added a comment - As it seems, the transition from error() to print_error() does now not only go to HEAD, but to MOODLE_19_STABLE as well. Thus, should error() no more be used when creating new error checks in the STABLE branch? Why using the error string cannotuseadminadminorteacher (cannotuse admin admin or teacher) at http://cvs.moodle.org/moodle/backup/backup.php?r1=1.50&r2=1.48 ?
            Hide
            dongsheng Dongsheng Cai added a comment -

            763 left

            Show
            dongsheng Dongsheng Cai added a comment - 763 left
            Hide
            dongsheng Dongsheng Cai added a comment -

            Hi, Robert, you'd better use print_error from now on, you can easily merge new error check to HEAD.

            Show
            dongsheng Dongsheng Cai added a comment - Hi, Robert, you'd better use print_error from now on, you can easily merge new error check to HEAD.
            Hide
            dongsheng Dongsheng Cai added a comment -

            620 left

            Show
            dongsheng Dongsheng Cai added a comment - 620 left
            Hide
            ppichet Pierre Pichet added a comment -

            Hi Dongsheng,
            Thanks for your tedious but necessary work on print_error.
            However you have done some changes on multianswer/questiontype (19 may)
            /// MULTIANSWER /// (Embedded - cloze)
            @@ -439,7 +439,7 @@
            echo $feedbackimg;
            break;
            default:

            • print_error("Unable to recognize questiontype of question part #$positionkey.");
              + print_error('unknownquestiontype', 'question');
              break;
              }
              echo "</label>"; // MDL-7497
              @@ -672,7 +672,7 @@
              $answer = $exploded[1];
              // $sequence is an ordered array of the question ids.
              if (!$sequence = get_field('question_multianswer', 'sequence', 'question', $state->question)) { - print_error("The cloze question $state->question is missing its options"); + print_error('missingoption', 'question', '', $state->question); }

              $sequence = explode(',', $sequence);
              // The id of the current question.
              @@ -799,7 +799,7 @@
              $wrapped->partiallycorrectfeedback = '';
              $wrapped->incorrectfeedback = '';
              } else

              { - print_error("Cannot identify qtype $answerregs[2]"); + print_error('unknownquestiontype', 'question', '', $answerregs[2]); return false; }

              The multianswer being the only one of its type in the actual question bank, identifying which subquestion have a problem needs specific infos.
              So I propose to create specific message in lang qtype_multianswer.php and refer to them in the print_erro call.
              If you agree, I will do the necessary work so that you can continue on the 620 left ...

            Show
            ppichet Pierre Pichet added a comment - Hi Dongsheng, Thanks for your tedious but necessary work on print_error. However you have done some changes on multianswer/questiontype (19 may) /// MULTIANSWER /// (Embedded - cloze) @@ -439,7 +439,7 @@ echo $feedbackimg; break; default: print_error("Unable to recognize questiontype of question part #$positionkey."); + print_error('unknownquestiontype', 'question'); break; } echo "</label>"; // MDL-7497 @@ -672,7 +672,7 @@ $answer = $exploded [1] ; // $sequence is an ordered array of the question ids. if (!$sequence = get_field('question_multianswer', 'sequence', 'question', $state->question)) { - print_error("The cloze question $state->question is missing its options"); + print_error('missingoption', 'question', '', $state->question); } $sequence = explode(',', $sequence); // The id of the current question. @@ -799,7 +799,7 @@ $wrapped->partiallycorrectfeedback = ''; $wrapped->incorrectfeedback = ''; } else { - print_error("Cannot identify qtype $answerregs[2]"); + print_error('unknownquestiontype', 'question', '', $answerregs[2]); return false; } The multianswer being the only one of its type in the actual question bank, identifying which subquestion have a problem needs specific infos. So I propose to create specific message in lang qtype_multianswer.php and refer to them in the print_erro call. If you agree, I will do the necessary work so that you can continue on the 620 left ...
            Hide
            dongsheng Dongsheng Cai added a comment -

            Hi, Pierre, feel free to do it, TIA

            Show
            dongsheng Dongsheng Cai added a comment - Hi, Pierre, feel free to do it, TIA
            Hide
            ppichet Pierre Pichet added a comment -

            OK

            Show
            ppichet Pierre Pichet added a comment - OK
            Hide
            ppichet Pierre Pichet added a comment -

            There are some hidden error calls in question code related to the return from questiontype save_question_options()
            i.e. line 366 of question/type/questiontype.php
            $result->error = "No data for field $field when saving " .
            $this->name() . ' question id ' . $question->id;
            and used line 318 as
            if (!empty($result->error))

            { print_error($result->error); }

            they can be found by searching ->error in question files.
            I will take care of them

            Show
            ppichet Pierre Pichet added a comment - There are some hidden error calls in question code related to the return from questiontype save_question_options() i.e. line 366 of question/type/questiontype.php $result->error = "No data for field $field when saving " . $this->name() . ' question id ' . $question->id; and used line 318 as if (!empty($result->error)) { print_error($result->error); } they can be found by searching ->error in question files. I will take care of them
            Hide
            dongsheng Dongsheng Cai added a comment -

            In addition, some print_error like:
            print_error( $this->error );

            This command will help you find them:
            grep "print_error([$ ]" --color -rn .

            Show
            dongsheng Dongsheng Cai added a comment - In addition, some print_error like: print_error( $this->error ); This command will help you find them: grep "print_error( [$ ] " --color -rn .
            Hide
            ppichet Pierre Pichet added a comment -

            Thanks I located them using "->error"

            Show
            ppichet Pierre Pichet added a comment - Thanks I located them using "->error"
            Hide
            dongsheng Dongsheng Cai added a comment -

            All done!! (Is this the longest change list in moodle?)

            Please reopen it if you find more unfixed print_error calls or hidden ones, TIA.

            Show
            dongsheng Dongsheng Cai added a comment - All done!! (Is this the longest change list in moodle?) Please reopen it if you find more unfixed print_error calls or hidden ones, TIA.
            Hide
            ppichet Pierre Pichet added a comment -

            It is OK for direct print_error calls, however the main project is also to remove all hardcoded error messages and in question code the work on indirect error is not finished. so don't close.

            Show
            ppichet Pierre Pichet added a comment - It is OK for direct print_error calls, however the main project is also to remove all hardcoded error messages and in question code the work on indirect error is not finished. so don't close.
            Hide
            mits Mitsuhiro Yoshida added a comment -

            Hi Dongsheng,

            Thanks for your hardworking
            I fixed some of your new strings as below.

            [error.php]
            -$string['csvloaderror'] = 'Error occur during loading vcs file!';
            +$string['csvloaderror'] = 'Error occur during loading CSV file!';

            [scorm.php]
            -$string['cannotfindsco'] = 'Cannot found SCO';
            +$string['cannotfindsco'] = 'Could not find SCO';

            And we need to add one more string $string['secretalreadyused'] to error.php.
            This string is used for login/forget_password.php.

            Mits

            Show
            mits Mitsuhiro Yoshida added a comment - Hi Dongsheng, Thanks for your hardworking I fixed some of your new strings as below. [error.php] -$string ['csvloaderror'] = 'Error occur during loading vcs file!'; +$string ['csvloaderror'] = 'Error occur during loading CSV file!'; [scorm.php] -$string ['cannotfindsco'] = 'Cannot found SCO'; +$string ['cannotfindsco'] = 'Could not find SCO'; And we need to add one more string $string ['secretalreadyused'] to error.php. This string is used for login/forget_password.php. Mits
            Hide
            dongsheng Dongsheng Cai added a comment -

            Pierre,I changed the indirect error:
            print_error($result->error);
            to:
            print_error('questionsaveerror', 'question', '', $result->error);

            It is ugly, do you have any better idea?
            I found more indirect error in other modules, I made a "TODO" mark in source code.

            Show
            dongsheng Dongsheng Cai added a comment - Pierre,I changed the indirect error: print_error($result->error); to: print_error('questionsaveerror', 'question', '', $result->error); It is ugly, do you have any better idea? I found more indirect error in other modules, I made a "TODO" mark in source code.
            Hide
            dongsheng Dongsheng Cai added a comment -

            Thanks, Mitsuhiro, there may be many other mistakes, this issue reopened

            Show
            dongsheng Dongsheng Cai added a comment - Thanks, Mitsuhiro, there may be many other mistakes, this issue reopened
            Hide
            dongsheng Dongsheng Cai added a comment -

            I found $string['secretalreadyused'] in lang/en_utf8/moodle.php, I copied one to error.php

            Show
            dongsheng Dongsheng Cai added a comment - I found $string ['secretalreadyused'] in lang/en_utf8/moodle.php, I copied one to error.php
            Hide
            ppichet Pierre Pichet added a comment -

            The main idea is that result->error contains the correct string using the get_string('erroe..','question or qtype')
            so that you use print_error($result->error);

            Show
            ppichet Pierre Pichet added a comment - The main idea is that result->error contains the correct string using the get_string('erroe..','question or qtype') so that you use print_error($result->error);
            Hide
            roal Robert Allerstorfer added a comment -

            Hi Dongsheng,

            there are still calls to error() in the HEAD versions of mod/data/lib.php and mod/data/preset.php

            Show
            roal Robert Allerstorfer added a comment - Hi Dongsheng, there are still calls to error() in the HEAD versions of mod/data/lib.php and mod/data/preset.php
            Hide
            dongsheng Dongsheng Cai added a comment -

            Thanks, Robert, I will fix them tomorrow

            Show
            dongsheng Dongsheng Cai added a comment - Thanks, Robert, I will fix them tomorrow
            Hide
            dongsheng Dongsheng Cai added a comment -

            Robert, I replaced all error() calls in data mod. It seems some print_error calls are reverted when I focus on this issue, that why I didn't find them.
            More work need to be done

            Show
            dongsheng Dongsheng Cai added a comment - Robert, I replaced all error() calls in data mod. It seems some print_error calls are reverted when I focus on this issue, that why I didn't find them. More work need to be done
            Hide
            roal Robert Allerstorfer added a comment -

            Thank you for your quick response, Dongsheng!

            I just replaced "invildpreset" by the proper wording "invalidpreset" introduced by your last commit.

            Show
            roal Robert Allerstorfer added a comment - Thank you for your quick response, Dongsheng! I just replaced "invildpreset" by the proper wording "invalidpreset" introduced by your last commit.
            Hide
            dongsheng Dongsheng Cai added a comment -

            There are a few error() in filtering system, they will be fixed in MDL-14582.
            Other error()s are removed from moodle HEAD, please report to here if you find more.
            Thanks.

            Show
            dongsheng Dongsheng Cai added a comment - There are a few error() in filtering system, they will be fixed in MDL-14582 . Other error()s are removed from moodle HEAD, please report to here if you find more. Thanks.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  24/Nov/10