Moodle
  1. Moodle
  2. MDL-28099

filepicker form element does not work with element names with an index such as image[0]

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.4
    • Fix Version/s: 2.1
    • Component/s: Files API, Forms Library
    • Labels:
      None
    • Rank:
      18088

      Description

      draftitemid is not returned by form when the element name of the item has an index eg. image[0]. Draft item does not get passed through from POST data. So repeat_elements or anything else in forms including filepicker element with an index will not work.

      1. filepicker_test.php
        1 kB
        Jamie Pratt
      2. filepicker_test.php
        1 kB
        Jamie Pratt

        Activity

        Hide
        Sam Hemelryk added a comment -

        Hi Jamie,

        I'm sending this back for more work sorry.
        The code looked fine however a quick test before integration revealed a regression in the code.
        To test I created a single upload assignment, logged in as a student and attempted to upload a file.
        Before the patch it worked fine.
        With the patch applied I get the following error:

        Warning: Invalid argument supplied for foreach() in /var/git/integration/master/lib/pear/HTML/QuickForm.php on line 1263
        Call Stack

        1. Time Memory Function Location
          1 0.0004 695664
          Unknown macro: {main}

          ( ) ../upload.php:0
          2 0.2447 42907032 assignment_uploadsingle->upload( ) ../upload.php:67
          3 0.2447 42907184 assignment_uploadsingle->upload_file( ) ../assignment.class.php:166
          4 0.3524 42968032 moodleform->get_new_filename( ) ../assignment.class.php:195
          5 0.3524 42968032 MoodleQuickForm->exportValues( ) ../formslib.php:561
          6 0.3541 42969832 HTML_QuickForm->arrayMerge( ) ../formslib.php:1559

        Unfortunately I am pressed for time so I haven't investigated it further however hopefully it is not too much more work.
        When you resubmit could you please also fill in some testing instructions.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Jamie, I'm sending this back for more work sorry. The code looked fine however a quick test before integration revealed a regression in the code. To test I created a single upload assignment, logged in as a student and attempted to upload a file. Before the patch it worked fine. With the patch applied I get the following error: Warning: Invalid argument supplied for foreach() in /var/git/integration/master/lib/pear/HTML/QuickForm.php on line 1263 Call Stack Time Memory Function Location 1 0.0004 695664 Unknown macro: {main} ( ) ../upload.php:0 2 0.2447 42907032 assignment_uploadsingle->upload( ) ../upload.php:67 3 0.2447 42907184 assignment_uploadsingle->upload_file( ) ../assignment.class.php:166 4 0.3524 42968032 moodleform->get_new_filename( ) ../assignment.class.php:195 5 0.3524 42968032 MoodleQuickForm->exportValues( ) ../formslib.php:561 6 0.3541 42969832 HTML_QuickForm->arrayMerge( ) ../formslib.php:1559 Unfortunately I am pressed for time so I haven't investigated it further however hopefully it is not too much more work. When you resubmit could you please also fill in some testing instructions. Cheers Sam
        Hide
        Jamie Pratt added a comment -

        Thanks Sam, will take a look to see what the problem might be now.

        Show
        Jamie Pratt added a comment - Thanks Sam, will take a look to see what the problem might be now.
        Hide
        Jamie Pratt added a comment -

        I have found the problem that was causing the regression. The code as it stood did not respect the $assoc parameter for function MoodleQuickForm_filepicker::exportValue, it just ignores it. My new code did not ignore that parameter. What appears to be a bug inside formslib / QuickForms itself was then causing a notice and causing a bug where the draftitemid was not being passed. Now my code ignores that param too but does work with element names with indexes involved.

        Show
        Jamie Pratt added a comment - I have found the problem that was causing the regression. The code as it stood did not respect the $assoc parameter for function MoodleQuickForm_filepicker::exportValue, it just ignores it. My new code did not ignore that parameter. What appears to be a bug inside formslib / QuickForms itself was then causing a notice and causing a bug where the draftitemid was not being passed. Now my code ignores that param too but does work with element names with indexes involved.
        Hide
        Jamie Pratt added a comment -

        I attach a file that can be used to test the required and proper functionality of the filepicker element.

        The script is a basic form with two filepicker elements with and without an index in the filename.

        You can see before the patch proposed, the draftitemid of the filepicker item, with a element name including an index, is not passed as one of the values from the form. The value is passed for the element without the [0] index part of the element name. With the patch both are passed.

        With this patch moodleform::repeat_elements will work as expected with filepicker elements.

        Please put the script in the root of your Moodle directory and open the url for the script. Try to upload 2 files with and without the patch.

        Show
        Jamie Pratt added a comment - I attach a file that can be used to test the required and proper functionality of the filepicker element. The script is a basic form with two filepicker elements with and without an index in the filename. You can see before the patch proposed, the draftitemid of the filepicker item, with a element name including an index, is not passed as one of the values from the form. The value is passed for the element without the [0] index part of the element name. With the patch both are passed. With this patch moodleform::repeat_elements will work as expected with filepicker elements. Please put the script in the root of your Moodle directory and open the url for the script. Try to upload 2 files with and without the patch.
        Hide
        Jamie Pratt added a comment -

        Oops. Forget the last file. This is the one I meant to upload.

        Show
        Jamie Pratt added a comment - Oops. Forget the last file. This is the one I meant to upload.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Not knowing much about the changes performed I've used your test for to look for differences, plus have tested assignment upload, restore upload and questions import upload (and pick from recent files).

        All them worked perfectly so passing this. Thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Not knowing much about the changes performed I've used your test for to look for differences, plus have tested assignment upload, restore upload and questions import upload (and pick from recent files). All them worked perfectly so passing this. Thanks!
        Hide
        Jamie Pratt added a comment -

        Thanks a lot for your quick response Eloy!

        Show
        Jamie Pratt added a comment - Thanks a lot for your quick response Eloy!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Yay, this is now part of the just released Moodle 2.1 ! Thanks for all the hard work!

        Show
        Eloy Lafuente (stronk7) added a comment - Yay, this is now part of the just released Moodle 2.1 ! Thanks for all the hard work!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: