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:
    • Rank:
      32689

      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.

        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: