Problem/Motivation

I am attempting to restore from an upload a file which is 2.6M on disk compressed. After upload is sucsessfull, Backup and Migrate thows and exception. 

  /**
   * Ensure, that the restore file does not exceed the server's upload_limit.
   *
   * @param BackupFileReadableInterface $file
   *
   * @return BackupFileReadableInterface
   */
  public function beforeRestore(BackupFileReadableInterface $file) {
    if ($file->getMeta('filesize') > file_upload_max_size()) {
      throw new BackupMigrateException('The input file exceeds the servers upload_max_filesize or post_max_size limit.', ['!id' => $file->getMeta('id')]);
    }

    return $file;
  }

The file size after decompression is 36M, but the file that gets sent to POST is 2.6M this exception couldn't be triggered if the file was beyond the POST size as set by php. My POST limit is set to 35M, if I attempt to upload a file that is too large nothing happens at all to tell me that it failed. This code run after the decompression happens and thus the $file->getMeta('filesize') call happens too late to be useful.

At first I thought that I must be missing something about how this module works, but I commented out this code and everything works fine.

Proposed resolution

Remove this check or relocate it to pre-upload so it can be useful.

Remaining tasks

Write tests, change code, etc

User interface changes

none

API changes

none

Data model changes

none

CommentFileSizeAuthor
#9 backup_migrate-n2998626-9.patch988 bytesDamienMcKenna
#2 unneeded-filesize-logic-2998626-3.patch1.14 KBfrob
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

frob created an issue. See original summary.

frob’s picture

frob
Primary language English
CreditAttribution: frob at Clarity Innovations, Inc. commented
FileSize
unneeded-filesize-logic-2998626-3.patch1.14 KB

Here is a patch that removes the logic but leaves the method.

docsaj’s picture

docsaj CreditAttribution: docsaj commented

Where and how to apply this patch. I am total noob in custom modules and changing / patching existing modules. Please help. Thanks

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Active » Needs review

@frob: Thanks for the patch! Please try to remember to set the issue status to "needs review" when you upload a patch, that'll trigger the testbot to review it.

@docsaj: Please check the documentation: https://www.drupal.org/patch/apply

frob’s picture

frob
Primary language English
CreditAttribution: frob at Clarity Innovations, Inc. commented

@DamienMcKenna Oh wow, can't believe I forgot to do that.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

No worries :)

JonMcL’s picture

JonMcL CreditAttribution: JonMcL commented
Status: Needs review » Reviewed & tested by the community

This patch looks good to me. I think it also addresses: #2971107: Unable to restore my database backup, #2937502: Is Backup and Migrate ignoring upload_max_filesize and post_max_size?, #3035262: "The input file exceeds", and #3029855: The input file exceeds the servers max file size

What I discovered from some (admittedly brief) testing is this BackupMigrate\Drupal\Filter\DrualUtils::beforeRestore, which currently checks if the file's filesize is greater than file_upload_max_size, is not actually called before the file is uploaded, but instead is called after a file is unzipped and before it is used for a restore (see BackupMigrate\Core\Main\BackupMigrate::restore). The file may not be getting uploaded at all, and might be available in a local or remote filesystem. I think this is exactly opposite of when the check should be made. The unzipping happens in the temporary directory, and when it is unzipped it may very likely be larger than file_uload_max_size, but it does not need to be uploaded at that point.

In my case, a 16MB gzip became a 110MB mysql document. The server has a 100MB upload limit, so 16MB was uploaded fine, but then the check is run in the unzipped file and the error is thrown incorrectly.

So my vote is to implement this patch and essentially remove this check completely.

As it happens, I ran some quick tests by trying to upload the 110MB mysql document into a server with a 100MB limit. Unfortunately, nothing happens at all -- no error is displayed and the Restore form just refreshes after part of the file is uloaded. So I think some work needs to be done on the Restore upload form to check if user is attempting to upload/post a file that is too large.

JonMcL’s picture

JonMcL CreditAttribution: JonMcL commented
Status: Reviewed & tested by the community » Needs review

I probably shouldn’t change the status. :) Changing back.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Parent issue: » #3009642: Plan for Backup and Migrate 8.x-4.1
FileSize
backup_migrate-n2998626-9.patch988 bytes

Thanks for the debugging, JonMcL.

This updates the comment a little.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Title: Bad logic halts functionality if the the uncompressed file is too large » Bad logic halts functionality if the uncompressed file is too large
Status: Needs review » Fixed

Committed. Thank you.

  • DamienMcKenna committed f57a229 on 8.x-4.x authored by frob 
    Issue #2998626 by frob, DamienMcKenna, JonMcL: Bad logic halts...

  • DamienMcKenna committed 1f85abb on 8.x-4.x 
    Revert "Issue #2998626 by frob, DamienMcKenna, JonMcL: Bad logic halts...
  • DamienMcKenna committed d8149c5 on 8.x-4.x authored by frob
    Issue #2998626 by frob, DamienMcKenna, JonMcL: Bad logic halts...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.