SEC-1647: Avoid error should the media not have a file. #36
Conversation
WalkthroughIntroduced a guard in UrlGenerationTrait::getGeneratedUrl to throw CacheableNotFoundHttpException when the resolved file_id is empty. All other URL generation logic remains unchanged, including file loading, missing image handling, and URL construction from the image URI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Trait as UrlGenerationTrait
participant MS as Media Source
participant Media
participant Files as File Storage
Client->>Trait: getGeneratedUrl(node, media)
Trait->>MS: resolve file_id from media source
MS-->>Trait: file_id (may be empty)
alt file_id is empty (new guard)
Trait-->>Client: throw CacheableNotFoundHttpException\n"No file ID for discovered media (mid) for node (nid)"
else file_id present
Trait->>Files: load file by file_id
alt file not found / missing image
Trait-->>Client: existing handling (e.g., not found)
else file found
Trait->>Trait: build URL from image URI
Trait-->>Client: return generated URL
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Plugin/dgi_image_discovery/url_generator/UrlGenerationTrait.php (2)
62-69: Harden type validation and cast $file_id before loading.Depending on media source implementations, getSourceFieldValue($media) can sometimes be non-scalar (e.g., array-like structures) or a non-integer string. Strengthen the check to ensure a positive integer and cast before load to avoid unexpected type errors.
Apply this diff:
$media_source = $media->getSource(); $file_id = $media_source->getSourceFieldValue($media); - if (empty($file_id)) { - throw new CacheableNotFoundHttpException($generated_url, "No file ID for discovered media ({$media->id()}) for node ({$node->id()})."); - } + // Ensure we have a positive integer file ID; reject non-scalar or non-numeric values. + if (!is_scalar($file_id) || !ctype_digit((string) $file_id) || (int) $file_id <= 0) { + throw new CacheableNotFoundHttpException( + $generated_url, + "No valid file ID for discovered media ({$media->id()}) for node ({$node->id()})." + ); + } + $file_id = (int) $file_id; /** @var \Drupal\file\FileInterface|null $image */ $image = $this->getEntityTypeManager()->getStorage('file')->load($file_id);
48-76: Add regression tests for “no media/file” scenariosTo ensure the behavior of UrlGenerationTrait remains locked and clearly documented, please add tests covering these cases:
• Test UrlGenerationTrait::getGeneratedUrl directly
– Stub a NodeInterface and ImageDiscoveryInterface so that
• getImage() returns an event with no media
• getImage() returns an event whose media’s source field value is empty
• getImage() returns an event whose media source yields a file ID that cannot be loaded
– In each scenario, assert that getGeneratedUrl throws a Drupal\Core\Http\Exception\CacheableNotFoundHttpException (as declared in UrlGenerationTrait) [see UrlGenerationTrait.php lines 55–71].• Test PreGenerated::generate maps exceptions to null
– Call PreGenerated::generate(...) with the same stubbed failure scenarios above.
– Assert it returns null (confirming it catches NotFoundHttpException, including CacheableNotFoundHttpException) [see PreGenerated.php lines 49–53].(Optional) If you rely on the deferred plugins (Redirect, Subrequest) elsewhere, consider an integration or functional test to verify that they propagate the exception as expected when resolving URLs.
By adding these tests, you’ll guard against regressions and clearly document the intended “no image” behavior for both indexers (via PreGenerated) and direct trait usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Plugin/dgi_image_discovery/url_generator/UrlGenerationTrait.php(1 hunks)
🔇 Additional comments (1)
src/Plugin/dgi_image_discovery/url_generator/UrlGenerationTrait.php (1)
64-66: Good guard to prevent fatal on NULL file IDs.This early return via CacheableNotFoundHttpException avoids the prior assertion on load(NULL) and maintains consistent cacheability behavior with the other not-found branches. Looks correct and aligned with the intent of SEC-1647.
Something of an odd case... wanting to say that this seems like an issue with some invalid media entities kicking around which are missing a value pointing at an image? Indeed, found evidence of "image" media lacking any related file being referenced.
Stacktrace
Summary by CodeRabbit