add support for Flysystem V2

[Warxcell]

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

Add support for https://flysystem.thephpleague.com/v2/docs/what-is-new/

[coveralls]

Coverage decreased (-0.1%) to 83.539% when pulling 348c5bf on Warxcell:flysystem_v2 into 1cb215c on liip:2.x.

[mynameisbogdan]

Hello @Warxcell,

Can you please take a look at FlysystemResolver as well?

Thank you.

[lsmith77]

there seems to be some coding style issues that need to be addressed

[Warxcell]

Yes, but it doesn't tell anything useful. For example diff and files.

Run php-cs-fixer fix --dry-run --format checkstyle | cs2pr
  php-cs-fixer fix --dry-run --format checkstyle | cs2pr
  shell: /usr/bin/bash -e {0}
Loaded config default from "/home/runner/work/LiipImagineBundle/LiipImagineBundle/.php_cs.dist".
Warning: Found violation(s) of type: phpdoc_order
Warning: Found violation(s) of type: phpdoc_separation
Warning: Found violation(s) of type: phpdoc_trim
Warning: Found violation(s) of type: phpdoc_align
Error: Process completed with exit code 1.

[lsmith77]

ah .. I see .. @fbourigault I guess we need to adjust this action to give useful output.

[fbourigault]

Those should appear in this MR “Files changed” tab.

[Warxcell]

Those should appear in this MR “Files changed” tab.

Not quite. I will see them tomorrow locally.

[lsmith77]

@fbourigault it just shows this

not sure if we had used https://styleci.io/ on this repo in the past and if we removed it when we added GithubActions but it shows the specific issues and even makes a PR if you want.

[fbourigault]

My bad PHP-CS-Fixer doesn't report which line contains the error.

I opened #1355 to remove use of cs2pr and display diff in action output.

[lsmith77]

thx .. merged to 2.x .. can you rebase @Warxcell ?

[Warxcell]

Done. BTW can you give me a hand with tests - we need to run test with version 1 and version 2 installed. How can we achieve that?

[fbourigault]

You have to find something that make a difference between version 1 and 2 of Flysystem (V1 has League\Flysystem\FilesystemInterface interface but V2 hasn't)and then skip V1 tests when V2 is detected and skip V2 tests when V1 is detected.

[fbourigault]

You have to remove this line as we don't use PHP-CS-Fixer as a dev dependency anymore.

[Warxcell]

But I don't have global php-cs-fixer and the only way to use it is as dependency. That way package is fully packaged with all dependencies. Isn't it better?

[mynameisbogdan]

Why don't you use this instead composer global require friendsofphp/php-cs-fixer, export PATH="$PATH:$HOME/.composer/vendor/bin" and php-cs-fixer?

[Warxcell]

You have to find something that make a difference between version 1 and 2 of Flysystem (V1 has League\Flysystem\FilesystemInterface interface but V2 hasn't)and then skip V1 tests when V2 is detected and skip V2 tests when V1 is detected.

Already did that, but now V1 tests fails. Or they will be skipped and never ran.

[Warxcell]

I think travis must be configured to run tests on both versions.

[lsmith77]

we are using GithubActions .. but yeah I think we likely have to have a separate build for this.
I propose we test flysystem v2 by default and have one separate build where we test flysystem v1

wdyt @fbourigault ?

[fbourigault]

You have to skip V1 tests when the interface is not installed. There is 0 chance to successfully run Flysystem V1 tests with highest dependencies!

[Warxcell]

Old tests are not working.

[fbourigault]

Have you tried to skip FlysystemV1 related tests when V2 is installed?

Something like this in FlysystemLoaderTest and FlysystemResolverTest should do the work:

        if (!class_exists(FilesystemInterface::class)) {
            $this->markTestSkipped('Requires the league/flysystem:^1.0 package.');
        }

[Warxcell]

Done, skipped tests for V1, but why Coveralls complains about old tests now?

[lsmith77]

I guess the reduction in coverage is a side effect of now using either V1 or V2.
I retriggered scrutinizer

[fbourigault]

There is something wrong. If you look at /~https://github.com/liip/LiipImagineBundle/pull/1349/checks?check_run_id=1914419168, both V1 and V2 tests are skipped.

[Warxcell]

Fixed that. Seems class_exists doesn't work for interfaces. I always thought it works.

Now overalls is good but some of the tests fails. Will review them later today.

[Warxcell]

Finally, all green :)

[lsmith77]

LGTM .. @fbourigault WDYT?

[fbourigault]

Could you remove friendsofphp/php-cs-fixer from require-dev now everything is ok?

Once removed, this one could be merged.

[mynameisbogdan]

I'm sorry to ask, but isn't #1357 a more proper implementation?

[fbourigault]

Why #1357 would be better?

[mynameisbogdan]

@fbourigault because it handles the integration for the loader and resolver by adding service prototypes for them, and the configuration factories detects if you're using v1 or v2 to know which service to inject.

[lsmith77]

Thank you @Warxcell for working through this process with us! You helped mature our new Github Actions based setup and also bring Flysystem2 support to the Bundle.

@mynameisbogdan also thank you for your PR. I will merge this PR here and would then appreciate if you could rebase your PR to add the additional pieces on top of @Warxcell’s work.