From 96c1ad35b725783a638605c86579cec74efe9345 Mon Sep 17 00:00:00 2001 From: neznaika0 Date: Sun, 2 Feb 2025 21:30:27 +0300 Subject: [PATCH] refactor: Improve URL handling for `asset()` with tests --- src/Assets/Helpers/assets_helper.php | 46 ++++++++++++++++------- tests/Assets/AssetHelperTest.php | 55 ++++++++++++++++++++++++---- 2 files changed, 80 insertions(+), 21 deletions(-) diff --git a/src/Assets/Helpers/assets_helper.php b/src/Assets/Helpers/assets_helper.php index 7ae3a9f2..e311517d 100644 --- a/src/Assets/Helpers/assets_helper.php +++ b/src/Assets/Helpers/assets_helper.php @@ -1,5 +1,7 @@ $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; diff --git a/tests/Assets/AssetHelperTest.php b/tests/Assets/AssetHelperTest.php index 2386d79b..746ec505 100644 --- a/tests/Assets/AssetHelperTest.php +++ b/tests/Assets/AssetHelperTest.php @@ -1,8 +1,20 @@ + * + * 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,18 +28,32 @@ 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.'); @@ -35,7 +61,19 @@ public function testAssetThrowsNoExtension() 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]); } }