refactor: Improve URL handling for asset() with tests

src/Assets/Helpers/assets_helper.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 /**
  * This file is part of Bonfire.
  *
@@ -8,11 +10,12 @@
  * For the full copyright and license information, please view
  * the LICENSE file that was distributed with this source code.
  */
-if (!defined('asset_link')) {
+
+if (! defined('asset_link')) {
     /**
-     * Generates the URL to serve an asset to the client
+     * Generates the HTML tag with URL to serve an asset to the client
      *
-     * @param string $location   url to asset file
+     * @param string $location   Relative URL to asset file
      * @param string $type       css, js
      * @param mixed  $attributes Additional attributes to include in the asset link tag.
      *                           Can be provided as a string (for value-less attributes like "defer")
@@ -21,22 +24,23 @@
      */
     function asset_link(string $location, string $type, mixed $attributes = null): string
     {
-        $url = asset($location, $type);
-
         $tag = '';
+        $url = asset($location, $type);
 
         $additionalAttr = '';
         $defaultAttr    = $type === 'css' ? "rel='stylesheet'" : '';
 
         if (is_string($attributes)) {
             $additionalAttr = $attributes;
         }
+
         if (is_array($attributes)) {
             foreach ($attributes as $key => $value) {
                 // if the array already includes the 'rel', remove the default
                 if ($key === 'rel') {
                     $defaultAttr = '';
                 }
+
                 $additionalAttr .= "{$key}='{$value}' ";
             }
         }
@@ -57,25 +61,37 @@ function asset_link(string $location, string $type, mixed $attributes = null): s
 }
 
 if (!defined('asset')) {
+    /**
+     * Generates the URL to serve an asset to the client
+     *
+     * @param string $location Relative URL to asset file
+     * @param string $type     'file|version' string
+     */
     function asset(string $location, string $type): string
     {
-        $config   = config('Assets');
-        $location = trim($location, ' /');
+        $location     = trim($location, ' /');
+        $relativePath = parse_url($location, PHP_URL_PATH);
+
+        if (str_contains($location, '://') || $relativePath === false || $relativePath === '') {
+            throw new InvalidArgumentException('$location must be a relative URL to the file.');
+        }
 
         // Add a cache-busting fingerprint to the filename
-        $segments   = explode('/', $location);
-        $filename   = array_pop($segments);
-        $ext        = substr($filename, strrpos($filename, '.'));
-        $namelength = strlen($filename) - strlen($ext);
-        $name       = substr($filename, 0, $namelength);
+        $config   = config('Assets');
+        $segments = explode('/', ltrim($location, '/'));
+        $filename = array_pop($segments);
+        $pathinfo = pathinfo($filename);
+        $ext      = $pathinfo['extension'] ?? '';
+        $name     = $pathinfo['filename'];
 
-        if (empty($filename) || empty($ext) || $filename === $ext || $segments === []) {
+        if ($filename === '' || $name === '' || $ext === '') {
             throw new RuntimeException('You must provide a valid filename and extension to the asset() helper.');
         }
 
         // VERSION cache-busting
         $fingerprint = '';
         $separator   = $config->separator ?? '~~';
+
         if ($config->bustingType === 'version') {
             switch (ENVIRONMENT) {
                 case 'testing':
@@ -87,6 +103,7 @@ function asset(string $location, string $type): string
                     $fingerprint = $separator . $config->versions[$type];
             }
         }
+
         // FILE cache-busting
         if ($config->bustingType === 'file') {
             $tempSegments = $segments;
@@ -101,10 +118,11 @@ function asset(string $location, string $type): string
             if (!$filetime) {
                 throw new RuntimeException('Unable to get modification time of asset file: ' . $filename);
             }
+
             $fingerprint = $separator . $filetime;
         }
 
-        $filename = $name . $fingerprint . $ext;
+        $filename = $name . $fingerprint . '.' . $ext;
 
         // Stitch the location back together
         $segments[] = $filename;

tests/Assets/AssetHelperTest.php

@@ -1,8 +1,20 @@
 <?php
 
+declare(strict_types=1);
+
+/**
+ * This file is part of Bonfire.
+ *
+ * (c) Lonnie Ezell <[email protected]>
+ *
+ * For the full copyright and license information, please view
+ * the LICENSE file that was distributed with this source code.
+ */
+
 namespace Tests\Assets;
 
 use CodeIgniter\Config\Factories;
+use InvalidArgumentException;
 use RuntimeException;
 use Tests\Support\TestCase;
 
@@ -16,26 +28,52 @@ protected function setUp(): void
         $this->mockCache();
         parent::setUp();
 
-        helper('Bonfire\Assets\Helpers\assets');
+        helper(['Bonfire\Assets\Helpers\assets']);
     }
 
-    public function testAssetThrowsNoFilename()
+    public function testAssetThrowsNoFilenameExtension(): void
     {
         $this->expectException(RuntimeException::class);
         $this->expectExceptionMessage('You must provide a valid filename and extension to the asset() helper.');
 
-        \asset_link('foo', 'css');
+        asset_link('foo', 'css');
+    }
+
+    public function testAssetThrowsNoFilename(): void
+    {
+        $this->expectException(RuntimeException::class);
+
+        asset_link('/admin/css/admin_missing.css', 'css');
+    }
+
+    public function testAssetThrowsEmptyLocation(): void
+    {
+        $this->expectException(InvalidArgumentException::class);
+
+        asset_link('/', 'css');
     }
 
-    public function testAssetThrowsNoExtension()
+    public function testAssetThrowsNoExtension(): void
     {
         $this->expectException(RuntimeException::class);
         $this->expectExceptionMessage('You must provide a valid filename and extension to the asset() helper.');
 
         asset_link('admin/foo', 'css');
     }
 
-    public function testAssetVersion()
+    public function testAssetIfIncorrectType(): void
+    {
+        $this->assertEmpty(asset_link('/admin/css/admin.css', 'map'));
+    }
+
+    public function testAssetThrowsLocationAsFullURL(): void
+    {
+        $this->expectException(InvalidArgumentException::class);
+
+        asset_link('http://example.com/admin/css/admin.css', 'css');
+    }
+
+    public function testAssetVersion(): void
     {
         $config = config('Assets');
 
@@ -48,12 +86,14 @@ public function testAssetVersion()
         // In testing environment, would be the current timestamp
         // so just test the pattern to ensure that works.
         preg_match('|assets/admin/css/admin~~([\d]+).css|i', $link, $matches);
+
         $this->assertIsNumeric($matches[1]);
     }
 
-    public function testAssetFile()
+    public function testAssetFile(): void
     {
-        $config              = config('Assets');
+        $config = config('Assets');
+
         $config->bustingType = 'file';
         $config->separator   = '~~';
         Factories::injectMock('config', 'Assets', $config);
@@ -63,6 +103,7 @@ public function testAssetFile()
         // In testing environment, would be the current timestamp
         // so just test the pattern to ensure that works.
         preg_match('|assets/admin/css/admin~~([\d]+).css|i', $link, $matches);
+
         $this->assertSame(filemtime(BFPATH . '../themes/Admin/css/admin.css'), (int) $matches[1]);
     }
 }