fix(cdn): resolve the eight, and one of them was real - #11
Conversation
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 <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
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: stringand passedconfig('cdn.bunny.private.pull_zone')straight intostr_starts_with(). Unconfigured that isnull, andstr_starts_with(null, …)is a TypeError on PHP 8 — so signing with a token set and no pull zone configured didn't 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. Fixing the test found the bug.
Also code — a consistency defect
withVersion()emittedid=whileCore\Helpers\Cdn::versioned()— the helper applications actually call from templates — emitsv=. 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 rather than the parameter, and its only appearance in the history is an unrelated Rector pass. Nowv. One deploy's worth of cache misses, which is what a version parameter is for.The harness one
cdn.pathswas absent — the baseTestCaseregisters onlyLifecycleEventProviderand nothing merges the package's cdn config, sopathPrefix()fell back to the raw category name.'avatar'stayed'avatar'where the config maps it to'avatars'. The test was right all along.Deliberately one key, not the file: merging the whole config overrode the test disks configured below it and took the file from 6 failures to 9. I did that first and reverted it.
Two further cases needed the Config package's migrations —
BunnyStorageServicereads zone credentials throughConfigService, which is a query againstconfig_resolved, soisConfigured()died on "no such table" rather than on anything it was testing.The stale tests — four, each pinning something the code has never done
signed()cdn.signing_key,cdn.token_lifetimecdn.bunny.private.token; neither of those keys is read anywhereurls()cdn_url/origin_urlcdn/origin, asallUrls()documentscopy()size()fake()->create('test.txt', 50)The last one measured rather than guessed:
So the stored file was empty and
size()answered0correctly — the assertion was measuring the fixture. NowcreateWithContentwith known content, asserting the exact size rather than "more than nothing".Receipts
🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io