Moodle
  1. Moodle
  2. MDL-34841

Error writing to Database -uploading GIFT Questions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.5, 2.3.1, 2.4
    • Fix Version/s: 2.2.6, 2.3.3
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      The unit tests provide a pretty good test to make sure these changes do not break anything. However, a bit of manual testing through the Moodle UI to verify there are no regressions would be appreciated.

      1. Choose a format to test (e.g. Moodle XML or GIFT).

      2. Find a sample file (e.g. question/format/gift/tests/fixtures/questions.gift.txt).

      3. Import it into the question bank.

      4. Check that the names of the imported questions are what you would expect.

      5. Also try importing questions into the lesson module.

      Show
      The unit tests provide a pretty good test to make sure these changes do not break anything. However, a bit of manual testing through the Moodle UI to verify there are no regressions would be appreciated. 1. Choose a format to test (e.g. Moodle XML or GIFT). 2. Find a sample file (e.g. question/format/gift/tests/fixtures/questions.gift.txt). 3. Import it into the question bank. 4. Check that the names of the imported questions are what you would expect. 5. Also try importing questions into the lesson module.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      43346

      Description

      When I try to import questions into the question bank using GIFT format I get error writing to databse and following debugging information:
      Debug info:
      Stack trace:
      line 410 of /lib/dml/moodle_database.php: dml_write_exception thrown
      line 1029 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
      line 1071 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->insert_record_raw()
      line 412 of /question/format.php: call to mysqli_native_moodle_database->insert_record()
      line 119 of /question/import.php: call to qformat_default->importprocess()

        Activity

        Hide
        Jean-Michel Vedrine added a comment -

        Hello Vernellia,
        if it's possible could you post the problematic gift file (or if you don't want to publish your questions on the web to mail this file privately to me) ?

        Show
        Jean-Michel Vedrine added a comment - Hello Vernellia, if it's possible could you post the problematic gift file (or if you don't want to publish your questions on the web to mail this file privately to me) ?
        Hide
        Tim Hunt added a comment -

        Unless we get more information, we will have to close this cannot reproduce.

        Show
        Tim Hunt added a comment - Unless we get more information, we will have to close this cannot reproduce.
        Hide
        Georgi Yadkov added a comment -

        I have the same problem when importing questions in GIFT format. My version of MOODLE 2.3.1+ (Build: 20120802)

        Here is a link to a question (in Bulgarian) where the above described problem appears: http://pastebin.com/adUQv2YE

        Show
        Georgi Yadkov added a comment - I have the same problem when importing questions in GIFT format. My version of MOODLE 2.3.1+ (Build: 20120802) Here is a link to a question (in Bulgarian) where the above described problem appears: http://pastebin.com/adUQv2YE
        Hide
        Jean-Michel Vedrine added a comment -

        Hello,
        The problem is in the code that create question name from question text : your question text is wrongly truncated to produce a question name and this produce an invalid utf-8 character at the end of the question name.
        Until this bug is fixed, you can use the following workaround : include a valid question name in all your questions using the syntax ::title:: and import will be fine.
        For instance I can import your question with no problem if I add a title :
        ::Q1::Част от финансовия отчет, която показва приходите, разходите и финансовия резултат на фирмата за определен период (месец, тримесечие, година) е нейният

        { ~Счетоводен баланс =Отчет на приходите и разходите ~Отчет на паричните потоци ~Отчет на печалбите и загубите}
        Show
        Jean-Michel Vedrine added a comment - Hello, The problem is in the code that create question name from question text : your question text is wrongly truncated to produce a question name and this produce an invalid utf-8 character at the end of the question name. Until this bug is fixed, you can use the following workaround : include a valid question name in all your questions using the syntax ::title:: and import will be fine. For instance I can import your question with no problem if I add a title : ::Q1::Част от финансовия отчет, която показва приходите, разходите и финансовия резултат на фирмата за определен период (месец, тримесечие, година) е нейният { ~Счетоводен баланс =Отчет на приходите и разходите ~Отчет на паричните потоци ~Отчет на печалбите и загубите}
        Hide
        Jean-Michel Vedrine added a comment -

        To Tim,
        Maybe it would be good to think to a bulletproof function to provide a valid question name from question text and use it in all imports ? I had this idea when working on examview and blackboard imports but I haven't done it.

        Show
        Jean-Michel Vedrine added a comment - To Tim, Maybe it would be good to think to a bulletproof function to provide a valid question name from question text and use it in all imports ? I had this idea when working on examview and blackboard imports but I haven't done it.
        Hide
        Tim Hunt added a comment -

        Yes, that is definitely the right way to fix this.

        Show
        Tim Hunt added a comment - Yes, that is definitely the right way to fix this.
        Hide
        Jean-Michel Vedrine added a comment - - edited

        Well as expected the first line :
        $question->name = shorten_text($question->name, 200);
        does not produce any invalid character because shorten text is perfectly multibyte aware but the result is a string with a length in bytes of 276
        And the second line is the culprit because doing
        $question->name = strip_tags(substr($question->name, 0, 250));
        is certainly bad as substr know nothing on multibytes characters.
        So the function should

        • only use multibyte aware functions to truncate the string
        • strip all html tags
        • not produce a string length (in bytes) too long for the database field
        • not produce an empty string

        I wonder if a combination of html_to_text and shorten_text may be a good solution ?

        Show
        Jean-Michel Vedrine added a comment - - edited Well as expected the first line : $question->name = shorten_text($question->name, 200); does not produce any invalid character because shorten text is perfectly multibyte aware but the result is a string with a length in bytes of 276 And the second line is the culprit because doing $question->name = strip_tags(substr($question->name, 0, 250)); is certainly bad as substr know nothing on multibytes characters. So the function should only use multibyte aware functions to truncate the string strip all html tags not produce a string length (in bytes) too long for the database field not produce an empty string I wonder if a combination of html_to_text and shorten_text may be a good solution ?
        Hide
        Tim Hunt added a comment -

        Jean-Michel, do you have time to look at this proposed fix?

        Show
        Tim Hunt added a comment - Jean-Michel, do you have time to look at this proposed fix?
        Hide
        Jean-Michel Vedrine added a comment -

        Hello Tim,
        Yes I think I will be able to look at this tomorrow (fortunately I don't have any course on Wednesday) and also test it on the multiple sample files I have.

        Show
        Jean-Michel Vedrine added a comment - Hello Tim, Yes I think I will be able to look at this tomorrow (fortunately I don't have any course on Wednesday) and also test it on the multiple sample files I have.
        Hide
        Jean-Michel Vedrine added a comment - - edited

        Hello Tim,
        This seems quite good to me. Question's name creation now uses clean_param, textlib::strlen and shorten_text. So it should be a lot more bulletproof.
        Using clean_param($name, PARAM_TEXT); // Matches what the question editing form does.
        is a step further in your idea that all validations should be pretty similar in the question creation and in question import so I quite agree.
        Looking at the tests, my first idea was that a test with non-latin string like to one in Georgi's sample would have been good, but thinking further as now question name truncation rely exclusively on shorten_text, surely non latin tests belongs more to shorten_text tests than here. I looked in moodlelib_test and there are quite a lot of such tests so all is OK.
        Studying the code I found no problem or typos.
        Additionnaly I tested the code to import lots of questions (including all the samples used to test blackboard_six) and I didn't spotted any problem in question names.
        So my +1 to send this to integration.
        I hope I didn't missed too many things.

        Show
        Jean-Michel Vedrine added a comment - - edited Hello Tim, This seems quite good to me. Question's name creation now uses clean_param, textlib::strlen and shorten_text. So it should be a lot more bulletproof. Using clean_param($name, PARAM_TEXT); // Matches what the question editing form does. is a step further in your idea that all validations should be pretty similar in the question creation and in question import so I quite agree. Looking at the tests, my first idea was that a test with non-latin string like to one in Georgi's sample would have been good, but thinking further as now question name truncation rely exclusively on shorten_text, surely non latin tests belongs more to shorten_text tests than here. I looked in moodlelib_test and there are quite a lot of such tests so all is OK. Studying the code I found no problem or typos. Additionnaly I tested the code to import lots of questions (including all the samples used to test blackboard_six) and I didn't spotted any problem in question names. So my +1 to send this to integration. I hope I didn't missed too many things.
        Hide
        Jean-Michel Vedrine added a comment -

        I suppose this is what I need to do

        Show
        Jean-Michel Vedrine added a comment - I suppose this is what I need to do
        Hide
        Tim Hunt added a comment -

        Thank you very much Jean-Michel. That sounds like a very thorough peer review.

        I have cherry-picked to stable branches, and am now submitting for integration.

        Show
        Tim Hunt added a comment - Thank you very much Jean-Michel. That sounds like a very thorough peer review. I have cherry-picked to stable branches, and am now submitting 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
        Eloy Lafuente (stronk7) added a comment -

        Integrated (22, 23 & master), thanks!

        To testers, this needs testing under 22_STABLE and 23_STABLE.

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (22, 23 & master), thanks! To testers, this needs testing under 22_STABLE and 23_STABLE.
        Hide
        Tim Barker added a comment -

        +1 for these testing instructions Tim

        Show
        Tim Barker added a comment - +1 for these testing instructions Tim
        Hide
        Jason Fowler added a comment -

        works fine for me Tim

        Show
        Jason Fowler added a comment - works fine for me Tim
        Hide
        Dan Poltawski added a comment -

        Congratulations, you've done it!

        Thanks, this change is now in the latest weekly release!

        Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

        Show
        Dan Poltawski added a comment - Congratulations, you've done it! Thanks, this change is now in the latest weekly release! Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: