diff --git a/app/Http/Requests/ImageUploadRequest.php b/app/Http/Requests/ImageUploadRequest.php index d3516b1f18..6022f6b6e2 100644 --- a/app/Http/Requests/ImageUploadRequest.php +++ b/app/Http/Requests/ImageUploadRequest.php @@ -112,10 +112,19 @@ class ImageUploadRequest extends Request $ext = $image->guessExtension(); $file_name = $type.'-'.$form_fieldname.($item->id ?? '-'.$item->id).'-'.str_random(10).'.'.$ext; + // Track whether the new file actually landed on disk. Storage::put + // can return false without throwing (disks default to non-throwing + // mode). Before, the ordering was put -> deleteExistingImage -> + // reassign, all unconditional, so a failed put still destroyed + // the current image and left the model referencing a file that + // was never written. Keep the old image intact unless we confirm + // the new one is there. + $wroteNewFile = false; + if (($image->getMimeType() == 'image/vnd.microsoft.icon') || ($image->getMimeType() == 'image/x-icon') || ($image->getMimeType() == 'image/avif') || ($image->getMimeType() == 'image/webp')) { // If the file is an icon, webp or avif, we need to just move it since gd doesn't support resizing // icons or avif, and webp support and needs to be compiled into gd for resizing to be available - Storage::disk('public')->put($prefix.$file_name, file_get_contents($image)); + $wroteNewFile = (bool) Storage::disk('public')->put($prefix.$file_name, file_get_contents($image)); } elseif ($image->getMimeType() == 'image/svg+xml') { // If the file is an SVG, we need to clean it and NOT encode it @@ -124,7 +133,7 @@ class ImageUploadRequest extends Request $cleanSVG = $sanitizer->sanitize($dirtySVG); try { - Storage::disk('public')->put($prefix.$file_name, $cleanSVG); + $wroteNewFile = (bool) Storage::disk('public')->put($prefix.$file_name, $cleanSVG); } catch (\Exception $e) { Log::debug($e); } @@ -145,13 +154,25 @@ class ImageUploadRequest extends Request } // This requires a string instead of an object, so we use ($string) - Storage::disk('public')->put($prefix.$file_name, (string) $upload->encode()); + $wroteNewFile = (bool) Storage::disk('public')->put($prefix.$file_name, (string) $upload->encode()); } - // Remove Current image if exists - $item = $this->deleteExistingImage($item, $path, $db_fieldname); - $item->{$db_fieldname} = $file_name; + if ($wroteNewFile) { + // Only touch the existing image and the model reference AFTER + // confirming the new file is on disk. deleteExistingImage + // itself now also refuses to null the model when the delete + // fails, so a partial cleanup does not leave the model + // pointing at a phantom file either way. + $item = $this->deleteExistingImage($item, $path, $db_fieldname); + $item->{$db_fieldname} = $file_name; + } else { + Log::warning('Image upload failed to write to disk; keeping existing image reference intact.', [ + 'item_type' => $type, + 'item_id' => $item->id ?? null, + 'target_path' => $prefix.$file_name, + ]); + } // If the user isn't uploading anything new but wants to delete their old image, do so } elseif ($this->input('image_delete') == '1') { @@ -166,13 +187,28 @@ class ImageUploadRequest extends Request if ($item->{$db_fieldname} != '') { try { - // Same path normalization as handleImages — branding callers + // Same path normalization as handleImages. Branding callers // pass '' for the disk root, and we don't want to produce a // leading-slash key on S3. $path = trim((string) $path, '/'); $key = $path === '' ? $item->{$db_fieldname} : $path.'/'.$item->{$db_fieldname}; - Storage::disk('public')->delete($key); - $item->{$db_fieldname} = null; + $deleted = Storage::disk('public')->delete($key); + + // Only null the model reference if the delete actually + // succeeded. Before, the field was cleared unconditionally + // even when Storage::delete returned false (silent-fail + // mode on the default local disk). The result was a model + // row that reported "no image" while the file remained on + // disk, orphaned. + if ($deleted) { + $item->{$db_fieldname} = null; + } else { + Log::warning('Storage delete returned false; keeping model reference so operators can retry.', [ + 'item_type' => class_basename(get_class($item)), + 'item_id' => $item->id ?? null, + 'key' => $key, + ]); + } } catch (\Exception $e) { Log::debug($e); } diff --git a/tests/Feature/Uploads/ImageUploadRequestFailurePathsTest.php b/tests/Feature/Uploads/ImageUploadRequestFailurePathsTest.php new file mode 100644 index 0000000000..9869236710 --- /dev/null +++ b/tests/Feature/Uploads/ImageUploadRequestFailurePathsTest.php @@ -0,0 +1,146 @@ +image = $new; // unconditional + * + * If put returned false (silent-fail mode on the default local disk), the + * old file was destroyed and the model row ended up referencing a file + * that never landed on disk. The fix captures the put return, only + * proceeds to delete + reassign when the write succeeded, and mirrors the + * same check inside deleteExistingImage so a failed delete does not null + * out the model reference either. + * + * We exercise the fix through the manufacturers update endpoint because it + * has a straightforward image field and the fewest confounding variables + * of the many handleImages callers, but the fix applies to every model + * whose controller ultimately routes through ImageUploadRequest. + */ +class ImageUploadRequestFailurePathsTest extends TestCase +{ + public function test_failed_new_image_write_preserves_existing_image_reference(): void + { + Storage::fake('public'); + $publicDisk = Storage::disk('public'); + + // Seed a real "existing image" file so the test can assert it still + // exists on disk after the failed write. + $publicDisk->put('manufacturers/manufacturer-image-pre-existing.png', 'original bytes'); + + $manufacturer = Manufacturer::factory()->create([ + 'image' => 'manufacturer-image-pre-existing.png', + ]); + + // Proxy the public disk so put() returns false (simulating a write + // failure that Storage does not throw on) while every other call + // still passes through to the real fake disk. + $proxy = Mockery::mock($publicDisk); + $proxy->shouldReceive('put')->andReturn(false); + $proxy->shouldReceive('exists')->andReturnUsing(fn (...$a) => $publicDisk->exists(...$a)); + $proxy->shouldReceive('delete')->andReturnUsing(fn (...$a) => $publicDisk->delete(...$a)); + $proxy->shouldReceive('makeDirectory')->andReturnUsing(fn (...$a) => $publicDisk->makeDirectory(...$a)); + Storage::shouldReceive('disk')->with('public')->andReturn($proxy); + + $this->actingAs(User::factory()->superuser()->create()) + ->put(route('manufacturers.update', $manufacturer), [ + 'name' => $manufacturer->name, + 'image' => UploadedFile::fake()->image('replacement.png'), + ]) + ->assertStatus(302) + ->assertSessionHasNoErrors(); + + $manufacturer->refresh(); + $this->assertEquals( + 'manufacturer-image-pre-existing.png', + $manufacturer->image, + 'A failed new-image write must not clear or replace the existing image reference on the model.', + ); + $this->assertTrue( + $publicDisk->exists('manufacturers/manufacturer-image-pre-existing.png'), + 'A failed new-image write must not delete the existing image from disk.', + ); + } + + public function test_failed_delete_preserves_model_reference(): void + { + // Directly exercises ImageUploadRequest::deleteExistingImage in + // isolation because the ManufacturersController path nulls the + // model reference itself before handing off to handleImages (see + // ManufacturersController::update line 153). The controller-level + // pre-null defeats the deleteExistingImage guard for that specific + // caller. Every other caller (ProfileController::update for the + // user avatar, and any future callsite that relies on + // deleteExistingImage to null the field) benefits from the guard. + Storage::fake('public'); + $publicDisk = Storage::disk('public'); + $publicDisk->put('manufacturers/manufacturer-image-still-there.png', 'still here'); + + $manufacturer = Manufacturer::factory()->create([ + 'image' => 'manufacturer-image-still-there.png', + ]); + + $proxy = Mockery::mock($publicDisk); + $proxy->shouldReceive('delete')->andReturn(false); + $proxy->shouldReceive('exists')->andReturnUsing(fn (...$a) => $publicDisk->exists(...$a)); + $proxy->shouldReceive('put')->andReturnUsing(fn (...$a) => $publicDisk->put(...$a)); + $proxy->shouldReceive('makeDirectory')->andReturnUsing(fn (...$a) => $publicDisk->makeDirectory(...$a)); + Storage::shouldReceive('disk')->with('public')->andReturn($proxy); + + $request = app(\App\Http\Requests\ImageUploadRequest::class); + $result = $request->deleteExistingImage($manufacturer, 'manufacturers', 'image'); + + $this->assertEquals( + 'manufacturer-image-still-there.png', + $result->image, + 'A failed delete must not null the model image reference - the file may still be on disk and orphaning it is worse than leaving the reference intact.', + ); + } + + public function test_successful_write_still_replaces_existing_image(): void + { + // Sanity: the fix must not regress the happy path. Successful writes + // continue to delete the old file and update the model reference. + Storage::fake('public'); + $publicDisk = Storage::disk('public'); + $publicDisk->put('manufacturers/manufacturer-image-old.png', 'old bytes'); + + $manufacturer = Manufacturer::factory()->create([ + 'image' => 'manufacturer-image-old.png', + ]); + + $this->actingAs(User::factory()->superuser()->create()) + ->put(route('manufacturers.update', $manufacturer), [ + 'name' => $manufacturer->name, + 'image' => UploadedFile::fake()->image('new.png'), + ]) + ->assertStatus(302) + ->assertSessionHasNoErrors(); + + $manufacturer->refresh(); + $this->assertNotEquals('manufacturer-image-old.png', $manufacturer->image); + $this->assertNotNull($manufacturer->image); + $this->assertStringContainsString('Manufacturer-image', $manufacturer->image); + $this->assertFalse( + $publicDisk->exists('manufacturers/manufacturer-image-old.png'), + 'On a successful new write, the old image file is deleted from disk.', + ); + $this->assertTrue( + $publicDisk->exists('manufacturers/'.$manufacturer->image), + 'On a successful new write, the new file lands at the expected path.', + ); + } +}