Moodle
  1. Moodle
  2. MDL-30335

repeat_elements does not work with groups that have $appendName = false

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide

      Save the attached testrepeatgroups.php in the top level of you moodle install, then go there (http://path.to.your/moodle/testrepeatgroups.php) and make sure the form works. That is:

      1. Inspect the HTML and look at what the name="" attributes are.

      2. Type in some data, and submit the form and look at the submitted data. (The test script just does print_object on the submitted data).

      3. (optional) edit the test script to try with other field types, etc. (But not that I have not attempted to fix MDL-20441 at this time.)

      Show
      Save the attached testrepeatgroups.php in the top level of you moodle install, then go there ( http://path.to.your/moodle/testrepeatgroups.php ) and make sure the form works. That is: 1. Inspect the HTML and look at what the name="" attributes are. 2. Type in some data, and submit the form and look at the submitted data. (The test script just does print_object on the submitted data). 3. (optional) edit the test script to try with other field types, etc. (But not that I have not attempted to fix MDL-20441 at this time.)
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      When you add a group to a form, you can choose whether the field names get messed around with. This is the last argument to addGroup. That is, if you do

              $mform->addGroup(array(
                  $mform->createElement('text', 'a', 'A = '),
                  $mform->createElement('text', 'b', 'B = '),
              ), 'groupname', 'Fields', array(' '), false);

      The two fields end up with names a and b, but if you do

              $mform->addGroup(array(
                  $mform->createElement('text', 'a', 'A = '),
                  $mform->createElement('text', 'b', 'B = '),
              ), 'groupname', 'Fields', array(' '), true);

      The two fields end up with names groupname[a] and groupname[b].

      The problem comes when you try to use this in combination with repeat_elements. With $appendName = true, there is no problem. The field names become groupname[0][a], groupname[1][a], etc. The issue is with $appendName = false. Then, with the existing code, the field names are just a, b for each repeat, which means that the fields just overwrite each other.

      I am about to submit a patch so that with repeat_elements and $appendName = false, the field names are a[0], a[1]. b[0], b[1], etc.

        Gliffy Diagrams

          Activity

          Hide
          Tim Hunt added a comment -

          This seems to work.

          Show
          Tim Hunt added a comment - This seems to work.
          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
          Colin Chambers added a comment -

          Works fine for me.

          Only comment is minor and related to performance.
          Should the result of $elementclone->getElements() be stored before

          foreach ($elementclone->getElements() as $el) {

          I forget if this saves a little processing. Not really an issue though.

          Tested using supplied test file and on moodle install. elements names in html are correct. Able to save questions.

          Show
          Colin Chambers added a comment - Works fine for me. Only comment is minor and related to performance. Should the result of $elementclone->getElements() be stored before foreach ($elementclone->getElements() as $el) { I forget if this saves a little processing. Not really an issue though. Tested using supplied test file and on moodle install. elements names in html are correct. Able to save questions.
          Hide
          Tim Hunt added a comment -

          Thanks for the review, Colin. I'm submitting for integration now.

          foreach (expensive_functions() as $thing)

          is fine, just like

          for ($i = expensive_functions(); $i < $max; $i += 1)

          would be. expensive_functions() will only be called once. The mistake is

          for ($i = 0; $i < expensive_functions(); $i += 1)

          where expensive_functions() will be called for each iteration of the loop.

          Show
          Tim Hunt added a comment - Thanks for the review, Colin. I'm submitting for integration now. foreach (expensive_functions() as $thing) is fine, just like for ($i = expensive_functions(); $i < $max; $i += 1) would be. expensive_functions() will only be called once. The mistake is for ($i = 0; $i < expensive_functions(); $i += 1) where expensive_functions() will be called for each iteration of the loop.
          Hide
          Aparup Banerjee added a comment -

          Thanks this has been integrated into master. Ready for testing!

          (I've added the last param to the phpdoc too)

          Show
          Aparup Banerjee added a comment - Thanks this has been integrated into master. Ready for testing! (I've added the last param to the phpdoc too)
          Hide
          Rajesh Taneja added a comment -

          Works Great
          Thanks for fixing this Tim

          Show
          Rajesh Taneja added a comment - Works Great Thanks for fixing this Tim
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has landed upstream, just on time for the upcoming new releases week. Thanks for it!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has landed upstream, just on time for the upcoming new releases week. Thanks for it! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: