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

Adding Opt In Support for symlinks to local adapter #599

Closed
PatrickRose opened this issue Feb 3, 2016 · 15 comments
Closed

Adding Opt In Support for symlinks to local adapter #599

PatrickRose opened this issue Feb 3, 2016 · 15 comments

Comments

@PatrickRose
Copy link

Flysystem explicitly doesn't support symlinks as of #474 (and it's mentioned again #502). I would like to add symlink support as an opt in feature to the LocalAdapter.

I think that symlinks should be treated as such:

Method Symlink to file Symlink to directory
write/writeStream Writes to target Fails
update/updateStream Writes to target Fails
rename Renames the symlink Renames the symlink
copy Copies the symlink Copies the symlink
delete Deletes the symlink Deletes the symlink
deleteDir Fails Fails
createDir Fails Fails
setVisibility Sets the visibility of the symlink Sets the visibility of the symlink
has Returns true Returns true
read/readStream Reads the target Fails
listContents Returns empty array Returns the contents of target
getSize etc Returns the value of the target Returns the value of the target

Essentially, a symlink should be treated as a file, with special behaviour for listContents (which is easily handled by using FilesystemIterator::FOLLOW_SYMLINKS for a recurisive directory iterator).

From a quick test on my end, SplFileInfo returns correctly for isFile() and isDir() depending on the symlink type so there shouldn't be much in the way of required changes.

Are you opposed to a PR that adds this feature?

@frankdejonge
Copy link
Member

@PatrickRose symlinks break the one of the "principles" in flysystem, which is the "all paths are relative" one. With support of symlinks, this constraint can't be easily enforced, and the way to enforce is would have a lot of arbitrary decision making. Like: Given strategy X, when a symlink is encountered, either ignore, disallow or follow. When following the outputted path should A not change, B be the linked path. Listings would be increasingly difficult. All these things weigh into me not wanting to go into this direction. But the main reason is: Given all paths are relative, all paths should reside within a directory, symlinks can only like to things within the root path, which is a very rare use-case.

@dsas
Copy link

dsas commented Feb 3, 2016

Symlinks not working flys in the face of people's expectations. Swapping out a file/dir for a symlink ordinarily just works, except with flysystem.

I'm not sure about the principle of all paths residing with the root directory and symlinks allowing the escape of that root. My thought is that a symlink inside the root directory is equivalent to putting that target inside of that root.

I think that the how to deal with symlink strategy should be do what unix does rather than ignoring or disallowing. De-reference or treat the symlink as it's own thing depending on what the equivalent unix syscalls do (in summary: treat a symlink as its own file except when changing permissions or reading, when it should be dereferenced). Given that php directly uses the unix syscalls then this is probably not surprising behaviour to anyone.

@jasonvarga
Copy link

I would ​_love_​ to see symlinks supported as an option.

We've been using Flysystem in Statamic 2 and it's been a joy. We have run into the wall however, trying to accommodate popular workflows involving symlinking, like moving the content directory so it can be managed in a (public) repo apart from the rest of the (private) app or building addons in a separate repo and symlinking them into an instance of Statamic. We've been using this approach for almost 4 years now, but since adopting Flysystem we've lost that ability. We just didn't realize it at first.

We're now in the process of reinventing the wheel, duplicating various parts of Flysystem in our own filesystem classes, creating a Frankenstein monster for the sole purpose of supporting a single symlinked folder.

I totally understand wanting to stay true to the "principle" of Flysytem, but an opt-in, opinionated, clear, and non-arbtriary (as detailed by @PatrickRose) solution wouldn't fly in the face of the "principle", but rather enhance it through wider adoption.

Just my two cents, but hopefully they're worth something.

@frankdejonge
Copy link
Member

@jasonvarga what you describe is actually still possible. Your root can be a symlink, which would allow you to have the cross deploy user files directory. The only restriction is that there are no symlinks within that root.

@jasonvarga
Copy link

I don't mean to hijack this thread into my own support issue, but I'm not quite understanding what you mean. How can the root be a symlink but have no symlinks in the root?

To clarify our use case, our directory structure looks something like this:

/
|-- app/ (first repo)
|   |-- .git/
|   |-- site/
|   |   |-- content/
|   |-- statamic/
|   |-- index.php
|
|-- shared/ (second repo)
    |-- .git/
    |-- content/

We'd want to symlink the site/content/ directory in the first repo to the content/ folder in the second repo. Our Flysystem root would be /app/.

@frankdejonge
Copy link
Member

@jasonvarga well, the solution would be to have the flysystem root set to /app/site/content/, this is in many regards a better setup that the classical 1 filesystem to rule them all. Multiple filesystems is 👍especially in the case of statamic where you want to root directory encapsulation.

@phuib
Copy link

phuib commented Feb 18, 2016

Here's another use case.

We're using a 3rd party media manager library (that requires flysystem) to manage images and videos. It's been working great until, for very good reasons, we decided to separate images and videos into separate symlinked sub-directories. But to continue using the media manager we'd have to hack it to use two separate instances of the local flysystem, one for images and one for video. This would be a horrible solution.

@jasonvarga
Copy link

@frankdejonge Thanks for the explanation, that cleared it up. I've reworked Statamic to use the multiple filesystem approach and it's already feeling a lot better. So our use-case is a non-issue now.

Carry on.

@frankdejonge
Copy link
Member

@phuib using two separate filesystems, even for local, is actually what I advocate.

@cassianotartari
Copy link

So, I'm wondering to use symbolic links to move content under web root directory to other hard disk using Local adapter. Is there any workaround to achieve this? I'm using Flysystem with Symfony and inside web folder I have:

$ ls -la
media -> /media/archive/media/

But instead of my app mount a path like http://my.app/media/somefile.txt it gives a http://my.app/media/archive/media/somefile.txt

@frankdejonge
Copy link
Member

@cassianotartari For these cases I mostly just create case-specific path formatters, I never pass the path through "blindly". Flysystem is unaware (and should be) of how a particular path may be exposed to the outside world.

@funivan
Copy link

funivan commented Nov 7, 2016

Hello. Thanks for the great library @frankdejonge
Here is a simple case with symlinks.
Create custom package with composer. Add phpunit package. Try to delete all project with flysystem. Fail(
For example we have following structure:

/myApp/
 src/
 vendor/
   bin/
     phpunit (symlinks to the phpunit execution file) 
  phpunit/
   phpunit/  (package)

What we should do to fix this task:

  1. Extend local adapter and deal with symlinks ?
  2. Write custom class that can find all symlinks, delete it and then run Delete operation with flysystem?

@frankdejonge
Copy link
Member

@funivan Hi, for this case I wouldn't use flysystem. It's never ever going to be the case that your composer packages are installed on S3 or dropbox, so the case for flysystem doesn't really apply. I'd just use this simple snipped:

function deleteDirectory(string $dir) {
    $contents = new RecursiveIteratorIterator(
        new RecursiveDirectoryIterator($dir, RecursiveDirectoryIterator::SKIP_DOTS),
        RecursiveIteratorIterator::CHILD_FIRST
    );

    foreach($contents as $item) {
        $item->isDir()
            ? deleteDirectory($item->getRealPath())
            : unlink($item->getRealPath());
    }

    rmdir($dir);
}

@funivan
Copy link

funivan commented Nov 7, 2016

@frankdejonge thanks for the fast response. I understand your position.
My last question about symlinks:
When I use logger or other staff connected to the flysystem (track write statistic, log deletion ..) then I should copy my logic to the proposed deleteDirectory method?
FlySystem gives us a good abstraction and it is pretty cool. But in this case all developers should deal with this situations in different ways.

@frankdejonge
Copy link
Member

@funivan I think using Flysystem there is a bit overkill, a deleteDirectory function call would do just as well 👍

@twistor twistor closed this as completed Aug 8, 2017
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

No branches or pull requests

8 participants