[WIP] upload vs upload_multiple file display inconsistent behavior.

[pxpm]

While working on #5478 @karandatwani92 found out an inconsistency between this two field types.

Both fields store full paths on database, so for a field with prefix => 'images', the path in the database would be images/my-image.jpg.

They differ when rendering the stored results from the database.

// upload_multiple
isset($field['disk'])?asset(\Storage::disk($field['disk'])->url($file_path)):asset($file_path)

// upload (simplified)
 @if (isset($field['disk']))
    <a target="_blank" href="{{ (asset(\Storage::disk($field['disk'])->url(Arr::get($field, 'prefix', '').$field['value']))) }}">
@else
{{ $field['value'] }}

The «highlight» here is the ->url() part. One specifically use the $field['prefix'] while the other completely ignores it.

In upload the condition to display the database value is the isset($disk), so to display the database value you would define your field with no 'disk'.
Using the ->withFiles() (Backpack uploaders), you always use a disk (uploaders define that for you in your field, and path in uploaders translates to prefix on fields).

upload_multiple use the same condition, check for disk, to either display the file from the disk or the database value, but it does not append the prefix/path.

In that regard, the uploaders return the full database value (path+field) for multiple uploads, and only the file name for single uploads when the retrieved model event fires. (basically uploaders adapted to current field behavior to avoid breaking changes).

@karandatwani92 spotted this because he created upload fields like:

CRUD::field('upload')->type('upload')->withFiles(['path' => 'something']); 
CRUD::field('upload_multiple')->type('upload_multiple')->withFiles(['path' => 'something']); 

Saved the files correctly to something/my-file.pdf, and then @karandatwani92 changed the path to ['path' => 'something-else'].
In upload_multiple everything still worked just fine, as the displayed file was something/my-file.pdf (as in database).
In upload, since we build the file from disk and prefix, the return result was something-else/my-file.pdf that broke it, as that file does not exist in that disk.

This is where things start to smell fishy to me.
Let's say a real world example, I built an MVP, is ramping off and I need to scale.
I was saving profile_pictures locally on my public disk, so it was like:

CRUD::field('profile_picture')->type('upload')->withFiles([
'disk' => 'public', //can be ommited, it's the default
'path' => 'profile_pictures'
]);

I need now to move to S3. I copy all my local profile pictures to the bucket I created, configure the disk and go to my field to change the configuration:

CRUD::field('profile_picture')->type('upload')->withFiles([
+'disk' => 's3',
-'disk' => 'public', //can be ommited, it's the default
'path' => 'profile_pictures'
]);

I don't expect that some pictures to still be local, and others to be s3. That would be a mess IMO. In that regard, I disagree with @karandatwani92 , and I think the issue is in upload_multiple and not in upload.

The reason to store the "prefix+file" in the database is for "convenience" when storing the files in the public folder (notice that's not the public disk, it's the public folder). In a short and generalized version, the difference between them is that you commit the public folder, but the public disk is application dependent.
So you will have the same files on your public folder in both production and staging environments, but application uploaded files in the public disk.

If you ask me, I think we should NOT store anything that's not the file name in the database.
First and the biggest benefit is to completely un-tie the database from your storage. You can freely move stuff from folders or disks and just need to change the configuration without having to change values in the database.

The biggest downside, is that at the moment, if you want to store your stuff in a public path, you just need to define a disk for it, for example:

// in config/filesystems.php
'uploads' => [ 
    'driver' => 'local',
    'root'   => public_path(),
],

// and then use it as the disk:
CRUD::field('upload')->type('upload')->withFiles(['disk' => 'uploads', 'prefix' => 'my-prefix']);

// the files will be uploaded to the `/public/my-prefix` folder, and can be directly access by `asset($entry->upload)`

Removing the prefix from the database means that you would need now to do asset('my-prefix/'.$entry->upload).
But like I said, this only works in this specific scenario, where you store the user files in the public folder. That's not how it's supposed to work, the public folder is not completely .gitignored, only the linked storage/public and a few other build folders https://github.com/laravel/laravel/blob/0259455a1201b9019615f8bd1b1af6470747a4d5/.gitignore#L5

That means that if you are uploading your files into your public/uploads without manually adding that folder to gitignore, the uploaded files would still be commited.

Having all this strange behavior just to support asset($entry->upload) seems NOT worth it at all. This should be solved by using an accessor or something on $entry->upload to return the $prefix+$file.

All this would be a breaking change, that's why I moved it out of the other uploaders issue for further discussion.