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

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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
          timhunt Tim Hunt added a comment -

          This seems to work.

          Show
          timhunt Tim Hunt added a comment - This seems to work.
          Hide
          salvetore Michael de Raadt added a comment -

          Thanks for working on this.

          Show
          salvetore Michael de Raadt added a comment - Thanks for working on this.
          Hide
          colchambers 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
          colchambers 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
          timhunt 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
          timhunt 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
          nebgor 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
          nebgor 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
          rajeshtaneja Rajesh Taneja added a comment -

          Works Great
          Thanks for fixing this Tim

          Show
          rajeshtaneja Rajesh Taneja added a comment - Works Great Thanks for fixing this Tim
          Hide
          stronk7 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
          stronk7 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:
                Fix Release Date:
                5/Dec/11