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

[Flystem] Add FlysystemV2 adapter for version 2 support of flysystem. #650

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jenkoian
Copy link
Contributor

Done as a new adapter to allows users to use the existing adapter for
flysystem version 1.

--

Let me know if are ok with the V2 adapter or if you think we should just overwrite the existing one.

Haven't touched any phpstan or psalm config, will examine the output of the github actions but may need some pointers here.

Done as a new adapter to allows users to use the existing adapter for
flysystem version 1.
@jum4
Copy link

jum4 commented Sep 23, 2021

Nice job @jenkoian, I'm interest in this adapter, maybe I can help,

league/flysystem:^1.0 seems to be fixed also in appveyor.yml and the Makefile (used by PHPStan analysis).
Because you kept the flysystem version 1 for compatibilty (which is a good choice in my opinion), you cannot depend on both versions.
Maybe the old Adapter may be excluded from PHPStan analysis ? So the requirement could be replaced by league/flysystem:^2.0.

The PHP tests seems to be failed because of trying to install olders versions of PHP (7.1, 7.2, 7.3) on ubuntu-latest, maybe an older version of ubuntu should be required for theses tests.

@jenkoian
Copy link
Contributor Author

Thanks @jum4 so it sounds like keeping this as a v2 and keeping the existing adapter is the way to go. In which case, yeah as you mention, it looks like the static analysis isn't going to work for both. Unless we have explicit static analysis just for FSv2 which seems like overkill, or, as you mention, we only statically analyse the newer version.

So, I'm happy to update this PR, but is anyone able to confirm the direction?

@jum4
Copy link

jum4 commented Sep 24, 2021

@nicolasmure

@jenkoian
Copy link
Contributor Author

Version 3 of Flysystem out now too https://github.com/thephpleague/flysystem/releases/tag/3.0.0

@dmitryuk
Copy link

Finally done in #688

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.

7 participants