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

Question definition html_to_text incorrectly call format_text on text with urls not rewritten

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.9, 2.3.6, 2.4.3
    • Fix Version/s: 2.4.6, 2.5.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      0. For this test, you need some questions in the question bank with images in the question text.

      1. In the question bank view (question/edit.php) turn on the 'Show question text in the question list' option. Ensure there are no debugging warnings output.

      2. Add your questions to a quiz, and make some attempts as a student.

      3. Go to the statistics report of that quiz, and click thorugh to the detailed analysis for a question with an impage in the question text. Ensure there are no debugging warnings output.

      4. Export your questions from the question bank in XHMTL format. Open the exported file. Ensure that you can see the images.

      5. Export your qusetions as Moodle XML. Re-import them into another course. Ensure there are no debugging warnings output.

      Show
      0. For this test, you need some questions in the question bank with images in the question text. 1. In the question bank view (question/edit.php) turn on the 'Show question text in the question list' option. Ensure there are no debugging warnings output. 2. Add your questions to a quiz, and make some attempts as a student. 3. Go to the statistics report of that quiz, and click thorugh to the detailed analysis for a question with an impage in the question text. Ensure there are no debugging warnings output. 4. Export your questions from the question bank in XHMTL format. Open the exported file. Ensure that you can see the images. 5. Export your qusetions as Moodle XML. Re-import them into another course. Ensure there are no debugging warnings output.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      While testing David patch for MDL-39490 I observed debugger output like:

      Before calling format_text(), the content must be processed with file_rewrite_pluginfile_urls()

      line 1184 of \lib\weblib.php: call to debugging()
      line 302 of \question\type\questionbase.php: call to format_text()
      line 89 of \question\type\match\question.php: call to question_definition->html_to_text()
      line 284 of \question\behaviour\behaviourbase.php: call to qtype_match_question->get_question_summary()
      line 898 of \question\engine\questionattempt.php: call to question_behaviour->get_question_summary()
      line 464 of \question\engine\questionusage.php: call to question_attempt->start()
      line 117 of \question\preview.php: call to question_usage_by_activity->start_question()

      This is related to the function html_to_text in question/type/questionbase.php

      public function html_to_text($text, $format) {
              return html_to_text(format_text($text, $format, array('noclean' => true)), 0, false);
          }
      

      as stated in MDL-39490 it is not correct to call format_text with a text containing non rewritten urls with @@PLUGINFILE@@

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Linking to the issue that helped me discover this

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Linking to the issue that helped me discover this
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I reviewed all calls to format_text in the question component
            There are only a few places where the same problem can occur:

            • format/xhtml/format.php (easy to correct: needs to call $question->format_text rather than format_text)
            • question/format.php in the format_question_text function (the result is passed to weblib html_to_text after calling format_text)
            • question/type/essay/question.php in the summarise_response function (another one where result is passed to weblib html_to_text after calling format_text)
            • question/type/multichoice/questiontype.php in the get_possible_responses function in 2 different places (here also weblib html_to_text is called afterward)

            So as you see apart the xhtml qformat issuee all are places were html_to text is called just after format_text so they are places where we don't really care or need urls as html_to_text will kill them anyway !

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I reviewed all calls to format_text in the question component There are only a few places where the same problem can occur: format/xhtml/format.php (easy to correct: needs to call $question->format_text rather than format_text) question/format.php in the format_question_text function (the result is passed to weblib html_to_text after calling format_text) question/type/essay/question.php in the summarise_response function (another one where result is passed to weblib html_to_text after calling format_text) question/type/multichoice/questiontype.php in the get_possible_responses function in 2 different places (here also weblib html_to_text is called afterward) So as you see apart the xhtml qformat issuee all are places were html_to text is called just after format_text so they are places where we don't really care or need urls as html_to_text will kill them anyway !
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Now for the quiz component
            Only the quiz_question_tostring function in mod/quiz/editlib.php may be affected.
            And fortunately here also we don't need tags or urls

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Now for the quiz component Only the quiz_question_tostring function in mod/quiz/editlib.php may be affected. And fortunately here also we don't need tags or urls
            Hide
            timhunt Tim Hunt added a comment -

            You missed html_to_text in question/type/questionbase.php. That does not need to do all the work to rewrite the URLs, it could just replace @@pluginfile@@ with http://example.com/ which would be faster and safe.

            Show
            timhunt Tim Hunt added a comment - You missed html_to_text in question/type/questionbase.php. That does not need to do all the work to rewrite the URLs, it could just replace @@pluginfile@@ with http://example.com/ which would be faster and safe.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            html_to_text in question/type/questionbase.php was the initial place were I spotted the problem and pushed me to create this issue so I forgot to repeat it in the list.
            Yep in fact in all the places except one we can do the same thing !
            Only exception is qformat_xhtml but as I said we just need to call $question->format_text
            As I am sure you have more important things to do, if you agree I can prepare a patch ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - html_to_text in question/type/questionbase.php was the initial place were I spotted the problem and pushed me to create this issue so I forgot to repeat it in the list. Yep in fact in all the places except one we can do the same thing ! Only exception is qformat_xhtml but as I said we just need to call $question->format_text As I am sure you have more important things to do, if you agree I can prepare a patch ?
            Hide
            timhunt Tim Hunt added a comment -

            Well, I am on holiday today and tomorrow, so feel free to make a patch.

            Show
            timhunt Tim Hunt added a comment - Well, I am on holiday today and tomorrow, so feel free to make a patch.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Have a nice holiday
            Other French teachers go back to school today but, fortunately I have one more holiday week because I don't work on Monday and Tuesday and the University close on Wednesday until next Monday.
            Rather than repeat code in multiple places my idea is to have a general function

                public function html_to_text($text, $format, $options) {
                    $text = str_replace('@@PLUGINFILE@@/', 'http://example.com/', $text);
                    return html_to_text(format_text($text, $format, $options), 0, false);
                }

            And to call it when needed.
            But I need to have it available everywhere. What is the good place to put it ? questionlib ? questiontypebase ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Have a nice holiday Other French teachers go back to school today but, fortunately I have one more holiday week because I don't work on Monday and Tuesday and the University close on Wednesday until next Monday. Rather than repeat code in multiple places my idea is to have a general function public function html_to_text($text, $format, $options) { $text = str_replace('@@PLUGINFILE@@/', 'http://example.com/', $text); return html_to_text(format_text($text, $format, $options), 0, false); } And to call it when needed. But I need to have it available everywhere. What is the good place to put it ? questionlib ? questiontypebase ?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Of course need to call it something other than html_to_text

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Of course need to call it something other than html_to_text
            Hide
            timhunt Tim Hunt added a comment -

            Yes, good to make a function for this. Is $question->to_plain_text() a good name?

            Show
            timhunt Tim Hunt added a comment - Yes, good to make a function for this. Is $question->to_plain_text() a good name?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Tim, aren't you still on holiday ? If weather is the same for you and me you should no be at your computer desk
            Yes,to_plain_text seems like a very good name.
            But I wonder if making it a question (or question_type) method is the best solution because we need it in some places where there is no question or question-type available (see for instance format_question_text in question/format.php where $question is a stdclass) ? Of course we can declare it static ?

            Another note: as usual we also need to take care of lesson because there is the same code in mod/lesson/format.php in the format_question_text function.

            Good thing is as MDL-39470 has been wisely delayed, we are not in a hurry. I was uncomfortable with the idea of developer debugging message everywhere in the question bank with a freshly released Moodle version !

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, aren't you still on holiday ? If weather is the same for you and me you should no be at your computer desk Yes,to_plain_text seems like a very good name. But I wonder if making it a question (or question_type) method is the best solution because we need it in some places where there is no question or question-type available (see for instance format_question_text in question/format.php where $question is a stdclass) ? Of course we can declare it static ? Another note: as usual we also need to take care of lesson because there is the same code in mod/lesson/format.php in the format_question_text function. Good thing is as MDL-39470 has been wisely delayed, we are not in a hurry. I was uncomfortable with the idea of developer debugging message everywhere in the question bank with a freshly released Moodle version !
            Hide
            timhunt Tim Hunt added a comment -

            I was not sure where this function/method should go. Sorry, I should not have typed the $question-> bit since as you say, that is probably the wrong design.

            Show
            timhunt Tim Hunt added a comment - I was not sure where this function/method should go. Sorry, I should not have typed the $question-> bit since as you say, that is probably the wrong design.
            Hide
            timhunt Tim Hunt added a comment -

            Jean-Michel, how far did you get with this? I don't want to do it again if you have already made a start. However, I am happy to finish this off if you have started. Can you send me any code?

            Show
            timhunt Tim Hunt added a comment - Jean-Michel, how far did you get with this? I don't want to do it again if you have already made a start. However, I am happy to finish this off if you have started. Can you send me any code?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Tim,
            Yes I have a git branch somewhere I will push it on github and explain why I am not satisfied with it.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, Yes I have a git branch somewhere I will push it on github and explain why I am not satisfied with it.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            My attempt to fix this is on branch MDL-39507 of my github account https://github.com/jmvedrine/moodle.git
            Here is a diff view https://github.com/jmvedrine/moodle/compare/master...MDL-39507
            But I don't like it, because I find it messy:

            • as I said I don't know the right place to put the to_plain_text function
            • If we look at all the places where we need that function in the question component, in all but question/type/essay/question.php we need exactly the same option: 'noclean' => true (in essay we need 'para' => false instead)
            • But we can't use $question->html_to_text in all places because it is defined as a question definition method so for instance in question/type/multichoice/questiontype.php or question/format.php we must resort to calling directly to_plain_text
              It surely need some more work or thinking

            Last thing: I will be unreachable starting from tomorrow morning until Thursday evening because I am in Disneyland Paris and I certainly will not bring my laptop with me

            Show
            jmvedrine Jean-Michel Vedrine added a comment - My attempt to fix this is on branch MDL-39507 of my github account https://github.com/jmvedrine/moodle.git Here is a diff view https://github.com/jmvedrine/moodle/compare/master...MDL-39507 But I don't like it, because I find it messy: as I said I don't know the right place to put the to_plain_text function If we look at all the places where we need that function in the question component, in all but question/type/essay/question.php we need exactly the same option: 'noclean' => true (in essay we need 'para' => false instead) But we can't use $question->html_to_text in all places because it is defined as a question definition method so for instance in question/type/multichoice/questiontype.php or question/format.php we must resort to calling directly to_plain_text It surely need some more work or thinking Last thing: I will be unreachable starting from tomorrow morning until Thursday evening because I am in Disneyland Paris and I certainly will not bring my laptop with me
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Tim,
            I am back from my holidays. Had you a look at my attempt to solve this ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, I am back from my holidays. Had you a look at my attempt to solve this ?
            Hide
            timhunt Tim Hunt added a comment -

            Sorry, Jean-Michel, I have not had time to look at this, and I am very unlikely to have time soon, but I do want to work on this when I get time.

            I think your patch is a useful starting point, because it makes it clear where most of the problems lie. However I agree with yous assessment "I don't like it, because I find it messy".

            So, more thought required. I you want to work on this, be my guest. Alternatively, if you want to leave it for me, that is also fine.

            Show
            timhunt Tim Hunt added a comment - Sorry, Jean-Michel, I have not had time to look at this, and I am very unlikely to have time soon, but I do want to work on this when I get time. I think your patch is a useful starting point, because it makes it clear where most of the problems lie. However I agree with yous assessment "I don't like it, because I find it messy". So, more thought required. I you want to work on this, be my guest. Alternatively, if you want to leave it for me, that is also fine.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Tim,
            When you come back we need to look at this because MDL-39490 is integrated this week.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, When you come back we need to look at this because MDL-39490 is integrated this week.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            What about this solution:

            • Deprecate questiontypebase html_to_text function (warning in 2.6 and suppressed in 2.7) just in case some third party question types use it
            • Replace all use of it everywhere with calls to the to_plain_text function with the right options as third parameter
            Show
            jmvedrine Jean-Michel Vedrine added a comment - What about this solution: Deprecate questiontypebase html_to_text function (warning in 2.6 and suppressed in 2.7) just in case some third party question types use it Replace all use of it everywhere with calls to the to_plain_text function with the right options as third parameter
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Just a note here that David Monllaó found a similar issue in the quiz module while testing MDL-28019.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Just a note here that David Monllaó found a similar issue in the quiz module while testing MDL-28019 .
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I think that in mod/quiz/editlib.php the quiz_question_tostring function should call our to_plain_text function when the showquestiontext parameter is true.
            I may be wrong but looking at the quiz code this is the only place where I see a problem with format_text.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I think that in mod/quiz/editlib.php the quiz_question_tostring function should call our to_plain_text function when the showquestiontext parameter is true. I may be wrong but looking at the quiz code this is the only place where I see a problem with format_text.
            Hide
            timhunt Tim Hunt added a comment -

            Jean-Michel, I finally got around to looking at this. It would be really great if you could peer review what I have done.

            What I did was:

            1. Took your branch and re-based it onto latest master.

            2. Moved the to_plain_text to the question_utils class. That seemed better than questionlib.php, but I am still not sure it is great. Still, probably good enough.

            3. Made the noclean => true the default option, so we did not have to specify it in most places.

            4. Used the recent MDL-35053 fix to make images in question text work in XHTML export. If you are interested, you could use that to make all images in the question work. (But that would be a separate bug.)

            5. Fixed all the to_plain_text calls.

            Note that, while testing this, I found MDL-41003.

            Show
            timhunt Tim Hunt added a comment - Jean-Michel, I finally got around to looking at this. It would be really great if you could peer review what I have done. What I did was: 1. Took your branch and re-based it onto latest master. 2. Moved the to_plain_text to the question_utils class. That seemed better than questionlib.php, but I am still not sure it is great. Still, probably good enough. 3. Made the noclean => true the default option, so we did not have to specify it in most places. 4. Used the recent MDL-35053 fix to make images in question text work in XHTML export. If you are interested, you could use that to make all images in the question work. (But that would be a separate bug.) 5. Fixed all the to_plain_text calls. Note that, while testing this, I found MDL-41003 .
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Tim,
            I will do the peer review either this evening or tomorrow.
            Does your patch fix the similar problem in quiz editlib too ? Or do we need to create another tracker issue ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, I will do the peer review either this evening or tomorrow. Does your patch fix the similar problem in quiz editlib too ? Or do we need to create another tracker issue ?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Looking at the patch answer my question Yes it does !

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Looking at the patch answer my question Yes it does !
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Tim, this looks good and is a lot better than my original patch.
            I followed your work on MDL-35053 but I didn't realize that this could be used on XHTML export
            [Y] Syntax
            [Y] Whitespace
            [-] Output
            [-] Language
            [-] Databases
            [-] Testing
            [-] Security
            [-] Documentation
            [Y] Git
            [Y] Sanity check

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, this looks good and is a lot better than my original patch. I followed your work on MDL-35053 but I didn't realize that this could be used on XHTML export [Y] Syntax [Y] Whitespace [-] Output [-] Language [-] Databases [-] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check
            Hide
            timhunt Tim Hunt added a comment -

            Thanks for the review Jean-Michel. Since we both worked on it, I wonder if we should get someone else to look? I guess the integration reviewer will be another pair of eyes, and that is enough. On Monday I will back-port this and submit for integration.

            Show
            timhunt Tim Hunt added a comment - Thanks for the review Jean-Michel. Since we both worked on it, I wonder if we should get someone else to look? I guess the integration reviewer will be another pair of eyes, and that is enough. On Monday I will back-port this and submit for integration.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I forgot to change the workflow

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I forgot to change the workflow
            Hide
            timhunt Tim Hunt added a comment -

            Backported, and submitting for integration.

            Show
            timhunt Tim Hunt added a comment - Backported, and submitting for integration.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Tim,

            Just wondering why you are using http://example.com for str_replace() (https://github.com/timhunt/moodle/compare/master...MDL-39507#L1R830).

            Would it be better to use $CFG->wwwroot or ''?

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Tim, Just wondering why you are using http://example.com for str_replace() ( https://github.com/timhunt/moodle/compare/master...MDL-39507#L1R830 ). Would it be better to use $CFG->wwwroot or ''?
            Hide
            timhunt Tim Hunt added a comment -

            The html_to_text call on the following line will strip out all links, so it does not matter what the links are. However, format_text now outputs a warning if it detects unprocessed @@PLUGINFILE@@ URLs in the HTML.

            So, we have to convert @@PLUGINFILE@@ to real URLs, but we don't care what they are. I was using example.com to try to make it clear that the acutal URL is not significant. I guess adding a comment would have been a good idea. Do you want to rebase all the branches to add the comment?

            Show
            timhunt Tim Hunt added a comment - The html_to_text call on the following line will strip out all links, so it does not matter what the links are. However, format_text now outputs a warning if it detects unprocessed @@PLUGINFILE@@ URLs in the HTML. So, we have to convert @@PLUGINFILE@@ to real URLs, but we don't care what they are. I was using example.com to try to make it clear that the acutal URL is not significant. I guess adding a comment would have been a good idea. Do you want to rebase all the branches to add the comment?
            Hide
            timhunt Tim Hunt added a comment -

            Clearly the comment is needed. Commit amended to add it.

            Show
            timhunt Tim Hunt added a comment - Clearly the comment is needed. Commit amended to add it.
            Hide
            poltawski Dan Poltawski added a comment -

            Hmm, I know this is a bit nipicking, but I went looking at the html_to_text function definition because I initially misread the comment and thought the assertion was wrong.

            When I intially misread the comment, I thought it said 'html_to_text strips out links' - which is not true by default and I was just writing a comment explaining that I thought that could be misleading. But I see it says 'the html_to_text call', I missed.

            So, could I suggest an alternate start to the comment:

            The call html_to_text requests URLs to be stripped out, but format_text complains [..]

            ..or something like that to make it more explicit that this isn't the default behaviour of html_to_text?

            I will wait and hear what you think, I am happy to integrate it as is, but if you think we cold make it a bit clearer too, i'd prefer that.

            Show
            poltawski Dan Poltawski added a comment - Hmm, I know this is a bit nipicking, but I went looking at the html_to_text function definition because I initially misread the comment and thought the assertion was wrong. When I intially misread the comment, I thought it said 'html_to_text strips out links' - which is not true by default and I was just writing a comment explaining that I thought that could be misleading. But I see it says 'the html_to_text call ', I missed. So, could I suggest an alternate start to the comment: The call html_to_text requests URLs to be stripped out, but format_text complains [..] ..or something like that to make it more explicit that this isn't the default behaviour of html_to_text? I will wait and hear what you think, I am happy to integrate it as is, but if you think we cold make it a bit clearer too, i'd prefer that.
            Hide
            timhunt Tim Hunt added a comment -

            Well, the purpose of the comment is to communicate understanding to other developers. You are another developer, so if you think you can make it clearer, please do.

            Can you amend the commit with the clearer words, or do you want me to do it?

            Show
            timhunt Tim Hunt added a comment - Well, the purpose of the comment is to communicate understanding to other developers. You are another developer, so if you think you can make it clearer, please do. Can you amend the commit with the clearer words, or do you want me to do it?
            Hide
            poltawski Dan Poltawski added a comment -

            Please could you do it (and feel free to improve my language if you think it could be better )

            Show
            poltawski Dan Poltawski added a comment - Please could you do it (and feel free to improve my language if you think it could be better )
            Hide
            timhunt Tim Hunt added a comment -

            Comment amended.

            Show
            timhunt Tim Hunt added a comment - Comment amended.
            Hide
            timhunt Tim Hunt added a comment -

            Grrr! forgot the -f on the push. Really amended this time.

            Show
            timhunt Tim Hunt added a comment - Grrr! forgot the -f on the push. Really amended this time.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Tim, integrated to master, 25 and 24

            Show
            poltawski Dan Poltawski added a comment - Thanks Tim, integrated to master, 25 and 24
            Show
            poltawski Dan Poltawski added a comment - - edited Boom, this is breaking unit tests on all branches: http://integration.moodle.org/job/07.%20Run%20phpunit%20UnitTests%20(24_STABLE)/769/testReport/ http://integration.moodle.org/job/20.%20Run%20phpunit%20UnitTests%20(25_STABLE)/268/testReport/ http://integration.moodle.org/job/20.%20Run%20phpunit%20UnitTests%20(master)/1484/testReport/ Undefined index: answerformat
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Tim,

            In order for us to continue integrating issues without new errors being masked by this problem i've pushed a hacky fix for the tests to all branches:
            http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=ed0724271388f293033663fd96d491fa7e1f5f9b;hp=969de2a200e7c162508209b097f78b46fbe5789c

            I don't know if you just consider this as a bug in the tests, or if that case needs to be handled, so leaving this failed and you can decide what is best.

            Show
            poltawski Dan Poltawski added a comment - Hi Tim, In order for us to continue integrating issues without new errors being masked by this problem i've pushed a hacky fix for the tests to all branches: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=ed0724271388f293033663fd96d491fa7e1f5f9b;hp=969de2a200e7c162508209b097f78b46fbe5789c I don't know if you just consider this as a bug in the tests, or if that case needs to be handled, so leaving this failed and you can decide what is best.
            Hide
            timhunt Tim Hunt added a comment -

            Fix is here: https://github.com/timhunt/moodle/compare/master...MDL-39980#diff-0, so actually your guess was close. I did not realise those test failures came from here.

            Show
            timhunt Tim Hunt added a comment - Fix is here: https://github.com/timhunt/moodle/compare/master...MDL-39980#diff-0 , so actually your guess was close. I did not realise those test failures came from here.
            Hide
            poltawski Dan Poltawski added a comment -

            Not sure what you'd prefer I do with this issue?

            Show
            poltawski Dan Poltawski added a comment - Not sure what you'd prefer I do with this issue?
            Hide
            timhunt Tim Hunt added a comment -

            I was implicitly asking the same question of you

            Show
            timhunt Tim Hunt added a comment - I was implicitly asking the same question of you
            Hide
            poltawski Dan Poltawski added a comment -

            Heh, well - I feel like i've done my part here by cleaning up your unit test failure and i've had enough arguments about the peer review rule for today that I don't feel like breaking that rule today, so i'm afraid I just created some bad conflicts for your other issue.

            Show
            poltawski Dan Poltawski added a comment - Heh, well - I feel like i've done my part here by cleaning up your unit test failure and i've had enough arguments about the peer review rule for today that I don't feel like breaking that rule today, so i'm afraid I just created some bad conflicts for your other issue.
            Hide
            timhunt Tim Hunt added a comment -

            Yes, you have created crappy merge conflicts, and that issue is blocking another one that will also have to be rebased. Such is life.

            Show
            timhunt Tim Hunt added a comment - Yes, you have created crappy merge conflicts, and that issue is blocking another one that will also have to be rebased. Such is life.
            Hide
            fred Frédéric Massart added a comment - - edited

            The test failed on 2.4 and 2.5.

            Exporting to XHTML is not possible, this error occurs on both versions:

            Fatal error: Call to undefined function question_rewrite_question_preview_urls() in /home/fred/www/repositories/i25/moodle/question/format/xhtml/format.php on line 70
             
            Call Stack:
                0.0001     644688   1. {main}() /home/fred/www/repositories/i25/moodle/pluginfile.php:0
                0.1274   34772160   2. file_pluginfile() /home/fred/www/repositories/i25/moodle/pluginfile.php:38
                0.1505   39755120   3. question_pluginfile() /home/fred/www/repositories/i25/moodle/lib/filelib.php:4311
                0.2324   60365696   4. qformat_default->exportprocess() /home/fred/www/repositories/i25/moodle/lib/questionlib.php:1859
                0.2623   61041944   5. qformat_xhtml->writequestion() /home/fred/www/repositories/i25/moodle/question/format.php:816
            

            On a side note, sometimes when landing on the question bank, the "Show question text in the question list" checkbox is ticked, but it is not displaying the question text. I had to untick, and tick again.

            Show
            fred Frédéric Massart added a comment - - edited The test failed on 2.4 and 2.5. Exporting to XHTML is not possible, this error occurs on both versions: Fatal error: Call to undefined function question_rewrite_question_preview_urls() in /home/fred/www/repositories/i25/moodle/question/format/xhtml/format.php on line 70   Call Stack: 0.0001 644688 1. {main}() /home/fred/www/repositories/i25/moodle/pluginfile.php:0 0.1274 34772160 2. file_pluginfile() /home/fred/www/repositories/i25/moodle/pluginfile.php:38 0.1505 39755120 3. question_pluginfile() /home/fred/www/repositories/i25/moodle/lib/filelib.php:4311 0.2324 60365696 4. qformat_default->exportprocess() /home/fred/www/repositories/i25/moodle/lib/questionlib.php:1859 0.2623 61041944 5. qformat_xhtml->writequestion() /home/fred/www/repositories/i25/moodle/question/format.php:816 On a side note, sometimes when landing on the question bank, the "Show question text in the question list" checkbox is ticked, but it is not displaying the question text. I had to untick, and tick again.
            Hide
            timhunt Tim Hunt added a comment -

            Drat! Will make a patch to fix stable branches in a few hours.

            Show
            timhunt Tim Hunt added a comment - Drat! Will make a patch to fix stable branches in a few hours.
            Hide
            timhunt Tim Hunt added a comment -

            The issue is that the master code relies on the changes in MDL-35053. I will re-code in stable branches using the older way of doing things.

            Show
            timhunt Tim Hunt added a comment - The issue is that the master code relies on the changes in MDL-35053 . I will re-code in stable branches using the older way of doing things.
            Hide
            timhunt Tim Hunt added a comment -

            New commit added to the _25 and _24 branches. Please reintegrate.

            Show
            timhunt Tim Hunt added a comment - New commit added to the _25 and _24 branches. Please reintegrate.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Tim.

            Back to testing (rather than peer reviewing..) Frédéric Massart

            Show
            poltawski Dan Poltawski added a comment - Thanks Tim. Back to testing (rather than peer reviewing..) Frédéric Massart
            Hide
            fred Frédéric Massart added a comment -

            Passing after re-testing the Step 4 on 2.4 and 2.5. Thanks

            Show
            fred Frédéric Massart added a comment - Passing after re-testing the Step 4 on 2.4 and 2.5. Thanks
            Hide
            poltawski Dan Poltawski added a comment -

            Cảm ơn!

            Your changes have now been merged upstream in git and will be available on the Moodle download sites shortly!

            Một hai ba, yo

            Show
            poltawski Dan Poltawski added a comment - Cảm ơn! Your changes have now been merged upstream in git and will be available on the Moodle download sites shortly! Một hai ba, yo

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Sep/13