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

          Attachments

            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