dpd-fileupload icon indicating copy to clipboard operation
dpd-fileupload copied to clipboard

New refactoring should be backward compatible

Open NicolasRitouet opened this issue 9 years ago • 4 comments

@EvilDrW sent us a nice PR to refactor dpd-fileupload and inherit from collection (https://github.com/NicolasRitouet/dpd-fileupload/pull/47). This introduced some backward compatibility. We should collect in this ticket the backward-compatibility issues we have with the existing instances of dpd-fileupload and the new version of dpd-fileupload. I know that @rgolea has already found some issues.

In order to test, you can modify your package.json with this: https://github.com/NicolasRitouet/dpd-fileupload-demo/blob/dpd-fileupload-next/package.json#L17 After this step, a npm install will get the latest version of dpd-fileupload and you'll be able to see what kind of issue you have.

As soon as I have the time, I'll refactor dpd-fileupload-demo to use the latest version and we'll have a good base to test.

Ideally, each issue we have should be fixed and have a corresponding test to cover it.

NicolasRitouet avatar Jan 19 '16 12:01 NicolasRitouet

+1 i like the new approach of inerhiting from collection.

the only drawback on my site is that i require to store files in public/user_uploads/... - where as my resource is called fileupload - with the newest version. there is no way to configure the upload base-path.

another problem is that we have patched dpd-fileupload to be able to have an event right after the file has been saved to disk, so we can process the images - where we need the fullPath.

var gm=require("gm");
gm(fullPath).autoOrient().write(fullPath, function (err) {
  if (!err) console.log('done');
  console.log(err);
});
console.log(fullPath);

edit: i found out that if i change the fullDirectory in the config.json, it seems to work, any chances we can modify that from the dashboard? - is it safe to manually edit the value?

edit2: having the fixed path stored in config.json, breaks the portability of DPD - because if another developer does not have the same folder structure, it will fail to start.

i have a branch with fixed user_uploads - and fullDirectory getting live-loaded so it stays transportable. https://github.com/hjanuschka/dpd-fileupload/tree/newupload

hjanuschka avatar Feb 17 '16 22:02 hjanuschka

@NicolasRitouet ok please forget the previous post.

i came up with this POST handler. does what i required - before the new version - the only patch i did was, not storing fullDirectory to the config.json.

and setting the _fileupload fixed to user_uploads

var gm=require("gm");
var BASE_PATH = dpd.fileupload.getResource().options.config.fullDirectory;
var fullPath = BASE_PATH + "/" + this.subdir + "/" + this.filename;

this.fullPath = fullPath;

//Read the mime type
const readChunk = require('read-chunk'); // npm install read-chunk
const fileType = require('file-type');
const buffer = readChunk.sync(fullPath, 0, 262);
const mimeType = fileType(buffer);
if(!mimeType.mime.match(/^video|image/)) {
    cancel("UPLOAD ERROR", 503);
    return;
}

if(mimeType.mime.match(/^video/)) {
    return;
}
gm(fullPath).autoOrient().write(fullPath, function (err) {
  if (!err) console.log('done');
  console.log(err);

});

hjanuschka avatar Feb 18 '16 07:02 hjanuschka

As I can tell, the uploaderfileupload had the following issues:

  • the folder had changed the name and this didn't allow me to upload in the same folder once I've updated.
  • the events should have: get, post, put, delete, validate, beforerequest, aftercommit. The put event should allow you to validate the stuff inside the put event.
  • furthermore, the config.json file modifies the current config.json file. Every single time the node instance get restarted, the config.json file gets replaced. This doesn't allow you to set up the config.json file manually. Since it allways gets replaced, there is no way to know if the folder gets changed.

Has anyone found any more issues?

rgolea avatar Feb 18 '16 18:02 rgolea

@rgolea thx for this investigation! :) Any idea how to solve the config.json issue (since it seems to be the most important one)?

NicolasRitouet avatar Apr 05 '16 09:04 NicolasRitouet