Laravel-Backpack / CRUD

Build custom admin panels. Fast!
https://backpackforlaravel.com
MIT License
3.17k stars 896 forks source link

uploadFileToDisk not saving full path #1213

Closed PiotrKrzyzek closed 6 years ago

PiotrKrzyzek commented 6 years ago

Bug report

The CrudTrait.php function uploadFileToDisk (line 132) does not save the correct full path. It's saving the path without any respect to the filesystem $disk used.

What I expected to happen:

When using my own disk I that was setup like this:

'realestatephotos' => [
            'driver' => 'local',
            'root' => storage_path('app/public/re/'),
            'url' => '/assets/re/photos/',
            'visibility' => 'public',
        ],

The file (photos) upload function simply tells it which disk to use and to use the year and month for folders:

public function setFeaturedImageAttribute($value){
        $attribute_name = "featured_image";
        $disk = "realestatephotos";
        $destination_path = date("y") . "/" .date("m");
        $this->uploadFileToDisk($value, $attribute_name, $disk, $destination_path);
    }

I expected the file path saved to the database to be: "/assets/re/photos/18/01/12345.jpg".

What happened:

The featured image file path saved was:

"18/01/12345.jpg"

What I've already tried to fix it:

I fixed this by updating the uploadFileToDisk function with the following:

// 3. Save the complete path to the database
$this->attributes[$attribute_name] = \Storage::disk($disk)->url($file_path);
lloy0076 commented 6 years ago

I don't know if it should be the full path, though.

Do the docs say so?

PiotrKrzyzek commented 6 years ago

A partial path isn't useful. The front-end (or wherever the file path is pulled to) shouldn't have to make assumptions or guesses about where a file is.

Let's the simple example of a user profile photo. Upon login we'd want to display that profile photo in-place of the generic profile image, right? The front-end template wouldn't know if it should be searching in the folder assets/images/images-from-css-templates/ or images/profile-images or just plain ol' images/. Having the front-end do more guess work is unnessary work that the front-end shouldn't have to handle.

Let's pick another example, this time having a team of coders. There is the front-end dev team, the DB team and and the back-end models team (not the best setup, but it illustrates the point). The front-end team would never know where to figure out where to pull the images from. All the data they should ever get is "here is the link to the graphic" and now go use it ... or a bit more realistic: use this function to call the image and pick a size (thumbnail, medium, large, etc).

The front-end team shouldn't have to care if the file is on S3, a CDN somewhere or on one of many disks.

Also, the comments in the CrudTrait file specifically say "// 3. Save the complete path to the database". The current code doesn't save the complete path, it's saving the path sans disk path.

Another way to look at it is that we cannot assume that everyone will be using the default 'public' or local disk. Therefore, the actual full path should be stored.

Also, if I'm using my own filesystem disk setup (in my case 'realestatephotos') and I upload a file using the simple file upload crudTrait stuff via the admin dashboard that image link won't work. It'll give me a link, but if I click said link it'll just 404 due to the path being wrong. Just for the reason alone it should be changed.

iMokhles commented 6 years ago

did you saw the documents ? https://laravel-backpack.readme.io/docs/crud-fields#section-image there are 'prefix' attribute for the image field I guess that's what you are looking for ;)

PiotrKrzyzek commented 6 years ago

Nope, not images. I'm uploading files (and multiple files). I used uploading photos as an example. The file upload doesn't have a prefix attribute, but it does have a disk attribute which isn't being used when storing the path in the DB (but it correctly being used to store the file on disk).

https://laravel-backpack.readme.io/docs/crud-fields#section-upload

iMokhles commented 6 years ago

okey great so documentations say ( If you wish to have a different functionality, you can delete the method call from your mutator and do your own thing. ) so you have to override uploadFileToDisk function to serve your needs ( i used to do that fo some projects

my uploadFileToDisk function ( inside my Model ) capture d ecran 2018-02-02 a 16 09 29

and here's how i generate urls ( of course you can make your own ) [it's just an example] capture d ecran 2018-02-02 a 16 09 51

and here's my mutator function capture d ecran 2018-02-02 a 16 12 49

and here's my public disk code inside ( config/filesystem.php ) capture d ecran 2018-02-02 a 16 14 24

lloy0076 commented 6 years ago

Actually, @iMokhles has the right solution and this is not actually a bug.