From 88366fe8af3baa566ad625743016acb85a0cf345 Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Fri, 19 Jun 2020 22:16:03 +0200 Subject: [PATCH] Clean up usages / deprecate path helpers (#2155) * Write source map without creating temp file Less I/O, and one less place where we access the global path helpers. * Drop useless app_path() helper This was probably taken straight from Laravel. There is no equivalent concept in Flarum, so this should be safe to remove. * Deprecate global path helpers Developers using these helpers can inject the `Paths` class instead. * Stop storing paths as strings in container * Avoid using path helpers from Application class * Deprecate path helpers from Application class * Avoid using public_path() in prerequisite check a) The comparison was already outdated, as a different path was passed. b) We're trying to get rid of these global helpers. --- src/Formatter/FormatterServiceProvider.php | 3 +- src/Foundation/Application.php | 18 ++--- src/Frontend/Compiler/JsCompiler.php | 10 +-- src/Frontend/FrontendServiceProvider.php | 7 +- src/Install/InstallServiceProvider.php | 9 --- src/Install/Installation.php | 31 ++++---- src/Install/Prerequisite/WritablePaths.php | 31 ++++++-- src/Locale/LocaleServiceProvider.php | 3 +- src/Queue/QueueServiceProvider.php | 3 +- src/helpers.php | 23 ++---- tests/fixtures/writable_paths/writable/.keep | 0 tests/integration/setup.php | 11 +-- .../Prerequisite/WritablePathsTest.php | 71 +++++++++++++++++++ 13 files changed, 143 insertions(+), 77 deletions(-) create mode 100644 tests/fixtures/writable_paths/writable/.keep create mode 100644 tests/unit/Install/Prerequisite/WritablePathsTest.php diff --git a/src/Formatter/FormatterServiceProvider.php b/src/Formatter/FormatterServiceProvider.php index 31d48051d..31aef917a 100644 --- a/src/Formatter/FormatterServiceProvider.php +++ b/src/Formatter/FormatterServiceProvider.php @@ -10,6 +10,7 @@ namespace Flarum\Formatter; use Flarum\Foundation\AbstractServiceProvider; +use Flarum\Foundation\Paths; use Illuminate\Cache\Repository; use Illuminate\Contracts\Container\Container; @@ -24,7 +25,7 @@ class FormatterServiceProvider extends AbstractServiceProvider return new Formatter( new Repository($container->make('cache.filestore')), $container->make('events'), - $this->app['flarum']->storagePath().'/formatter' + $this->app[Paths::class]->storage.'/formatter' ); }); diff --git a/src/Foundation/Application.php b/src/Foundation/Application.php index 4c91de121..7463b2f50 100644 --- a/src/Foundation/Application.php +++ b/src/Foundation/Application.php @@ -86,8 +86,6 @@ class Application $this->registerBaseBindings(); $this->registerBaseServiceProviders(); $this->registerCoreContainerAliases(); - - $this->bindPathsInContainer(); } /** @@ -161,22 +159,11 @@ class Application $this->register(new EventServiceProvider($this->container)); } - /** - * Bind all of the application paths in the container. - * - * @return void - */ - protected function bindPathsInContainer() - { - foreach (['base', 'public', 'storage', 'vendor'] as $path) { - $this->container->instance('path.'.$path, $this->paths->$path); - } - } - /** * Get the base path of the Laravel installation. * * @return string + * @deprecated Will be removed in Beta.15. */ public function basePath() { @@ -187,6 +174,7 @@ class Application * Get the path to the public / web directory. * * @return string + * @deprecated Will be removed in Beta.15. */ public function publicPath() { @@ -197,6 +185,7 @@ class Application * Get the path to the storage directory. * * @return string + * @deprecated Will be removed in Beta.15. */ public function storagePath() { @@ -207,6 +196,7 @@ class Application * Get the path to the vendor directory where dependencies are installed. * * @return string + * @deprecated Will be removed in Beta.15. */ public function vendorPath() { diff --git a/src/Frontend/Compiler/JsCompiler.php b/src/Frontend/Compiler/JsCompiler.php index 01ebd0386..1549ecc61 100644 --- a/src/Frontend/Compiler/JsCompiler.php +++ b/src/Frontend/Compiler/JsCompiler.php @@ -50,16 +50,12 @@ class JsCompiler extends RevisionCompiler } // Add a comment to the end of our file to point to the sourcemap - // we just constructed. We will then write the JS file, save the - // map to a temporary location, and then move it to the asset dir. + // we just constructed. We will then store the JS file and the map + // in our asset directory. $output[] = '//# sourceMappingURL='.$this->assetsDir->url($mapFile); $this->assetsDir->put($file, implode("\n", $output)); - - $mapTemp = @tempnam(storage_path('tmp'), $mapFile); - $map->save($mapTemp); - $this->assetsDir->put($mapFile, file_get_contents($mapTemp)); - @unlink($mapTemp); + $this->assetsDir->put($mapFile, json_encode($map, JSON_UNESCAPED_SLASHES)); return true; } diff --git a/src/Frontend/FrontendServiceProvider.php b/src/Frontend/FrontendServiceProvider.php index 6ba2980c3..088d3e2e7 100644 --- a/src/Frontend/FrontendServiceProvider.php +++ b/src/Frontend/FrontendServiceProvider.php @@ -10,6 +10,7 @@ namespace Flarum\Frontend; use Flarum\Foundation\AbstractServiceProvider; +use Flarum\Foundation\Paths; use Flarum\Frontend\Compiler\Source\SourceCollector; use Flarum\Http\UrlGenerator; use Flarum\Settings\SettingsRepositoryInterface; @@ -21,14 +22,16 @@ class FrontendServiceProvider extends AbstractServiceProvider { $this->app->singleton('flarum.assets.factory', function () { return function (string $name) { + $paths = $this->app[Paths::class]; + $assets = new Assets( $name, $this->app->make('filesystem')->disk('flarum-assets'), - $this->app['flarum']->storagePath() + $paths->storage ); $assets->setLessImportDirs([ - $this->app['flarum']->vendorPath().'/components/font-awesome/less' => '' + $paths->vendor.'/components/font-awesome/less' => '' ]); $assets->css([$this, 'addBaseCss']); diff --git a/src/Install/InstallServiceProvider.php b/src/Install/InstallServiceProvider.php index dea076ac3..90a0648fd 100644 --- a/src/Install/InstallServiceProvider.php +++ b/src/Install/InstallServiceProvider.php @@ -23,15 +23,6 @@ class InstallServiceProvider extends AbstractServiceProvider $this->app->singleton('flarum.install.routes', function () { return new RouteCollection; }); - - $this->app->singleton(Installation::class, function () { - return new Installation( - $this->app->basePath(), - $this->app->publicPath(), - $this->app->storagePath(), - $this->app->vendorPath() - ); - }); } /** diff --git a/src/Install/Installation.php b/src/Install/Installation.php index ed3f6f263..8ddd58250 100644 --- a/src/Install/Installation.php +++ b/src/Install/Installation.php @@ -9,12 +9,14 @@ namespace Flarum\Install; +use Flarum\Foundation\Paths; + class Installation { - private $basePath; - private $publicPath; - private $storagePath; - private $vendorPath; + /** + * @var Paths + */ + private $paths; private $configPath; private $debug = false; @@ -34,12 +36,9 @@ class Installation /** @var \Illuminate\Database\ConnectionInterface */ private $db; - public function __construct($basePath, $publicPath, $storagePath, $vendorPath) + public function __construct(Paths $paths) { - $this->basePath = $basePath; - $this->publicPath = $publicPath; - $this->storagePath = $storagePath; - $this->vendorPath = $vendorPath; + $this->paths = $paths; } public function configPath($path) @@ -98,9 +97,9 @@ class Installation 'tokenizer', ]), new Prerequisite\WritablePaths([ - $this->basePath, - $this->getAssetPath(), - $this->storagePath, + $this->paths->base, + $this->getAssetPath().'/*', + $this->paths->storage, ]) ); } @@ -140,11 +139,11 @@ class Installation }); $pipeline->pipe(function () { - return new Steps\PublishAssets($this->vendorPath, $this->getAssetPath()); + return new Steps\PublishAssets($this->paths->vendor, $this->getAssetPath()); }); $pipeline->pipe(function () { - return new Steps\EnableBundledExtensions($this->db, $this->vendorPath, $this->getAssetPath()); + return new Steps\EnableBundledExtensions($this->db, $this->paths->vendor, $this->getAssetPath()); }); return $pipeline; @@ -152,12 +151,12 @@ class Installation private function getConfigPath() { - return $this->basePath.'/'.($this->configPath ?? 'config.php'); + return $this->paths->base.'/'.($this->configPath ?? 'config.php'); } private function getAssetPath() { - return "$this->publicPath/assets"; + return $this->paths->public.'/assets'; } private function getMigrationPath() diff --git a/src/Install/Prerequisite/WritablePaths.php b/src/Install/Prerequisite/WritablePaths.php index 945402a04..5dcbbf8a4 100644 --- a/src/Install/Prerequisite/WritablePaths.php +++ b/src/Install/Prerequisite/WritablePaths.php @@ -10,14 +10,20 @@ namespace Flarum\Install\Prerequisite; use Illuminate\Support\Collection; +use Illuminate\Support\Str; class WritablePaths implements PrerequisiteInterface { - protected $paths; + /** + * @var Collection + */ + private $paths; + + private $wildcards = []; public function __construct(array $paths) { - $this->paths = $paths; + $this->paths = $this->normalize($paths); } public function problems(): Collection @@ -28,7 +34,7 @@ class WritablePaths implements PrerequisiteInterface private function getMissingPaths(): Collection { - return (new Collection($this->paths)) + return $this->paths ->reject(function ($path) { return file_exists($path); })->map(function ($path) { @@ -41,13 +47,13 @@ class WritablePaths implements PrerequisiteInterface private function getNonWritablePaths(): Collection { - return (new Collection($this->paths)) + return $this->paths ->filter(function ($path) { return file_exists($path) && ! is_writable($path); - })->map(function ($path) { + })->map(function ($path, $index) { return [ 'message' => 'The '.$this->getAbsolutePath($path).' directory is not writable.', - 'detail' => 'Please chmod this directory'.($path !== public_path() ? ' and its contents' : '').' to 0775.' + 'detail' => 'Please chmod this directory'.(in_array($index, $this->wildcards) ? ' and its contents' : '').' to 0775.' ]; }); } @@ -71,4 +77,17 @@ class WritablePaths implements PrerequisiteInterface return (substr($path, 0, 1) == '/' ? '/' : '').implode(DIRECTORY_SEPARATOR, $absolutes); } + + private function normalize(array $paths): Collection + { + return (new Collection($paths)) + ->map(function ($path, $index) { + if (Str::endsWith($path, '/*')) { + $this->wildcards[] = $index; + $path = substr($path, 0, -2); + } + + return $path; + }); + } } diff --git a/src/Locale/LocaleServiceProvider.php b/src/Locale/LocaleServiceProvider.php index 0ff5360b4..05ff17b2e 100644 --- a/src/Locale/LocaleServiceProvider.php +++ b/src/Locale/LocaleServiceProvider.php @@ -11,6 +11,7 @@ namespace Flarum\Locale; use Flarum\Foundation\AbstractServiceProvider; use Flarum\Foundation\Event\ClearingCache; +use Flarum\Foundation\Paths; use Flarum\Settings\SettingsRepositoryInterface; use Illuminate\Contracts\Events\Dispatcher; use Illuminate\Contracts\Translation\Translator as TranslatorContract; @@ -73,6 +74,6 @@ class LocaleServiceProvider extends AbstractServiceProvider private function getCacheDir(): string { - return $this->app['flarum']->storagePath().'/locale'; + return $this->app[Paths::class]->storage.'/locale'; } } diff --git a/src/Queue/QueueServiceProvider.php b/src/Queue/QueueServiceProvider.php index b42d3a488..3db372ee2 100644 --- a/src/Queue/QueueServiceProvider.php +++ b/src/Queue/QueueServiceProvider.php @@ -13,6 +13,7 @@ use Flarum\Console\Event\Configuring; use Flarum\Foundation\AbstractServiceProvider; use Flarum\Foundation\ErrorHandling\Registry; use Flarum\Foundation\ErrorHandling\Reporter; +use Flarum\Foundation\Paths; use Illuminate\Contracts\Debug\ExceptionHandler as ExceptionHandling; use Illuminate\Contracts\Queue\Factory; use Illuminate\Contracts\Queue\Queue; @@ -70,7 +71,7 @@ class QueueServiceProvider extends AbstractServiceProvider // Override the Laravel native Listener, so that we can ignore the environment // option and force the binary to flarum. $this->app->singleton(QueueListener::class, function ($app) { - return new Listener($app->basePath()); + return new Listener($app[Paths::class]->base); }); // Bind a simple cache manager that returns the cache store. diff --git a/src/helpers.php b/src/helpers.php index eb6a6db5c..4d0e121c0 100644 --- a/src/helpers.php +++ b/src/helpers.php @@ -7,6 +7,7 @@ * LICENSE file that was distributed with this source code. */ +use Flarum\Foundation\Paths; use Illuminate\Container\Container; if (! function_exists('app')) { @@ -27,29 +28,17 @@ if (! function_exists('app')) { } } -if (! function_exists('app_path')) { - /** - * Get the path to the application folder. - * - * @param string $path - * @return string - */ - function app_path($path = '') - { - return app('path').($path ? DIRECTORY_SEPARATOR.$path : $path); - } -} - if (! function_exists('base_path')) { /** * Get the path to the base of the install. * * @param string $path * @return string + * @deprecated Will be removed in Beta.15. */ function base_path($path = '') { - return app()->basePath().($path ? DIRECTORY_SEPARATOR.$path : $path); + return app(Paths::class)->base.($path ? DIRECTORY_SEPARATOR.$path : $path); } } @@ -59,10 +48,11 @@ if (! function_exists('public_path')) { * * @param string $path * @return string + * @deprecated Will be removed in Beta.15. */ function public_path($path = '') { - return app()->publicPath().($path ? DIRECTORY_SEPARATOR.$path : $path); + return app(Paths::class)->public.($path ? DIRECTORY_SEPARATOR.$path : $path); } } @@ -72,10 +62,11 @@ if (! function_exists('storage_path')) { * * @param string $path * @return string + * @deprecated Will be removed in Beta.15. */ function storage_path($path = '') { - return app('path.storage').($path ? DIRECTORY_SEPARATOR.$path : $path); + return app(Paths::class)->storage.($path ? DIRECTORY_SEPARATOR.$path : $path); } } diff --git a/tests/fixtures/writable_paths/writable/.keep b/tests/fixtures/writable_paths/writable/.keep new file mode 100644 index 000000000..e69de29bb diff --git a/tests/integration/setup.php b/tests/integration/setup.php index afe5a6063..2a18ca4a6 100644 --- a/tests/integration/setup.php +++ b/tests/integration/setup.php @@ -7,6 +7,7 @@ * LICENSE file that was distributed with this source code. */ +use Flarum\Foundation\Paths; use Flarum\Install\AdminUser; use Flarum\Install\BaseUrl; use Flarum\Install\DatabaseConfig; @@ -38,10 +39,12 @@ echo "\nOff we go...\n"; */ $installation = new Installation( - __DIR__.'/tmp', - __DIR__.'/tmp/public', - __DIR__.'/tmp/storage', - __DIR__.'/../../vendor' + new Paths([ + 'base' => __DIR__.'/tmp', + 'public' => __DIR__.'/tmp/public', + 'storage' => __DIR__.'/tmp/storage', + 'vendor' => __DIR__.'/../../vendor', + ]) ); $pipeline = $installation diff --git a/tests/unit/Install/Prerequisite/WritablePathsTest.php b/tests/unit/Install/Prerequisite/WritablePathsTest.php new file mode 100644 index 000000000..dd6703828 --- /dev/null +++ b/tests/unit/Install/Prerequisite/WritablePathsTest.php @@ -0,0 +1,71 @@ +assertCount(0, $writable->problems()); + } + + public function test_paths_can_be_given_with_wildcard() + { + $writable = new WritablePaths([ + __DIR__.'/../../../fixtures/writable_paths/writable/*', + ]); + + $this->assertCount(0, $writable->problems()); + } + + public function test_problems_when_one_path_is_missing() + { + $writable = new WritablePaths([ + __DIR__.'/../../../fixtures/writable_paths/missing', + __DIR__.'/../../../fixtures/writable_paths/writable', + ]); + + $problems = $writable->problems(); + $this->assertCount(1, $problems); + $this->assertRegExp( + "%^The .+/tests/fixtures/writable_paths/missing directory doesn't exist$%", + $problems[0]['message'] + ); + $this->assertEquals( + 'This directory is necessary for the installation. Please create the folder.', + $problems[0]['detail'] + ); + } + + public function test_problem_details_filter_out_wildcard() + { + $writable = new WritablePaths([ + __DIR__.'/../../../fixtures/writable_paths/missing/*', + ]); + + $problems = $writable->problems(); + $this->assertCount(1, $problems); + $this->assertRegExp( + "%^The .+/tests/fixtures/writable_paths/missing directory doesn't exist$%", + $problems[0]['message'] + ); + $this->assertEquals( + 'This directory is necessary for the installation. Please create the folder.', + $problems[0]['detail'] + ); + } +}