From 3e173afa2444f37933e2efbdd3e1ef73cb2d78a1 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com> Date: Mon, 19 Apr 2021 15:11:03 -0400 Subject: [PATCH] Use Laravel filesystem interface for assets and avatars (#2729) * WIP: Use Laravel filesystem interface where possible * Drop vendorFilesystem * Support getting URL of cloud-based logo and favicon * FilesystemAdapter should always be cloud * Get base avatar URL from filesystem adapter * Restore deleted getAsset method Co-authored-by: Alexander Skvortsov --- .../Controller/DeleteFaviconController.php | 13 ++++++----- .../Api/Controller/DeleteLogoController.php | 13 ++++++----- .../Api/Controller/UploadImageController.php | 13 ++++++----- .../src/Api/Serializer/ForumSerializer.php | 20 ++++++++++++++--- framework/core/src/Extension/Extension.php | 20 ++++++----------- .../core/src/Extension/ExtensionManager.php | 22 +++++++++++-------- .../core/src/Foundation/InstalledSite.php | 3 ++- .../Install/Steps/EnableBundledExtensions.php | 3 ++- .../src/Settings/SettingsServiceProvider.php | 20 ----------------- framework/core/src/User/AvatarUploader.php | 12 ++++++---- framework/core/src/User/User.php | 4 ++-- .../core/src/User/UserServiceProvider.php | 15 ------------- .../tests/unit/User/AvatarUploaderTest.php | 16 +++++++++----- 13 files changed, 82 insertions(+), 92 deletions(-) diff --git a/framework/core/src/Api/Controller/DeleteFaviconController.php b/framework/core/src/Api/Controller/DeleteFaviconController.php index 9a3a14cc1..4afd9324d 100644 --- a/framework/core/src/Api/Controller/DeleteFaviconController.php +++ b/framework/core/src/Api/Controller/DeleteFaviconController.php @@ -11,8 +11,9 @@ namespace Flarum\Api\Controller; use Flarum\Http\RequestUtil; 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 @@ -23,18 +24,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'); } /** @@ -48,7 +49,7 @@ class DeleteFaviconController extends AbstractDeleteController $this->settings->set('favicon_path', null); - if ($this->uploadDir->has($path)) { + if ($this->uploadDir->exists($path)) { $this->uploadDir->delete($path); } diff --git a/framework/core/src/Api/Controller/DeleteLogoController.php b/framework/core/src/Api/Controller/DeleteLogoController.php index e443a85b0..121698c6d 100644 --- a/framework/core/src/Api/Controller/DeleteLogoController.php +++ b/framework/core/src/Api/Controller/DeleteLogoController.php @@ -11,8 +11,9 @@ namespace Flarum\Api\Controller; use Flarum\Http\RequestUtil; 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 @@ -23,18 +24,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'); } /** @@ -48,7 +49,7 @@ class DeleteLogoController extends AbstractDeleteController $this->settings->set('logo_path', null); - if ($this->uploadDir->has($path)) { + if ($this->uploadDir->exists($path)) { $this->uploadDir->delete($path); } diff --git a/framework/core/src/Api/Controller/UploadImageController.php b/framework/core/src/Api/Controller/UploadImageController.php index d0558adfd..7794c59e6 100644 --- a/framework/core/src/Api/Controller/UploadImageController.php +++ b/framework/core/src/Api/Controller/UploadImageController.php @@ -11,10 +11,11 @@ namespace Flarum\Api\Controller; use Flarum\Http\RequestUtil; 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; @@ -27,7 +28,7 @@ abstract class UploadImageController extends ShowForumController protected $settings; /** - * @var FilesystemInterface + * @var Filesystem */ protected $uploadDir; @@ -48,12 +49,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'); } /** @@ -73,7 +74,7 @@ abstract class UploadImageController extends ShowForumController $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); diff --git a/framework/core/src/Api/Serializer/ForumSerializer.php b/framework/core/src/Api/Serializer/ForumSerializer.php index 65d53b3fa..e0681c60d 100644 --- a/framework/core/src/Api/Serializer/ForumSerializer.php +++ b/framework/core/src/Api/Serializer/ForumSerializer.php @@ -13,6 +13,8 @@ use Flarum\Foundation\Application; 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 { @@ -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; } @@ -107,7 +116,7 @@ class ForumSerializer extends AbstractSerializer { $logoPath = $this->settings->get('logo_path'); - return $logoPath ? $this->url->to('forum')->path('assets/'.$logoPath) : null; + return $logoPath ? $this->getAssetUrl($logoPath) : null; } /** @@ -117,6 +126,11 @@ class ForumSerializer extends AbstractSerializer { $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); } } diff --git a/framework/core/src/Extension/Extension.php b/framework/core/src/Extension/Extension.php index c016d161a..46a9f869d 100644 --- a/framework/core/src/Extension/Extension.php +++ b/framework/core/src/Extension/Extension.php @@ -12,15 +12,12 @@ namespace Flarum\Extension; 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 @@ -428,16 +425,13 @@ class Extension implements Arrayable 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)); } } diff --git a/framework/core/src/Extension/ExtensionManager.php b/framework/core/src/Extension/ExtensionManager.php index a7c16813d..50f2b127a 100644 --- a/framework/core/src/Extension/ExtensionManager.php +++ b/framework/core/src/Extension/ExtensionManager.php @@ -19,6 +19,8 @@ use Flarum\Foundation\Paths; 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; @@ -48,6 +50,11 @@ class ExtensionManager */ protected $filesystem; + /** + * @var FilesystemInterface + */ + protected $assetsFilesystem; + /** * @var Collection|null */ @@ -59,7 +66,8 @@ class ExtensionManager Container $container, Migrator $migrator, Dispatcher $dispatcher, - Filesystem $filesystem + Filesystem $filesystem, + Factory $filesystemFactory ) { $this->config = $config; $this->paths = $paths; @@ -67,6 +75,7 @@ class ExtensionManager $this->migrator = $migrator; $this->dispatcher = $dispatcher; $this->filesystem = $filesystem; + $this->assetsFilesystem = $filesystemFactory->disk('flarum-assets'); } /** @@ -252,12 +261,7 @@ class ExtensionManager */ 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); } /** @@ -267,7 +271,7 @@ class ExtensionManager */ protected function unpublishAssets(Extension $extension) { - $this->filesystem->deleteDirectory($this->paths->public.'/assets/extensions/'.$extension->getId()); + $this->assetsFilesystem->deleteDirectory('extensions/'.$extension->getId()); } /** @@ -279,7 +283,7 @@ class ExtensionManager */ public function getAsset(Extension $extension, $path) { - return $this->paths->public.'/assets/extensions/'.$extension->getId().$path; + return $this->assetsFilesystem->url($extension->getId()."/$path"); } /** diff --git a/framework/core/src/Foundation/InstalledSite.php b/framework/core/src/Foundation/InstalledSite.php index ec52fb9cc..ec0bd8f51 100644 --- a/framework/core/src/Foundation/InstalledSite.php +++ b/framework/core/src/Foundation/InstalledSite.php @@ -177,7 +177,8 @@ class InstalledSite implements SiteInterface ], 'flarum-avatars' => [ 'driver' => 'local', - 'root' => $this->paths->public.'/assets/avatars' + 'root' => $this->paths->public.'/assets/avatars', + 'url' => $app->url('assets/avatars') ] ] ], diff --git a/framework/core/src/Install/Steps/EnableBundledExtensions.php b/framework/core/src/Install/Steps/EnableBundledExtensions.php index 496a44fe8..d77a6109f 100644 --- a/framework/core/src/Install/Steps/EnableBundledExtensions.php +++ b/framework/core/src/Install/Steps/EnableBundledExtensions.php @@ -15,6 +15,7 @@ use Flarum\Extension\Extension; 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; @@ -79,7 +80,7 @@ class EnableBundledExtensions implements Step foreach ($extensions as $extension) { $extension->migrate($this->getMigrator()); $extension->copyAssetsTo( - new Filesystem(new Local($this->assetPath)) + new FilesystemAdapter(new Filesystem(new Local($this->assetPath))) ); } diff --git a/framework/core/src/Settings/SettingsServiceProvider.php b/framework/core/src/Settings/SettingsServiceProvider.php index f7378cf31..29ab6bd95 100644 --- a/framework/core/src/Settings/SettingsServiceProvider.php +++ b/framework/core/src/Settings/SettingsServiceProvider.php @@ -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 { @@ -35,18 +28,5 @@ class SettingsServiceProvider extends AbstractServiceProvider }); $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); } } diff --git a/framework/core/src/User/AvatarUploader.php b/framework/core/src/User/AvatarUploader.php index 2071664b1..ec66511eb 100644 --- a/framework/core/src/User/AvatarUploader.php +++ b/framework/core/src/User/AvatarUploader.php @@ -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'); } /** @@ -52,7 +56,7 @@ class AvatarUploader $avatarPath = $user->getRawOriginal('avatar_url'); $user->afterSave(function () use ($avatarPath) { - if ($this->uploadDir->has($avatarPath)) { + if ($this->uploadDir->exists($avatarPath)) { $this->uploadDir->delete($avatarPath); } }); diff --git a/framework/core/src/User/User.php b/framework/core/src/User/User.php index 110f5d2e3..dcf8f08db 100644 --- a/framework/core/src/User/User.php +++ b/framework/core/src/User/User.php @@ -18,7 +18,6 @@ use Flarum\Foundation\EventGeneratorTrait; use Flarum\Group\Group; use Flarum\Group\Permission; use Flarum\Http\AccessToken; -use Flarum\Http\UrlGenerator; use Flarum\Notification\Notification; use Flarum\Post\Post; use Flarum\User\DisplayName\DriverInterface; @@ -32,6 +31,7 @@ use Flarum\User\Event\Registered; use Flarum\User\Event\Renamed; use Flarum\User\Exception\NotAuthenticatedException; use Flarum\User\Exception\PermissionDeniedException; +use Illuminate\Contracts\Filesystem\Factory; use Illuminate\Contracts\Hashing\Hasher; use Illuminate\Support\Arr; @@ -312,7 +312,7 @@ class User extends AbstractModel public function getAvatarUrlAttribute(string $value = null) { if ($value && strpos($value, '://') === false) { - return app(UrlGenerator::class)->to('forum')->path('assets/avatars/'.$value); + return resolve(Factory::class)->disk('flarum-avatars')->url($value); } return $value; diff --git a/framework/core/src/User/UserServiceProvider.php b/framework/core/src/User/UserServiceProvider.php index c5d5a3665..558f61d8d 100644 --- a/framework/core/src/User/UserServiceProvider.php +++ b/framework/core/src/User/UserServiceProvider.php @@ -24,10 +24,7 @@ use Flarum\User\DisplayName\UsernameDriver; use Flarum\User\Event\EmailChangeRequested; use Flarum\User\Event\Registered; use Flarum\User\Event\Saving; -use Illuminate\Contracts\Container\Container; -use Illuminate\Contracts\Filesystem\Factory; use Illuminate\Support\Arr; -use League\Flysystem\FilesystemInterface; class UserServiceProvider extends AbstractServiceProvider { @@ -36,7 +33,6 @@ class UserServiceProvider extends AbstractServiceProvider */ public function register() { - $this->registerAvatarsFilesystem(); $this->registerDisplayNameDrivers(); $this->registerPasswordCheckers(); @@ -78,17 +74,6 @@ class UserServiceProvider extends AbstractServiceProvider $this->container->alias('flarum.user.display_name.driver', DriverInterface::class); } - protected function registerAvatarsFilesystem() - { - $avatarsFilesystem = function (Container $container) { - return $container->make(Factory::class)->disk('flarum-avatars')->getDriver(); - }; - - $this->container->when(AvatarUploader::class) - ->needs(FilesystemInterface::class) - ->give($avatarsFilesystem); - } - protected function registerPasswordCheckers() { $this->container->singleton('flarum.user.password_checkers', function () { diff --git a/framework/core/tests/unit/User/AvatarUploaderTest.php b/framework/core/tests/unit/User/AvatarUploaderTest.php index 75bdd0a12..0252f9f4c 100644 --- a/framework/core/tests/unit/User/AvatarUploaderTest.php +++ b/framework/core/tests/unit/User/AvatarUploaderTest.php @@ -13,15 +13,17 @@ use Flarum\Testing\unit\TestCase; use Flarum\User\AvatarUploader; use Flarum\User\User; use Illuminate\Contracts\Events\Dispatcher; +use Illuminate\Contracts\Filesystem\Factory; +use Illuminate\Contracts\Filesystem\Filesystem; use Illuminate\Database\Eloquent\Model; use Intervention\Image\ImageManagerStatic; -use League\Flysystem\FilesystemInterface; use Mockery as m; class AvatarUploaderTest extends TestCase { private $dispatcher; private $filesystem; + private $filesystemFactory; private $uploader; /** @@ -33,13 +35,15 @@ class AvatarUploaderTest extends TestCase $this->dispatcher->shouldIgnoreMissing(); Model::setEventDispatcher($this->dispatcher); - $this->filesystem = m::mock(FilesystemInterface::class); - $this->uploader = new AvatarUploader($this->filesystem); + $this->filesystem = m::mock(Filesystem::class); + $this->filesystemFactory = m::mock(Factory::class); + $this->filesystemFactory->shouldReceive('disk')->with('flarum-avatars')->andReturn($this->filesystem); + $this->uploader = new AvatarUploader($this->filesystemFactory); } public function test_removing_avatar_removes_file() { - $this->filesystem->shouldReceive('has')->with('ABCDEFGHabcdefgh.png')->andReturn(true); + $this->filesystem->shouldReceive('exists')->with('ABCDEFGHabcdefgh.png')->andReturn(true); $this->filesystem->shouldReceive('delete')->with('ABCDEFGHabcdefgh.png')->once(); $user = new User(); @@ -61,7 +65,7 @@ class AvatarUploaderTest extends TestCase public function test_removing_url_avatar_removes_no_file() { - $this->filesystem->shouldReceive('has')->with('https://example.com/avatar.png')->andReturn(false)->once(); + $this->filesystem->shouldReceive('exists')->with('https://example.com/avatar.png')->andReturn(false)->once(); $this->filesystem->shouldNotReceive('delete'); $user = new User(); @@ -82,7 +86,7 @@ class AvatarUploaderTest extends TestCase public function test_changing_avatar_removes_file() { $this->filesystem->shouldReceive('put')->once(); - $this->filesystem->shouldReceive('has')->with('ABCDEFGHabcdefgh.png')->andReturn(true); + $this->filesystem->shouldReceive('exists')->with('ABCDEFGHabcdefgh.png')->andReturn(true); $this->filesystem->shouldReceive('delete')->with('ABCDEFGHabcdefgh.png')->once(); $user = new User();