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.

Sign up for GitHub

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

Use Laravel filesystem interface for assets and avatars #2729

Merged
merged 10 commits into from
Apr 19, 2021
Merged

Use Laravel filesystem interface for assets and avatars #2729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5e12cfe
WIP: Use Laravel filesystem interface where possible
askvortsov1 Mar 22, 2021
1f3f5df
Apply fixes from StyleCI
askvortsov1 Mar 22, 2021
1717615
Drop vendorFilesystem
askvortsov1 Mar 22, 2021
823c64f
Apply fixes from StyleCI
askvortsov1 Mar 22, 2021
4145a15
Support getting URL of cloud-based logo and favicon
askvortsov1 Mar 22, 2021
836f684
FilesystemAdapter should always be cloud
askvortsov1 Mar 22, 2021
66c53bf
Get base avatar URL from filesystem adapter
askvortsov1 Mar 22, 2021
ad20e97
Apply fixes from StyleCI
askvortsov1 Mar 22, 2021
657e33f
Restore deleted getAsset method
askvortsov1 Mar 23, 2021
92b2d72
Apply fixes from StyleCI
askvortsov1 Mar 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions src/Api/Controller/DeleteFaviconController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@
namespace Flarum\Api\Controller;

use Flarum\Settings\SettingsRepositoryInterface;
use Illuminate\Contracts\Filesystem\Factory;
use Illuminate\Contracts\Filesystem\Filesystem;
use Laminas\Diactoros\Response\EmptyResponse;
use League\Flysystem\FilesystemInterface;
use Psr\Http\Message\ServerRequestInterface;

class DeleteFaviconController extends AbstractDeleteController
Expand All @@ -22,18 +23,18 @@ class DeleteFaviconController extends AbstractDeleteController
protected $settings;

/**
* @var FilesystemInterface
* @var Filesystem
*/
protected $uploadDir;

/**
* @param SettingsRepositoryInterface $settings
* @param FilesystemInterface $uploadDir
* @param Factory $filesystemFactory
*/
public function __construct(SettingsRepositoryInterface $settings, FilesystemInterface $uploadDir)
public function __construct(SettingsRepositoryInterface $settings, Factory $filesystemFactory)
{
$this->settings = $settings;
$this->uploadDir = $uploadDir;
$this->uploadDir = $filesystemFactory->disk('flarum-assets');
}

/**
Expand All @@ -47,7 +48,7 @@ protected function delete(ServerRequestInterface $request)

$this->settings->set('favicon_path', null);

if ($this->uploadDir->has($path)) {
if ($this->uploadDir->exists($path)) {
$this->uploadDir->delete($path);
}

Expand Down
13 changes: 7 additions & 6 deletions src/Api/Controller/DeleteLogoController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@
namespace Flarum\Api\Controller;

use Flarum\Settings\SettingsRepositoryInterface;
use Illuminate\Contracts\Filesystem\Factory;
use Illuminate\Contracts\Filesystem\Filesystem;
use Laminas\Diactoros\Response\EmptyResponse;
use League\Flysystem\FilesystemInterface;
use Psr\Http\Message\ServerRequestInterface;

class DeleteLogoController extends AbstractDeleteController
Expand All @@ -22,18 +23,18 @@ class DeleteLogoController extends AbstractDeleteController
protected $settings;

/**
* @var FilesystemInterface
* @var Filesystem
*/
protected $uploadDir;

/**
* @param SettingsRepositoryInterface $settings
* @param FilesystemInterface $uploadDir
* @param Factory $filesystemFactory
*/
public function __construct(SettingsRepositoryInterface $settings, FilesystemInterface $uploadDir)
public function __construct(SettingsRepositoryInterface $settings, Factory $filesystemFactory)
{
$this->settings = $settings;
$this->uploadDir = $uploadDir;
$this->uploadDir = $filesystemFactory->disk('flarum-assets');
}

/**
Expand All @@ -47,7 +48,7 @@ protected function delete(ServerRequestInterface $request)

$this->settings->set('logo_path', null);

if ($this->uploadDir->has($path)) {
if ($this->uploadDir->exists($path)) {
$this->uploadDir->delete($path);
}

Expand Down
13 changes: 7 additions & 6 deletions src/Api/Controller/UploadImageController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@
namespace Flarum\Api\Controller;

use Flarum\Settings\SettingsRepositoryInterface;
use Illuminate\Contracts\Filesystem\Factory;
use Illuminate\Contracts\Filesystem\Filesystem;
use Illuminate\Support\Arr;
use Illuminate\Support\Str;
use Intervention\Image\Image;
use League\Flysystem\FilesystemInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Message\UploadedFileInterface;
use Tobscure\JsonApi\Document;
Expand All @@ -26,7 +27,7 @@ abstract class UploadImageController extends ShowForumController
protected $settings;

/**
* @var FilesystemInterface
* @var Filesystem
*/
protected $uploadDir;

Expand All @@ -47,12 +48,12 @@ abstract class UploadImageController extends ShowForumController

/**
* @param SettingsRepositoryInterface $settings
* @param FilesystemInterface $uploadDir
* @param Factory $filesystemFactory
*/
public function __construct(SettingsRepositoryInterface $settings, FilesystemInterface $uploadDir)
public function __construct(SettingsRepositoryInterface $settings, Factory $filesystemFactory)
{
$this->settings = $settings;
$this->uploadDir = $uploadDir;
$this->uploadDir = $filesystemFactory->disk('flarum-assets');
}

/**
Expand All @@ -72,7 +73,7 @@ public function data(ServerRequestInterface $request, Document $document)

$uploadName = $this->filenamePrefix.'-'.Str::lower(Str::random(8)).'.'.$this->fileExtension;

$this->uploadDir->write($uploadName, $encodedImage);
$this->uploadDir->put($uploadName, $encodedImage);

$this->settings->set($this->filePathSettingKey, $uploadName);

Expand Down
20 changes: 17 additions & 3 deletions src/Api/Serializer/ForumSerializer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
use Flarum\Foundation\Config;
use Flarum\Http\UrlGenerator;
use Flarum\Settings\SettingsRepositoryInterface;
use Illuminate\Contracts\Filesystem\Cloud;
use Illuminate\Contracts\Filesystem\Factory;

class ForumSerializer extends AbstractSerializer
{
Expand All @@ -36,14 +38,21 @@ class ForumSerializer extends AbstractSerializer
*/
protected $url;

/**
* @var Cloud
*/
protected $assetsFilesystem;

/**
* @param Config $config
* @param Factory $filesystemFactory
* @param SettingsRepositoryInterface $settings
* @param UrlGenerator $url
*/
public function __construct(Config $config, SettingsRepositoryInterface $settings, UrlGenerator $url)
public function __construct(Config $config, Factory $filesystemFactory, SettingsRepositoryInterface $settings, UrlGenerator $url)
{
$this->config = $config;
$this->assetsFilesystem = $filesystemFactory->disk('flarum-assets');
$this->settings = $settings;
$this->url = $url;
}
Expand Down Expand Up @@ -107,7 +116,7 @@ protected function getLogoUrl()
{
$logoPath = $this->settings->get('logo_path');

return $logoPath ? $this->url->to('forum')->path('assets/'.$logoPath) : null;
return $logoPath ? $this->getAssetUrl($logoPath) : null;
}

/**
Expand All @@ -117,6 +126,11 @@ protected function getFaviconUrl()
{
$faviconPath = $this->settings->get('favicon_path');

return $faviconPath ? $this->url->to('forum')->path('assets/'.$faviconPath) : null;
return $faviconPath ? $this->getAssetUrl($faviconPath) : null;
}

public function getAssetUrl($assetPath): string
{
return $this->assetsFilesystem->url($assetPath);
Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

}
}
20 changes: 7 additions & 13 deletions src/Extension/Extension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,12 @@
use Flarum\Database\Migrator;
use Flarum\Extend\LifecycleInterface;
use Illuminate\Contracts\Container\Container;
use Illuminate\Contracts\Filesystem\Filesystem as FilesystemInterface;
use Illuminate\Contracts\Support\Arrayable;
use Illuminate\Filesystem\Filesystem;
use Illuminate\Support\Arr;
use Illuminate\Support\Collection;
use Illuminate\Support\Str;
use League\Flysystem\Adapter\Local;
use League\Flysystem\Filesystem;
use League\Flysystem\FilesystemInterface;
use League\Flysystem\MountManager;
use League\Flysystem\Plugin\ListFiles;

/**
* @property string $name
Expand Down Expand Up @@ -428,16 +425,13 @@ public function copyAssetsTo(FilesystemInterface $target)
return;
}

$mount = new MountManager([
'source' => $source = new Filesystem(new Local($this->getPath().'/assets')),
'target' => $target,
]);
$source = new Filesystem();

$source->addPlugin(new ListFiles);
$assetFiles = $source->listFiles('/', true);
$assetFiles = $source->allFiles("$this->path/assets");

foreach ($assetFiles as $file) {
$mount->copy("source://$file[path]", "target://extensions/$this->id/$file[path]");
foreach ($assetFiles as $fullPath) {
$relPath = substr($fullPath, strlen("$this->path/assets"));
$target->put("extensions/$this->id/$relPath", $source->get($fullPath));
}
}

Expand Down
22 changes: 13 additions & 9 deletions src/Extension/ExtensionManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
use Flarum\Settings\SettingsRepositoryInterface;
use Illuminate\Contracts\Container\Container;
use Illuminate\Contracts\Events\Dispatcher;
use Illuminate\Contracts\Filesystem\Cloud as FilesystemInterface;
use Illuminate\Contracts\Filesystem\Factory;
use Illuminate\Database\ConnectionInterface;
use Illuminate\Database\Schema\Builder;
use Illuminate\Filesystem\Filesystem;
Expand Down Expand Up @@ -48,6 +50,11 @@ class ExtensionManager
*/
protected $filesystem;

/**
* @var FilesystemInterface
*/
protected $assetsFilesystem;

/**
* @var Collection|null
*/
Expand All @@ -59,14 +66,16 @@ public function __construct(
Container $container,
Migrator $migrator,
Dispatcher $dispatcher,
Filesystem $filesystem
Filesystem $filesystem,
Factory $filesystemFactory
) {
$this->config = $config;
$this->paths = $paths;
$this->container = $container;
$this->migrator = $migrator;
$this->dispatcher = $dispatcher;
$this->filesystem = $filesystem;
$this->assetsFilesystem = $filesystemFactory->disk('flarum-assets');
}

/**
Expand Down Expand Up @@ -252,12 +261,7 @@ public function uninstall($name)
*/
protected function publishAssets(Extension $extension)
{
if ($extension->hasAssets()) {
$this->filesystem->copyDirectory(
$extension->getPath().'/assets',
$this->paths->public.'/assets/extensions/'.$extension->getId()
);
}
$extension->copyAssetsTo($this->assetsFilesystem);
}

/**
Expand All @@ -267,7 +271,7 @@ protected function publishAssets(Extension $extension)
*/
protected function unpublishAssets(Extension $extension)
{
$this->filesystem->deleteDirectory($this->paths->public.'/assets/extensions/'.$extension->getId());
$this->assetsFilesystem->deleteDirectory('extensions/'.$extension->getId());
}

/**
Expand All @@ -279,7 +283,7 @@ protected function unpublishAssets(Extension $extension)
*/
public function getAsset(Extension $extension, $path)
{
return $this->paths->public.'/assets/extensions/'.$extension->getId().$path;
return $this->assetsFilesystem->url($extension->getId()."/$path");
Comment on lines -282 to +286
Copy link
Member

@SychO9 SychO9 Apr 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@luceos luceos Apr 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

/**
Expand Down
3 changes: 2 additions & 1 deletion src/Foundation/InstalledSite.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ protected function getIlluminateConfig(Application $app)
],
'flarum-avatars' => [
'driver' => 'local',
'root' => $this->paths->public.'/assets/avatars'
'root' => $this->paths->public.'/assets/avatars',
'url' => $app->url('assets/avatars')
]
]
],
Expand Down
3 changes: 2 additions & 1 deletion src/Install/Steps/EnableBundledExtensions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Flarum\Install\Step;
use Flarum\Settings\DatabaseSettingsRepository;
use Illuminate\Database\ConnectionInterface;
use Illuminate\Filesystem\FilesystemAdapter;
use Illuminate\Support\Arr;
use Illuminate\Support\Collection;
use League\Flysystem\Adapter\Local;
Expand Down Expand Up @@ -56,7 +57,7 @@ public function run()
foreach ($extensions as $extension) {
$extension->migrate($this->getMigrator());
$extension->copyAssetsTo(
new Filesystem(new Local($this->assetPath))
new FilesystemAdapter(new Filesystem(new Local($this->assetPath)))
);
}

Expand Down
20 changes: 0 additions & 20 deletions src/Settings/SettingsServiceProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,8 @@

namespace Flarum\Settings;

use Flarum\Api\Controller\DeleteFaviconController;
use Flarum\Api\Controller\DeleteLogoController;
use Flarum\Api\Controller\UploadFaviconController;
use Flarum\Api\Controller\UploadLogoController;
use Flarum\Foundation\AbstractServiceProvider;
use Illuminate\Contracts\Container\Container;
use Illuminate\Contracts\Filesystem\Factory;
use Illuminate\Database\ConnectionInterface;
use League\Flysystem\FilesystemInterface;

class SettingsServiceProvider extends AbstractServiceProvider
{
Expand All @@ -35,18 +28,5 @@ public function register()
});

$this->container->alias(SettingsRepositoryInterface::class, 'flarum.settings');

$assets = function (Container $container) {
return $container->make(Factory::class)->disk('flarum-assets')->getDriver();
};

$this->container->when([
DeleteFaviconController::class,
DeleteLogoController::class,
UploadFaviconController::class,
UploadLogoController::class,
])
->needs(FilesystemInterface::class)
->give($assets);
}
}
12 changes: 8 additions & 4 deletions src/User/AvatarUploader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,21 @@

namespace Flarum\User;

use Illuminate\Contracts\Filesystem\Factory;
use Illuminate\Contracts\Filesystem\Filesystem;
use Illuminate\Support\Str;
use Intervention\Image\Image;
use League\Flysystem\FilesystemInterface;

class AvatarUploader
{
/**
* @var Filesystem
*/
protected $uploadDir;

public function __construct(FilesystemInterface $uploadDir)
public function __construct(Factory $filesystemFactory)
{
$this->uploadDir = $uploadDir;
$this->uploadDir = $filesystemFactory->disk('flarum-avatars');
}

/**
Expand Down Expand Up @@ -52,7 +56,7 @@ protected function removeFileAfterSave(User $user)
$avatarPath = $user->getRawOriginal('avatar_url');

$user->afterSave(function () use ($avatarPath) {
if ($this->uploadDir->has($avatarPath)) {
if ($this->uploadDir->exists($avatarPath)) {
$this->uploadDir->delete($avatarPath);
}
});
Expand Down
Loading