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.

Sign up for GitHub

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

Jump to bottom

Add Flysystem 2 Storage #138

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add Flysystem 2 Storage #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
684d321
Add Flysystem 2 Storage
marcus-at-localhost Feb 3, 2021
dcfe44b
Update Readme.md how to use Flysystem 2
marcus-at-localhost Feb 3, 2021
3b7502e
Add Flysystem 2 Example
marcus-at-localhost Feb 3, 2021
b7e0456
Use `FilesystemAdapter`
marcus-at-localhost Jun 26, 2021
fdc6039
Add Flystorage 1 Heading
marcus-at-localhost Jun 26, 2021
8b6eed5
Correct Capitalization
marcus-at-localhost Jul 6, 2021
987e815
Merge branch 'Kevinrob:master' into feature-flysystem2
marcus-at-localhost Jul 13, 2021
ef74fb6
Add Test For Flysystem2Storage
marcus-at-localhost Jul 14, 2021
44900e5
Add Test For Flysystem2Storage
marcus-at-localhost Jul 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ $stack->push(
```

## Flysystem

For Flysystem 1

```php
[...]
use League\Flysystem\Adapter\Local;
Expand All @@ -133,6 +136,28 @@ $stack->push(
);
```

For Flysystem 2

```php
[...]
use League\Flysystem\Local\LocalFilesystemAdapter;
use Kevinrob\GuzzleCache\Strategy\PrivateCacheStrategy;
use Kevinrob\GuzzleCache\Storage\Flysystem2Storage;

[...]

$stack->push(
new CacheMiddleware(
new PrivateCacheStrategy(
new Flysystem2Storage(
new LocalFilesystemAdapter('/path/to/cache')
)
)
),
'cache'
);
```

## WordPress Object Cache
```php
[...]
Expand Down
61 changes: 61 additions & 0 deletions src/Storage/Flysystem2Storage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
<?php

namespace Kevinrob\GuzzleCache\Storage;

use Kevinrob\GuzzleCache\CacheEntry;
use League\Flysystem\FilesystemAdapter;
use League\Flysystem\Filesystem;
use League\Flysystem\FileNotFoundException;

class Flysystem2Storage implements CacheStorageInterface
{

/**
* @var Filesystem
*/
protected $filesystem;

public function __construct(FilesystemAdapter $adapter)
{
$this->filesystem = new Filesystem($adapter);
}

/**
* @inheritdoc
*/
public function fetch($key)
{
if ($this->filesystem->fileExists($key)) {
// The file exist, read it!
$data = @unserialize(
$this->filesystem->read($key)
);

if ($data instanceof CacheEntry) {
return $data;
}
}

return;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\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.

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
try {
return $this->filesystem->delete($key);
} catch (FileNotFoundException $ex) {
return true;
}
}
}
3 changes: 3 additions & 0 deletions tests/PrivateCacheTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@
use Kevinrob\GuzzleCache\Storage\CompressedDoctrineCacheStorage;
use Kevinrob\GuzzleCache\Storage\DoctrineCacheStorage;
use Kevinrob\GuzzleCache\Storage\FlysystemStorage;
use Kevinrob\GuzzleCache\Storage\Flysystem2Storage;
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;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@dpi dpi Jul 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

use PHPUnit\Framework\TestCase;

class PrivateCacheTest extends TestCase
Expand All @@ -33,6 +35,7 @@ public function testCacheProvider()
new DoctrineCacheStorage(new FilesystemCache($TMP_DIR)),
new DoctrineCacheStorage(new PhpFileCache($TMP_DIR)),
new FlysystemStorage(new Local($TMP_DIR)),
new Flysystem2Storage(new LocalFilesystemAdapter($TMP_DIR)),
new Psr6CacheStorage(new ArrayCachePool()),
new Psr16CacheStorage(new SimpleCacheBridge(new ArrayCachePool())),
new CompressedDoctrineCacheStorage(new ArrayCache()),
Expand Down
3 changes: 3 additions & 0 deletions tests/PublicCacheTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@
use Kevinrob\GuzzleCache\Storage\CompressedDoctrineCacheStorage;
use Kevinrob\GuzzleCache\Storage\DoctrineCacheStorage;
use Kevinrob\GuzzleCache\Storage\FlysystemStorage;
use Kevinrob\GuzzleCache\Storage\Flysystem2Storage;
use Kevinrob\GuzzleCache\Storage\Psr6CacheStorage;
use Kevinrob\GuzzleCache\Storage\Psr16CacheStorage;
use Kevinrob\GuzzleCache\Storage\VolatileRuntimeStorage;
use Kevinrob\GuzzleCache\Strategy\PublicCacheStrategy;
use League\Flysystem\Adapter\Local;
use League\Flysystem\Local\LocalFilesystemAdapter;
use Psr\Http\Message\RequestInterface;
use PHPUnit\Framework\TestCase;

Expand Down Expand Up @@ -101,6 +103,7 @@ public function testCacheProvider()
new DoctrineCacheStorage(new FilesystemCache($TMP_DIR)),
new DoctrineCacheStorage(new PhpFileCache($TMP_DIR)),
new FlysystemStorage(new Local($TMP_DIR)),
new Flysystem2Storage(new LocalFilesystemAdapter($TMP_DIR)),
new Psr6CacheStorage(new ArrayCachePool()),
new Psr16CacheStorage(new SimpleCacheBridge(new ArrayCachePool())),
new CompressedDoctrineCacheStorage(new ArrayCache()),
Expand Down