Skip to content
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

Flysystem v2 support #1357

Merged
merged 9 commits into from
Feb 23, 2021
Merged

Flysystem v2 support #1357

merged 9 commits into from
Feb 23, 2021

Conversation

mynameisbogdan
Copy link
Contributor

@mynameisbogdan mynameisbogdan commented Feb 20, 2021

Q A
Branch? 2.0
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1308
License MIT
Doc PR

Adding support for Flysystem v2 as drop-in replacement in FlysystemLoaderFactory and FlysystemResolverFactory.

@coveralls
Copy link

coveralls commented Feb 20, 2021

Coverage Status

Coverage increased (+0.6%) to 84.146% when pulling 98ce75a on mynameisbogdan:2.x into 7a9740c on liip:2.x.

@mynameisbogdan mynameisbogdan marked this pull request as draft February 21, 2021 05:17
@mynameisbogdan mynameisbogdan marked this pull request as ready for review February 21, 2021 05:36
@mynameisbogdan
Copy link
Contributor Author

@lsmith77 I updated this PR.

I fixed some mismatches in the tests from the previous PR and put back the check on ExtensionGuesserInterface & MimeTypesInterface on the loader to maintain BC with Symfony pre-4.3.

@lsmith77
Copy link
Contributor

thx .. also thx for removing phpcs dependency.

you re-added support for pre 4.3 versions because you need this yourself?

@mynameisbogdan
Copy link
Contributor Author

mynameisbogdan commented Feb 22, 2021

No, I tried to maintain BC by keeping the same logic as the loader for v1. But now looking at composer.json, it makes sense to remove it since symfony/mime is mandatory even for symfony v3.

I pushed an update.

Thank you.

@fbourigault
Copy link
Contributor

Is it possible to not introduce a flysystem2 configuration value and choose the proper implementation by detecting if V1 or V2 is installed?

@mynameisbogdan
Copy link
Contributor Author

mynameisbogdan commented Feb 22, 2021

@fbourigault well that's case now. Since the loader and resolver must have an unique service id, I'm handling that in FlysystemLoaderFactory (eg liip_imagine.cache.resolver.prototype.flysystem2 / liip_imagine.binary.loader.prototype.flysystem2).

But the configuration value remained the same flysystem, so no BC breaks.

@lsmith77
Copy link
Contributor

awesome thx.

@lsmith77 lsmith77 merged commit 56924e4 into liip:2.x Feb 23, 2021
@mynameisbogdan
Copy link
Contributor Author

Thank you as well.

@lsmith77
Copy link
Contributor

ah damn .. hit merge and then I remembered that we have failed to update the UPGRADING.md since a while but would like to get back to doing this .. could you add a new section with config instructions for users upgrading to flysystem2?

@mynameisbogdan
Copy link
Contributor Author

mynameisbogdan commented Feb 23, 2021

@lsmith77 Well, there are no configuration changes. One can simply run composer require -W liip/imagine-bundle:2.x-dev league/flysystem:^2.0 and that's it.

Should I make a new PR with this information?

@lsmith77
Copy link
Contributor

yes please

@mynameisbogdan
Copy link
Contributor Author

Done in #1359

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