From fcedfb289fd3cc44f8d8068513564172bbf5083d Mon Sep 17 00:00:00 2001 From: Snider Date: Sat, 8 Aug 2026 13:20:10 +0100 Subject: [PATCH] fix(cdn): resolve the eight, and one of them was real MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The eight that survived #10 were real disagreements rather than one crash worn eight times, so each got decided on its own evidence. Six were the test being wrong, one was the harness, and one was the code. THE CODE ONE. CdnUrlBuilder::buildSignedUrlBase() is typed to return string and passed config('cdn.bunny.private.pull_zone') straight into str_starts_with(). Unconfigured, that is null, and str_starts_with(null, …) is a TypeError on PHP 8 — so signing with a token set and no pull zone did not fail politely, it threw from inside the URL builder. Only reachable once a token exists, which is why no test had ever got there: the test that would have was setting the wrong config key and taking the empty-token early return instead. Cast and defaulted. Also code, and a consistency defect rather than a crash: withVersion() emitted `id=` while Core\Helpers\Cdn::versioned() — the helper applications actually call from templates — emits `v=`. The same package cache-busted the same assets under two parameter names depending on which door you came in by. Nothing required `id`: no CDN documentation here mentions it, every docblock names the purpose and not the parameter, and its only appearance in the history is an unrelated Rector pass. Now `v`, consistent with the helper and with the test. One deploy's worth of cache misses, which is what a version parameter is for. THE HARNESS ONE. cdn.paths was absent, because the base TestCase registers only LifecycleEventProvider and nothing merges the package's cdn config. pathPrefix() falls back to the raw category name, so 'avatar' stayed 'avatar' where the config maps it to 'avatars' — the test was right all along. Set that one key. Deliberately one key and not the file: merging the whole config wholesale overrode the test disks configured below it and took the file from 6 failures to 9. I did that first and reverted it. Two more needed the Config package's migrations. BunnyStorageService reads its zone credentials through ConfigService, which is a database query against config_resolved, so isConfigured() died on "no such table" rather than on anything it was testing. THE STALE TESTS, four of them, each pinning something the code has never done: signed() set cdn.signing_key and cdn.token_lifetime; the implementation reads cdn.bunny.private.token, and neither of those keys is read anywhere urls() asserted cdn_url / origin_url; the contract is cdn / origin, which is what allUrls() documents too copy() passed the destination path as the source bucket and two disk names after it — every argument after the first in the wrong parameter, written against a signature this method has not had size() used UploadedFile::fake()->create('test.txt', 50), which reports getSize() 51200 and writes zero real bytes. Measured, not guessed: create_getSize 51200 / create_realBytes 0, against image_getSize 695 / image_realBytes 695. So the stored file was empty and size() answered 0 correctly — the assertion was measuring the fixture. Now createWithContent with known content, asserting the exact size rather than "more than nothing". CdnIntegrationTest 8 failed / 22 passed -> 30 passed, 0 failed Module suite 426 -> 418 failed, 330 -> 338 passed gate suites 268 passed, 0 failed — unchanged pint, phpstan clean Co-Authored-By: Virgil --- src/Core/Cdn/Services/CdnUrlBuilder.php | 20 +++++- src/Core/Tests/Feature/CdnIntegrationTest.php | 61 +++++++++++++++---- 2 files changed, 67 insertions(+), 14 deletions(-) diff --git a/src/Core/Cdn/Services/CdnUrlBuilder.php b/src/Core/Cdn/Services/CdnUrlBuilder.php index 6fd9ec5..f5f3344 100644 --- a/src/Core/Cdn/Services/CdnUrlBuilder.php +++ b/src/Core/Cdn/Services/CdnUrlBuilder.php @@ -203,6 +203,17 @@ public function asset(string $path, string $context = 'public'): string /** * Build a URL with version query parameter for cache busting. * + * The parameter is `v`, which is what {@see \Core\Helpers\Cdn::versioned()} + * has always emitted — and that helper is the one applications actually call + * from their templates. This method emitted `id` instead, so the same package + * cache-busted the same assets under two different parameter names depending + * on which door you came in by. Nothing required `id`: no CDN documentation + * here mentions it, every docblock describes the purpose rather than the name, + * and its only appearance in the history is an unrelated Rector pass. + * + * One deploy's worth of cache misses when this changes, which is what a + * version parameter is for. + * * @param string $url The base URL * @param string|null $version Version hash for cache busting * @return string URL with version parameter @@ -215,7 +226,7 @@ public function withVersion(string $url, ?string $version): string $separator = str_contains($url, '?') ? '&' : '?'; - return sprintf('%s%sid=%s', $url, $separator, $version); + return sprintf('%s%sv=%s', $url, $separator, $version); } /** @@ -294,7 +305,12 @@ public function build(?string $baseUrl, string $path): string */ protected function buildSignedUrlBase(): string { - $pullZone = config('cdn.bunny.private.pull_zone'); + // Cast, because an unconfigured pull zone is null and this method is + // typed to return a string. str_starts_with(null, ...) is a TypeError on + // PHP 8, so signing with a token set and no pull zone configured did not + // fail politely — it threw from inside the URL builder. Reachable only + // once a token exists, which is why no test had ever got here. + $pullZone = (string) config('cdn.bunny.private.pull_zone', ''); // Support both full URL and just hostname in config if (str_starts_with($pullZone, 'https://') || str_starts_with($pullZone, 'http://')) { diff --git a/src/Core/Tests/Feature/CdnIntegrationTest.php b/src/Core/Tests/Feature/CdnIntegrationTest.php index b4abd07..36e8032 100644 --- a/src/Core/Tests/Feature/CdnIntegrationTest.php +++ b/src/Core/Tests/Feature/CdnIntegrationTest.php @@ -47,7 +47,26 @@ protected function setUp(): void { parent::setUp(); - // Configure CDN URLs + // BunnyStorageService reads its zone credentials through ConfigService, + // which is backed by the config_resolved table — so isConfigured() is a + // database query, and without the Config package's migrations the + // storage tests die on "no such table" rather than on anything they + // are testing. Same pattern as Bouncer's ActionGateTest. + $this->loadMigrationsFrom(__DIR__.'/../../Config/Migrations'); + + // cdn.paths, from the package's own config. Core\Cdn\Boot merges the + // whole file in a real application; the base TestCase registers only + // LifecycleEventProvider, so the cdn.* tree is absent here and + // pathPrefix() silently falls back to the raw category name. That is why + // the category test saw avatar/ rather than avatars/ — cdn.paths maps + // 'avatar' => 'avatars', and the test was right all along. + // + // Only this key, not the whole file: the rest of that config names real + // disks and zone settings, and merging it wholesale overrode the test + // disks configured below and broke three tests that were passing. + Config::set('cdn.paths', (require __DIR__.'/../../Cdn/config.php')['paths']); + + // Configure CDN URLs. Config::set('cdn.urls.cdn', 'https://cdn.example.com'); Config::set('cdn.urls.public', 'https://public.example.com'); Config::set('cdn.urls.private', 'https://private.example.com'); @@ -236,13 +255,21 @@ public function test_asset_pipeline_checks_file_existence(): void public function test_asset_pipeline_returns_file_size(): void { - $file = UploadedFile::fake()->create('test.txt', 50); // 50KB + // createWithContent, not create(name, kilobytes). The latter reports + // getSize() as 51200 and writes a file of zero real bytes, so what got + // stored was empty and size() answered 0 — correctly. The assertion was + // measuring the fixture, not the pipeline. + // + // With known content the size is known too, so this asserts the exact + // number rather than "more than nothing". + $contents = str_repeat('a', 1024); + $file = UploadedFile::fake()->createWithContent('test.txt', $contents); $result = $this->assetPipeline->store($file, 'media'); $size = $this->assetPipeline->size($result['path']); $this->assertNotNull($size); - $this->assertGreaterThan(0, $size); + $this->assertSame(strlen($contents), $size); } public function test_asset_pipeline_returns_mime_type(): void @@ -261,11 +288,16 @@ public function test_asset_pipeline_copies_between_public_and_private(): void $file = UploadedFile::fake()->image('test.jpg'); $publicResult = $this->assetPipeline->store($file, 'media'); + // copy(sourcePath, sourceBucket, destBucket, destPath) — buckets are + // 'public' or 'private', not disk names. This passed the destination path + // as the source bucket and two disk names after it, so every argument + // after the first landed in the wrong parameter. Written against a + // signature this method has not had. $privateResult = $this->assetPipeline->copy( $publicResult['path'], - 'private/copy.jpg', - 'hetzner-public', - 'hetzner-private' + 'public', + 'private', + 'private/copy.jpg' ); $this->assertIsArray($privateResult); @@ -315,8 +347,10 @@ public function test_cdn_url_with_query_parameters(): void public function test_signed_url_generation(): void { - Config::set('cdn.signing_key', 'test-secret-key'); - Config::set('cdn.token_lifetime', 3600); + // signed() reads cdn.bunny.private.token — the keys this used to set, + // cdn.signing_key and cdn.token_lifetime, are not read anywhere in the + // package, so signed() took its empty-token path and returned null. + Config::set('cdn.bunny.private.token', 'test-secret-key'); $url = $this->urlBuilder->signed('private/document.pdf', 3600); @@ -395,9 +429,12 @@ public function test_url_resolver_provides_both_cdn_and_origin_urls(): void $urls = $this->assetPipeline->urls($result['path']); $this->assertIsArray($urls); - $this->assertArrayHasKey('cdn_url', $urls); - $this->assertArrayHasKey('origin_url', $urls); - $this->assertStringStartsWith('https://cdn.example.com/', $urls['cdn_url']); - $this->assertStringStartsWith('https://public.example.com/', $urls['origin_url']); + // urls() returns 'cdn' and 'origin'. The _url suffixes this asserted + // are not the contract and never were — allUrls() documents the same + // unsuffixed shape. + $this->assertArrayHasKey('cdn', $urls); + $this->assertArrayHasKey('origin', $urls); + $this->assertStringStartsWith('https://cdn.example.com/', $urls['cdn']); + $this->assertStringStartsWith('https://public.example.com/', $urls['origin']); } }