Update the download all files location #261
Conversation
Also, added a fixme comment where there is a logic error that needs to be fixed.
ddavisgraphics
left a comment
ddavisgraphics
left a comment
There was a problem hiding this comment.
I've left commentary. I don't want to merge code in with Fix Me or Todos in them.
| } | ||
|
|
||
| //determine the field name | ||
| // FIXME: this will only grab the LAST file field, if a form has Multiple |
There was a problem hiding this comment.
This appears incomplete, if there is a FIXME in the comments your acknowledging that the code doesn't work properly. These are the things I want to avoid merging into the codebase.
There was a problem hiding this comment.
there is absolutely nothing wrong with fixme and todos in code. They are reminders and notes. This code does not introduce a new issue. it is noting an existing issue that was discovered while reviewing code.
that existing issue is in no way related to the current problem. It should be addressed at a later date.
| header("Content-Description: File Transfer"); | ||
| header("Content-type: application/zip"); | ||
| header("Content-Disposition: attachment; filename=\"".$object['idno'].".".$type."\""); | ||
| header(sprintf('Content-Disposition: attachment; filename="%s.%s"',$object['idno'],$type)); |
There was a problem hiding this comment.
The previous version included slashes for the directory, is this not necessary?
There was a problem hiding this comment.
the previous version included slashed escaping the quotes
There was a problem hiding this comment.
Duh... I see that now. Sorry, I was thinking filesystem while looking at it.
| $destinationFile = sys_get_temp_dir()."/".time().".".$type; | ||
| $destinationFile = sprintf("%s/%s.%s",mfcs::config('mfcsDownloadAllDir'),time(),$type); | ||
|
|
||
| if ($type == "zip") { |
There was a problem hiding this comment.
I'm curious, would it be better to do something similar to the export scripts where you use symbolic linking, follow the links, and then zip the file following the symbolic links.
There was a problem hiding this comment.
That might make it more performant (likely would), but that should be addressed in a separate pull request.
Doing that would not address the problem that the temporary directory isn't configurable, which this does.
On large downloads, the /tmp directory was filling up and causing the download to be 0 bytes. This addresses that issue.
This is to address the issue in ticket: https://systems.lib.wvu.edu/helpdesk/viewTicket/?id=14773
THis has been tested on MFCS dev with record P31 and P3001. P31 worked previously and continues to work with this update. P3001 was too large to download previously, and now works correctly.