Skip to content

[6.x] Guard ImageGenerator against a null asset #15447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wakqasahmed wants to merge 3 commits into
base: 6.x
Choose a base branch
from
+46 −2
Open

[6.x] Guard ImageGenerator against a null asset #15447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c94f024
Guard ImageGenerator::generateByAsset() against a null asset (#15359)
wakqasahmed Sep 11, 2026
199dbca
Merge remote-tracking branch 'origin/6.x' into fix/issue-15359-glide-…
wakqasahmed Sep 11, 2026
7536259
Log the missing item, not a flat message, when an asset can't be reso…
wakqasahmed Sep 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/Imaging/ImageGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Statamic\Imaging;

use Facades\Statamic\Imaging\ImageValidator;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Facades\Storage;
use League\Flysystem\Filesystem;
use League\Flysystem\UnableToReadFile;
Expand Down Expand Up @@ -150,11 +151,17 @@ public function generateVideoThumbnail($asset, array $params)
/**
* Generate a manipulated image by an asset.
*
* @param \Statamic\Contracts\Assets\Asset $asset
* @param \Statamic\Contracts\Assets\Asset|null $asset
* @return mixed
*/
public function generateByAsset($asset, array $params)
{
if (! $asset) {

@wakqasahmed wakqasahmed Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the crash is real: Glide::generateImage() (src/Tags/Glide.php:180) does call generateByAsset(Asset::find($item), $params) unguarded, and Asset::find() can return null (e.g. when an item resolves as an asset ID/instance rather than a raw path, so it skips the Str::isUrl() branch and falls through to this line). So a null asset really can reach isVideo() here pre-fix — good catch, and the regression test faithfully reproduces the exact Error from the issue (fails pre-fix, passes post-fix).\n\nOne gap though: this silently returns '' with no Log::error() (or equivalent). The issue's own "Expected" behavior is "an asset that cannot be resolved is logged and skipped, like every other error inside the tag" — every other failure mode in Glide::generate()'s closure hits the catch (\Exception $e) { Log::error(...) } and gets logged. This new path is the one exception: it degrades gracefully but leaves zero trace, so the underlying "asset repository returned null for a resolvable file" condition (the actual bug, per statamic/eloquent-driver#609) becomes invisible/undebuggable in production — which is exactly the visibility gap that made the original 500 take a day to notice at scale.\n\nWorth considering: either add a Log::error()/Log::warning() call here before returning '', or (closer to the issue's first suggested alternative) guard in Glide::generateImage() by throwing an \Exception when Asset::find($item) is null, letting the tag's existing catch (\Exception) log it — that would also mean the 3 other callers of generateByAsset() (PresetGenerator, StaticUrlBuilder, ThumbnailController), which always pass an already-resolved asset, don't carry a defensive null-check they don't need. Not a blocker — the fix does stop the 500 — but as-is a real production trigger of this bug will still go unnoticed.

@wakqasahmed wakqasahmed Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair catch, added — Log::error() now fires before the early return (also rebased onto 6.x, which had picked up an unrelated conflicting change to this same method in the meantime).

@jasonvarga jasonvarga Sep 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new Log::error('Cannot generate an image for a missing asset.') carries no identifying context (no item, path, or asset ID). The original issue's complaint was that a bad asset silently 500'd for a day before anyone noticed; this line does add a log, but by the time generateByAsset() sees $asset === null, the identifying info from the caller ($item in Glide::generateGlideUrl/generateImage) is already gone. Every other failure path in Glide::generate() logs $e->getMessage(), which carries exception context — this one is a flat string that will look identical for every occurrence, so it's still not possible to tell which asset/URL is failing without reproducing it.\n\nWorth either passing/logging the original $item reference here, or (per the earlier thread on this line) throwing from Glide::generateImage() so the existing catch (\\Exception $e) logs a message that includes the offending item.

@wakqasahmed wakqasahmed Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair — moved the guard to Glide::generateImage() per your first suggestion: it now throws with $item in the message when Asset::find() comes back null, so the existing catch (\Exception $e) { Log::error($e->getMessage()); } in generate() logs it with the identifying item instead of a flat string. generateByAsset()'s own null guard stays as a defensive fallback for any other caller, but nothing currently reaches it with null. Added a test asserting the log message contains the item.

Log::error('Cannot generate an image for a missing asset.');

return '';
}

if ($asset->isVideo() && ThumbnailExtractor::available()) {
return $this->generateVideoThumbnail($asset, $params);
}
Expand Down
13 changes: 12 additions & 1 deletion src/Tags/Glide.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,18 @@ private function generateImage($item)
: $this->getGenerator()->generateByPath($item, $params);
}

return $this->getGenerator()->generateByAsset(Asset::find($item), $params);
$asset = Asset::find($item);

if (! $asset) {
// Thrown (rather than logged here directly) so the calling closure's
// existing catch (\Exception $e) { Log::error($e->getMessage()); }
// in generate() logs it with the identifying $item, instead of the
// flat, context-free message generateByAsset()'s own null-asset
// guard would otherwise produce.
throw new \Exception('Cannot generate an image for a missing asset: '.(is_string($item) ? $item : json_encode($item)));
}

return $this->getGenerator()->generateByAsset($asset, $params);
}

/**
Expand Down
6 changes: 6 additions & 0 deletions tests/Imaging/ImageGeneratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,12 @@ public function it_generates_an_image_by_asset()
Event::assertDispatchedTimes(GlideImageGenerated::class, 1);
}

#[Test]
public function it_does_not_generate_an_image_for_a_missing_asset()
{
$this->assertSame('', $this->makeGenerator()->generateByAsset(null, ['w' => 100]));
}

#[Test]
public function it_does_not_check_ffmpeg_availability_for_non_video_assets()
{
Expand Down
20 changes: 20 additions & 0 deletions tests/Tags/GlideTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Tests\Tags;

use Illuminate\Http\UploadedFile;
use Illuminate\Support\Facades\Log;
use Orchestra\Testbench\Attributes\DefineEnvironment;
use PHPUnit\Framework\Attributes\Test;
use Statamic\Facades\File;
Expand All @@ -11,6 +12,25 @@

class GlideTest extends TestCase
{
#[Test]
/**
* https://github.com/statamic/cms/pull/15447
*/
public function it_logs_the_item_when_the_asset_cannot_be_resolved()
{
Log::shouldReceive('error')
->once()
->with(\Mockery::pattern('/Cannot generate an image for a missing asset.*nonexistent\.jpg/'));

$result = (string) Parse::template(
'{{ glide:foo width="100" }}',
['foo' => 'nonexistent.jpg'],
trusted: true
);

$this->assertSame('', $result);
}

#[Test]
#[DefineEnvironment('relativeRouteUrl')]
public function it_outputs_a_relative_url_by_default_when_the_glide_route_is_relative()
Expand Down