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 adapter #390

Merged
merged 14 commits into from Mar 7, 2016

Conversation

Projects
None yet
5 participants
@umpirsky
Contributor

umpirsky commented Mar 4, 2016

As a bonus, I added "Implementing new Adapter" documentation.

@umpirsky umpirsky added the In Progress label Mar 4, 2016

Show outdated Hide outdated src/Gaufrette/Adapter/Flysistem.php
public function isDirectory($key)
{
// TODO: create proper exception class if Flysistem does not support isDirectory()
throw new \BadMethodCallException('isDirectory is not supported by this adapter.');

This comment has been minimized.

@umpirsky

umpirsky Mar 4, 2016

Contributor

Anyone knows if this is supported?

@umpirsky

umpirsky Mar 4, 2016

Contributor

Anyone knows if this is supported?

Show outdated Hide outdated src/Gaufrette/Adapter/Flysistem.php
use League\Flysystem\AdapterInterface;
use League\Flysystem\Util;
class Flysistem implements Adapter

This comment has been minimized.

@akovalyov

akovalyov Mar 4, 2016

Contributor

Flysystem

@akovalyov

akovalyov Mar 4, 2016

Contributor

Flysystem

Show outdated Hide outdated doc/implementing-new-adapter.md
## Implementing adapter interface
Now all we need to do is to make `Gaufrette\Adapter\KnpStorage` implement all `Gaufrette\Adapter` interface methods.

This comment has been minimized.

@akovalyov

akovalyov Mar 4, 2016

Contributor

implements

@akovalyov

akovalyov Mar 4, 2016

Contributor

implements

@umpirsky umpirsky referenced this pull request Mar 5, 2016

Open

Flysystem adapter tests #2

@umpirsky umpirsky changed the title from [WIP] Flysystem adapter to Flysystem adapter Mar 5, 2016

@umpirsky umpirsky removed the In Progress label Mar 5, 2016

@docteurklein

This comment has been minimized.

Show comment
Hide comment
@docteurklein

docteurklein Mar 5, 2016

Contributor

👍

Contributor

docteurklein commented Mar 5, 2016

👍

Show outdated Hide outdated doc/adapters/flysystem.md
# Flysystem
Folks from [thephpleague](http://thephpleague.com/) built extraordinary [Flysystem](https://github.com/thephpleague/flysystem) package which does exactly the same thing as Gaufrette, but with slightly different API.

This comment has been minimized.

@akovalyov

akovalyov Mar 5, 2016

Contributor

I suggest have built instead of built.

@akovalyov

akovalyov Mar 5, 2016

Contributor

I suggest have built instead of built.

Show outdated Hide outdated doc/implementing-new-adapter.md
## Implementing adapter interface
Now all we need to do is to make `Gaufrette\Adapter\KnpStorage` implements all `Gaufrette\Adapter` interface methods.

This comment has been minimized.

@akovalyov

akovalyov Mar 5, 2016

Contributor

Now all we need to do is to implement all Gaufrette\Adapter interface methods in Gaufrette\Adapter\KnpStorage.
Or
Now all we need to do is to make sureGaufrette\Adapter\KnpStorageimplements allGaufrette\Adapterinterface methods.
All depends an what you wanted to say.

@akovalyov

akovalyov Mar 5, 2016

Contributor

Now all we need to do is to implement all Gaufrette\Adapter interface methods in Gaufrette\Adapter\KnpStorage.
Or
Now all we need to do is to make sureGaufrette\Adapter\KnpStorageimplements allGaufrette\Adapterinterface methods.
All depends an what you wanted to say.

Show outdated Hide outdated src/Gaufrette/Adapter/Flysystem.php
class Flysystem implements Adapter, ListKeysAware
{
private $adapter;

This comment has been minimized.

@akovalyov

akovalyov Mar 5, 2016

Contributor

Add phpdoc, please.

@akovalyov

akovalyov Mar 5, 2016

Contributor

Add phpdoc, please.

@jenkoian

This comment has been minimized.

Show comment
Hide comment
@jenkoian

jenkoian Mar 7, 2016

Contributor

Please tell me there is a PR to flysystem with a Gaufrette adapter :)

Contributor

jenkoian commented Mar 7, 2016

Please tell me there is a PR to flysystem with a Gaufrette adapter :)

@umpirsky

This comment has been minimized.

Show comment
Hide comment
@umpirsky

umpirsky Mar 7, 2016

Contributor

@jenkoian I don't see anything wrong with making both tools compatible with each other. This will allow us to focus on improving performance and memory consumption instead of reinventing existing adapters.

Contributor

umpirsky commented Mar 7, 2016

@jenkoian I don't see anything wrong with making both tools compatible with each other. This will allow us to focus on improving performance and memory consumption instead of reinventing existing adapters.

@jenkoian

This comment has been minimized.

Show comment
Hide comment
@jenkoian

jenkoian Mar 7, 2016

Contributor

@umpirsky I'm happy to PR to flysystem if you'd like...

Contributor

jenkoian commented Mar 7, 2016

@umpirsky I'm happy to PR to flysystem if you'd like...

@umpirsky

This comment has been minimized.

Show comment
Hide comment
@umpirsky

umpirsky Mar 7, 2016

Contributor

@jenkoian Would be cool in deed. But maybe it will make sense to do it for upcoming version of Gaufrette. Implementation already started, and we are writing a blog post about it.

What do you think @akovalyov @docteurklein @NiR-?

Contributor

umpirsky commented Mar 7, 2016

@jenkoian Would be cool in deed. But maybe it will make sense to do it for upcoming version of Gaufrette. Implementation already started, and we are writing a blog post about it.

What do you think @akovalyov @docteurklein @NiR-?

@umpirsky

This comment has been minimized.

Show comment
Hide comment
@umpirsky

umpirsky Mar 7, 2016

Contributor

@akovalyov BTW, after the build is green you can merge this.

Contributor

umpirsky commented Mar 7, 2016

@akovalyov BTW, after the build is green you can merge this.

@jenkoian

This comment has been minimized.

Show comment
Hide comment
@jenkoian

jenkoian Mar 7, 2016

Contributor

@umpirsky happy to hold off, don't want to step on your toes either if you're in the middle of something :)

Contributor

jenkoian commented Mar 7, 2016

@umpirsky happy to hold off, don't want to step on your toes either if you're in the middle of something :)

@akovalyov

This comment has been minimized.

Show comment
Hide comment
@akovalyov

akovalyov Mar 7, 2016

Contributor

@umpirsky I was waiting for your rebase :)

@jenkoian it is open source, so you can write any PR you want and if community accepts it - then it gets merged. In Gaufrette, we wanted to focus on brand new version, which means that we are less available for adapters maintenance. Since there already adapters which cover the similar functionality, we have decided to gain some time with this movement.

Contributor

akovalyov commented Mar 7, 2016

@umpirsky I was waiting for your rebase :)

@jenkoian it is open source, so you can write any PR you want and if community accepts it - then it gets merged. In Gaufrette, we wanted to focus on brand new version, which means that we are less available for adapters maintenance. Since there already adapters which cover the similar functionality, we have decided to gain some time with this movement.

akovalyov added a commit that referenced this pull request Mar 7, 2016

@akovalyov akovalyov merged commit c798a2b into master Mar 7, 2016

3 checks passed

Scrutinizer 4 new issues, 12 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@akovalyov akovalyov deleted the feature/flysystem-adapter branch Mar 7, 2016

@EmiiKhaos

This comment has been minimized.

Show comment
Hide comment
@EmiiKhaos

EmiiKhaos Mar 7, 2016

Contributor

Awesome work indeed, but feels a bit like below 😬

image

Contributor

EmiiKhaos commented Mar 7, 2016

Awesome work indeed, but feels a bit like below 😬

image

@docteurklein

This comment has been minimized.

Show comment
Hide comment
@docteurklein

docteurklein Mar 7, 2016

Contributor

totally :) that's the first thing we said to ourselves actually :)

And directly afterwards came the word "cross-pollination".

Contributor

docteurklein commented Mar 7, 2016

totally :) that's the first thing we said to ourselves actually :)

And directly afterwards came the word "cross-pollination".

@EmiiKhaos

This comment has been minimized.

Show comment
Hide comment
@EmiiKhaos

EmiiKhaos Mar 7, 2016

Contributor

But it's awesome and allows to profit of newly adapters which are created in the other one and respective (if a reverse PR is done).

Let hybridization happen and the system evolves :trollface:

Contributor

EmiiKhaos commented Mar 7, 2016

But it's awesome and allows to profit of newly adapters which are created in the other one and respective (if a reverse PR is done).

Let hybridization happen and the system evolves :trollface:

@umpirsky

This comment has been minimized.

Show comment
Hide comment
@umpirsky

umpirsky Mar 7, 2016

Contributor

@patkar Well, yes. It is amazing how similar they are.

But, that makes it logical, let's reinvent less wheels.

And, it is just a tiny layer, one more call in your trace. No harm done. :)

Contributor

umpirsky commented Mar 7, 2016

@patkar Well, yes. It is amazing how similar they are.

But, that makes it logical, let's reinvent less wheels.

And, it is just a tiny layer, one more call in your trace. No harm done. :)

@jenkoian

This comment has been minimized.

Show comment
Hide comment
@jenkoian

jenkoian Mar 8, 2016

Contributor

https://github.com/jenkoian/flysystem-gaufrette feedback welcomed, particularly around those unsupported methods (can we update via write? can we createDir for adapters that implement filefactory?) will then send over to the flysystem guys to take a look.

Contributor

jenkoian commented Mar 8, 2016

https://github.com/jenkoian/flysystem-gaufrette feedback welcomed, particularly around those unsupported methods (can we update via write? can we createDir for adapters that implement filefactory?) will then send over to the flysystem guys to take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment