Skip to content

feat: add Azure OSS adapter #181

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 2 commits into
base: 3.x
Choose a base branch
from
Open

feat: add Azure OSS adapter #181

wants to merge 2 commits into from

Conversation

jonag
Copy link
Contributor

@jonag jonag commented Mar 29, 2025

Hello,
Here is my attempt to add support for azure-oss/storage-blob-flysystem to the bundle (#178).
I am not sure about the name I chose to identify the adapter (azureoss), maybe you have a better suggestion?

Jonag

@jonag jonag force-pushed the feat/178 branch 2 times, most recently from 818c9a7 to 1425a52 Compare March 29, 2025 18:16
@jonag
Copy link
Contributor Author

jonag commented Mar 29, 2025

Should I find a way to tell PHPUnit to ignore the test if running with PHP < 8.1?

@jonag
Copy link
Contributor Author

jonag commented Mar 29, 2025

Actually, since the issue arises from composer install I'm not sure how to fix it without bumping the minimum required PHP version (which we probably want to avoid)...

Removed azure-oss/storage-blob-flysystem from composer.json when testing with PHP 8.0 due to compatibility issues.
@jonag
Copy link
Contributor Author

jonag commented Mar 31, 2025

I found a working workaround: removing the dependency from the composer.json when using PHP 8.0. Let me know if you can think of a better solution...

@ZZromanZZ
Copy link

Wow, thank you very much @jonag

@maxhelias
Copy link
Collaborator

Hi!

Shouldn't we deprecate the use of the old adapter?

This is the second external adapter (BunnyCDN), I think we shloud consider allowing the registration of a custom adapter in the bundle configuration, a bit like Symfony's security component for firewalls.

I'm a bit busy at the moment, but I'd like to explore this possibility before merging them, to avoid any back-and-forth or introducing a BC break.

@jonag
Copy link
Contributor Author

jonag commented Apr 11, 2025

Yes I think the old adapter should be deprecated since the "official" blob storage adapter is now abandonned.

Regarding your other comment, if I understand correctly, it means that each custom adapter should create its own bundle that will add its configuration to this bundle? Something like this for example?

@maxhelias
Copy link
Collaborator

@jonag Yes exactly. Or another solution if it is better to extend this list: https://github.com/thephpleague/flysystem-bundle/blob/3.x/src/Adapter/AdapterDefinitionFactory.php#L31-L43

jonag added a commit to jonag/flysystem-bundle that referenced this pull request Apr 12, 2025
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());
    }
}
```
@cancan101
Copy link

What is the status of the PR? I would love to see it merged so we can start using the new Azure adapter.

@maxhelias
Copy link
Collaborator

I would like to see this approche instead #182

@cancan101
Copy link

any updates on getting this merged?

@maxhelias
Copy link
Collaborator

Please read my previous message and #186

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.

4 participants