Add Flysystem 2 Storage #138

marcus-at-localhost · 2021-02-03T15:24:17Z

No description provided.

Just upgraded to V2 https://flysystem.thephpleague.com/v2/docs/advanced/upgrade-to-2.0.0/ `use League\Flysystem\AdapterInterface;` => `use League\Flysystem\Local\LocalFilesystemAdapter;` `$this->filesystem->has` => `$this->filesystem->fileExists` `$this->filesystem->put` => `$this->filesystem->write`

diego-sorribas · 2021-02-17T19:44:27Z

Why are you using LocalFilesystemAdapter instead of FilesystemAdapter (Every adapter must implement the League\Flysystem\FilesystemAdapter interface.) says the doc?

I think we need an update to Flysystem 2.0

marcus-at-localhost · 2021-02-17T20:16:55Z

Why are you using LocalFilesystemAdapter instead of FilesystemAdapter (Every adapter must implement the League\Flysystem\FilesystemAdapter interface.) says the doc?

I'm not 100% sure if that could be done differently. I just adjusted the existing flysystem implementation according to the docs. https://flysystem.thephpleague.com/v2/docs/adapter/local/

marcus-at-localhost · 2021-06-26T12:32:28Z

Is this going to get merged?

dpi

Few things, this also needs the at least the same level of test coverage as Flysystem 1 version.

dpi · 2021-06-26T12:35:15Z

README.md

@@ -133,6 +135,28 @@ $stack->push(
 );
 ```

+for Flysystem 2


Either this should be its own ## heading or the Flysystem 1 version should also get this 'for xxx' heading.

The newer version should be before the older version

If keeping 'for' headings, should be capitalized.

Just added that.

dpi · 2021-06-26T12:36:05Z

src/Storage/Flysystem2Storage.php

+     */
+    protected $filesystem;
+
+    public function __construct(LocalFilesystemAdapter $adapter)


I agree with @diego-sorribas this shouldnt be fixed to local

Ok, I 'll try to understand what that means and will fix it.

Ok, I think I got it. :)
Thanks for the hints!

Kevinrob

Thanks for this addition!

Can you please:

Add a test for it
Respond to @dpi comment

marcus-at-localhost · 2021-07-13T14:05:27Z

I have honestly no idea how to add testing and how to perform it :/

Kevinrob · 2021-07-14T10:28:16Z

I have honestly no idea how to add testing and how to perform it :/

You can add it to PrivateCacheTest and PublicCacheTest in $cacheProviders like FlysystemStorage 👌

Kevinrob · 2021-07-16T17:02:29Z

tests/PrivateCacheTest.php

 use Kevinrob\GuzzleCache\Storage\Psr6CacheStorage;
 use Kevinrob\GuzzleCache\Storage\Psr16CacheStorage;
 use Kevinrob\GuzzleCache\Storage\VolatileRuntimeStorage;
 use Kevinrob\GuzzleCache\Strategy\PrivateCacheStrategy;
 use League\Flysystem\Adapter\Local;
+use League\Flysystem\Local\LocalFilesystemAdapter;


The tests doesn't run.
https://github.com/Kevinrob/guzzle-cache-middleware/pull/138/checks?check_run_id=3088224072

There were 2 errors: 1) Kevinrob\GuzzleCache\Tests\PrivateCacheTest::testCacheProvider Error: Class 'League\Flysystem\Local\LocalFilesystemAdapter' not found /home/runner/work/guzzle-cache-middleware/guzzle-cache-middleware/tests/PrivateCacheTest.php:38 2) Kevinrob\GuzzleCache\Tests\PublicCacheTest::testCacheProvider Error: Class 'League\Flysystem\Local\LocalFilesystemAdapter' not found /home/runner/work/guzzle-cache-middleware/guzzle-cache-middleware/tests/PublicCacheTest.php:106

Maybe, there are something to add to composer.json in require-dev?

Of course that's missing :). It seems to be not possible to add the same package twice.
This is what I've found: https://stackoverflow.com/questions/35679166/composer-using-two-different-versions-of-guzzle and https://stackoverflow.com/a/56677612/814031

How to solve that in a satisfying way, since this is just a problem in the test not when using the code itself? Right now I'm using my version as VCS repository but I assume an implementation of Flysystem 2 is of interest in the future.

Would it be odd to bump the middleware version number, and just include Flysystem 2 (at the end, Flysystem 1 can still be used it's just not tested anymore)?

But can that be justified, because a newer (and incompatible version) of an adapter that not everybody is using, was included?

Lots of questions. Thanks for your patience.

Can you cherry the commit (dpi@24d30ef) from https://github.com/dpi/guzzle-cache-middleware/tree/feature-flysystem2, it allows multiple flysystem versions and adds each version as a matrix. Not sure if it works as its unclear to me how to run Github actions on forks/PR's. If anyone has this info please enlighten/link me.

At least tests succeed when running the require command with each version.

dpi

Adding further changes needed, which was caught by PHPStan static analysis. In summary, return values don't comply with CacheStorageInterface from this project.

dpi · 2021-07-23T09:27:51Z

src/Storage/Flysystem2Storage.php

+            }
+        }
+
+        return;


this needs to return NULL not void, per \Kevinrob\GuzzleCache\Storage\CacheStorageInterface::fetch

dpi · 2021-07-23T09:29:34Z

src/Storage/Flysystem2Storage.php

+     */
+    public function save($key, CacheEntry $data)
+    {
+        return $this->filesystem->write($key, serialize($data));


\Kevinrob\GuzzleCache\Storage\CacheStorageInterface::save requires a bool return value, but \League\Flysystem\Filesystem::write returns void, and throws exceptions. So this call should be wrapped in try catch and return true/false.

dpi · 2021-07-23T09:31:25Z

src/Storage/Flysystem2Storage.php

+    /**
+     * {@inheritdoc}
+     */
+    public function delete($key)


Almost the same as above:

\Kevinrob\GuzzleCache\Storage\CacheStorageInterface::delete requires a bool return value, but \League\Flysystem\Filesystem::delete returns void, and throws exceptions. So you should add a return true after the ->delete call, and change the return the catch to return false

dpi · 2021-07-23T09:35:31Z

Changes can be based off https://github.com/dpi/dogit/pull/2/files#diff-807a9ba3b18ae226d5e918d14500c7503f96864c697a8d97a8fdb3396312ff97R19

marcus-at-localhost · 2021-07-27T12:56:37Z

Apologies, I'm kind of lost, since this goes deep with the testing and my knowledge here is still rudimentary.
Is it possible for someone else to add/merge those proposed changes so it passes all tests?
I would really appreciate that.

dpi · 2022-02-19T16:35:26Z

Flysystem 3 is now a thing ;)

marcus-at-localhost added 3 commits February 3, 2021 16:15

Update Readme.md how to use Flysystem 2

dcfe44b

Add Flysystem 2 Example

3b7502e

dpi suggested changes Jun 26, 2021

View reviewed changes

marcus-at-localhost added 2 commits June 26, 2021 14:56

Use FilesystemAdapter

b7e0456

Add Flystorage 1 Heading

fdc6039

marcus-at-localhost changed the title ~~Add Flysystem 2 Storage~~ Add Flysystem 1 Heading Jun 26, 2021

marcus-at-localhost changed the title ~~Add Flysystem 1 Heading~~ Add Flysystem 2 Storage Jun 26, 2021

Kevinrob reviewed Jul 4, 2021

View reviewed changes

Correct Capitalization

8b6eed5

Merge branch 'Kevinrob:master' into feature-flysystem2

987e815

marcus-at-localhost added 2 commits July 14, 2021 22:22

Add Test For Flysystem2Storage

ef74fb6

Add Test For Flysystem2Storage

44900e5

marcus-at-localhost requested a review from Kevinrob July 14, 2021 20:27

Kevinrob requested changes Jul 16, 2021

View reviewed changes

dpi suggested changes Jul 23, 2021

View reviewed changes

dpi mentioned this pull request Jul 23, 2021

Dont require a fork of kevinrob/guzzle-cache-middleware dpi/dogit#2

Merged

Kevinrob added the waiting label May 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Flysystem 2 Storage #138

Add Flysystem 2 Storage #138

marcus-at-localhost commented Feb 3, 2021

diego-sorribas commented Feb 17, 2021 •

edited

Loading

marcus-at-localhost commented Feb 17, 2021

marcus-at-localhost commented Jun 26, 2021

dpi left a comment

dpi Jun 26, 2021 •

edited

Loading

marcus-at-localhost Jul 6, 2021

dpi Jun 26, 2021

marcus-at-localhost Jun 26, 2021

marcus-at-localhost Jun 26, 2021

Kevinrob left a comment

marcus-at-localhost commented Jul 13, 2021

Kevinrob commented Jul 14, 2021

Kevinrob Jul 16, 2021

marcus-at-localhost Jul 17, 2021

dpi Jul 18, 2021 •

edited

Loading

dpi left a comment

dpi Jul 23, 2021

dpi Jul 23, 2021

dpi Jul 23, 2021

dpi commented Jul 23, 2021

marcus-at-localhost commented Jul 27, 2021

dpi commented Feb 19, 2022

+                          }
+                      }
+                      return;

Add Flysystem 2 Storage #138

Are you sure you want to change the base?

Add Flysystem 2 Storage #138

Conversation

marcus-at-localhost commented Feb 3, 2021

diego-sorribas commented Feb 17, 2021 • edited Loading

marcus-at-localhost commented Feb 17, 2021

marcus-at-localhost commented Jun 26, 2021

dpi left a comment

Choose a reason for hiding this comment

dpi Jun 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kevinrob left a comment

Choose a reason for hiding this comment

marcus-at-localhost commented Jul 13, 2021

Kevinrob commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpi Jul 18, 2021 • edited Loading

Choose a reason for hiding this comment

dpi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpi commented Jul 23, 2021

marcus-at-localhost commented Jul 27, 2021

dpi commented Feb 19, 2022

diego-sorribas commented Feb 17, 2021 •

edited

Loading

dpi Jun 26, 2021 •

edited

Loading

dpi Jul 18, 2021 •

edited

Loading