Flysystem support added. #674

graundas · 2015-12-11T15:58:05Z

Hi. I have added support for Flysystem filesystem support. I have created a Flysystem loader and some tests. For more details check docs I updated.

makasim · 2015-12-11T17:23:20Z

Binary/Loader/FlysystemLoader.php

+
+        try {
+            $sFileSystemService = sprintf('oneup_flysystem.%s_filesystem', $sFileSystem);
+            $this->filesystem = $container->get($sFileSystemService);


Can we inject this service directly. Injecting of the container and
"guessing" the filesystem service in the constructor looks strange to me.

Sander-Toonen · 2015-12-11T21:56:29Z

Hi, I was just thinking to pick development on this issue up. What is your point of view regarding stream support in OneupFlysystemBundle and what would be the up/downside of using this approach?

makasim · 2015-12-12T13:11:20Z

I am fine with Flysystem loader if it does not coupled to the bundle. See my comments.

graundas · 2015-12-12T13:20:50Z

I agree with you, @makasim . I will rewrite my code in a few days, so that a service would be injected instead of getting It from the container. @Sander-Toonen , I don't think a wrapper would be a better solution, because we would have to register a global php wrapper, which, in my opinion, is a slower and less configurable solution compared to dependency injection.

…ue\Flysystem\Filesystem.

Applied fixes from StyleCI

graundas · 2015-12-16T20:17:17Z

Hi. I have updated this Loader to use a service, which is being injected now. Check docs.
Travis-CI check has failed, because Flysystem library requires PHP 5.4.0 and Travis-CI checks compatibility with PHP 5.3. I am not sure if I should update .travis.yml by removing PHP 5.3 check. Should I do that?

makasim · 2015-12-17T09:22:34Z

Check php version and do composer require flysystem only if php version is higher than 5.3.

makasim · 2015-12-17T09:22:53Z

also add skip method to the tests, if php version is 5.3

makasim · 2015-12-17T09:23:52Z

Let's discuss drop of php5.3 support here: #679

makasim · 2015-12-17T09:25:44Z

Resources/config/imagine.xml

@@ -205,6 +206,11 @@
            <argument><!-- will be injected by StreamLoaderFactory --></argument>
            <argument><!-- will be injected by StreamLoaderFactory --></argument>
        </service>
+
+        <service id="liip_imagine.binary.loader.prototype.flysystem" class="%liip_imagine.binary.loader.flysystem.class%">


add abstract=true

Applied fixes from StyleCI

graundas · 2015-12-23T19:34:16Z

All you asked is done. Will you merge It to your repository?

makasim · 2015-12-23T19:55:26Z

Binary/Loader/FlysystemLoader.php

+
+    public function __construct(
+        ExtensionGuesserInterface $extensionGuesser,
+        Filesystem $filesystem)


Is there interface?

on the new line ) {

Are you talking about "Filesystem $filesystem" on line 24?

graundas · 2015-12-28T10:24:10Z

Hi. All done.

Sander-Toonen · 2015-12-28T10:33:34Z

Very nice work @graundas. Can't wait to see this merged.

makasim · 2016-01-04T12:44:40Z

Looks good to me. @javiereguiluz Could you please have a look at the docs?

Fixed build issues

Fixed doc errors reported by docs build tool

heikokrebs · 2016-01-13T12:41:56Z

👍

makasim · 2016-01-13T12:44:11Z

@javiereguiluz ping, @graundas could you please rebase the PR?

javiereguiluz · 2016-01-13T13:38:26Z

Resources/doc/data-loader/flysystem.rst

@@ -0,0 +1,35 @@
+FlysystemLoader
+============


This line should be as long as the preceding heading.

Updated docs. Loader is rewritten to support any service, which is instance of League\Flysystem\Filesystem. revert Applied fixes from StyleCI update Applied fixes from StyleCI minor fixes docs update

# Conflicts: # Resources/doc/data-loader/flysystem.rst # Resources/doc/data-loaders.rst

graundas · 2016-01-13T20:33:19Z

Hi. I have just rebased my PR. Docs corrected also.

Flysystem support added.

makasim · 2016-01-14T07:16:44Z

Thanks @graundas for the contribution and @javiereguiluz for the review.

graundas added 2 commits December 11, 2015 17:21

Added Flysystem support. /~https://github.com/1up-lab/OneupFlysystemBundle

322e9e3

Updated docs.

48069d1

makasim reviewed Dec 11, 2015
View reviewed changes

lsmith77 added the State: Reviewing This item is being reviewed to determine if it should be accepted. label Dec 16, 2015

graundas and others added 4 commits December 16, 2015 21:36

Loader is rewritten to support any service, which is instance of Leag…

d87364e

…ue\Flysystem\Filesystem.

revert

7f7cc71

Applied fixes from StyleCI

6714020

Merge pull request #3 from graundas/analysis-XaKrVz

0e8f3c6

Applied fixes from StyleCI

makasim reviewed Dec 17, 2015
View reviewed changes

graundas and others added 3 commits December 17, 2015 19:15

update

fe25f18

Applied fixes from StyleCI

7112fe0

Merge pull request #4 from graundas/analysis-zeRAr8

25e2c04

Applied fixes from StyleCI

makasim reviewed Dec 23, 2015
View reviewed changes

minor fixes

4cc2914

yceruto and others added 4 commits January 8, 2016 10:42

Fixed indent

91af895

Fixed typo

cc8c1b7

Merge pull request liip#691 from yceruto/fixed-build-issues

f5341df

Fixed build issues

Merge pull request liip#690 from javiereguiluz/more_doc_fixes

bf3f931

Fixed doc errors reported by docs build tool

javiereguiluz reviewed Jan 13, 2016
View reviewed changes

graundas added 2 commits January 13, 2016 22:14

Added Flysystem support. /~https://github.com/1up-lab/OneupFlysystemBundle

3647634

Updated docs. Loader is rewritten to support any service, which is instance of League\Flysystem\Filesystem. revert Applied fixes from StyleCI update Applied fixes from StyleCI minor fixes docs update

Merge remote-tracking branch 'origin/Flysystem-support'

4dfc6b6

# Conflicts: # Resources/doc/data-loader/flysystem.rst # Resources/doc/data-loaders.rst

makasim added a commit that referenced this pull request Jan 14, 2016

Merge pull request #674 from graundas/master

22d625e

Flysystem support added.

makasim merged commit 22d625e into liip:master Jan 14, 2016

lsmith77 removed the State: Reviewing This item is being reviewed to determine if it should be accepted. label Jan 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flysystem support added. #674

Flysystem support added. #674

graundas commented Dec 11, 2015

makasim Dec 11, 2015

Sander-Toonen commented Dec 11, 2015

makasim commented Dec 12, 2015

graundas commented Dec 12, 2015

graundas commented Dec 16, 2015

makasim commented Dec 17, 2015

makasim commented Dec 17, 2015

makasim commented Dec 17, 2015

makasim Dec 17, 2015

graundas commented Dec 23, 2015

makasim Dec 23, 2015

makasim Dec 23, 2015

graundas Dec 23, 2015

graundas commented Dec 28, 2015

Sander-Toonen commented Dec 28, 2015

makasim commented Jan 4, 2016

heikokrebs commented Jan 13, 2016

makasim commented Jan 13, 2016

javiereguiluz Jan 13, 2016

graundas commented Jan 13, 2016

makasim commented Jan 14, 2016

		@@ -0,0 +1,35 @@
		FlysystemLoader
		============

Flysystem support added. #674

Flysystem support added. #674

Conversation

graundas commented Dec 11, 2015

makasim Dec 11, 2015

Choose a reason for hiding this comment

Sander-Toonen commented Dec 11, 2015

makasim commented Dec 12, 2015

graundas commented Dec 12, 2015

graundas commented Dec 16, 2015

makasim commented Dec 17, 2015

makasim commented Dec 17, 2015

makasim commented Dec 17, 2015

makasim Dec 17, 2015

Choose a reason for hiding this comment

graundas commented Dec 23, 2015

makasim Dec 23, 2015

Choose a reason for hiding this comment

makasim Dec 23, 2015

Choose a reason for hiding this comment

graundas Dec 23, 2015

Choose a reason for hiding this comment

graundas commented Dec 28, 2015

Sander-Toonen commented Dec 28, 2015

makasim commented Jan 4, 2016

heikokrebs commented Jan 13, 2016

makasim commented Jan 13, 2016

javiereguiluz Jan 13, 2016

Choose a reason for hiding this comment

graundas commented Jan 13, 2016

makasim commented Jan 14, 2016