Moodle
  1. Moodle
  2. MDL-41451

Large forms are truncated by max_input_vars

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.6, 2.5.2, 2.6
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide

      BEFORE TESTING:

      0a. If you don't already have one, create a SIZE_S and a SIZE_L course using the admin tool from MDL-38197.
      For 24 and 25 - you can use the attached backup file and adjust the test instructions accordingly (wing it!).

      The SIZE_L course has 1,000 activities, which is enough to demonstrate this problem. The SIZE_S course fits comfortably within the default max_input_vars limit.

      TESTING:

      1. Go to the SIZE_L course and choose Backup.
      2. Click Next.
      3. The next screen contains over 2,000 checkboxes. Scroll down to the end of the page (around topic 500).
      4. Make a note of the last three page numbers as A, B, and C. For example, A = Page 492, B = Page 9, C = Page 606.
      5. For page B, turn off user data (right checkbox).
      6. For page A, turn off the entire activity (left checkbox).
      7. Click Next.
      7b. Scroll down to the bottom of the review screen.

      EXPECTED: All activities and topics should show a green tickbox against the content and for user data, except for page B which should show an X next to user data, and page A which should show an X next to both the activity and user data.

      (At this point we do NOT continue with the backup; it is expected that the backup may NOT complete unless MDL-38190 and MDL-41146 have been applied. Test step numbers 8 to 11 are not used. We will do a full backup/restore of the smaller course instead, which follows.)

      12. Go to the SIZE_S course and choose Backup.
      13. Click Next, then Next again.
      14. Continue through the backup process until it completes.
      15. Restore the backup as a new course and continue through process.
      16. Verify that course restore completes without error.

      Show
      BEFORE TESTING: 0a. If you don't already have one, create a SIZE_S and a SIZE_L course using the admin tool from MDL-38197 . For 24 and 25 - you can use the attached backup file and adjust the test instructions accordingly (wing it!). The SIZE_L course has 1,000 activities, which is enough to demonstrate this problem. The SIZE_S course fits comfortably within the default max_input_vars limit. TESTING: 1. Go to the SIZE_L course and choose Backup. 2. Click Next. 3. The next screen contains over 2,000 checkboxes. Scroll down to the end of the page (around topic 500). 4. Make a note of the last three page numbers as A, B, and C. For example, A = Page 492, B = Page 9, C = Page 606. 5. For page B, turn off user data (right checkbox). 6. For page A, turn off the entire activity (left checkbox). 7. Click Next. 7b. Scroll down to the bottom of the review screen. EXPECTED: All activities and topics should show a green tickbox against the content and for user data, except for page B which should show an X next to user data, and page A which should show an X next to both the activity and user data. (At this point we do NOT continue with the backup; it is expected that the backup may NOT complete unless MDL-38190 and MDL-41146 have been applied. Test step numbers 8 to 11 are not used. We will do a full backup/restore of the smaller course instead, which follows.) 12. Go to the SIZE_S course and choose Backup. 13. Click Next, then Next again. 14. Continue through the backup process until it completes. 15. Restore the backup as a new course and continue through process. 16. Verify that course restore completes without error.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull 2.5 Branch:
    • Pull Master Branch:
      MDL-41451_master
    • Rank:
      52478

      Description

      Large forms (Grader report, backup/restore, quizzes, etc.) are truncated by PHP 5.3.9 and above. Some forms have been adjusted to work around the limit in PHP 5.3.9, as it was applied only to each nesting level of an array - but PHP 5.4 applies max_input_vars as a hard limit on the number of fields it will process.

      The limit can be bypassed by parsing the POST body (php://input) in chunks using parse_str() (chunks necessitated by parse_str's adherence to max_input_vars). This does not work for forms submitted using the "multipart/form-data" enctype, but the main culprits don't use it anyway as uploads should all be done using file manager or file picker fields rather than done inline with regular HTML file input fields.

        Issue Links

          Activity

          Hide
          Paul Nicholls added a comment -

          Added testing instructions, taken from MDL-34491 with some changes.

          Show
          Paul Nicholls added a comment - Added testing instructions, taken from MDL-34491 with some changes.
          Hide
          Paul Nicholls added a comment -

          Cleanly picks onto MOODLE_25_STABLE and MOODLE_24_STABLE - branches provided.

          Show
          Paul Nicholls added a comment - Cleanly picks onto MOODLE_25_STABLE and MOODLE_24_STABLE - branches provided.
          Hide
          Paul Nicholls added a comment -

          Added a few watchers - sorry if any of you aren't interested

          Show
          Paul Nicholls added a comment - Added a few watchers - sorry if any of you aren't interested
          Hide
          Rajesh Taneja added a comment -

          Thanks for reporting this and fixing this Paul,

          Patch looks good, but not sure if this is the correct way to go for it.
          Alternative is to suggest site owner/admin to set max_input_vars appropriately, but seems we should handle this in case we can.

          Few questions:

          1. Should this be handled for $_GET as well, probably not but not sure.
          2. As 'php://input' resource may only be read once, can this break anything? I can see use of 'php://input' in moodle, but seems nothing will break currently. Although if you have a repository like poodll, it might break as it gets recording from 'php://input'
          3. Will suggest you to set $HTTP_RAW_POST_DATA = file_get_contents( 'php://input' ) in case it is needed by any other code.

          Have added Petr as watcher, to get his inputs.

          Show
          Rajesh Taneja added a comment - Thanks for reporting this and fixing this Paul, Patch looks good, but not sure if this is the correct way to go for it. Alternative is to suggest site owner/admin to set max_input_vars appropriately, but seems we should handle this in case we can. Few questions: Should this be handled for $_GET as well, probably not but not sure. As 'php://input' resource may only be read once, can this break anything? I can see use of 'php://input' in moodle, but seems nothing will break currently. Although if you have a repository like poodll, it might break as it gets recording from 'php://input' Will suggest you to set $HTTP_RAW_POST_DATA = file_get_contents( 'php://input' ) in case it is needed by any other code. Have added Petr as watcher, to get his inputs.
          Hide
          Sam Marshall added a comment -

          Looks good!

          Regarding Rajesh's point 2, if I read this code correctly it looks like this new code is only used for cases when the max input vars is exceeded - in other words it will only be used for a very small number of cases such as the backup form. So the risk of it breaking anything is quite small.

          Two trivia points:

          1) you have an incorrect variable name (with _ symbol in the name). Needs to be $allvalues not $all_values.

          2) if that's a class function it should probably have an access level (e.g. protected)

          Show
          Sam Marshall added a comment - Looks good! Regarding Rajesh's point 2, if I read this code correctly it looks like this new code is only used for cases when the max input vars is exceeded - in other words it will only be used for a very small number of cases such as the backup form. So the risk of it breaking anything is quite small. Two trivia points: 1) you have an incorrect variable name (with _ symbol in the name). Needs to be $allvalues not $all_values. 2) if that's a class function it should probably have an access level (e.g. protected)
          Hide
          Paul Nicholls added a comment -

          I'm just preparing an updated set of branches to address a few issues:
          1) Use array_merge_recursive instead of array_merge to prevent loss of some array elements if the array spanned multiple chunks
          2) Count elements in $_POST recursively (use COUNT_RECURSIVE flag) to account for arrays of elements
          2) Removed underscore from variable name
          3) Added "protected" access level

          As for Raj's questions:
          1) No, I don't think it should be handled for $_GET. If you've got thousands of query parameters in your URL, you're doing something wrong... and some browsers, proxies and/or servers may impose limits on the length of URLs which will irreversibly truncate it before we could do anything about it.
          2) It's only being read if the form is a moodleform, has more than max_input_vars fields and is not a multipart/form-data submission. I think it's sufficiently unlikely that anybody else would need to read from php://input in these cases that we can probably cross that bridge should we come to it (probably by modifying whatever the other thing is that's trying to read from php://input).
          3) Looking at Poodll, not only does it not have enough form fields to trigger this, but I think that setting $HTTP_RAW_POST_DATA would actually break it - it's actually only reading from php://input if $HTTP_RAW_POST_DATA is set, so we could potentially cause it to do the wrong thing by setting $HTTP_RAW_POST_DATA. I guess that's technically a flaw in Poodll - if anybody has always_populate_raw_post_data turned on in their PHP config, Poodll will assume that it's receiving raw file data and dump it to a temp file instead of processing it as a regular form submission. Even so, I'm hesitant to set $HTTP_RAW_POST_DATA, as I'm not sure that it's quite the right thing to do. If you really want to make sure that the raw data is available if anyone else needs it, perhaps we should unconditionally read it into the moodleform instance and expose a public function to allow access to it?

          Show
          Paul Nicholls added a comment - I'm just preparing an updated set of branches to address a few issues: 1) Use array_merge_recursive instead of array_merge to prevent loss of some array elements if the array spanned multiple chunks 2) Count elements in $_POST recursively (use COUNT_RECURSIVE flag) to account for arrays of elements 2) Removed underscore from variable name 3) Added "protected" access level As for Raj's questions: 1) No, I don't think it should be handled for $_GET. If you've got thousands of query parameters in your URL, you're doing something wrong... and some browsers, proxies and/or servers may impose limits on the length of URLs which will irreversibly truncate it before we could do anything about it. 2) It's only being read if the form is a moodleform, has more than max_input_vars fields and is not a multipart/form-data submission. I think it's sufficiently unlikely that anybody else would need to read from php://input in these cases that we can probably cross that bridge should we come to it (probably by modifying whatever the other thing is that's trying to read from php://input). 3) Looking at Poodll, not only does it not have enough form fields to trigger this, but I think that setting $HTTP_RAW_POST_DATA would actually break it - it's actually only reading from php://input if $HTTP_RAW_POST_DATA is set, so we could potentially cause it to do the wrong thing by setting $HTTP_RAW_POST_DATA. I guess that's technically a flaw in Poodll - if anybody has always_populate_raw_post_data turned on in their PHP config, Poodll will assume that it's receiving raw file data and dump it to a temp file instead of processing it as a regular form submission. Even so, I'm hesitant to set $HTTP_RAW_POST_DATA, as I'm not sure that it's quite the right thing to do. If you really want to make sure that the raw data is available if anyone else needs it, perhaps we should unconditionally read it into the moodleform instance and expose a public function to allow access to it?
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam and Paul for answering my queries.

          I agree risk of breaking anything is quite low.
          As per point 3: Poodll was just an example, I haven't gone though each usage of php://input. I only suggested if you read anything from php://input then should you set it to global? In case some other code wants to use it, then any code reading for this stream should set the global for consistency. Currently, every code just read's it, use data and forgets about it.

          Leaving that decision to you, as chances of breaking things are low, but Moodle being opensource you might be surprised to see different use-case.

          In addition to above points, can you please update comment at #L0R267 to comply with coding guidelines, I know this is old comment but as you are doing changes on this line, it might be nice to fix it as well.

          Thanks again Paul, for all the hard work on this.
          Great effort

          Show
          Rajesh Taneja added a comment - Thanks Sam and Paul for answering my queries. I agree risk of breaking anything is quite low. As per point 3: Poodll was just an example, I haven't gone though each usage of php://input. I only suggested if you read anything from php://input then should you set it to global? In case some other code wants to use it, then any code reading for this stream should set the global for consistency. Currently, every code just read's it, use data and forgets about it. Leaving that decision to you, as chances of breaking things are low, but Moodle being opensource you might be surprised to see different use-case. In addition to above points, can you please update comment at #L0R267 to comply with coding guidelines, I know this is old comment but as you are doing changes on this line, it might be nice to fix it as well. Thanks again Paul, for all the hard work on this. Great effort
          Hide
          Paul Nicholls added a comment -

          Hi Raj,
          I've just finished pushing updated branches (rebased + amended commit), including the comment coding guidelines change.

          Although I can't think of a case where this would be triggered and there'd be another bit of code wanting to read php://input in the same request, it may not be a problem for most people anyway - although php://input doesn't support seeking (meaning that you can't re-read from the same file handle), you can simply open another handle and read it again from the start, as long as the SAPI supports it. The apache2handler SAPI supports opening php://input multiple times (at least on my dev box - PHP 5.3.5 / Apache 2.2 / Ubuntu). In configurations where the SAPI doesn't support this, always_populate_raw_post_data can be enabled (either in php.ini or a .htaccess file) to populate the global without us needing to do it ourselves.

          -Paul

          Show
          Paul Nicholls added a comment - Hi Raj, I've just finished pushing updated branches (rebased + amended commit), including the comment coding guidelines change. Although I can't think of a case where this would be triggered and there'd be another bit of code wanting to read php://input in the same request, it may not be a problem for most people anyway - although php://input doesn't support seeking (meaning that you can't re-read from the same file handle), you can simply open another handle and read it again from the start, as long as the SAPI supports it. The apache2handler SAPI supports opening php://input multiple times (at least on my dev box - PHP 5.3.5 / Apache 2.2 / Ubuntu). In configurations where the SAPI doesn't support this, always_populate_raw_post_data can be enabled (either in php.ini or a .htaccess file) to populate the global without us needing to do it ourselves. -Paul
          Hide
          Rajesh Taneja added a comment -

          Sounds Good Paul,

          Pushing for integration review.

          Show
          Rajesh Taneja added a comment - Sounds Good Paul, Pushing for integration review.
          Hide
          Marina Glancy added a comment -

          Awesome Paul, thanks a lot for working on it. Just one comment - we don't allow inline functions in moodle because they don't always work. See MDL-39432 and linked issues. Can you please change it to create_function() or maybe avoid it completely there.

          Similar issues:
          https://github.com/moodle/moodle/commit/91a8b40427d1556ec5028016deb23ccd9c3d53a6
          https://github.com/moodle/moodle/commit/d81a603ee2c914d7ac3c05f8c34a4b9306c613a1

          Show
          Marina Glancy added a comment - Awesome Paul, thanks a lot for working on it. Just one comment - we don't allow inline functions in moodle because they don't always work. See MDL-39432 and linked issues. Can you please change it to create_function() or maybe avoid it completely there. Similar issues: https://github.com/moodle/moodle/commit/91a8b40427d1556ec5028016deb23ccd9c3d53a6 https://github.com/moodle/moodle/commit/d81a603ee2c914d7ac3c05f8c34a4b9306c613a1
          Hide
          Paul Nicholls added a comment -

          Sam Hemelryk, I've just tweaked it to use create_function() as per Marina's comment - should I push a second commit or amend the existing one to make the change, given that you've just put it into integration?

          Show
          Paul Nicholls added a comment - Sam Hemelryk , I've just tweaked it to use create_function() as per Marina's comment - should I push a second commit or amend the existing one to make the change, given that you've just put it into integration?
          Hide
          Marina Glancy added a comment -

          Hi Paul, thanks. It is not yet in the integration review status, so you can do either.

          Show
          Marina Glancy added a comment - Hi Paul, thanks. It is not yet in the integration review status, so you can do either.
          Hide
          Paul Nicholls added a comment -

          Hi Marina and Sam,
          I've just pushed a second commit to all three branches - I think it's a little cleaner, and it's quicker for me to do.

          -Paul

          Show
          Paul Nicholls added a comment - Hi Marina and Sam, I've just pushed a second commit to all three branches - I think it's a little cleaner, and it's quicker for me to do. -Paul
          Hide
          Damyon Wiese added a comment -

          Thanks,

          My general comment is that this is a workaround - and at some point php will probably change something else to prevent this. There are only 2 real solutions to this problem:

          1. increase max input vars (not possible for e.g. shared hosting)
          2. break up large forms to a reasonable size

          That being my 2c, this patch does not seem to break anything and is a quicker fix to help some people who cannot help themselves.

          Show
          Damyon Wiese added a comment - Thanks, My general comment is that this is a workaround - and at some point php will probably change something else to prevent this. There are only 2 real solutions to this problem: 1. increase max input vars (not possible for e.g. shared hosting) 2. break up large forms to a reasonable size That being my 2c, this patch does not seem to break anything and is a quicker fix to help some people who cannot help themselves.
          Hide
          Damyon Wiese added a comment -

          Thanks Paul,

          Integrated to 24, 25 and master. I tweaked the testing instructions to allow testing on 24 and 25 without the new course generator.

          Show
          Damyon Wiese added a comment - Thanks Paul, Integrated to 24, 25 and master. I tweaked the testing instructions to allow testing on 24 and 25 without the new course generator.
          Hide
          Paul Nicholls added a comment -

          Thanks Damyon. My impression is that although this is a workaround, it probably should keep working for the foreseeable future - the recent php change which broke the array workaround was fixing a hole in the initial implementation of max_input_vars. I haven't researched the history of the issue which prompted the addition of max_input_vars, but I suspect that as this workaround requires the developer to wilfully work around max_input_vars, it should be incredibly unlikely that it'll be implemented except in cases where a form legitimately contains an awful lot of fields.

          That said, it'd certainly be good to simplify huge forms in Moodle wherever possible; the backup and restore flows would certainly benefit hugely from a well-designed revamp, as they quickly reach absurd length. The grader report might be harder to deal with, but I'm sure somebody suitably skilled in UI/UX design could come up with a better interface which reduces the form length. The biggest issue with radical interface changes is accessibility and non-JS compatibility...

          Show
          Paul Nicholls added a comment - Thanks Damyon. My impression is that although this is a workaround, it probably should keep working for the foreseeable future - the recent php change which broke the array workaround was fixing a hole in the initial implementation of max_input_vars. I haven't researched the history of the issue which prompted the addition of max_input_vars, but I suspect that as this workaround requires the developer to wilfully work around max_input_vars, it should be incredibly unlikely that it'll be implemented except in cases where a form legitimately contains an awful lot of fields. That said, it'd certainly be good to simplify huge forms in Moodle wherever possible; the backup and restore flows would certainly benefit hugely from a well-designed revamp, as they quickly reach absurd length. The grader report might be harder to deal with, but I'm sure somebody suitably skilled in UI/UX design could come up with a better interface which reduces the form length. The biggest issue with radical interface changes is accessibility and non-JS compatibility...
          Hide
          Frédéric Massart added a comment -

          Passing. Thanks!

          For 2.4 and 2.5, I had to proceed a bit differently than explained as I couldn't restore the attached backup (page was loading forever). I set max_input_vars to 50, and backed up/restored Moodle Features Demo course. Then I set it back to 1000 and did the same.

          I ensured that the input limit was effective by checking my error log as saw many:

          [Wed Sep 11 14:52:13 2013] [error] [client 192.168.100.54] PHP Warning:  Unknown: Input variables exceeded 50. To increase the limit change max_input_vars in php.ini. in Unknown on line 0, referer: http://fred.moodle.local/i24/backup/restore.php
          
          Show
          Frédéric Massart added a comment - Passing. Thanks! For 2.4 and 2.5, I had to proceed a bit differently than explained as I couldn't restore the attached backup (page was loading forever). I set max_input_vars to 50, and backed up/restored Moodle Features Demo course. Then I set it back to 1000 and did the same. I ensured that the input limit was effective by checking my error log as saw many: [Wed Sep 11 14:52:13 2013] [error] [client 192.168.100.54] PHP Warning: Unknown: Input variables exceeded 50. To increase the limit change max_input_vars in php.ini. in Unknown on line 0, referer: http://fred.moodle.local/i24/backup/restore.php
          Hide
          Damyon Wiese added a comment -

          This issue along with 77 of it's friends has been sent upstream and released to the world.

          Thankyou for your contribution.

          Show
          Damyon Wiese added a comment - This issue along with 77 of it's friends has been sent upstream and released to the world. Thankyou for your contribution.

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: