Media Manager upload size and file name validation #1248
Conversation
| @@ -732,7 +732,8 @@ | |||
| paramName: 'file_data', | |||
| timeout: 0, | |||
| headers: {}, | |||
| createImageThumbnails: false | |||
| createImageThumbnails: false, | |||
| maxFilesize: this.options.maxFilesize, | |||
There was a problem hiding this comment.
We should probably just populate this from an ini_get(post_max_size) instead
There was a problem hiding this comment.
I am unsure how i'd proceed with this part.
There was a problem hiding this comment.
Make sure you're using the latest version of winter/storm and add data-max-filesize="<?= e(\File::getMaxUploadSize()); ?>" to https://github.com/wintercms/winter/blob/develop/modules/backend/widgets/mediamanager/partials/_body.php#L13
| */ | ||
| public function onUpload(): ?\Illuminate\Http\Response | ||
| { | ||
| ini_set('memory_limit', Config::get('cms.storage.media.memoryLimit', ini_get('memory_limit'))); |
There was a problem hiding this comment.
Why do you have this here? Why not just put this code in the event listener instead if you really need it for your application? Why do you even need to increase the memory limit here in the first place?
There was a problem hiding this comment.
I had explained earlier when i was uploading large files I got a memory exhausted error (this was after fixing the error above). I can move it out if it works in the event listener (at the same time i was unsure if the event listener even work because everything else uses $this->fireSystemEvent that I thought that Event was unused.
There was a problem hiding this comment.
A memory exhausted error feels more like the image is being processed in some way, I don't think I've seen it happen just from a file being uploaded before. I'd need more evidence that this is actually a problem that affects most Winter users before I'd want it in the core, you should be able to implement for your own project just with the existing event listener for now though.
| @@ -171,7 +176,7 @@ public function validateMediaFileName(string $fileName, string $extension): stri | |||
| /* | |||
| * File name contains non-latin characters, attempt to slug the value | |||
| */ | |||
| if (!$this->validateFileName($fileName)) { | |||
| if (!$this->validateFileName($fileName) && $this->validateMediaFileName) { | |||
There was a problem hiding this comment.
Why would we want to allow a filename to skip validation? The validation exists for a reason.
There was a problem hiding this comment.
When I'm adding multiple files and their have name formatting that i use elsewhere in a plugin so they can be associated with a relation for upload after batch uploading (An argument can be made to use fileupload, but the batch uploading is for individual relations that use file upload not just one using multiupload). I made it opt-out, but I do need it to keep the name formatting.
There was a problem hiding this comment.
I wouldn't recommend abusing the media library system for that. I'd need to know more about your exact use case, but I could see you using an attachMany to accept multiple files and then re-assigning them to the relevant attachOne relationships post upload. Also why can't you have the files renamed when they get moved out of the medialibrary?
There was a problem hiding this comment.
I decided to create a clone of the mediamanager as a plugin/widget rather than using that media manager.
LukeTowers
added
the
needs response
This is more of a getting started guideline as I'm not sure how to proceed with the changes or if they're good enough quality.
The first issue I encountered is the fact that by default
mediamanager.jswill have a max filesize of 256MB (dropzone default) and cant be increased any further.2nd issue was actually trying to find why an error was popping up which lead to the first issue. When the file size is too big it just gives a console error. As it turns out the xhr property doesn't exist within the context so i had to make it conditional so it'd go to the actual popup.
3rd issue (or rather more of a preference) is that in some cases I'd prefer to keep the file names intact without any cleanup. So I made it so that you can optionally disable file name validation cleanup.
I'm not that familiar with dropzone, so im unsure how to integrate filesizes with winter so it works like how the fileupload widget does for now it's set to
0. There also came an issue where because it's usingpost_max_sizeto upload a file to media manager, a memory exhaustion error popped up, and rather than having to globally set the memory limit higher, it would be better to useini_settemporarily.