Laravel-Backpack / medialibrary-uploaders

MIT License
10 stars 4 forks source link

Default upload path #13

Closed tabacitu closed 1 year ago

tabacitu commented 1 year ago

Bug report

What I did

Created a new Book migration, model, CRUD. Inside it I have a few uploads too - cover (upload or image) and first_pages (upload_multiple).

What I expected to happen

Files to be moved to a location that is separated... either:

What happened

Files were stored in public/3/bla-bla.png:

CleanShot 2023-05-10 at 15 28 04@2x

This looks weird to me.. it looks too general... Ok I understand spatie/medialibrary uploads stuff in individual directories, based on media id... but at least I expected it to be inside public/media/3/bla-bla.png... right?

Is this Spatie's default... or a default we've chosen @pxpm ? Do you think we should do anything about this? And if so, would it be difficult (hope we don't need to publish and customize spatie's config file).

pxpm commented 1 year ago

Indeed, using spatie media library requires you to provide an implementation of the PathGenerator interface to change directory structure.

Not documented but spatie also allows to have a "generator per model", so that you can use PathGenerator1 on App\User model and PathGenerator2 on App\Article model too.

To ease developer life, and to make it "more similar" to other uploads we can create our own BackpackPathGenerator implementing the spatie Interface.

We then need to decide how to approach the overwrite. We have a few options:

a) publish the spatie config file. Need to publish OUR spatie config file (weird), very prone to mistakes and BC's when upgrading.

b) Bind our BackpackPathGenerator to spatie PathGenerator Too intrusive at first sight, but ... but if "it's the same" as PathGenerator unless you pass it a "path" we can assume it's safe since only "Backpack components" would be passing the path there. Otherwise it will behave exactly as expected from spatie PathGenerator. If developer changed spatie default we are not going to overwrite nothing and use the developer provided PathGenerator. Not 100% sure if this is feasible without checking spatie core files.

c) change the config value on the fly: config(['medialibrary.path_generator' => BackpackPathGenerator::class]) It seems safe at first, but only if we can swap it back to the configured value after the uploaders do their job. Because developer may be using the spatie package outside of the CRUD panel too, and that may happen after the uploaders changed the config.

d) do nothing, refer users to change the config if they need an alternative (even if we provide that alternative coded by our selfs) they can just change it if they don't want the default spatie.

I vouche for d. Not intrusive, keep defaults, if developer change it's a consequential action of their desire.

tabacitu commented 1 year ago

Totally D, very good asessment and writeout, thanks Pedro! In the end you’re right, choosing where the file gets uploades is either a model thing… or a global thing. Not a Backpack thing.

So we can provide a PathGenerator we recommend they use globally, and tell them how. I’d tag this as a COULD-DO then, right?

pxpm commented 1 year ago

solved in #15