Laravel-Backpack / medialibrary-uploaders

MIT License
9 stars 3 forks source link

[Feature Request] File too large - should we make uploaders have a max file size by default? #14

Closed tabacitu closed 1 year ago

tabacitu commented 1 year ago

Bug report

What I did

In Books I have an image field that has ->withMedia(). I tried to upload a 16MB image to it, and after I click the Save button it throws Allowed memory size of 536870912 bytes exhausted (tried to allocate 251658272 bytes):

CleanShot 2023-05-10 at 15 35 43@2x

Error stack: https://flareapp.io/share/4m48GxZm

What I expected to happen

Well honestly... this, since I have absolutely no validation to it 😅 And the simple way to solve this is to add a max file size validation to that input, which I am sure works. So I don't necessarily think we should do anything about this per-se.

What happened

Nothing different tbh. But it made me think... Since we know this error will be thrown... and most devs will definitely forget to add max file size validation... should our Upload validation rules... add max file size validation by default? With the max value from PHP? 👀🤷‍♂️ Would that be difficult / too much work / too intrusive or presumptive of our rules - what do you think @pxpm ?

tabacitu commented 1 year ago

Hmm wait... that only happened for the image field... when I tried the same file on the upload field I didn't get the same error... why is that? Because it's submitting a base64 image? But... isn't the base64 image then stored as a file, by the uploader, in the end?

Mnope... looks like the image uploader is storing the base64 in the db column... this isn't really intended @pxpm is it? That's what base64_image field is supposed to do... not image... right?! 🤷‍♂️

CleanShot 2023-05-10 at 15 43 39@2x
pxpm commented 1 year ago

I couldn't reproduce this issue. My image does not get saved in database, is it possible that you used the same entitites to test withFiles and withMedia ? Or that you attemped to save the entity before adding the withMedia() ?

image

image

// field definitions:
CRUD::field('avatar')->type('image')->size(6)->withMedia([
    'collection' => 'avatars',
]);
CRUD::field('testatar')->type('image')->size(6)->withMedia();

// the avatars collection test the creation of collections in models:
// User.php
public function registerMediaCollections(): void
    {
        $this->addMediaCollection('avatars');
    }

    public function registerMediaConversions(Media $media = null): void
    {
        $this->addMediaConversion('thumb')
        ->performOnCollections('avatars')
        ->width(368)
        ->height(232)
        ->keepOriginalImageFormat()
        ->nonQueued();
    }

In both scenarios it worked as expected.

I don't think the error is related to the file size. Media library already has a max file size by default in config and throws a custom exception:

image

It seems some script in debugbar exausted the memory in your case 🤷

tabacitu commented 1 year ago

Couldn't reproduce either 🤦‍♂️ Will re-open if/when I can.