sonata-project / SonataMediaBundle

Symfony SonataMediaBundle
https://docs.sonata-project.org/projects/SonataMediaBundle
MIT License
449 stars 496 forks source link

Multiupload Native in Sonata Media #2209

Closed silasjoisten closed 2 years ago

silasjoisten commented 2 years ago

Feature Request

Hello together, long time ago i created a multi upload bundle for SonataMedia. https://github.com/silasjoisten/sonata-multiupload-bundle.

Do you have any Idea how i can make that compatible with the new Version of the MediaBundle or can i integrate that natively in that MediaBundle.

In my bundle the user needs to extend the provider that should be used with multi upload. and use a Trait. (simply getter setter.) then i got a compiler pass wich was setting it. How ever the Provider Classes are final then so that this flow wont work. Also when i tried to update my bundle: I extended the MediaAdminController to override a specific part of the create action. This wont work as well anymore because its final. Do you guys have any idea for a nice flow how we can enable multi upload here?

silasjoisten commented 2 years ago

cc. @OskarStark

VincentLanglet commented 2 years ago

@jordisala1991 If the Media is not final, maybe the controller shouldn't too ?

jordisala1991 commented 2 years ago

To add more controller actions, you don't need to extend the main controller, you can create another class with only that action and bind it on the configureRoutes

Media can't be final because it has to be extended in order to add the doctrine annotations and the $id property.

silasjoisten commented 2 years ago

I mean i need to override the create route in MediaAdminController and only one small line 29 part like in here https://github.com/silasjoisten/sonata-multiupload-bundle/blob/3.3.0/src/Controller/MultiUploadController.php

And in the for example ImageProvider the user has to add a Trait to that Provider described in here https://github.com/silasjoisten/sonata-multiupload-bundle/tree/3.3.0#step-3-configuration

silasjoisten commented 2 years ago

I am not talking about the entity, i meant the Provider and Controller. So for me it makes totally sense to make these classes final. But i have no idea to make my bundle work and that is still simple for Users to use it. The only idea i got is: Adding the multiupload feature natively

jordisala1991 commented 2 years ago

I mean i need to override the create route in MediaAdminController and only one small line 29 part like in here https://github.com/silasjoisten/sonata-multiupload-bundle/blob/3.3.0/src/Controller/MultiUploadController.php

And in the for example ImageProvider the user has to add a Trait to that Provider described in here https://github.com/silasjoisten/sonata-multiupload-bundle/tree/3.3.0#step-3-configuration

Let's separate Controller and Provider.

For the controller you are already overriding the whole create method, you can add your own controller without extending media controller and do your logic, changing the route pointing to your controller will do the trick.

for the provider we will have to think a little more

silasjoisten commented 2 years ago

Okay for the controller i need to maintain the listAction as well then. not only the Action.

For the Provider thats tricky and i also got no idea to fix that.

OskarStark commented 2 years ago

You need the list action for the button to upload via multi upload?

If yes @sonata-project/contributors maybe we can add a new entrypoint for providing buttons in the list?

cc @core23 any idea?

silasjoisten commented 2 years ago

Yes if the multiupload is enabled a differnt URI is rendered for the select_providers.html.twig buttons which are pointing to the multiupload controller or the normal create action.

silasjoisten commented 2 years ago

You need the list action for the button to upload via multi upload?

No but it is overwritten in the MediaAdminController and i need to override the MediaAdminController as well to render custom buttons.

jordisala1991 commented 2 years ago

For the provider, you are only adding a bool. Why don't you create your own configuration on the MultiUploadBundle to configure what providers have multi upload enabled and avoid adding trait on the provider?

core23 commented 2 years ago

For the Provider thats tricky and i also got no idea to fix that.

Maybe you can use a service decorator here? https://symfony.com/doc/current/service_container/service_decoration.html

silasjoisten commented 2 years ago

Sorry for the delay. Ich checked the service Decoration. This is also no solution I have tried it out and it is not working.

  1. Because of the protected function doTransform which is not possible to call on the @.inner service
  2. i also need to implement all of the 35 methods from the MediaProviderInterface. (This feels not right to me)

My solution would be make the functions final not the class for the providers. Because all i want to add is a trait with getters/setters. and one extra property.

VincentLanglet commented 2 years ago

2. i also need to implement all of the 35 methods from the MediaProviderInterface. (This feels not right to me)

Can't the BaseProvider help you on that https://github.com/sonata-project/SonataMediaBundle/blob/4.x/src/Provider/BaseProvider.php ?

Because all i want to add is a trait with getters/setters. and one extra property.

Maybe the logic could be adapted from

{% if provider.multiUpload is defined and provider.multiUpload == true %}

to a twig extension supports_multi_upload which would check if the provider is in a list of providers supporting multi upload. And this list would be set by the CompilerPass.

can i integrate that natively in that MediaBundle.

That may also be a great solution. WDYT @jordisala1991 ?

jordisala1991 commented 2 years ago

I don't think it is a good idea to integrate it directly. It requires a lot of js and css to make it work, and currently Sonata is not ready for that (talkin about architecture). (And also we don't have a lot of maintainers)

jordisala1991 commented 2 years ago

Also, I don't see a relation between those two facts:

The problem will be the same. IMO the solution in that bundle needs some readaptation , like @VincentLanglet is proposing, and maybe we need to open some points and remove some finals if needed.

silasjoisten commented 2 years ago

So i found a different solution. I changed the compiler pass in my bundle and created a ProviderChain in the config of sonata_multi_upload you have to define the providers where you want to use MultiUpload similar to sonata_media config.

The compiler pass adds the providers of sonata to that chain. and in the template i just check if the provider in select_provider.html.twig contains that chain. and there we go.

silasjoisten commented 2 years ago

@jordisala1991 i agree and i understand that you closed the API. It just makes sense.

VincentLanglet commented 2 years ago

So i found a different solution. I changed the compiler pass in my bundle and created a ProviderChain in the config of sonata_multi_upload you have to define the providers where you want to use MultiUpload similar to sonata_media config.

The compiler pass adds the providers of sonata to that chain. and in the template i just check if the provider in select_provider.html.twig contains that chain. and there we go.

I assume we can close this then ? :)

silasjoisten commented 2 years ago

Yes!