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

An extra question field cannot be set back to null when saving a question

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      Of the core question types, only shortanswer uses this feature. If you have any contrib question types installed, then it would be good to test them too. Please test:
      1. Create new question.
      2. Edit question.
      3. Preview question.
      4. Export / Import questions as Moodle XML.

      Show
      Of the core question types, only shortanswer uses this feature. If you have any contrib question types installed, then it would be good to test them too. Please test: 1. Create new question. 2. Edit question. 3. Preview question. 4. Export / Import questions as Moodle XML.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      questiontypebase.php does not allow setting a question option field to null. To reproduce:

      1. Define in the xml a question_blah_options field x, say, with NOTNULL false.
      2. Call qtype_blah.save_question_options($q) with $q.x set to some non-null value.
      3. Now try to resave the question with $q.x set to null.

      The code in questiontypebase.save_question_options will give an error around line 457 due to the test "if (!isset($question->$field))". The error probably goes undetected because the return value of the function cannot meaningfully be checked as it's not always defined. Instead the call to save the question options silently aborts.

      Fix: change the test in line 457 (or thereabouts) to

      if (!property_exists($question, $field))

      Also, would it be safer to throw a coding_exception if this condition occurs rather than silently aborting the execution?

        Gliffy Diagrams

          Activity

          Hide
          r.lobb Richard Lobb added a comment -

          Actually, while the above fix solves the immediate problem I had, I'm now finding various other places where a null question field causes problems, e.g. get_question_options and export_to_xml. I've changed my code to allow an empty text field in lieu of a null field though I'd rather have used the null option. All the same, I think a fix like the one I mention above, coupled with the throwing of a coding_exception if a field is null, might be worth doing anyway, if only to save other people the time of trying to find why the save_options isn't working. And perhaps there should be a comment somewhere to say that null fields in question options aren't currently supported?

          Your call, Tim.

          – Richard

          Show
          r.lobb Richard Lobb added a comment - Actually, while the above fix solves the immediate problem I had, I'm now finding various other places where a null question field causes problems, e.g. get_question_options and export_to_xml. I've changed my code to allow an empty text field in lieu of a null field though I'd rather have used the null option. All the same, I think a fix like the one I mention above, coupled with the throwing of a coding_exception if a field is null, might be worth doing anyway, if only to save other people the time of trying to find why the save_options isn't working. And perhaps there should be a comment somewhere to say that null fields in question options aren't currently supported? Your call, Tim. – Richard
          Hide
          timhunt Tim Hunt added a comment -

          Thank you for finding this. I reviewed all the places that extra_question_fields is used, and actually they are mostly OK.

          I don't think it is right to throw exceptions if the fields are missing when the question is saved. It should be OK to have a default defined in the database definition, and have that work.

          Anyway, I think this patch is the correct fix.

          Show
          timhunt Tim Hunt added a comment - Thank you for finding this. I reviewed all the places that extra_question_fields is used, and actually they are mostly OK. I don't think it is right to throw exceptions if the fields are missing when the question is saved. It should be OK to have a default defined in the database definition, and have that work. Anyway, I think this patch is the correct fix.
          Hide
          timhunt Tim Hunt added a comment -

          Submitting for integration.

          Show
          timhunt Tim Hunt added a comment - Submitting for integration.
          Hide
          stronk7 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
          stronk7 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
          r.lobb Richard Lobb added a comment -

          (Belatedly) Thanks Tim, Eloy.

          Show
          r.lobb Richard Lobb added a comment - (Belatedly) Thanks Tim, Eloy.
          Hide
          poltawski Dan Poltawski added a comment -

          Integrated to master, 24 and 23 thanks Tim

          Show
          poltawski Dan Poltawski added a comment - Integrated to master, 24 and 23 thanks Tim
          Hide
          fred Frédéric Massart added a comment -

          Failing the test (only tested master), the sort of following wild warning appears on create/edit:

          Warning: property_exists() expects exactly 2 parameters, 1 given in /home/fred/www/repositories/im/moodle/question/type/edit_question_form.php on line 494
           
          Call Stack:
              0.0006     836352   1. {main}() /home/fred/www/repositories/im/moodle/question/question.php:0
              0.1960   48811288   2. question_edit_form->set_data() /home/fred/www/repositories/im/moodle/question/question.php:221
              0.2012   48818600   3. property_exists() /home/fred/www/repositories/im/moodle/question/type/edit_question_form.php:494
           
           
          Warning: property_exists() expects exactly 2 parameters, 1 given in /home/fred/www/repositories/im/moodle/question/type/edit_question_form.php on line 494
           
          Call Stack:
              0.0006     836352   1. {main}() /home/fred/www/repositories/im/moodle/question/question.php:0
              0.1960   48811288   2. question_edit_form->set_data() /home/fred/www/repositories/im/moodle/question/question.php:221
              0.2014   48819848   3. property_exists() /home/fred/www/repositories/im/moodle/question/type/edit_question_form.php:494
          

          Show
          fred Frédéric Massart added a comment - Failing the test (only tested master), the sort of following wild warning appears on create/edit: Warning: property_exists() expects exactly 2 parameters, 1 given in /home/fred/www/repositories/im/moodle/question/type/edit_question_form.php on line 494   Call Stack: 0.0006 836352 1. {main}() /home/fred/www/repositories/im/moodle/question/question.php:0 0.1960 48811288 2. question_edit_form->set_data() /home/fred/www/repositories/im/moodle/question/question.php:221 0.2012 48818600 3. property_exists() /home/fred/www/repositories/im/moodle/question/type/edit_question_form.php:494     Warning: property_exists() expects exactly 2 parameters, 1 given in /home/fred/www/repositories/im/moodle/question/type/edit_question_form.php on line 494   Call Stack: 0.0006 836352 1. {main}() /home/fred/www/repositories/im/moodle/question/question.php:0 0.1960 48811288 2. question_edit_form->set_data() /home/fred/www/repositories/im/moodle/question/question.php:221 0.2014 48819848 3. property_exists() /home/fred/www/repositories/im/moodle/question/type/edit_question_form.php:494
          Hide
          poltawski Dan Poltawski added a comment -

          Wow. Disappointed that this clearly got sent for integration without being tested.

          Show
          poltawski Dan Poltawski added a comment - Wow. Disappointed that this clearly got sent for integration without being tested.
          Hide
          timhunt Tim Hunt added a comment -

          Yes, and disappointing that a trivial patch that someone could have peer-reviewed in 5 minutes sat there for 3 days with no attention. I'll do a fix in a few hours when I get into the office.

          Show
          timhunt Tim Hunt added a comment - Yes, and disappointing that a trivial patch that someone could have peer-reviewed in 5 minutes sat there for 3 days with no attention. I'll do a fix in a few hours when I get into the office.
          Hide
          timhunt Tim Hunt added a comment -

          OK, new commit pushed to each branch. I have really tested myself this time. Apologies for being an idiot.

          Show
          timhunt Tim Hunt added a comment - OK, new commit pushed to each branch. I have really tested myself this time. Apologies for being an idiot.
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks Tim.

          Back to testing Fred

          Show
          poltawski Dan Poltawski added a comment - Thanks Tim. Back to testing Fred
          Hide
          fred Frédéric Massart added a comment -

          Thanks guys, the test is passing now. Cheers!

          Show
          fred Frédéric Massart added a comment - Thanks guys, the test is passing now. Cheers!
          Hide
          poltawski Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          poltawski Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                11/Mar/13