Address review: dedupe memory-budget logic, fix stale doc, cover crop errors
- Extract estimatedDecodeMemory() + a MAX_DECODE_MEMORY_BYTES constant so optimizeImage (fallback) and the crop/fit guard (throw) share one estimate instead of duplicating the formula and threshold. - Correct the cropFailureException docblock: it now covers download, crop, and story-fit failures, not just downloads. - Add a Facebook crop process-failure test and an Instagram cropped-temp-leak test so the crop path matches the fit path's error coverage.
This commit is contained in:
parent
90ce205763
commit
2d63de7a97
4 changed files with 74 additions and 25 deletions
|
|
@ -12,6 +12,8 @@
|
|||
|
||||
class MediaOptimizer
|
||||
{
|
||||
private const MAX_DECODE_MEMORY_BYTES = 256 * 1024 * 1024;
|
||||
|
||||
private ImageManager $manager;
|
||||
|
||||
public function __construct()
|
||||
|
|
@ -27,29 +29,18 @@ public function optimizeImage(string $filePath, Platform $platform): string
|
|||
{
|
||||
$config = $this->getImageConfig($platform);
|
||||
|
||||
// Check image dimensions to prevent GD memory overflow
|
||||
$imageInfo = @getimagesize($filePath);
|
||||
if ($imageInfo) {
|
||||
$width = $imageInfo[0];
|
||||
$height = $imageInfo[1];
|
||||
$channels = $imageInfo['channels'] ?? 4;
|
||||
$estimatedMemory = $width * $height * $channels * 1.5; // 1.5x safety margin
|
||||
if ($imageInfo !== false && $this->estimatedDecodeMemory($imageInfo) > self::MAX_DECODE_MEMORY_BYTES) {
|
||||
Log::warning('MediaOptimizer: Image too large for GD processing', [
|
||||
'width' => $imageInfo[0],
|
||||
'height' => $imageInfo[1],
|
||||
'platform' => $platform->value,
|
||||
]);
|
||||
|
||||
// If image would use more than 256MB of RAM, skip optimization and return as-is
|
||||
if ($estimatedMemory > 256 * 1024 * 1024) {
|
||||
Log::warning('MediaOptimizer: Image too large for GD processing', [
|
||||
'width' => $width,
|
||||
'height' => $height,
|
||||
'estimated_memory' => $estimatedMemory,
|
||||
'platform' => $platform->value,
|
||||
]);
|
||||
$tempFile = tempnam(sys_get_temp_dir(), 'media_opt_');
|
||||
copy($filePath, $tempFile);
|
||||
|
||||
// Copy original file to temp location and return
|
||||
$tempFile = tempnam(sys_get_temp_dir(), 'media_opt_');
|
||||
copy($filePath, $tempFile);
|
||||
|
||||
return $tempFile;
|
||||
}
|
||||
return $tempFile;
|
||||
}
|
||||
|
||||
$image = $this->manager->decodePath($filePath);
|
||||
|
|
@ -186,6 +177,17 @@ public function fitToCanvas(string $filePath, int $width, int $height): string
|
|||
return $tempFile;
|
||||
}
|
||||
|
||||
/**
|
||||
* Estimated GD memory (bytes) needed to decode an image, from its
|
||||
* getimagesize() metadata.
|
||||
*
|
||||
* @param array{0: int, 1: int, channels?: int} $imageInfo
|
||||
*/
|
||||
private function estimatedDecodeMemory(array $imageInfo): float
|
||||
{
|
||||
return $imageInfo[0] * $imageInfo[1] * ($imageInfo['channels'] ?? 4) * 1.5;
|
||||
}
|
||||
|
||||
/**
|
||||
* Reject a source whose pixel dimensions would blow the GD memory budget,
|
||||
* before it is decoded — a small-byte, huge-dimension image would otherwise
|
||||
|
|
@ -200,9 +202,7 @@ private function assertWithinMemoryBudget(string $filePath): void
|
|||
return;
|
||||
}
|
||||
|
||||
$estimatedMemory = $imageInfo[0] * $imageInfo[1] * ($imageInfo['channels'] ?? 4) * 1.5;
|
||||
|
||||
if ($estimatedMemory > 256 * 1024 * 1024) {
|
||||
if ($this->estimatedDecodeMemory($imageInfo) > self::MAX_DECODE_MEMORY_BYTES) {
|
||||
throw new RuntimeException("Image dimensions ({$imageInfo[0]}x{$imageInfo[1]}) exceed the safe processing budget.");
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -97,8 +97,8 @@ protected function aspectRatioToFloat(string $ratio): float
|
|||
}
|
||||
|
||||
/**
|
||||
* The platform-specific exception thrown when the source image cannot be
|
||||
* downloaded for cropping.
|
||||
* The platform-specific exception thrown when an image can't be prepared for
|
||||
* publishing — a download, crop, or story-fit failure.
|
||||
*/
|
||||
abstract protected function cropFailureException(string $message): SocialPublishException;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -765,6 +765,24 @@ function facebookJpegBytes(int $width = 1200, int $height = 800): string
|
|||
->toThrow(FacebookPublishException::class, 'Failed to download image for cropping');
|
||||
});
|
||||
|
||||
test('facebook image post throws a clean exception when the crop source is not decodable', function () {
|
||||
Storage::fake();
|
||||
|
||||
$this->postPlatform->update(['meta' => ['aspect_ratio' => '4:5']]);
|
||||
$this->post->update([
|
||||
'media' => [
|
||||
['id' => 'm1', 'path' => 'media/a.jpg', 'url' => 'https://example.com/media/a.jpg', 'mime_type' => 'image/jpeg', 'original_filename' => 'a.jpg'],
|
||||
],
|
||||
]);
|
||||
|
||||
Http::fake([
|
||||
'https://example.com/media/a.jpg' => Http::response('<html>error</html>', 200, ['Content-Type' => 'text/html']),
|
||||
]);
|
||||
|
||||
expect(fn () => $this->publisher->publish($this->postPlatform))
|
||||
->toThrow(FacebookPublishException::class, 'Failed to process image for cropping');
|
||||
});
|
||||
|
||||
test('facebook single image post without aspect ratio uploads the original image (no crop)', function () {
|
||||
Storage::fake();
|
||||
|
||||
|
|
|
|||
|
|
@ -975,6 +975,37 @@ function fakeJpegBytes(int $width = 1200, int $height = 800): string
|
|||
->toThrow(InstagramPublishException::class, 'Failed to process image for cropping');
|
||||
});
|
||||
|
||||
test('instagram publisher does not leak the cropped temp file when hosting the feed image fails', function () {
|
||||
$this->postPlatform->update(['meta' => ['aspect_ratio' => '4:5']]);
|
||||
|
||||
$this->post->update([
|
||||
'media' => [
|
||||
['id' => 'm1', 'path' => 'media/a.jpg', 'url' => 'https://example.com/media/a.jpg', 'mime_type' => 'image/jpeg', 'original_filename' => 'a.jpg'],
|
||||
],
|
||||
]);
|
||||
|
||||
Http::fake([
|
||||
'https://example.com/media/a.jpg' => Http::response(fakeJpegBytes(1200, 800), 200),
|
||||
]);
|
||||
|
||||
$croppedPath = null;
|
||||
$mockOptimizer = Mockery::mock(MediaOptimizer::class);
|
||||
$mockOptimizer->shouldReceive('cropToAspectRatio')->once()->andReturnUsing(function (string $tempFile) use (&$croppedPath) {
|
||||
$croppedPath = tempnam(sys_get_temp_dir(), 'media_crop_');
|
||||
copy($tempFile, $croppedPath);
|
||||
|
||||
return $croppedPath;
|
||||
});
|
||||
app()->instance(MediaOptimizer::class, $mockOptimizer);
|
||||
|
||||
Storage::shouldReceive('put')->once()->andThrow(new RuntimeException('disk full'));
|
||||
|
||||
expect(fn () => $this->publisher->publish($this->postPlatform))->toThrow(RuntimeException::class);
|
||||
|
||||
expect($croppedPath)->not->toBeNull()
|
||||
->and(file_exists($croppedPath))->toBeFalse();
|
||||
});
|
||||
|
||||
test('feed image with original aspect ratio bypasses crop', function () {
|
||||
Storage::fake();
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue