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

Add Flysystem resolver #715

Merged
merged 1 commit into from
Jul 12, 2016
Merged

Add Flysystem resolver #715

merged 1 commit into from
Jul 12, 2016

Conversation

cedricziel
Copy link
Collaborator

As a counterpart to the flysystem loader, using this resolver
it is also possible to store the cached filter-files in
flysystem adapters.

  • add FlysystemResolver
  • add tests for FlysystemResolver
  • add documentation for FlysystemResolver
  • exclude correct Tests folder from scrutinizer analysis

@cedricziel
Copy link
Collaborator Author

There's a syntax error in the current symfony branch for 2.7.
The build will succeed once symfony/symfony#18031 is merged.
I consider the current state as my final suggestion and I'm open for your criticism :)

@lsmith77 lsmith77 added the State: Reviewing This item is being reviewed to determine if it should be accepted. label Mar 6, 2016
@lsmith77
Copy link
Contributor

the PR was merged .. I have retriggered the tests

@cedricziel
Copy link
Collaborator Author

Great. Thx.

@cedricziel
Copy link
Collaborator Author

All is green. Is there anything I can do to make this change more attractive?

@alexwilson
Copy link
Collaborator

👍 Looks like a super useful feature!

@cedricziel
Copy link
Collaborator Author

The CI was triggered because of a rebase. Tests are still green and the styleci fail is not on anything I touched :)

@dupuchba
Copy link

@cedricziel I really want this one to be merged ^^ can you fix the styleCI ?

As a counterpart to the flysystem loader, using this resolver
it is also possible to store the cached filter-files in
flysystem adapters.

* add FlysystemResolver
* add tests for FlysystemResolver
* add documentation for FlysystemResolver
* exclude correct `Tests` folder from scrutinizer analysis
@cedricziel
Copy link
Collaborator Author

So do I - and sure I can. I was just hesitating that it might break some other PRs.

Here we go.

@dupuchba
Copy link

great :-), it looks like @lsmith77 has now the power button ^^

/**
* @var Filesystem
*/
protected $flysystem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

$filesystem

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to keep the name flysystem. Is this a major problem?

@makasim
Copy link
Collaborator

makasim commented May 13, 2016

will this work with not local flysystem filesystem, let's an amazon s3 one?

@cedricziel
Copy link
Collaborator Author

@makasim Sure. Flysystem encapsulates all the logic for retrieving and storing files so one flysystem instance is as good as another one-be it local or remote.

@cedricziel
Copy link
Collaborator Author

ping @makasim @lsmith77

@cedricziel
Copy link
Collaborator Author

Hello there - is there anything holding you back from merging this one? I'd be happy to adjust my code if needed.

@lsmith77 lsmith77 merged commit 94aca9c into liip:master Jul 12, 2016
@lsmith77 lsmith77 removed the State: Reviewing This item is being reviewed to determine if it should be accepted. label Jul 12, 2016
@lsmith77
Copy link
Contributor

thx .. and sorry for the delay .. the Bundle needs more maintainer love :-/

@BartVB
Copy link

BartVB commented Jul 12, 2016

Great, thank you for merging!
Maintenance should be a bit easier with more quality pull requests like this one.

@alexwilson alexwilson mentioned this pull request Jul 12, 2016
@cedricziel
Copy link
Collaborator Author

Thank you for the merge!

Let me know if I can be of any help.

@cedricziel cedricziel deleted the flysystemResolver branch July 13, 2016 07:05
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.

6 participants