pkp/pkp-lib#1709 provide export XML for download #1876
Conversation
|
@asmecher, also here, I did the change, to use TemporaryFileManager. Could you please take a look (the last commit)? THANKS!!! |
| * @param $filePath string the location of the file to be deleted | ||
| * @return boolean returns true if successful | ||
| */ | ||
| function deleteFileByPath($filePath) { |
There was a problem hiding this comment.
Why not let the TemporaryFileManager persist the file in the database? Then you could get rid of deleteFileByPath, downloadFileByPath, and even ImportExportPlugin::getExportFileName, and let the TemporaryFileManager handle more of the details.
There was a problem hiding this comment.
Yes, I wasn't sure -- it seemed to me to be to much 'action' to write and read from the database and act like this would be like a file upload (considering those information as at a file upload), because this case is a little bit different -- those exported files are not like file uploads, that are usually treated by this manager -- they just write a content to a file. Do you think it would be better to do so?
There was a problem hiding this comment.
Maybe there will be also other cases, that are not file upload and could use this kind of functions...
There was a problem hiding this comment.
I agree that persisting to the database isn't useful in this case, but it does permit the TemporaryFileManager to take care of managing the file storage, cleanup, etc. The less we re-implement those concerns the better, I think.
For one-shot cases like this there might be a role for a simpler file manager class, but the extra overhead from persisting to the database is so little that I'm not sure it's worth the extra code.
There was a problem hiding this comment.
(I don't love the TemporaryFileManager in general, and I'm now directing your work without much recent experience with it -- so if you think we're going down a rabbit hole, please stop me :)
There was a problem hiding this comment.
OK, I'll try then to use the DB and TemporaryFileManager as it is... If I figure out problems I'll tell you... :-)
There was a problem hiding this comment.
Hmmm... Somehow I don't feel this is good ;-) -- I have first to create a file (from XML string) in order then to divide it apart, to fit the TemporaryFile structure, to get and save information into the DB, that I don't need at all (newFileName, fileType, fileSize, originalFileName). Thus, maybe then to:
- write a new class (ExportFileManager?) that will extend the PrivateFileManager and just add the new base path = '/temp/'. Or to
- use just a normal FileManager, giving it the manually constructed path to that temp folder? -- I actually only need that base path from the TemporaryFileManager (and to mark it as a temporary file for the code readers) :-P Everything else what I need is in the FileManager (writeFile, downloadFile, deleteFile -- that's it).
What do you think? Maybe the *6649* Updated German locale for pkp-lib: grid.xml #2?
There was a problem hiding this comment.
Hmm, OK, let's back off to option 2. Sorry to put you through all this. I would still like to avoid performing file operations outside of some kind of framework when possible, but it looks like using the TemporaryFileManager is a worse solution.
bozana
force-pushed
the
1709fix-downloadXML
branch
from
a87ffda to
c389ab6
Compare
|
@asmecher, I now changed the commit above to use just the normal FileManager... |
|
Merged, thanks! |
S. i) here: #1709
(This PR considers also other export plugins)