Skip to content

Conversation

@Yoann-TYT
Copy link
Contributor

@Yoann-TYT Yoann-TYT commented Jul 16, 2025

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

class PostCrudController extends AbstractCrudController { public function __construct( private readonly UploadedFileAdapterFactory $uploadedFileAdapterFactory, private readonly FilesystemOperator $defaultStorage ) {} public static function getEntityFqcn(): string { return Post::class; } public function configureFields(string $pageName): iterable { yield ImageField::new('picture') ->setUploadedFileAdapter( $this->uploadedFileAdapterFactory->createFlysystemFileAdapter($this->defaultStorage) ); } }
# config/packages/flysystem.yaml flysystem: storages: default.storage: adapter: 'local' options: directory: '%kernel.project_dir%/var/storage/default' public_url: '/storage/default'

Controller to serve files

#[Route('/storage/default/{path}', name: 'app_storage_default')] public function index(FilesystemOperator $defaultStorage, string $path): Response { $stream = $defaultStorage->readStream($path); return new Response(stream_get_contents($stream)); }

Without Flysystem :

 ->setUploadedFileAdapter( $this->uploadedFileAdapterFactory->createLocalFileAdapter('public/uploads/posts', 'uploads/posts') )

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 🙏

@Seb33300
Copy link
Contributor

Seb33300 commented Jul 21, 2025

Hi

I think the setUploadDir method should not be deprecated.
Currently, using the ImageField is very easy and requesting to provide a UploadedFileAdapter every time is IMO cumbersome.

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 FileAdapter.
if setFlysystemStorage is defined, it creates automatically the related FlysystemFileAdapter otherwise a LocalFileAdapter is created

@Yoann-TYT
Copy link
Contributor Author

Yoann-TYT commented Jul 21, 2025

Hi @Seb33300 , thanks for your feedback!

What you’re suggesting is actually already the case 🙂
If no adapter is defined and uploadDir is set, we fallback to that logic internally. Here’s the relevant part of the ImageConfigurator:

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 setUploadDir() and setBasePath() in the long run, precisely to remove this conditional logic in the ImageConfigurator, which feels a bit messy and makes less sense when using Flysystem (where the concept of an “upload directory” doesn’t really apply anymore). But of course, that’s open for discussion !

@maxhelias
Copy link
Contributor

maxhelias commented Jul 21, 2025

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, setBasePath could use LocalFileAdapter, and setFlysystemStorage could use FlysystemFileAdapter. This pattern could then be extended to support other storage adapter in the future if we want ?

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 setAdapter(Adapter::new()) or another similar approach.

@Yoann-TYT
Copy link
Contributor Author

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 API

To 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 Flysystem

Or:

ImageField::new('picture') ->setUploadDir('public/uploads/posts') ->setBasePath('uploads/posts') // For local filesystem

These methods would internally resolve the appropriate UploadedFileAdapter using a service (e.g. UploadedFileAdapterResolver), and call setUploadedFileAdapter(...) under the hood.

  • No need to inject anything in the CRUD controller
  • Familiar EasyAdmin-style DX
  • Keeps code concise and readable

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(...));
  • Maximum control
  • Supports advanced scenarios
  • Adapters can be easily tested, injected, etc.

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!

@maxhelias
Copy link
Contributor

I agree with that, let's see what @javiereguiluz says about it

@psihius
Copy link
Contributor

psihius commented Oct 9, 2025

What about fully converting to using just the flysystem library and putting in place default storage that mirrors the current storage for uploaded files?
Then you can have file uploads configured insite the field classes by default and alow only the overrides.

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.

@Yoann-TYT
Copy link
Contributor Author

Hi @psihius !
I think the proposed two-level API already goes in the same direction: it introduces Flysystem support through setStorage() and internal adapters, while keeping the current local upload flow intact.

That way we get all the benefits of Flysystem integration, but without forcing it as a hard dependency or breaking existing setups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants