Moodle
  1. Moodle
  2. MDL-17851 Make question_edit_form more suitable for inheritance
  3. MDL-18034

Rename answers field to answer in calculated qtype form to increase consistency

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Cannot Reproduce
    • Affects Version/s: 1.9.3
    • Fix Version/s: None
    • Component/s: Questions
    • Labels:
      None
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE
    • Rank:
      12811

      Description

      All qtypes except calculated, that have several fields, have it named 'answer' except calculated. Renaming it will increase integrity.

      The issue is pretty easy as database field is not affected (it named answer) - so renaming form field will increase consistency in calculated too.

      Pierre, would you prefer to do it by youself, or I must do it?

        Activity

        Hide
        Oleg Sychev added a comment -

        Hi, Pierre
        "Could you check that I can replace question->options->answers by question->options->answer without any side effects? "

        You don't need to replace question->options->answers - this is entirely another thing. You need to replace answers for answer on the form, not in question object. I.e. primary place are
        $repeated[] =& $mform->createElement('text', 'answers', get_string('correctanswerformula', 'quiz').'=', array('size' => 50));
        $repeatedoptions['answers']['type'] = PARAM_NOTAGS;

        There are corresponding things in set_data and validation functions, like
        $default_values['answers['.$key.']'] = $answer->answer;
        or
        $answers = $data['answers'];
        (and also all error setting in validation where error[] has answers as an argument)

        Also there is a need to edit save_question_options function in qtype class, as it uses data from the form, basicaly the string
        foreach ($question->answers as $key => $dataanswer) {

        P.S. Well, I can provide a tested patch if you want. This may be easier.

        Show
        Oleg Sychev added a comment - Hi, Pierre "Could you check that I can replace question->options->answers by question->options->answer without any side effects? " You don't need to replace question->options->answers - this is entirely another thing. You need to replace answers for answer on the form, not in question object. I.e. primary place are $repeated[] =& $mform->createElement('text', 'answers', get_string('correctanswerformula', 'quiz').'=', array('size' => 50)); $repeatedoptions ['answers'] ['type'] = PARAM_NOTAGS; There are corresponding things in set_data and validation functions, like $default_values['answers ['.$key.'] '] = $answer->answer; or $answers = $data ['answers'] ; (and also all error setting in validation where error[] has answers as an argument) Also there is a need to edit save_question_options function in qtype class, as it uses data from the form, basicaly the string foreach ($question->answers as $key => $dataanswer) { P.S. Well, I can provide a tested patch if you want. This may be easier.
        Hide
        Oleg Sychev added a comment -

        As save_question_options could be called not only from the editing form, but from the many other places too, it'll probably be nest to not rename $question->answers in $question->answer there, but just set $question->answers = $question->answer; if $question->answer is set.

        Show
        Oleg Sychev added a comment - As save_question_options could be called not only from the editing form, but from the many other places too, it'll probably be nest to not rename $question->answers in $question->answer there, but just set $question->answers = $question->answer; if $question->answer is set.
        Hide
        Pierre Pichet added a comment -

        Thanks Oleg,

        However I have to check about save_question_options function in qtype class because this function just not save from form but also from saving questions in the restore or from import format code.

        function save_question_options($question) {
        foreach ($question->answers as $key => $dataanswer) {
        if ( trim($dataanswer) != '' ) {
        $answer = new stdClass;
        $answer->question = $question->id;
        $answer->answer = trim($dataanswer);
        $answer->fraction = $question->fraction[$key];
        $answer->feedback = trim($question->feedback[$key]);
        And when coming from question edition form the type/questiontype/save_question()

        $result = $this->save_question_options($form);
        so they should be answers on the form....

        I think on the contrary that answer HTML element should be name answers as they are an array on the form and the "normal code convention" is to name answers an array of answer objects.
        So I propose that we do the reverse i.e. addelement "answers"...

        Show
        Pierre Pichet added a comment - Thanks Oleg, However I have to check about save_question_options function in qtype class because this function just not save from form but also from saving questions in the restore or from import format code. function save_question_options($question) { foreach ($question->answers as $key => $dataanswer) { if ( trim($dataanswer) != '' ) { $answer = new stdClass; $answer->question = $question->id; $answer->answer = trim($dataanswer); $answer->fraction = $question->fraction [$key] ; $answer->feedback = trim($question->feedback [$key] ); And when coming from question edition form the type/questiontype/save_question() $result = $this->save_question_options($form); so they should be answers on the form.... I think on the contrary that answer HTML element should be name answers as they are an array on the form and the "normal code convention" is to name answers an array of answer objects. So I propose that we do the reverse i.e. addelement "answers"...
        Hide
        Oleg Sychev added a comment -

        Pierre,

        I already spotted problem with renaming in save_question_options. However, I don't think that change 'asnwer' to 'asnwers' is a good idea: not only many qtypes use answer, but the convention is held on other answer blank fields: do you want to rename fraction, tolerance, feedback and so on in fractions etc? It's answers who break a convention.

        An easy way to avoid problems in save_question_options would be something like:

        function save_question_options($question) {
        if (isset($question->answer) && !isset($question->asnwers))

        { $question->answers=$question->answer; }

        Than renaming answers in the form would not be a problem.

        Show
        Oleg Sychev added a comment - Pierre, I already spotted problem with renaming in save_question_options. However, I don't think that change 'asnwer' to 'asnwers' is a good idea: not only many qtypes use answer, but the convention is held on other answer blank fields: do you want to rename fraction, tolerance, feedback and so on in fractions etc? It's answers who break a convention. An easy way to avoid problems in save_question_options would be something like: function save_question_options($question) { if (isset($question->answer) && !isset($question->asnwers)) { $question->answers=$question->answer; } Than renaming answers in the form would not be a problem.
        Hide
        Pierre Pichet added a comment -

        Done

        Show
        Pierre Pichet added a comment - Done
        Hide
        Pierre Pichet added a comment -

        You can work your experimental code using the new question type.
        You build the new calculated edit form.
        When everything works on your experimental site, you create a zip file so that I can test it on my site
        http://132.208.141.198/moodle_head/
        user:moodle pw;moodle
        and then we let Tim decide on the next step.

        Show
        Pierre Pichet added a comment - You can work your experimental code using the new question type. You build the new calculated edit form. When everything works on your experimental site, you create a zip file so that I can test it on my site http://132.208.141.198/moodle_head/ user:moodle pw;moodle and then we let Tim decide on the next step.
        Hide
        Oleg Sychev added a comment -

        Pierre,
        "Done" - will you now actually rename answers in edit_calculated_form? Or you leave it to me?

        "When everything works on your experimental site, you create a zip file so that I can test it on my site " - you don't work with patches?

        Well, I thought about excluding calculated from main patch for Tim, so that we can deal with this with you; but I may include it in main patch instead, so Tim could apply it with all others.

        Show
        Oleg Sychev added a comment - Pierre, "Done" - will you now actually rename answers in edit_calculated_form? Or you leave it to me? "When everything works on your experimental site, you create a zip file so that I can test it on my site " - you don't work with patches? Well, I thought about excluding calculated from main patch for Tim, so that we can deal with this with you; but I may include it in main patch instead, so Tim could apply it with all others.
        Hide
        Pierre Pichet added a comment -

        I don't have the time to rewrite the edit_calculated_form: the change of answers and inclusion of your new code.
        So I prefer that you do this.
        What I understand from the last days exchanges is that you are putting in your new structure enough flexibility to accomodate actual calculated needs and known future projects.
        So go ahead and build it.
        I just offer my collaboration for testing ( at least calculated..) and let Tim decide about the next steps.

        Show
        Pierre Pichet added a comment - I don't have the time to rewrite the edit_calculated_form: the change of answers and inclusion of your new code. So I prefer that you do this. What I understand from the last days exchanges is that you are putting in your new structure enough flexibility to accomodate actual calculated needs and known future projects. So go ahead and build it. I just offer my collaboration for testing ( at least calculated..) and let Tim decide about the next steps.
        Hide
        Oleg Sychev added a comment -

        Pierre,
        "Done" - Pierre, could you please apply this change to 19 branch? All previous changes of this project was applied to 19 too.

        Show
        Oleg Sychev added a comment - Pierre, "Done" - Pierre, could you please apply this change to 19 branch? All previous changes of this project was applied to 19 too.
        Hide
        Pierre Pichet added a comment -

        I will do it tomorrow when the week version testing will be closed.

        Show
        Pierre Pichet added a comment - I will do it tomorrow when the week version testing will be closed.
        Hide
        Pierre Pichet added a comment -

        Finally, given that Tim will not work on this in the next days and as I think that such a large change should be put first in 2.0 then merge to 1.9, I will wait until further developments.

        Show
        Pierre Pichet added a comment - Finally, given that Tim will not work on this in the next days and as I think that such a large change should be put first in 2.0 then merge to 1.9, I will wait until further developments.
        Hide
        Oleg Sychev added a comment -

        Pierre, remember that I should still do answers->answer conversion and test it before that change, and to do this properly i'll need this questiontype change (or it would unnecessary create some difficulties with different patches for 1.9 and 2.0).

        P.S. Are you joking about a "large" change?

        Show
        Oleg Sychev added a comment - Pierre, remember that I should still do answers->answer conversion and test it before that change, and to do this properly i'll need this questiontype change (or it would unnecessary create some difficulties with different patches for 1.9 and 2.0). P.S. Are you joking about a "large" change?
        Hide
        Pierre Pichet added a comment -

        As a general policy, I don't change things on a stable version unless this is a specific version bug to solve.
        I CVS my locally tested modifications first on HEAD which has +- an experimental status.
        When everything is set and tested then I merge on the stable versions where this can be applied.
        On my working computer I maintain moodle versions down to 1.6 with the stable version and experimental ones and as I use Tortoise, I don't use patch.
        This is not a pro set-up, takes more time but less prone to typo errors , like a turtle..

        "large" was refering to the complete project not this patch.

        Show
        Pierre Pichet added a comment - As a general policy, I don't change things on a stable version unless this is a specific version bug to solve. I CVS my locally tested modifications first on HEAD which has +- an experimental status. When everything is set and tested then I merge on the stable versions where this can be applied. On my working computer I maintain moodle versions down to 1.6 with the stable version and experimental ones and as I use Tortoise, I don't use patch. This is not a pro set-up, takes more time but less prone to typo errors , like a turtle.. "large" was refering to the complete project not this patch.
        Hide
        Oleg Sychev added a comment -

        Pirre, here is 1.9 calculated with modified form (about answers->answer), I modify save_question_options as described above too. (I may send a patch too, if you want). It works well on my testing site (thought you may want to test validation as you better know all possible errors that can be displayed for answer field).

        Please review and apply. I think that merging form change in 2.0 would be easy, as calculated editing form doesn't change much there.

        Show
        Oleg Sychev added a comment - Pirre, here is 1.9 calculated with modified form (about answers->answer), I modify save_question_options as described above too. (I may send a patch too, if you want). It works well on my testing site (thought you may want to test validation as you better know all possible errors that can be displayed for answer field). Please review and apply. I think that merging form change in 2.0 would be easy, as calculated editing form doesn't change much there.
        Hide
        Pierre Pichet added a comment -

        Thanks for the 1.9 but as I wrote in upper comments, I will wait until Tim is ready to work on this and by doing it first on HEAD unless Tim asks me to put this first on 1.9.
        This response was written in a pause in the writing of my two new courses...

        Show
        Pierre Pichet added a comment - Thanks for the 1.9 but as I wrote in upper comments, I will wait until Tim is ready to work on this and by doing it first on HEAD unless Tim asks me to put this first on 1.9. This response was written in a pause in the writing of my two new courses...
        Hide
        Oleg Sychev added a comment -

        Pierre, calculated editing form doesn't changed in 2.0 at all - I check this in WinMerge. I.e. you can just copy edit_calculated_form.php from my archive to 2.0 site and it would work (because you already changed save_question_options). You can test/apply this to 2.0 using that file (apply edit_calculated_form.php only, not other files). I just feel that testing on more stable version is more stable.

        Please remember, that this change is prerequisite for a change, you wait Tim working on, so it should be done first.

        Show
        Oleg Sychev added a comment - Pierre, calculated editing form doesn't changed in 2.0 at all - I check this in WinMerge. I.e. you can just copy edit_calculated_form.php from my archive to 2.0 site and it would work (because you already changed save_question_options). You can test/apply this to 2.0 using that file (apply edit_calculated_form.php only, not other files). I just feel that testing on more stable version is more stable. Please remember, that this change is prerequisite for a change, you wait Tim working on, so it should be done first.
        Hide
        Pierre Pichet added a comment -

        Done on HEAD...
        So test it...

        Show
        Pierre Pichet added a comment - Done on HEAD... So test it...
        Hide
        Oleg Sychev added a comment -

        I've already tested it, but since you insists I tested it on you site mentioned above too.

        All work well, but I found one bug in calculated doing so: formula validator accept things like

        {a}

        ?

        {b}

        , which than results in fatal errors during eval(), that get printed on the page. I think that best additional validation would be to place there eval in try...except clause, and report unknown error if exception was caught .

        Merging to 1.9 would be easy, as you can working code for it.

        Show
        Oleg Sychev added a comment - I've already tested it, but since you insists I tested it on you site mentioned above too. All work well, but I found one bug in calculated doing so: formula validator accept things like {a} ? {b} , which than results in fatal errors during eval(), that get printed on the page. I think that best additional validation would be to place there eval in try...except clause, and report unknown error if exception was caught . Merging to 1.9 would be easy, as you can working code for it.
        Hide
        Pierre Pichet added a comment -

        What I just mean is that I put it on HEAD so you can try on your site the HEAD version of your modifications.
        I did not put this on 1.9 for reasons already explained.
        Having a more robust testing and code handling of the eval() is effectively something to-do.
        You could create a bug related ot adding a try except clause.
        I was on the impression that this will work only on PHP5 and plan to review the HEAD code for new PHP5 possibilities.

        If you want to review the calculated code , go ahead, you are welcome.

        Show
        Pierre Pichet added a comment - What I just mean is that I put it on HEAD so you can try on your site the HEAD version of your modifications. I did not put this on 1.9 for reasons already explained. Having a more robust testing and code handling of the eval() is effectively something to-do. You could create a bug related ot adding a try except clause. I was on the impression that this will work only on PHP5 and plan to review the HEAD code for new PHP5 possibilities. If you want to review the calculated code , go ahead, you are welcome.
        Hide
        Oleg Sychev added a comment -

        The code is already tested - I don't really need to put something in CVS to place it on my testing site.
        My algorithm of testing is something like:
        1) update to the latest version available (the CVS will merge my existing modifications, if it possible, actually my site has about 10 modification not checked in CVS in several areas of Moodle right now, and there is no problems with merge for two months)
        2) edit this updated code to make a change
        3) test it
        4) generate a patch with CVS version

        The only thing this is not efficient is if you have to make to separate modification on one file - they got tied up together. (I usually use temporary partial check-outs to handle such changes separately but it's not convenient). That's is main reason I want this in 1.9 before Tim will really change MDL-18035 - I can't include calculated in this change before you check in this thing, or there would be errors. It will be late after Tim will decide there.

        You may test it by youself if you want to ensure it working and don't trust entirely my testing (there is no offence in this).

        Show
        Oleg Sychev added a comment - The code is already tested - I don't really need to put something in CVS to place it on my testing site. My algorithm of testing is something like: 1) update to the latest version available (the CVS will merge my existing modifications, if it possible, actually my site has about 10 modification not checked in CVS in several areas of Moodle right now, and there is no problems with merge for two months) 2) edit this updated code to make a change 3) test it 4) generate a patch with CVS version The only thing this is not efficient is if you have to make to separate modification on one file - they got tied up together. (I usually use temporary partial check-outs to handle such changes separately but it's not convenient). That's is main reason I want this in 1.9 before Tim will really change MDL-18035 - I can't include calculated in this change before you check in this thing, or there would be errors. It will be late after Tim will decide there. You may test it by youself if you want to ensure it working and don't trust entirely my testing (there is no offence in this).
        Hide
        Oleg Sychev added a comment -

        By the way, Pierre, what import formats support calculated? I'm wondering if I can bypass formula validation using import/restore with edited file. This will be quite worrisome thing, as right now validation may be the only thing staying between user ability to place an arbitrary PHP code in you evals.

        Show
        Oleg Sychev added a comment - By the way, Pierre, what import formats support calculated? I'm wondering if I can bypass formula validation using import/restore with edited file. This will be quite worrisome thing, as right now validation may be the only thing staying between user ability to place an arbitrary PHP code in you evals.
        Hide
        Pierre Pichet added a comment -

        As far as I know import through XML Moodle and WEBCT.

        an arbitrary PHP code in your evals.
        They are not mine but define before me.
        This problem of "security breaks" is greater as students can be allowed to create question.

        Show
        Pierre Pichet added a comment - As far as I know import through XML Moodle and WEBCT. an arbitrary PHP code in your evals. They are not mine but define before me. This problem of "security breaks" is greater as students can be allowed to create question.
        Hide
        Oleg Sychev added a comment -

        Pierre, I really can't understand why you are opposing inheriting calculated form from numerical. I just can't see the way such inheritance can restrict you (while you obviously get less code to maintain). If you don't like some particular function, you still can completely override it even when inheriting. I'm thinking of starting from reusing definition_inner code, leaving set_data and validation as they are now to future consideration.

        Would you future simplified calculated use question_edit_calculated_form class too, or it will have it's own editing form?

        P.S. I managed to get php code in eval using import, but first time failed to do something malicious with it. Will need to study calculated code more thorought to do it. I think either save_question_options should check formula validity, or it should be done before any eval to be sure (right now there is one eval before it isn't done).

        Show
        Oleg Sychev added a comment - Pierre, I really can't understand why you are opposing inheriting calculated form from numerical. I just can't see the way such inheritance can restrict you (while you obviously get less code to maintain). If you don't like some particular function, you still can completely override it even when inheriting. I'm thinking of starting from reusing definition_inner code, leaving set_data and validation as they are now to future consideration. Would you future simplified calculated use question_edit_calculated_form class too, or it will have it's own editing form? P.S. I managed to get php code in eval using import, but first time failed to do something malicious with it. Will need to study calculated code more thorought to do it. I think either save_question_options should check formula validity, or it should be done before any eval to be sure (right now there is one eval before it isn't done).
        Hide
        Pierre Pichet added a comment -

        "Pierre, I really can't understand why you are opposing inheriting calculated form from numerical."

        This is no more a problem,I have a better understanding of your proposal and I can live with it.
        I comment this in another bug ( just a side effect of having multiple bugs for this quite complex problem).

        "simplified calculated will have it's own editing form" as it will include some variant of datasetitemsform code.

        "P.S. I managed to get php code in eval using import, but first time failed to do something malicious with it. Will need to study calculated code more thorought to do it. I think either save_question_options should check formula validity, or it should be done before any eval to be sure (right now there is one eval before it isn't done)."
        This should be put in MDL-18141
        Done
        Effectively there is no checking on import.

        I will look at this.

        Show
        Pierre Pichet added a comment - "Pierre, I really can't understand why you are opposing inheriting calculated form from numerical." This is no more a problem,I have a better understanding of your proposal and I can live with it. I comment this in another bug ( just a side effect of having multiple bugs for this quite complex problem). "simplified calculated will have it's own editing form" as it will include some variant of datasetitemsform code. "P.S. I managed to get php code in eval using import, but first time failed to do something malicious with it. Will need to study calculated code more thorought to do it. I think either save_question_options should check formula validity, or it should be done before any eval to be sure (right now there is one eval before it isn't done)." This should be put in MDL-18141 Done Effectively there is no checking on import. I will look at this.
        Hide
        Oleg Sychev added a comment -

        Pierre, are you considered this issue resolved? The renaming is done...

        Show
        Oleg Sychev added a comment - Pierre, are you considered this issue resolved? The renaming is done...
        Hide
        Ray Lawrence added a comment -

        I hope you can help me here. The title of this issue suggests a quick language string change but other things seem to have been incorporated.. Could one of you summarise the other changes made here?

        Show
        Ray Lawrence added a comment - I hope you can help me here. The title of this issue suggests a quick language string change but other things seem to have been incorporated.. Could one of you summarise the other changes made here?
        Hide
        Oleg Sychev added a comment -

        Ray -
        This issue deals with renaming of a field for answer formula entering on the question editing page - by changing internal name of the field, not the language string. Quite a tricky thing really, but this a prerequisite for MDL-18035. You may get a quick impression from what was happened there from CVS diffs.

        Show
        Oleg Sychev added a comment - Ray - This issue deals with renaming of a field for answer formula entering on the question editing page - by changing internal name of the field, not the language string. Quite a tricky thing really, but this a prerequisite for MDL-18035 . You may get a quick impression from what was happened there from CVS diffs.
        Hide
        Ray Lawrence added a comment -

        Ah, OK. Thanks.

        Show
        Ray Lawrence added a comment - Ah, OK. Thanks.
        Hide
        Oleg Sychev added a comment -

        Ray - If you are looking for actually user-visible changes these issues are help to, you must look at MDL-17064. It has staled a bit so a new people are very welcome there.

        Show
        Oleg Sychev added a comment - Ray - If you are looking for actually user-visible changes these issues are help to, you must look at MDL-17064 . It has staled a bit so a new people are very welcome there.
        Hide
        Oleg Sychev added a comment -

        Pierre, what do you think about setting this issue to resolved? I can do this, but I want you to be aware. You never set it resolved, nor tell any reason why you want to keep it open. Is there any reason?

        We done it a long time ago, and I don't like many additional issues on my unresolved list. Also, QA people can give this one more look (and test) if it will be resolved.

        Show
        Oleg Sychev added a comment - Pierre, what do you think about setting this issue to resolved? I can do this, but I want you to be aware. You never set it resolved, nor tell any reason why you want to keep it open. Is there any reason? We done it a long time ago, and I don't like many additional issues on my unresolved list. Also, QA people can give this one more look (and test) if it will be resolved.
        Hide
        Pierre Pichet added a comment -

        Tim wrote elsewhere that
        if (isset($question->answer) && !isset($question->answers))

        { $question->answers=$question->answer; }

        is more an hack then a final solution.
        This is why I did not set it as resolved.
        I am coming back on coding after a 2 months srike and other delays .
        I need to clean all my open bugs before the end of summer(excluding holydays..)
        So I let it as a reminder but will give it a in progress state.

        Show
        Pierre Pichet added a comment - Tim wrote elsewhere that if (isset($question->answer) && !isset($question->answers)) { $question->answers=$question->answer; } is more an hack then a final solution. This is why I did not set it as resolved. I am coming back on coding after a 2 months srike and other delays . I need to clean all my open bugs before the end of summer(excluding holydays..) So I let it as a reminder but will give it a in progress state.
        Hide
        Oleg Sychev added a comment -

        Hmm, Pierre, could we at last solve this?

        Show
        Oleg Sychev added a comment - Hmm, Pierre, could we at last solve this?
        Hide
        Pierre Pichet added a comment -

        I have a mix feeling on this answer - answers translation.
        In PHP coding rule adding a plural i.e answers to a variable that is an array of answer is the recommended practice.

        When we migrate to use the moodle forms our coding reflect an HTML pratice to not set to plural on the form an element that can be single or plural so we use answer on the form.

        Which option should govern our PHP coding giving the new incentive for good PHP coding rules and the tool build by Tim to support this?

        Tim and you are the pro in coding and I just applied the code rules that were used in the original calculated question code ( 1,5 and 1,6).

        So what are the official Moodle rules that should be applied?

        Show
        Pierre Pichet added a comment - I have a mix feeling on this answer - answers translation. In PHP coding rule adding a plural i.e answers to a variable that is an array of answer is the recommended practice. When we migrate to use the moodle forms our coding reflect an HTML pratice to not set to plural on the form an element that can be single or plural so we use answer on the form. Which option should govern our PHP coding giving the new incentive for good PHP coding rules and the tool build by Tim to support this? Tim and you are the pro in coding and I just applied the code rules that were used in the original calculated question code ( 1,5 and 1,6). So what are the official Moodle rules that should be applied?
        Hide
        Oleg Sychev added a comment -

        Hmm, Pierre, this conversion was done in the sake of consistency - now all Moodle question use same name "answer" and there is code relying on it. E.g. you should rename this field further either in all question types (which I doubt you want to) or no at all.

        You kept this issue unresolved while we changed this code years ago. Do you still want it in this state? I just want to clear a backlog a little.

        Show
        Oleg Sychev added a comment - Hmm, Pierre, this conversion was done in the sake of consistency - now all Moodle question use same name "answer" and there is code relying on it. E.g. you should rename this field further either in all question types (which I doubt you want to) or no at all. You kept this issue unresolved while we changed this code years ago. Do you still want it in this state? I just want to clear a backlog a little.
        Hide
        Pierre Pichet added a comment -

        looking to the code more closely I notice

        /**
             * Get the list of form elements to repeat, one for each answer.
             * @param object $mform the form being built.
             * @param $label the label to use for each option.
             * @param $gradeoptions the possible grades for each answer.
             * @param $repeatedoptions reference to array of repeated options to fill
             * @param $answersoption reference to return the name of $question->options
             *      field holding an array of answers
             * @return array of form fields.
             */
            protected function get_per_answer_fields($mform, $label, $gradeoptions,
                    &$repeatedoptions, &$answersoption) {
        ...
        protected function add_per_answer_fields(&$mform, $label, $gradeoptions,
                    $minoptions = QUESTION_NUMANS_START, $addoptions = QUESTION_NUMANS_ADD) {
                $answersoption = '';
                $repeatedoptions = array();
                $repeated = $this->get_per_answer_fields($mform, $label, $gradeoptions,
                        $repeatedoptions, $answersoption);
        
                if (isset($this->question->options)) {
                    $countanswers = count($this->question->options->$answersoption);
                } else {
                    $countanswers = 0;
                }
        ...
           /**
             * Perform the necessary preprocessing for the fields added by
             * {@link add_per_answer_fields()}.
             * @param object $question the data being passed to the form.
             * @return object $question the modified data.
             */
            protected function data_preprocessing_answers($question, $withanswerfiles = false) {
                if (empty($question->options->answers)) {
                    return $question;
                }
        
                $key = 0;
                foreach ($question->options->answers as $answer) {
                    if ($withanswerfiles) {
                        // Prepare the feedback editor to display files in draft area
                        $draftitemid = file_get_submitted_draft_itemid('answer['.$key.']');
                        $question->answer[$key]['text'] = file_prepare_draft_area(
                            $draftitemid,          // draftid
                            $this->context->id,    // context
                            'question',            // component
                            'answer',              // filarea
                            !empty($answer->id) ? (int) $answer->id : null, // itemid
                            $this->fileoptions,    // options
                            $answer->answer        // text
                        );
                        $question->answer[$key]['itemid'] = $draftitemid;
                        $question->answer[$key]['format'] = $answer->answerformat;
        
        

        I understand that "ambiguous code" expects (or not ?) that the question type stored the data in an array "answers"
        although it returns to the question types the data in an array "answer"

        This should not be the case for a stable code.

        In the design there was a provision to good handling using

        • @param $answersoption reference to return the name of $question->options
        • field holding an array of answers
          but the actual code does not seem to handle this correctly from the question type viewpoint.

        Tim,
        I will appreciate your comments on this, perhaps there is something I don't understand well...

        Show
        Pierre Pichet added a comment - looking to the code more closely I notice /** * Get the list of form elements to repeat, one for each answer. * @param object $mform the form being built. * @param $label the label to use for each option. * @param $gradeoptions the possible grades for each answer. * @param $repeatedoptions reference to array of repeated options to fill * @param $answersoption reference to return the name of $question->options * field holding an array of answers * @return array of form fields. */ protected function get_per_answer_fields($mform, $label, $gradeoptions, &$repeatedoptions, &$answersoption) { ... protected function add_per_answer_fields(&$mform, $label, $gradeoptions, $minoptions = QUESTION_NUMANS_START, $addoptions = QUESTION_NUMANS_ADD) { $answersoption = ''; $repeatedoptions = array(); $repeated = $this->get_per_answer_fields($mform, $label, $gradeoptions, $repeatedoptions, $answersoption); if (isset($this->question->options)) { $countanswers = count($this->question->options->$answersoption); } else { $countanswers = 0; } ... /** * Perform the necessary preprocessing for the fields added by * {@link add_per_answer_fields()}. * @param object $question the data being passed to the form. * @return object $question the modified data. */ protected function data_preprocessing_answers($question, $withanswerfiles = false) { if (empty($question->options->answers)) { return $question; } $key = 0; foreach ($question->options->answers as $answer) { if ($withanswerfiles) { // Prepare the feedback editor to display files in draft area $draftitemid = file_get_submitted_draft_itemid('answer['.$key.']'); $question->answer[$key]['text'] = file_prepare_draft_area( $draftitemid, // draftid $this->context->id, // context 'question', // component 'answer', // filarea !empty($answer->id) ? (int) $answer->id : null, // itemid $this->fileoptions, // options $answer->answer // text ); $question->answer[$key]['itemid'] = $draftitemid; $question->answer[$key]['format'] = $answer->answerformat; I understand that "ambiguous code" expects (or not ?) that the question type stored the data in an array "answers" although it returns to the question types the data in an array "answer" This should not be the case for a stable code. In the design there was a provision to good handling using @param $answersoption reference to return the name of $question->options field holding an array of answers but the actual code does not seem to handle this correctly from the question type viewpoint. Tim, I will appreciate your comments on this, perhaps there is something I don't understand well...
        Hide
        Oleg Sychev added a comment -

        Pierre, $question->option->answers is an array of answers (e.g. all answers), while "answer" in a form is a field for each answer.

        Also, $question->option->answers contains objects (with fields answer, fraction etc within - and the field for actual answer inside object named "answer").

        So it's a sort of OK. asnwers array hold objects, while answer array from form - only array of the answer field for these objects. That is the way forms and DB works, it's not easy to have similar arrays in both question and form.

        Show
        Oleg Sychev added a comment - Pierre, $question->option->answers is an array of answers (e.g. all answers), while "answer" in a form is a field for each answer. Also, $question->option->answers contains objects (with fields answer, fraction etc within - and the field for actual answer inside object named "answer"). So it's a sort of OK. asnwers array hold objects, while answer array from form - only array of the answer field for these objects. That is the way forms and DB works, it's not easy to have similar arrays in both question and form.
        Hide
        Pierre Pichet added a comment -

        I understand that this is not "easy to have similar arrays in both question and form".

        The main problem appears when the question type is saving its options which could come from another way than the edit_question_form i.e. import.

        Should the restrictions set by the editform be applied to other sources
        or
        could we have a better use of $answersoption in the edit-question_form
        or
        an additional parameter from the edit_question_form to signal the "question->answer" structure
        or
        ?

        On the long term, which solution could give us the more stable code that will not change from moodle versions
        and help to the stability of core and contribute question types?

        Show
        Pierre Pichet added a comment - I understand that this is not "easy to have similar arrays in both question and form". The main problem appears when the question type is saving its options which could come from another way than the edit_question_form i.e. import. Should the restrictions set by the editform be applied to other sources or could we have a better use of $answersoption in the edit-question_form or an additional parameter from the edit_question_form to signal the "question->answer" structure or ? On the long term, which solution could give us the more stable code that will not change from moodle versions and help to the stability of core and contribute question types?
        Hide
        Oleg Sychev added a comment -

        Well, Pierre, we are returning to the fact that fraction, tolerance etc are similar arrays (for form, import etc) and they are all named without "s". Should they all be renamed?

        It's much easier to have import using conventions from form, than form using convention for input. $answeroption would not help with this problem, it's for $question->options-> data, not for form data. And other methods you suggests require tricky work with forms library which is clearly not the way leading to the more stable code.

        Show
        Oleg Sychev added a comment - Well, Pierre, we are returning to the fact that fraction, tolerance etc are similar arrays (for form, import etc) and they are all named without "s". Should they all be renamed? It's much easier to have import using conventions from form, than form using convention for input. $answeroption would not help with this problem, it's for $question->options-> data, not for form data. And other methods you suggests require tricky work with forms library which is clearly not the way leading to the more stable code.
        Hide
        Pierre Pichet added a comment -

        You have a good point.

        So OK, I will put this on the top of my moodle todo list (fist lest's start the winter course session).

        However, I think that this feature ( reading as answers and saving as answer ) should be clearly documented
        if not done.

        Should you do some corrections of your functions about the use or not of the $answersoption parameter ?

        Show
        Pierre Pichet added a comment - You have a good point. So OK, I will put this on the top of my moodle todo list (fist lest's start the winter course session). However, I think that this feature ( reading as answers and saving as answer ) should be clearly documented if not done. Should you do some corrections of your functions about the use or not of the $answersoption parameter ?
        Hide
        Michael de Raadt added a comment -

        Thanks for reporting this issue.

        We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported.

        If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

        Michael d.

        TW9vZGxlDQo=

        Show
        Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d. TW9vZGxlDQo=
        Hide
        Tim Hunt added a comment -

        I think this has been fixed since the new question engine in Moodle 2.1. Closing.

        Show
        Tim Hunt added a comment - I think this has been fixed since the new question engine in Moodle 2.1. Closing.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: