Skip to content

Commit 6f26fa9

Browse files
authored
fix(cdn): stop the lower layer implementing the upper layer's interface (#10)
* fix(cdn): stop the lower layer implementing the upper layer's interface BunnyStorageService imported two types from dappcore/service — the HealthCheckable interface it implemented, and HealthCheckResult, which its healthCheck() body constructed. Neither exists in this package. php requires laravel/framework, pennant and livewire, and nothing else. So this package had production source that could not be loaded in this package. It worked only because every real consumer installs dappcore/service alongside it, which is why host.uk.com is fine and nobody has felt this. Adding the dependency was not available: dappcore/service requires dappcore/php: *, so it would close a loop. The direction composer already declares is service-depends-on-php, and php reaching up into Core\Service\* was against that grain. So the health report moves to the layer that is entitled to build it. WHAT MOVES, and it is a relocation rather than a removal: healthCheck() goes to a decorator in dappcore/service, which composes this service and implements HealthCheckable there. That repo has no session yet and the work is held; until it lands, health reporting of this service is absent with a named destination. WHAT STAYS: the zone probing, which never depended on Core\Service at all. checkZoneHealth() keeps returning its {success, latency_ms, error} array and isReachable() keeps answering the boolean question. checkZoneHealth() is now public, because the decorator composes rather than extends — a protected probe would have made the relocation cost exactly the latency and error detail that makes a health report worth reading. Safe to carry with no consumer noticing, checked rather than assumed: nothing in host.uk.com's app or vendor tree calls healthCheck() on this service, and there is no `instanceof HealthCheckable` anywhere in it. The only references were this file and documentation inside dappcore/service. ALSO HERE, and it is why the numbers move: CdnIntegrationTest built StorageUrlResolver with its arguments wrong — __construct(protected BunnyStorageService $bunnyStorage, ?CdnUrlBuilder $urlBuilder = null) new StorageUrlResolver($this->urlBuilder) passing the optional second argument into the required first slot and omitting the required one, so all 30 cases died in setUp on one TypeError. Thirty failures, one line. Fixed by resolving BunnyStorageService from the container, which is how Core\Cdn\Boot wires it in production. That fix was pointless on its own — corrected, the tests failed one line later on the missing interface — which is why both halves are in one change. ./vendor/bin/pest --testsuite=Feature,Unit 268 passed, 0 failed — unchanged ./vendor/bin/pest --testsuite=Module 448 -> 426 failed, 308 -> 330 passed CdnIntegrationTest alone 30 failed / 0 assertions -> 8 failed / 22 passed / 52 assertions vendor/bin/pint --test pass vendor/bin/phpstan analyse no errors The 8 that remain are real assertions against a constructible object rather than a constructor error: a missing config_resolved table in the test database, a singular/plural category path, a ?id= versus ?v= query parameter, and four straightforward expectation mismatches. Each is now a visible disagreement about behaviour instead of 30 identical crashes hiding one line. Co-Authored-By: Virgil <virgil@lethean.io> * chore(psalm): drop the suppression that existed for the dependency now removed CI's Psalm went red on this branch with UnusedIssueHandlerSuppression: Suppressed issue type "MissingDependency" for /src/Core/Cdn/ was not thrown which is the fix reporting itself. The suppression's own comment said what it was for — "Pending Core\Service module — referenced by Cdn BunnyStorageService" — so it was config-level acknowledgement of the upward dependency this branch removes, pending indefinitely. With the dependency gone there is nothing left to suppress, and Psalm treats a suppression that never fires as an error in its own right. Read from the CI job log rather than a local run, deliberately: local and CI Psalm disagree in this repository — the Laravel plugin resolves differently — and I got that wrong earlier today by trusting the copy in front of me. CI is the authority for what CI thinks is unused, and here CI is the one calling it unused. Co-Authored-By: Virgil <virgil@lethean.io> * fix(psalm): repair the config I broke, and remove only what should go The previous commit's regex matched backwards from the wrong comment. Both blocks I meant to touch begin "Pending Core\Service module", and the pattern anchored on the first one — inside the UndefinedClass handler — then ran to the closing tag of the second, taking two unrelated referencedClass entries and a pair of closing tags with it. psalm.xml became invalid XML, and I pushed it. Restored and redone against the tags themselves rather than a shared comment prefix. What goes is exactly: - referencedClass Core\Service\Contracts\HealthCheckable - referencedClass Core\Service\HealthCheckResult - the MissingDependency handler suppressing src/Core/Cdn all three of which existed for the upward dependency this branch removes. What stays is what I should never have touched: the Core\Front\Client\Boot and Illuminate\Foundation\Auth\User entries, which have nothing to do with it. Fixed forward rather than rewritten — the broken commit is on the remote and stays there, with this on top. python3 -c "xml.etree.ElementTree.parse('psalm.xml')" valid ./vendor/bin/pest --testsuite=Feature,Unit 268 passed, 0 failed vendor/bin/pint --test pass Co-Authored-By: Virgil <virgil@lethean.io>
1 parent 5b9f022 commit 6f26fa9

3 files changed

Lines changed: 31 additions & 88 deletions

File tree

‎psalm.xml‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -64,22 +64,13 @@
6464
<referencedClass name="Core\Tenant\Models\User" />
6565
<referencedClass name="Core\Tenant\Services\EntitlementService" />
6666
<referencedClass name="Core\Config\Workspace" />
67-
<!-- Pending Core\Service module (see plans/code/core/lint/RFC.md) -->
68-
<referencedClass name="Core\Service\Contracts\HealthCheckable" />
69-
<referencedClass name="Core\Service\HealthCheckResult" />
7067
<!-- Pending Front\Client frontage subpackage -->
7168
<referencedClass name="Core\Front\Client\Boot" />
7269
<!-- Laravel framework classes Psalm CI doesn't always resolve -->
7370
<referencedClass name="Illuminate\Foundation\Auth\User" />
7471
</errorLevel>
7572
</UndefinedClass>
7673

77-
<!-- Pending Core\Service module — referenced by Cdn BunnyStorageService and app variant -->
78-
<MissingDependency>
79-
<errorLevel type="suppress">
80-
<directory name="src/Core/Cdn" />
81-
</errorLevel>
82-
</MissingDependency>
8374

8475
<!-- Suppress false positives from strict type analysis -->
8576
<NoValue>

‎src/Core/Cdn/Services/BunnyStorageService.php‎

Lines changed: 13 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,6 @@
1414
use Bunny\Storage\Client;
1515
use Core\Config\ConfigService;
1616
use Core\Crypt\LthnHash;
17-
use Core\Service\Contracts\HealthCheckable;
18-
use Core\Service\HealthCheckResult;
1917
use Illuminate\Support\Facades\Log;
2018
use Illuminate\Support\Facades\Storage;
2119

@@ -27,9 +25,11 @@
2725
* - Private zone: DRM/gated content
2826
*
2927
* Supports vBucket scoping for workspace-isolated CDN paths.
30-
* Implements HealthCheckable for monitoring CDN connectivity.
28+
* Zone reachability is exposed through checkZoneHealth() and isReachable();
29+
* turning that into a service health report belongs to dappcore/service,
30+
* which is the package that depends on this one.
3131
*/
32-
class BunnyStorageService implements HealthCheckable
32+
class BunnyStorageService
3333
{
3434
protected ?Client $publicClient = null;
3535

@@ -555,84 +555,19 @@ public function vBucketList(string $domain, string $path = '', string $zone = 'p
555555
return $this->list($scopedPath, $zone);
556556
}
557557

558-
// ─────────────────────────────────────────────────────────────────────────────
559-
// Health Check (implements HealthCheckable)
560-
// ─────────────────────────────────────────────────────────────────────────────
561-
562-
/**
563-
* Perform a health check on the CDN storage zones.
564-
*
565-
* Tests connectivity by listing the root directory of configured storage zones.
566-
* Returns a HealthCheckResult with status, latency, and zone information.
567-
*/
568-
public function healthCheck(): HealthCheckResult
569-
{
570-
$publicConfigured = $this->isConfigured('public');
571-
$privateConfigured = $this->isConfigured('private');
572-
573-
if (! $publicConfigured && ! $privateConfigured) {
574-
return HealthCheckResult::unknown('No CDN storage zones configured');
575-
}
576-
577-
$results = [];
578-
$startTime = microtime(true);
579-
$hasError = false;
580-
$isDegraded = false;
581-
582-
// Check public zone
583-
if ($publicConfigured) {
584-
$publicResult = $this->checkZoneHealth('public');
585-
$results['public'] = $publicResult;
586-
if (! $publicResult['success']) {
587-
$hasError = true;
588-
} elseif ($publicResult['latency_ms'] > 1000) {
589-
$isDegraded = true;
590-
}
591-
}
592-
593-
// Check private zone
594-
if ($privateConfigured) {
595-
$privateResult = $this->checkZoneHealth('private');
596-
$results['private'] = $privateResult;
597-
if (! $privateResult['success']) {
598-
$hasError = true;
599-
} elseif ($privateResult['latency_ms'] > 1000) {
600-
$isDegraded = true;
601-
}
602-
}
603-
604-
$totalLatency = (microtime(true) - $startTime) * 1000;
605-
606-
if ($hasError) {
607-
return HealthCheckResult::unhealthy(
608-
'One or more CDN storage zones are unreachable',
609-
['zones' => $results],
610-
$totalLatency
611-
);
612-
}
613-
614-
if ($isDegraded) {
615-
return HealthCheckResult::degraded(
616-
'CDN storage zones responding slowly',
617-
['zones' => $results],
618-
$totalLatency
619-
);
620-
}
621-
622-
return HealthCheckResult::healthy(
623-
'All configured CDN storage zones operational',
624-
['zones' => $results],
625-
$totalLatency
626-
);
627-
}
628-
629558
/**
630559
* Check health of a specific storage zone.
631560
*
561+
* Public because it is the seam the health-reporting decorator in
562+
* dappcore/service consumes. That decorator composes this service rather
563+
* than extending it, so a protected probe would be unreachable and the
564+
* relocation would cost the detail — latency and the error string — that
565+
* makes a health report worth reading.
566+
*
632567
* @param string $zone 'public' or 'private'
633568
* @return array{success: bool, latency_ms: float, error?: string}
634569
*/
635-
protected function checkZoneHealth(string $zone): array
570+
public function checkZoneHealth(string $zone): array
636571
{
637572
$startTime = microtime(true);
638573

@@ -677,7 +612,8 @@ protected function checkZoneHealth(string $zone): array
677612
/**
678613
* Perform a quick connectivity check.
679614
*
680-
* Simpler than healthCheck() - just returns true/false.
615+
* A boolean answer, for callers that only need reachability. The detailed
616+
* per-zone probe is checkZoneHealth().
681617
*
682618
* @param string $zone 'public', 'private', or 'any' (default)
683619
*/

‎src/Core/Tests/Feature/CdnIntegrationTest.php‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Core\Tests\Feature;
1313

1414
use Core\Cdn\Services\AssetPipeline;
15+
use Core\Cdn\Services\BunnyStorageService;
1516
use Core\Cdn\Services\CdnUrlBuilder;
1617
use Core\Cdn\Services\StorageUrlResolver;
1718
use Core\Tests\TestCase;
@@ -67,9 +68,24 @@ protected function setUp(): void
6768
'visibility' => 'private',
6869
]);
6970

70-
// Initialize services
71+
// Initialize services.
72+
//
73+
// StorageUrlResolver takes the storage service first and the URL builder
74+
// second, and the builder is the optional one. This passed the builder
75+
// into the first parameter and omitted the required argument entirely,
76+
// so every test in this file died in setUp on a TypeError — 30 of them,
77+
// reported as 30 failures rather than as the one line they are.
78+
//
79+
// BunnyStorageService comes from the container, which is how production
80+
// gets it: Core\Cdn\Boot registers StorageUrlResolver as a singleton and
81+
// lets Laravel autowire the chain. Resolving it here rather than
82+
// hand-building BunnyStorageService(ConfigService) keeps the test on the
83+
// same path the application uses.
7184
$this->urlBuilder = new CdnUrlBuilder();
72-
$this->urlResolver = new StorageUrlResolver($this->urlBuilder);
85+
$this->urlResolver = new StorageUrlResolver(
86+
app(BunnyStorageService::class),
87+
$this->urlBuilder,
88+
);
7389
$this->assetPipeline = new AssetPipeline($this->urlResolver);
7490
}
7591

0 commit comments

Comments
 (0)