Moodle
  1. Moodle
  2. MDL-37374

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

    Details

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

      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?

        Activity

        Hide
        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
        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
        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
        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
        Tim Hunt added a comment -

        Submitting for integration.

        Show
        Tim Hunt added a comment - 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
        Richard Lobb added a comment -

        (Belatedly) Thanks Tim, Eloy.

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

        Integrated to master, 24 and 23 thanks Tim

        Show
        Dan Poltawski added a comment - Integrated to master, 24 and 23 thanks Tim
        Hide
        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
        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
        Dan Poltawski added a comment -

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

        Show
        Dan Poltawski added a comment - Wow. Disappointed that this clearly got sent for integration without being tested.
        Hide
        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
        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
        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
        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
        Dan Poltawski added a comment -

        Thanks Tim.

        Back to testing Fred

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

        Thanks guys, the test is passing now. Cheers!

        Show
        Frédéric Massart added a comment - Thanks guys, the test is passing now. Cheers!
        Hide
        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
        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: