Use Laravel filesystem interface for assets and avatars

[askvortsov1]

Part of #1783
Part of flarum/issue-archive#121

Changes proposed in this pull request:
This PR refactors filesystem-involved code to use Laravel filesystem disks instead of the underlying Flysystem adaptors. I've refactored code relating to assets and avatars. Storage and cache will need to wait for a different PR.

I also considered introducing a "vendor filesystem". On paper it sounds nice, but there's too many places where we assume code will be located in the local filesystem. That's also a very fair assumption IMO. See 5e12cfe for how it looks with a partial transition to a vendor filesystem. I decided to revert that change, it wasn't a good solution IMO.

Reviewers should focus on:

• MigrateCommand includes copying fontawesome font files to the assets/fonts folder. I would propose leaving this unchanged for now, and creating a separate PublishAssets command that would include that step, as well as publishing assets for all extensions. Source: https://github.com/flarum/core/blob/171761587b8fdbf5cca48f5ea7659960183bacf6/src/Database/Console/MigrateCommand.php#L94-L94

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).

[askvortsov1]

@luceos Is this type of code what you were referring to when we talked about potential lag due to indexing the other day? It's a very clean and convenient way to get what we need, but if it might result in lag, that's something we should take into account.

[franzliedke]

Yay for a publish:assets command. 👏🏼

A more general question, though: If the goal is to fix #1783, is this really the only way to achieve this? Injecting a filesystem (as the old code does) still allows S3 or other backends to be configured, and it would be a less invasive change. Injecting a factory means that more knowledge about the concept of "disks" and how Flarum configures itself is littered throughout the codebase. Plus, a higher grade of coupling to Laravel which I spent a lot of time on reducing. 🙈

IMHO, this is service provider territory (mapping configuration to objects / instances). This coud be done through contextual bindings as you outlined in #1783 (comment), or similar to this:

$container->singleton('flarum.disk.avatars', function () {
    return new S3Filesystem();
});

// For clients that need this filesystem, use contextual binding or concrete factories:
$container->bind(MyController::Class, function ($app) {
    return new MyController('other', 'args', $app['flarum.disk.avatars']);
});

[askvortsov1]

I've thought about this, but I'm not sure there's a good solution. I would strongly prefer a solution where extensions can use the filesystem without having to register custom service providers. As you pointed out in #2437 (comment), service providers aren't use case driven. IMO, they should be used only when truly appropriate, EG by overriding bindings.

I also don't think it would be appropriate to have per-disk interfaces, as that doesn't scale too well (although it's possible, interfaces could extend Filesystem, and that could be used instead of disk string names).

To be honest, I think I prefer the current approach of injecting a disk manager (and obtaining the relevant disk) to the old system of injecting a Filesystem, for several reasons:

• Scalability to extensions, but this I've already said
• I think the question of "which filesystem?" is one that should be handled at the business logic layer, not the service provider layer. A class might need multiple disks at once, or it might need disks after instantiation (like with ExtensionManager). But even without those issues, the identity of the disk to which files are being written feels closer to logic than config. And the logic is already aware of disks; that's what it's currently receiving from the constructor. This approach just means that the class in question has explicit knowledge of which disk it's writing to; we haven't moved any of the disk construction or configuration (those, I agree, are service provider territory). Sorry, this point got a bit rambly.
• Unlike Illuminate\Filesystem\FilesystemManager (which is very implementation-heavy), Illuminate\Contracts\Filesystem\Factory is extremely simple: All it defines is a single disk method for fetching the appropriate disks. Given the above 2 points, I think this is pretty clean: classes declare that they need to access the filesystem via constructor argument, and get the disks they actually need in constructor logic.
• There's less confusion with Illuminate\Filesystem\Filesystem, which is also injected at times.
• For the most part, non-installer logic doesn't need to interact directly with the backing Flysystem adapters at all. There's one usage of NullAdapter left in the less validation logic. But even that we could swap out with a flarum-null disk (which might be a good idea now that I think about it).
• Laravel's APIs make it easier to get file URLs.

[luceos]

Over the years I have become less snappy to get a complete Illuminate component into Flarum due to the wisdom of among other Franz. But in this scenario, looking at the problems I tried to resolve using the fof filesystem drivers package, I can only agree that using disks makes for much cleaner and useful code.

[SychO9]

I can't find where this method is used to properly tell, but aren't we replacing a path with a url here ?

[askvortsov1]

This isn't used. Frankly I'm not sure why we even have it. I removed it in the first version of this PR, but reverted that change as it was a bit too spontaneous.

If assets aren't always on disk, then the URL to assets won't always be a path. If we want to keep it, I'll update the docblock.

[tankerkiller125]

I think it was used in very early versions of Flarum, today though I don't think that anyone uses it. I think that it would be safe to remove this.

[luceos]

I don't think this can be removed, @askvortsov1 is onto something here.

In case you use an S3 bucket to store assets files to your public directory you need to be able to resolve the actual, full URL that the asset is published as. Disks in Laravel support this functionality using the url method.

However, I don't think any extension currently is using it and that might be a problem in itself 😂

[askvortsov1]

Let's tackle the promotion or redesign of this feature separately, then.

[askvortsov1]

TODO: document that adapters for assets should generate the URL without pinging the remote filesystem