Allow injecting custom PathPrefixer

[alongosz]

Hello.
We're trying to upgrade our Product to use Flysystem v2 (can't switch to v3 due to PHP 7.4 being still in use).
However for us a path where we store files is dynamic - determined at runtime depending on a context.

If we had the ability to inject our custom PathPrefixer, it would solve all our issues. This PR enables that. Otherwise we needed to copy 1:1 src/Local/LocalFilesystemAdapter.php just to use our own Path Prefixer.

Known issues

Naming. Ideally I'd name the interface PathPrefixer and the implementation NativePathPrefixer or PortablePathPrefixer (trying to follow the existing convention). However due to BC (there's PathPrefixer class already), I called it simply Prefixer. Not sure if there's better name.

[alongosz]

Review ping @frankdejonge.

[frankdejonge]

You can construct a new local adapter, why do you need this?

[frankdejonge]

BTW, doing that ping stuff is rude in general, but especially if you do it with after creating your PR.

[Steveb-p]

You can construct a new local adapter, why do you need this?

Which is what we did. Unfortunately, we ended up with an exact copy, code for code, of the adapter that is here in the library, with tweaks related to the prefixer.
And extending the adapter would not work as well, because the Prefixer is both a private property (so we can't really replace it with a different one) and constructed in the constructor of the adapter (bar some reflection/closure access hacks and similar).

And then we would need to do it for each and every adapter, and instruct our developer-users to use our adapter instead of yours.

So, since we would like to not introduce technical debt to our solution, we would like to know if it's possible to expose the prefixer as a constructor argument (as in the PR). I have no idea what direction you want to take with the adapter, or if there is a specific thought behind not allowing the prefixer to be passed externally. I assumed you might want to get rid of it at some point, thinking it might be a remainder of some older code.

We could of course just run with the copy of an adapter, but then we would not benefit from any updates from your part (of course, flysystem is pretty much stable and does not really change in terms of adapters afaik, but still).

So that's the reason why we're reaching to you, and proposing to add Prefixer as constructor argument.

BTW, doing that ping stuff is rude in general, but especially if you do it with after creating your PR.

Sorry, we didn't mean to annoy you. Sorry for that 🙇

[frankdejonge]

@Steveb-p can you show me how you use this? Because in my experience, you do not need dynamic prefixed inside the adapter, but around it. Either for a dynamic prefix, you construct a new adapter. What I mean by this is not create an adapter in code, but rather create a new instance of the official adapter. That, or create your own wrapper which implements the adapter interface, which delegates to the official adapter. This doesn't require you to own any of the internal adapter code. The inner adapter would have a root of / and the wrapper changes the root, similar to how the PathPrefixing adapter works.

[alongosz]

Hello. Sorry for the late reply, I needed to check some things related to your response.

@Steveb-p can you show me how you use this? Because in my experience, you do not need dynamic prefixed inside the adapter, but around it. Either for a dynamic prefix, you construct a new adapter. What I mean by this is not create an adapter in code, but rather create a new instance of the official adapter.

We would have to either pre-compute all dynamic prefixes and create Adapter (and probably Filesystem) instances when compiling Symfony container or create Filesystem instance on the fly. It would involve replacing every call to an instance of FilesystemOperator Service with some sort of factory. I've explored a solution like that, but it has proven not to be very feasible, providing rather bad DX.

That, or create your own wrapper which implements the adapter interface, which delegates to the official adapter. This doesn't require you to own any of the internal adapter code. The inner adapter would have a root of / and the wrapper changes the root, similar to how the PathPrefixing adapter works.

Meaning something like:

use League\Flysystem\FilesystemAdapter;

class DynamicPathFilesystemAdapterDecorator implements FilesystemAdapter
{
    public function __construct(FilesystemAdapter $innerAdapter, MyPathPrefixer $prefixer)
    {
        $this->innerAdapter = $innerAdapter;
        $this->prefixer = $prefixer;
    }

    public function write(string $path, string $contents, Config $config): void
    {
        $path = $this->prefixer->prefixPath($path);

        $this->innerAdapter->write($path, $contents, $config);
    }
    
    // and so on...
}

where $innerAdapter is defined as:

use League\Flysystem\Local\LocalFilesystemAdapter;

$innerAdapter = new LocalFilesystemAdapter('/');

?

It could work, and produces a bit simpler extra code to maintain than re-definiting LocalFilesystemAdapter indeed, but doesn't setting inner path prefix as / defy the idea of path prefix itself?

We have paths like <some_root>/$var_dir$/$storage_dir$ where $var_dir$ and $storage_dir$ can be arbitrary - they're substituted on the fly based on a runtime context - and moreover system integrator can use there other so called "dynamic" settings. The thing is, that for us, business-domain-wise these are filesystem roots. Making PathPrefixer injectable seemed simpler to maintain, keeping that logic on our side. The background for the whole idea was that in Flysystem v1 we had overridden \League\Flysystem\Adapter\AbstractAdapter::setPathPrefix which was good enough.

Let me know please if the above solution is indeed the one you had in mind. In that case I guess we'll have to live with that :)

BTW, doing that ping stuff is rude in general, but especially if you do it with after creating your PR.

I'm sorry, it won't happen again. It's my experience, as OSS maintainer, that noticing a PR in one of our 60 packages without a ping is close to zero. But what works for us doesn't have to work for you, so I apologize for that.

[frankdejonge]

@alongosz the solution described above is indeed what I meant. Each piece has less concerns that way and is more easily maintainable, it also doesn't elevate the maintenance burden on me. I pretty much maintain all of Flysystem with all of the adapters and then some other libraries. To put it in perspective, some people have other decorators that expect the prefix never to change. If I allow path prefixing to change, they may want to insist on a setting that freezes it, or another mechanism to be notified on prefix changes. Instead, of people satisfy their own requirements, the general complexity stays low.

[alongosz]

Thanks for the feedback 👍 Closing this PR then.