-
- Notifications
You must be signed in to change notification settings - Fork 1.1k
Use flysystem to manage uploaded files #7039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.x
Are you sure you want to change the base?
Use flysystem to manage uploaded files #7039
Conversation
| Hi I think the Can we just do something like this? yield ImageField::new('picture') ->setStorage('default.storage') ->setUploadDir('...'); // define the path inside the specified storage // or ->setFlysystemStorage('default.storage') ->setUploadDir('...'); // define the path inside the specified storage // Wihtout Flysystem: no change yield ImageField::new('picture') ->setUploadDir('...');And then Easyadmin will be in charge to create the |
| Hi @Seb33300 , thanks for your feedback! What you’re suggesting is actually already the case 🙂 if (null === $uploadedFileAdapter) { if (null === $field->getCustomOption(ImageField::OPTION_UPLOAD_DIR)) { throw new \InvalidArgumentException(sprintf('The "%s" image field must define the directory where the images are uploaded using the setUploadDir() method.', $field->getProperty())); } $uploadedFileAdapter = new LocalFileAdapter($field->getCustomOption(ImageField::OPTION_UPLOAD_DIR), $configuredBasePath, $this->projectDir); }So nothing breaks: existing behavior still works as before for local files. That being said, I had considered deprecating |
| Interesting, I also find the syntax a bit too verbose. The FilesystemOperator injection could be avoided by passing only the name and letting EasyAdmin retrieve it itself with this tag : https://github.com/thephpleague/flysystem-bundle/blob/bf5ab3c072c0def47872e28f69876548d8669657/src/DependencyInjection/FlysystemExtension.php#L134C30-L134C47 Couldn't the system be smart ? For example, That said, I understand this could introduce too many specific methods into ImageField. In that case, an implementation similar to how fields are handled could be interesting — for example, something like |
| Thanks for the thoughtful feedback @maxhelias ! You’re absolutely right that the current approach, while flexible, can feel too verbose for common use cases. I designed it to maximize extensibility, by allowing any UploadedFileAdapterInterface to be passed explicitly. For example: ImageField::new('picture') ->setUploadedFileAdapter( new CustomFileAdapter(...) // Fully customizable );This gives developers complete control, and works well when integrating with custom file handling logic. Proposal: Two levels of APITo combine both developer freedom and ergonomic defaults, I suggest introducing two complementary layers: Level 1: Declarative & ergonomic API (90% use cases)Provide shortcuts like: ImageField::new('picture') ->setStorage('default.storage') // For FlysystemOr: ImageField::new('picture') ->setUploadDir('public/uploads/posts') ->setBasePath('uploads/posts') // For local filesystemThese methods would internally resolve the appropriate
Level 2: Explicit and extensible API (advanced use cases)Keep the current approach unchanged for full flexibility: ->setUploadedFileAdapter( $uploadedFileAdapterFactory->createFlysystemFileAdapter($storage) )Or even ->setUploadedFileAdapter(new CustomUploadedFileAdapter(...));
Let me know if you’re on board with this direction. I’m happy to update the PR accordingly, adding the higher-level helpers (setStorage(), setBasePath(), and a resolver behind the scenes) while keeping the current factory-based API fully intact. Thanks again for your time and insights! |
| I agree with that, let's see what @javiereguiluz says about it |
| What about fully converting to using just the flysystem library and putting in place default storage that mirrors the current storage for uploaded files? Yes, it's a migration problem that people will have to tackle (they will have to adapt how they use the setUploadDir by overriding the default storage or adding a new one), but I find a lot of upside to doing the transition going forward - containerisation is becomming bigger and bigger and storing uploaded files on external services like S3 or others is becomming the norm. So why not make that easy and a supported out of the box and abstracted away via the library. |
| Hi @psihius ! That way we get all the benefits of Flysystem integration, but without forcing it as a hard dependency or breaking existing setups. |
Hello everyone,
This PR is a follow-up to the one created by @emmanuel-averty #5555 about a year ago, which aimed to allow easy association of an image with an entity and storage on S3 via Flysystem in EasyAdmin.
Although that PR wasn’t completed, it was a great starting point. Huge thanks to @emmanuel-averty for the original work, which served as the foundation for this proposal. 🙏
I’ve picked it up, incorporated the feedback from @mahono (who suggested introducing adapters for better modularity), and wrapped it into a reusable and configurable solution.
That supports for both local and Flysystem storage (including S3, FTP, etc.)
Some examples
With Flysystem
Controller to serve files
Without Flysystem :
I’m open to any suggestions for improvements or changes, don’t hesitate to point out anything that could be better !
Thanks a lot for your time and review 🙏