Skip to content

feat: Allow custom adapter registration #182

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

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

jonag
Copy link
Contributor

@jonag jonag commented Apr 12, 2025

This PR implements a solution to the issue discussed in #181, enabling external bundles to register their custom storage adapters with Flysystem Bundle without requiring them to be directly added to the main bundle's codebase.

I had to remove the @internal annotation from AdapterDefinitionBuilderInterface and AbstractAdapterDefinitionBuilder to allow them to be referenced by other bundles.

Usage Example

External bundles can now register their adapters as follows:

class AzureBlobStorageAdapterBundle extends Bundle
{
    /**
     * {@inheritdoc}
     */
    public function build(ContainerBuilder $container): void
    {
        parent::build($container);

        /** @var FlysystemExtension $extension */
        $extension = $container->getExtension('flysystem');
        $extension->addAdapterDefinitionBuilder(new AzureOssAdapterDefinitionBuilder());
    }
}

If you think this is the right approach I can add a test to my PR.

This PR implements a solution to the issue discussed in thephpleague#181, enabling external bundles to register their custom storage adapters with Flysystem Bundle without requiring them to be directly added to the main bundle's codebase.

I had to remove the  `@internal` annotation from `AdapterDefinitionBuilderInterface` and `AbstractAdapterDefinitionBuilder` to allow them to be referenced by other bundles.

## Usage Example
External bundles can now register their adapters as follows:
``` php
class AzureBlobStorageAdapterBundle extends Bundle
{
    /**
     * {@inheritdoc}
     */
    public function build(ContainerBuilder $container): void
    {
        parent::build($container);

        /** @var FlysystemExtension $extension */
        $extension = $container->getExtension('flysystem');
        $extension->addAdapterDefinitionBuilder(new AzureOssAdapterDefinitionBuilder());
    }
}
```
@maxhelias
Copy link
Collaborator

Yes, that’s one way to do it. You’d also need to ensure the adapter name doesn’t conflict with another, or have a clear strategy for handling duplicates. Personally, I’d lean toward simply disallowing duplicate names.
WDYT @tgalopin ?

@jonag
Copy link
Contributor Author

jonag commented Apr 25, 2025

I tried to look for examples of how this situation was handled in other Symfony components. For the examples I found (Messenger, Mailer), they chose the "first one wins" approach.

Given this, do you still think it's necessary to check for potential duplicates? If so, I can take care of it and then add tests if the rest of the modifications seem correct to you.

@maxhelias
Copy link
Collaborator

So for now, it's fine if the first one wins -- we'll see based on the use case.

@maxhelias
Copy link
Collaborator

maxhelias commented May 17, 2025

I've been thinking more broadly about how we can open up the FlysystemBundle to third-party adapter builders in a clean and sustainable way. Before considering the removal of the current @internal flags, I'd like to propose a few structured changes that could improve the bundle's extensibility and overall developer experience:

Proposed changes

  1. Externalize the getRequiredPackages() logic

    • Move this method to the AdapterDefinitionBuilderInterface.
    • Let the AdapterDefinitionFactory handle the package check before calling createDefinition().
    • Remove the related logic from AbstractAdapterDefinitionBuilder
  2. Extract Unix visibility/permissions logic

    • Move configureUnixOptions() and createUnixDefinition() out of the abstract base class.
    • Possible solutions:
      • A trait (ProvidesUnixPermissions) – easier to share but harder to test
      • A static helper class (UnixVisibilityHelper) – more testable but less flexible
    • Remove this logic from AbstractAdapterDefinitionBuilder
  3. Make builder options discoverable via Symfony's config system

    • Introduce a new optional method in the interface: addConfiguration(NodeBuilder $builder) (similar to what Symfony Security does)
    • This allows builders to describe their expected configuration structure and default value
    • Deprecate the current adapter and options-based configuration style.
  4. Clean up the abstract class

    • Strip AbstractAdapterDefinitionBuilder down to only keep createDefinition().
      • Remove configureOptions
      • Remove configureDefinition
    • The class will be much lighter — and perhaps not needed at all — which improves separation of concerns.

Let me know what you think of this direction – happy to break it into smaller PRs if needed! poke @tgalopin

I'll start some parts soon, these just up in my todolist.

@maxhelias maxhelias mentioned this pull request Jun 13, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants