Moodle
  1. Moodle
  2. MDL-31498

Default for mform repeated elements does not work (and has never worked)

    Details

    • Testing Instructions:
      Hide

      Test1:

      1. copy repeatgrouptest.php in moodle site
      2. navigate to repeatgrouptest.php through browser
      3. Make sure field value is set to default value, as set in repeatgrouptest.php (change default value and make sure it is set accordingly)
        Note: Rules and disbaledif options will not work for above testcase
        Test 2:
      4. copy repeattest.php in moodle site
      5. navigate to repeattest.php through browser
      6. Make sure Limit value is set to 0 and is disabled.
      7. Select checkbox and limit field should be enabled.
      8. Enter text (alphanumeric) and you should see error message
      9. Try submit the form, it should not be submitted
      10. Click "Add 3 more fields", and you should be able to add more fields.
      11. Change limit values to numeric and enter some text in fields.
      12. Try submit form and check values. Make sure they are correct as set in form.

      Test 3:
      Follow MDL-30335 test instructions.

      Test 4:
      Run php unit for formslib_test.php

      Test 5:

      1. Create choice activity
      2. Make sure limit is set to 0
      3. Set Limit to enable and all limit fields should be enabled.
      4. Try enter alphanumeric characters in limit and make sure you see validation error
      5. Complete the form and save, make sure all values are saved properly.

      Test 6:
      Please also test some existing areas of Moodle that user repeat_elements and defaults. For example,

      1. In the qtype_calculated editing form, each answer should have a default answer length of 2, and a default tolerance of 0.01.
      2. In qtype_numerical, Accepted error should be set to 0 and alphanumeric value should not be saved. Trying to save alphanumeric will reset this to 0.
      Show
      Test1: copy repeatgrouptest.php in moodle site navigate to repeatgrouptest.php through browser Make sure field value is set to default value, as set in repeatgrouptest.php (change default value and make sure it is set accordingly) Note: Rules and disbaledif options will not work for above testcase Test 2: copy repeattest.php in moodle site navigate to repeattest.php through browser Make sure Limit value is set to 0 and is disabled. Select checkbox and limit field should be enabled. Enter text (alphanumeric) and you should see error message Try submit the form, it should not be submitted Click "Add 3 more fields", and you should be able to add more fields. Change limit values to numeric and enter some text in fields. Try submit form and check values. Make sure they are correct as set in form. Test 3: Follow MDL-30335 test instructions. Test 4: Run php unit for formslib_test.php Test 5: Create choice activity Make sure limit is set to 0 Set Limit to enable and all limit fields should be enabled. Try enter alphanumeric characters in limit and make sure you see validation error Complete the form and save, make sure all values are saved properly. Test 6: Please also test some existing areas of Moodle that user repeat_elements and defaults. For example, In the qtype_calculated editing form, each answer should have a default answer length of 2, and a default tolerance of 0.01. In qtype_numerical, Accepted error should be set to 0 and alphanumeric value should not be saved. Trying to save alphanumeric will reset this to 0.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-mdl-31498-default
    • Rank:
      38036

      Description

      Copy the attached php file in the root of your moodle head (close to config.php)
      Call it with a browser.
      As you can see by watching at the code, there are defaults for grouped repeated fields. (lines: 25, 30 and 35 of the attached file)
      In spite of this, default are not applied to respective fields at runtime.

      Now go to the function repeat_elements
      at line 962 of HEAD/lib/formslib.php
      and change lines around 1000

      from

      $pos=strpos($elementname, '[');
      if ($pos!==FALSE){
          $realelementname = substr($elementname, 0, $pos+1)."[$i]";
          $realelementname .= substr($elementname, $pos+1);
      }else {

      to

      if (strpos($elementname, '[') !== FALSE){
          $realelementname = str_replace('[', '['.$i.'][', $elementname);
      } else {

      Call again the same file.
      Now defaults are applied!!!

      One more issue (but without solution, this one):

      by adding

      $fieldname = 'color[green]';
      $repeateloptions[$fieldname]['rule'] = 'numeric';

      to my attached code
      I get the error:

       QuickForm Error: nonexistent html element Element 'color[0][green]' does not exist in HTML_QuickForm::addRule()

      About the other $options of repeat_elements function:
      -> helpbutton: works
      -> type: works

      -> disabledif: I don't know
      -> rule: doesn't work
      -> default: is the main topic of this issue

      1. mformtest.php
        2 kB
        Daniele Cordella
      2. mod_form.php
        38 kB
        Daniele Cordella
      3. repeatgrouptest.php
        2 kB
        Rajesh Taneja
      4. repeattest.php
        2 kB
        Rajesh Taneja

        Issue Links

          Activity

          Hide
          Daniele Cordella added a comment -

          Tim, I added you to this issue to let you understand why I was so inexplicably persistent over jabber developer chat.

          Show
          Daniele Cordella added a comment - Tim, I added you to this issue to let you understand why I was so inexplicably persistent over jabber developer chat.
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this.

          Show
          Michael de Raadt added a comment - Thanks for working on this.
          Hide
          Daniele Cordella added a comment -

          'disabledif' works fine

          Show
          Daniele Cordella added a comment - 'disabledif' works fine
          Hide
          Andrea Bicciolo added a comment -

          Hope the issue can be sorted out as it appears to influence developments we are currently working on. Any chance ?

          Show
          Andrea Bicciolo added a comment - Hope the issue can be sorted out as it appears to influence developments we are currently working on. Any chance ?
          Hide
          Rajesh Taneja added a comment -

          I have added Michael, for him to give some love to this bug, in next sprint
          Thanks Andrea.

          Show
          Rajesh Taneja added a comment - I have added Michael, for him to give some love to this bug, in next sprint Thanks Andrea.
          Hide
          Rajesh Taneja added a comment -

          Adding this to next sprint

          Show
          Rajesh Taneja added a comment - Adding this to next sprint
          Hide
          Rajesh Taneja added a comment -

          Hello Daniele,

          Can you please suggest how disableif is working for this? AFAIK it's not correctly coded. Anyways I have added patch to fix default, rule and disableif for repeated elements.

          It will be helpful, if you can try the patch and provide some feedback.

          Show
          Rajesh Taneja added a comment - Hello Daniele, Can you please suggest how disableif is working for this? AFAIK it's not correctly coded. Anyways I have added patch to fix default, rule and disableif for repeated elements. It will be helpful, if you can try the patch and provide some feedback.
          Hide
          Tim Hunt added a comment -

          I don't like your code changes, they seem highly dangerous. For example, why make this first change:

          1) https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-31498#L0L1003

          The old code safely inserted on "[$i]" in front of all other array indices (e.g. groupname[field1][text] -> groupname[1][field1][text]). Your new code will insert "[$i]" in front of any array index (e.g. groupname[field1][text] -> groupname[1][field1][1][text]).

          2) The second hunk of the patch changes the disabledIf code, although the there is a comment above saying that disabledIf already works fine. Why are you changing this? Have you looked at all the previous bugs about disabledIf? It took a lot of work to get it working. Please do not casually change it unless you are prepared to repeat all the testing instructions from all the fixed issues in the past.

          If the change really is necessary, then you need a much better commit comment than https://github.com/rajeshtaneja/moodle/commit/c0be045d932e1978adb02b9face0703d6fba424e (or comments inline in the code) to explain it.

          3) Actually, looking at the changes as a whole, I cannot see any part of the patch that fixes defaults. In fact, defaults do work for disabledIf elements. I am using that in qtype_stack: https://github.com/sangwinc/moodle-qtype_stack/blob/master/edit_stack_form.php#L441. So what is the point of the changes you are making in this code?

          -1 for this patch. Sorry.

          Show
          Tim Hunt added a comment - I don't like your code changes, they seem highly dangerous. For example, why make this first change: 1) https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-31498#L0L1003 The old code safely inserted on " [$i] " in front of all other array indices (e.g. groupname [field1] [text] -> groupname [1] [field1] [text] ). Your new code will insert " [$i] " in front of any array index (e.g. groupname [field1] [text] -> groupname [1] [field1] [1] [text] ). 2) The second hunk of the patch changes the disabledIf code, although the there is a comment above saying that disabledIf already works fine. Why are you changing this? Have you looked at all the previous bugs about disabledIf? It took a lot of work to get it working. Please do not casually change it unless you are prepared to repeat all the testing instructions from all the fixed issues in the past. If the change really is necessary, then you need a much better commit comment than https://github.com/rajeshtaneja/moodle/commit/c0be045d932e1978adb02b9face0703d6fba424e (or comments inline in the code) to explain it. 3) Actually, looking at the changes as a whole, I cannot see any part of the patch that fixes defaults. In fact, defaults do work for disabledIf elements. I am using that in qtype_stack: https://github.com/sangwinc/moodle-qtype_stack/blob/master/edit_stack_form.php#L441 . So what is the point of the changes you are making in this code? -1 for this patch. Sorry.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the review Tim,

          1. https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-31498#L0L1003 is adding $i after the opening bracket "[". So it was making $realelementname = color[[0]blue], whereas the element name should be color[0][blue].
            Not sure how this is working, probably I missed something.
          2. I was not sure about disableif, that's why I added it as second patch. Can you please update repeattest.php to show me how it's going to work in this case.
          3. Can you please confirm if, rule was working for repeat elements.

          FYI:
          I referred http://docs.moodle.org/dev/lib/formslib.php_repeat_elements and default works fine, but in attached testcase it doesn't.

          Show
          Rajesh Taneja added a comment - Thanks for the review Tim, https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-31498#L0L1003 is adding $i after the opening bracket "[". So it was making $realelementname = color[ [0] blue], whereas the element name should be color [0] [blue] . Not sure how this is working, probably I missed something. I was not sure about disableif, that's why I added it as second patch. Can you please update repeattest.php to show me how it's going to work in this case. Can you please confirm if, rule was working for repeat elements. FYI: I referred http://docs.moodle.org/dev/lib/formslib.php_repeat_elements and default works fine, but in attached testcase it doesn't.
          Hide
          Tim Hunt added a comment -

          1. OK, so there was a bug in the old code, but as I point out, there is also a bug in your new code. Best thing would be to fix this code in a way where it was possible to write unit tests. That is extract an add_index_to_field_name($originalname, $index) method, and write unit tests for it.

          2. Please look at the testing instructions for the fixed bugs in http://tracker.moodle.org/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+MDL+AND+%28summary+~+%22%2Bdisabledif%22+OR+description+~+%22%2Bdisabledif%22%29. There are some good test scripts.

          3. That list of old bugs will also give you a good idea of the current state of working/brokenness.

          Show
          Tim Hunt added a comment - 1. OK, so there was a bug in the old code, but as I point out, there is also a bug in your new code. Best thing would be to fix this code in a way where it was possible to write unit tests. That is extract an add_index_to_field_name($originalname, $index) method, and write unit tests for it. 2. Please look at the testing instructions for the fixed bugs in http://tracker.moodle.org/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+MDL+AND+%28summary+~+%22%2Bdisabledif%22+OR+description+~+%22%2Bdisabledif%22%29 . There are some good test scripts. 3. That list of old bugs will also give you a good idea of the current state of working/brokenness.
          Hide
          Rajesh Taneja added a comment -

          Thanks Tim,

          I will go though the list and see how well this can be fixed (without breaking current functionality).

          Show
          Rajesh Taneja added a comment - Thanks Tim, I will go though the list and see how well this can be fixed (without breaking current functionality).
          Hide
          Daniele Cordella added a comment -

          GRRRR
          I have been on jabber for days and days.
          Of course the output of your work started here just today when I am busy looking after children fee from school!!
          Ok, ok. I will write your name on black list.
          I quickly read you comments. As usual I did not understand you. Now I will read them again and try to provide my feedback whether requested. Anyway: thank you all.

          Show
          Daniele Cordella added a comment - GRRRR I have been on jabber for days and days. Of course the output of your work started here just today when I am busy looking after children fee from school!! Ok, ok. I will write your name on black list. I quickly read you comments. As usual I did not understand you. Now I will read them again and try to provide my feedback whether requested. Anyway: thank you all.
          Hide
          Daniele Cordella added a comment -

          In Italy we say: "Non buttare benzina sul fuoco" that may be translated as: "Do not throw gasoline on the fire" but I feel I am called to do it.
          I applied ONLY MY PATCH (the one I described in the issue description) and I can confirm that "disabledif" works fine.
          I tested it with the attached mod_form that I would not share because it belongs to a code commissioned from a customer.

          Show
          Daniele Cordella added a comment - In Italy we say: "Non buttare benzina sul fuoco" that may be translated as: "Do not throw gasoline on the fire" but I feel I am called to do it. I applied ONLY MY PATCH (the one I described in the issue description) and I can confirm that "disabledif" works fine. I tested it with the attached mod_form that I would not share because it belongs to a code commissioned from a customer.
          Hide
          Rajesh Taneja added a comment -

          Thanks Daniele

          Show
          Rajesh Taneja added a comment - Thanks Daniele
          Hide
          Rajesh Taneja added a comment -

          Hello Tim,

          Can you please review the changes again and provide some feedback.

          Show
          Rajesh Taneja added a comment - Hello Tim, Can you please review the changes again and provide some feedback.
          Hide
          Tim Hunt added a comment -

          Sorry Rajest, I have just remebered that I had to grapple with this problem in the past, and found a work-around. See https://github.com/moodle/moodle/blob/master/question/type/edit_question_form.php#L540 for a long comment about it. Anyway, with that bit of hackery, defaults and set_data for repeat elements has been working for me in the question types for over a year. Still, it would be nice to sort out formslib.php so that sort of hackery was not necessary.

          The patch is looking better. +1 for the unit tests. I still cannot say whether it is right or not. Formslib.php is so bloody complicated.

          I will, however, point out that you must test your code with repeated groups that have both $elementclone->_appendName set to true and set to false, to make sure that both those cases work. Also, you should extend the testing instructions to include creating and editing questions, since those are forms in Moodle core that currently rely on this part of formslib working properly. Thanks.

          Show
          Tim Hunt added a comment - Sorry Rajest, I have just remebered that I had to grapple with this problem in the past, and found a work-around. See https://github.com/moodle/moodle/blob/master/question/type/edit_question_form.php#L540 for a long comment about it. Anyway, with that bit of hackery, defaults and set_data for repeat elements has been working for me in the question types for over a year. Still, it would be nice to sort out formslib.php so that sort of hackery was not necessary. The patch is looking better. +1 for the unit tests. I still cannot say whether it is right or not. Formslib.php is so bloody complicated. I will, however, point out that you must test your code with repeated groups that have both $elementclone->_appendName set to true and set to false, to make sure that both those cases work. Also, you should extend the testing instructions to include creating and editing questions, since those are forms in Moodle core that currently rely on this part of formslib working properly. Thanks.
          Hide
          Rajesh Taneja added a comment -

          Thanks for all the feedback.
          On another thought, it might make sense to just fix default issue in this bug and fix disabledif and rule issues in another bug.
          I have created another branch https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-31498-default which is safe to fix. As we know, element name is suffixed with [$i], this seems to be a safe fix.

          As per disabledif and rule issue, it might be nice to look at them in another bug. Any feedback is highly appreciated

          Show
          Rajesh Taneja added a comment - Thanks for all the feedback. On another thought, it might make sense to just fix default issue in this bug and fix disabledif and rule issues in another bug. I have created another branch https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-31498-default which is safe to fix. As we know, element name is suffixed with [$i] , this seems to be a safe fix. As per disabledif and rule issue, it might be nice to look at them in another bug. Any feedback is highly appreciated
          Hide
          Tim Hunt added a comment -

          Yes. That simple change is a good fix. Get that in, then open new MDLs for the other problems. Thanks.

          Show
          Tim Hunt added a comment - Yes. That simple change is a good fix. Get that in, then open new MDLs for the other problems. Thanks.
          Hide
          Rajesh Taneja added a comment -

          Thanks Tim

          Show
          Rajesh Taneja added a comment - Thanks Tim
          Hide
          Dan Poltawski added a comment -

          Sorry Raj,

          but i'm running the test script and getting:

          QuickForm Error: nonexistent html element Element 'color[0][red]' does not exist in HTML_QuickForm::addRule()
          [...]

          Show
          Dan Poltawski added a comment - Sorry Raj, but i'm running the test script and getting: QuickForm Error: nonexistent html element Element 'color [0] [red] ' does not exist in HTML_QuickForm::addRule() [...]
          Hide
          Rajesh Taneja added a comment -

          My bad Dan,

          Forgot to update the testcase. This patch is just fixing default. I have opened MDL-32722 to fix disabledif and rules.
          Can you please check this again.

          Thanks

          Show
          Rajesh Taneja added a comment - My bad Dan, Forgot to update the testcase. This patch is just fixing default. I have opened MDL-32722 to fix disabledif and rules. Can you please check this again. Thanks
          Hide
          Dan Poltawski added a comment -

          I think this change looks good, i'd just like us to increase the test cases that we have in that test script because any change with formslib makes me very nervous..

          Raj - could you increase the test cases we run on that script? Things i'd think would be good to see are:

          • Ensuring that the actual output generated from the form doesn't change (i.e. the data posted is the same or comes back to forms the same). print_object() of output i'm sure is fine
          • Ensuring that cleaning is still applied correctly. (Test posting some html in a text only field)
          • Double checking the places in Moodle which are using the repeat elements
          Show
          Dan Poltawski added a comment - I think this change looks good, i'd just like us to increase the test cases that we have in that test script because any change with formslib makes me very nervous.. Raj - could you increase the test cases we run on that script? Things i'd think would be good to see are: Ensuring that the actual output generated from the form doesn't change (i.e. the data posted is the same or comes back to forms the same). print_object() of output i'm sure is fine Ensuring that cleaning is still applied correctly. (Test posting some html in a text only field) Double checking the places in Moodle which are using the repeat elements
          Hide
          Rajesh Taneja added a comment -

          Thanks Tim and Dan,

          I will try add more test coverage.

          Show
          Rajesh Taneja added a comment - Thanks Tim and Dan, I will try add more test coverage.
          Hide
          Rajesh Taneja added a comment -

          Hope this is fine now.

          Show
          Rajesh Taneja added a comment - Hope this is fine now.
          Hide
          Dan Poltawski added a comment -

          Raj and I discussed this and are not convinced there is a need to backport this since it will never have been working, so going for master only.

          Show
          Dan Poltawski added a comment - Raj and I discussed this and are not convinced there is a need to backport this since it will never have been working, so going for master only.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks!

          Show
          Dan Poltawski added a comment - Integrated, thanks!
          Hide
          Andrea Bicciolo added a comment -

          Dan, Rajesh,

          we found this bug as we are developing a module for 2.2 branch using repeated mform elements. Integrating in master only will be a problem for us. Can you please check if a backport can be re considered ?

          Show
          Andrea Bicciolo added a comment - Dan, Rajesh, we found this bug as we are developing a module for 2.2 branch using repeated mform elements. Integrating in master only will be a problem for us. Can you please check if a backport can be re considered ?
          Hide
          Rajesh Taneja added a comment -

          If it's required, then surely it can be backported to 22 as well.
          Will request Dan to reconsider it

          Thanks Andrea.

          Show
          Rajesh Taneja added a comment - If it's required, then surely it can be backported to 22 as well. Will request Dan to reconsider it Thanks Andrea.
          Hide
          Dan Poltawski added a comment - - edited

          OK, i've pulled this into the stable branches now. Thanks Andrea

          Show
          Dan Poltawski added a comment - - edited OK, i've pulled this into the stable branches now. Thanks Andrea
          Hide
          Andrea Bicciolo added a comment -

          Dan, Rajesh,

          thank you very much for reconsidering and pulling in stable branches.

          Show
          Andrea Bicciolo added a comment - Dan, Rajesh, thank you very much for reconsidering and pulling in stable branches.
          Hide
          Ankit Agarwal added a comment -

          works as expected
          Thanks

          Show
          Ankit Agarwal added a comment - works as expected Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome?

          Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome? Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U Closing, ciao

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: