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.
flarum · Jun 19, 2020 · 88366fe · 88366fe
1 parent b82504b
commit 88366fe
Show file tree

Hide file tree

Showing 13 changed files with 143 additions and 77 deletions.
diff --git 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 @@ public function register()
             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
@@ -86,8 +86,6 @@ public function __construct(Container $container, Paths $paths)
         $this->registerBaseBindings();
         $this->registerBaseServiceProviders();
         $this->registerCoreContainerAliases();
-
-        $this->bindPathsInContainer();
     }
 
     /**
@@ -161,22 +159,11 @@ protected function registerBaseServiceProviders()
         $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 @@ public function basePath()
      * Get the path to the public / web directory.
      *
      * @return string
+     * @deprecated Will be removed in Beta.15.
      */
     public function publicPath()
     {
@@ -197,6 +185,7 @@ public function publicPath()
      * Get the path to the storage directory.
      *
      * @return string
+     * @deprecated Will be removed in Beta.15.
      */
     public function storagePath()
     {
@@ -207,6 +196,7 @@ public function storagePath()
      * 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
@@ -50,16 +50,12 @@ protected function save(string $file, array $sources): bool
         }
 
         // 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
@@ -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 @@ public function register()
     {
         $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
@@ -23,15 +23,6 @@ public function register()
         $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
@@ -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 @@ public function prerequisites(): Prerequisite\PrerequisiteInterface
                 'tokenizer',
             ]),
             new Prerequisite\WritablePaths([
-                $this->basePath,
-                $this->getAssetPath(),
-                $this->storagePath,
+                $this->paths->base,
+                $this->getAssetPath().'/*',
+                $this->paths->storage,
             ])
         );
     }
@@ -140,24 +139,24 @@ function ($connection) {
         });
 
         $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;
     }
 
     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
@@ -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 @@ public function problems(): Collection
 
     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 @@ private function getMissingPaths(): Collection
 
     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 @@ private function getAbsolutePath($path)
 
         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
@@ -11,6 +11,7 @@
 
 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 @@ private function getDefaultLocale(): string
 
     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
@@ -13,6 +13,7 @@
 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 @@ public function register()
         // 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
@@ -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 @@ function app($make = null, $parameters = [])
     }
 }
 
-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 @@ function base_path($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 @@ function public_path($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