adds image management. #24
Conversation
📝 WalkthroughWalkthroughAdds a media library, picker modal and upload flow backed by Cloudinary; EditorJS embed support and renderer; CsrfToken now requires SessionManager; installer collects interactive email config; admin UI/nav updated; TESTING.md and debug-maintenance.php removed; controllers, services, views, and tests added/updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Browser
participant Modal as Media Picker Modal
participant Server as App Server
participant Cloudinary as Cloudinary API
rect rgb(220,235,250)
Note over User,Cloudinary: Library Browse & Select Flow
User->>Modal: openMediaPicker(targetInput)
Modal->>Server: GET /admin/media?cursor=...
Server->>Cloudinary: CloudinaryUploader.listResources(options)
Cloudinary-->>Server: resources + next_cursor
Server-->>Modal: Rendered media cards HTML
Modal->>Modal: Display grid, attach handlers
User->>Modal: Click media card
Modal->>Modal: Mark selection, enable "Select Image"
User->>Modal: Click "Select Image"
Modal->>User: Inject URL into target input, dispatch change, close modal
end
sequenceDiagram
participant User as User/Browser
participant Modal as Media Picker Modal
participant Server as App Server
participant Cloudinary as Cloudinary API
rect rgb(250,235,220)
Note over User,Cloudinary: Upload Flow
User->>Modal: Switch to Upload tab, choose file
User->>Modal: Click Upload
Modal->>Server: POST /admin/media/featured-upload (FormData + CSRF)
Server->>Cloudinary: Upload asset to folder
Cloudinary-->>Server: uploaded resource metadata
Server-->>Modal: Success response with URL
Modal->>Modal: Insert URL into target input, show success, refresh library or reload
end
sequenceDiagram
participant Editor as EditorJS
participant Renderer as EditorJsRenderer
participant Validator as Domain Validator
rect rgb(230,250,230)
Note over Editor,Validator: Embed Block Rendering
Editor->>Renderer: renderBlock(type='embed', data)
Renderer->>Renderer: renderEmbed(data)
Renderer->>Validator: Validate URL domain
alt domain trusted & url present
Validator-->>Renderer: OK
Renderer-->>Editor: <figure> with responsive iframe + optional caption
else missing or untrusted
Validator-->>Renderer: Reject
Renderer-->>Editor: HTML comment warning (no iframe)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
resources/views/admin/posts/create.php (1)
222-232: Consider using 'input' event and adding error handling for image preview.The
changeevent on URL inputs only fires on blur. Usinginputwould provide real-time feedback. Additionally, broken image URLs will show a broken image icon.🔎 Proposed improvement
-document.getElementById('featured_image').addEventListener('change', function() { +document.getElementById('featured_image').addEventListener('input', function() { const preview = document.getElementById('featured_image_preview'); const url = this.value.trim(); if (url) { preview.src = url; preview.classList.remove('d-none'); + preview.onerror = function() { + this.classList.add('d-none'); + }; } else { preview.classList.add('d-none'); } });resources/views/partials/media-picker-modal.php (2)
311-316: Add client-side file size validation before upload.The form text mentions "Max size: 5MB" but there's no validation before initiating the upload. Users could attempt to upload large files and only fail after waiting.
🔎 Proposed fix
uploadBtn?.addEventListener('click', function() { if (!imageFile.files.length) { uploadError.textContent = 'Please select a file'; uploadError.classList.remove('d-none'); return; } + + const maxSize = 5 * 1024 * 1024; // 5MB + if (imageFile.files[0].size > maxSize) { + uploadError.textContent = 'File size exceeds 5MB limit'; + uploadError.classList.remove('d-none'); + return; + } const formData = new FormData();
166-172: HTML parsing approach for media library is fragile.Fetching the full page HTML and parsing it with DOMParser couples the modal tightly to the media library page structure. Consider adding a dedicated JSON API endpoint for better maintainability.
This approach will break if the media library page structure changes. A dedicated API endpoint returning JSON would be more robust and performant.
src/Cms/Cli/Commands/Install/InstallCommand.php (1)
695-702: Consider not logging SMTP host and port in messages.Logging
"Email: SMTP ($host:$port)"to_messagesmay expose infrastructure details in the installation summary. While minor, this could be a security consideration.Consider logging just
"Email: SMTP configured"instead of including the host and port details.resources/views/admin/media/index.php (3)
165-176: Delete functionality is incomplete.The delete button shows a confirmation dialog but only displays an alert instead of actually deleting the image. This should either be implemented or the button should be hidden/disabled until the API is ready.
Would you like me to help implement the delete API endpoint, or should the delete button be hidden until the feature is complete?
186-188: Add file validation before initiating upload.Same as in the media picker modal - validate file size (5MB limit mentioned in the form text) and file type before starting the upload to provide immediate feedback.
🔎 Proposed fix
uploadBtn.addEventListener('click', function() { + if (!imageFile.files.length) { + uploadError.textContent = 'Please select a file'; + uploadError.classList.remove('d-none'); + return; + } + + const file = imageFile.files[0]; + const maxSize = 5 * 1024 * 1024; // 5MB + if (file.size > maxSize) { + uploadError.textContent = 'File size exceeds 5MB limit'; + uploadError.classList.remove('d-none'); + return; + } + const formData = new FormData(); formData.append('image', imageFile.files[0]);
150-163: Consider adding clipboard API fallback for older browsers.
navigator.clipboardis not available in all browsers or non-HTTPS contexts. Consider a fallback usingdocument.execCommand('copy')or gracefully degrading.🔎 Proposed fallback
btn.addEventListener('click', function() { const url = this.dataset.url; - navigator.clipboard.writeText(url).then(() => { + const copyToClipboard = (text) => { + if (navigator.clipboard) { + return navigator.clipboard.writeText(text); + } + // Fallback for older browsers + const textarea = document.createElement('textarea'); + textarea.value = text; + document.body.appendChild(textarea); + textarea.select(); + document.execCommand('copy'); + document.body.removeChild(textarea); + return Promise.resolve(); + }; + + copyToClipboard(url).then(() => { const originalHtml = this.innerHTML;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
TESTING.mddebug-maintenance.phpresources/app/Initializers/AuthInitializer.phpresources/config/routes.yamlresources/views/admin/dashboard/index.phpresources/views/admin/media/index.phpresources/views/admin/pages/create.phpresources/views/admin/pages/edit.phpresources/views/admin/posts/create.phpresources/views/admin/posts/edit.phpresources/views/layouts/admin.phpresources/views/partials/media-picker-modal.phpsrc/Cms/Cli/Commands/Install/InstallCommand.phpsrc/Cms/Controllers/Admin/Media.phpsrc/Cms/Services/Content/EditorJsRenderer.phpsrc/Cms/Services/Media/CloudinaryUploader.phptests/Unit/Cms/Services/Content/EditorJsRendererTest.phpversionlog.md
💤 Files with no reviewable changes (3)
- TESTING.md
- versionlog.md
- debug-maintenance.php
🧰 Additional context used
🧬 Code graph analysis (8)
src/Cms/Controllers/Admin/Media.php (1)
src/Cms/Services/Media/CloudinaryUploader.php (1)
listResources(149-184)
resources/views/partials/media-picker-modal.php (1)
src/Cms/View/helpers.php (1)
route_path(70-79)
resources/app/Initializers/AuthInitializer.php (1)
src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)
resources/views/admin/posts/edit.php (1)
src/Cms/Models/Post.php (1)
getFeaturedImage(191-194)
resources/views/admin/media/index.php (1)
src/Cms/View/helpers.php (1)
route_path(70-79)
tests/Unit/Cms/Services/Content/EditorJsRendererTest.php (2)
src/Cms/Services/Content/EditorJsRenderer.php (1)
render(27-37)tests/Unit/Cms/Services/Widget/WidgetTest.php (1)
render(16-19)
resources/views/layouts/admin.php (1)
src/Cms/View/helpers.php (1)
route_path(70-79)
src/Cms/Cli/Commands/Install/InstallCommand.php (2)
src/Cms/Cli/Commands/Maintenance/EnableCommand.php (1)
confirm(222-227)src/Cms/Cli/Commands/Maintenance/DisableCommand.php (1)
confirm(136-141)
🪛 PHPMD (2.15.0)
src/Cms/Services/Content/EditorJsRenderer.php
255-255: Avoid unused local variables such as '$service'. (undefined)
(UnusedLocalVariable)
256-256: Avoid unused local variables such as '$source'. (undefined)
(UnusedLocalVariable)
258-258: Avoid unused local variables such as '$width'. (undefined)
(UnusedLocalVariable)
259-259: Avoid unused local variables such as '$height'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (17)
resources/app/Initializers/AuthInitializer.php (1)
57-67: LGTM! Proper dependency injection implementation.The change correctly injects the
SessionManagerinstance into theCsrfTokenconstructor, aligning with the updated constructor signature. The dependency flow is sound:SessionManageris instantiated first (line 59), then properly passed toCsrfToken(line 62), and both are correctly utilized in the authentication filters.resources/views/layouts/admin.php (1)
32-55: LGTM! Well-structured navigation enhancement.The new Content and Users dropdown menus provide better organization for the admin interface. The implementation uses Bootstrap classes correctly and integrates properly with the routing system.
resources/views/admin/pages/edit.php (1)
135-135: LGTM! Consistent embed tool integration.The EditorJS embed plugin is properly loaded and configured with appropriate embed services. Version pinning ensures stability.
Also applies to: 187-201
resources/views/admin/pages/create.php (1)
116-116: LGTM! Consistent with the edit view.The embed tool integration follows the same pattern as the edit view, maintaining consistency across create and edit workflows.
Also applies to: 155-169
resources/config/routes.yaml (1)
254-260: LGTM! Proper route configuration.The new media management route is correctly configured with authentication. The CSRF protection on the upload route (line 265) is a security improvement.
src/Cms/Services/Media/CloudinaryUploader.php (1)
142-184: LGTM! Well-structured resource listing implementation.The method properly integrates with Cloudinary's Admin API, supports pagination, and reuses the existing
formatResultmethod for consistency. Error handling appropriately wraps exceptions.src/Cms/Services/Content/EditorJsRenderer.php (2)
269-329: Excellent security implementation for embed rendering.The method implements multiple security layers:
- Domain whitelist to prevent malicious embeds
htmlspecialchars()to prevent XSSsandboxattribute on iframes to restrict capabilities- Graceful handling of missing/invalid URLs
304-304: The project explicitly requires PHP 8.4+ (as stated in readme.md), sostr_ends_with()(introduced in PHP 8.0) is fully compatible and requires no polyfill or alternative implementation.Likely an incorrect or invalid review comment.
resources/views/admin/posts/edit.php (1)
47-60: LGTM! Comprehensive media management integration.The changes successfully integrate:
- Media picker with browse functionality for featured images
- Live image preview for better UX
- EditorJS embed tool for rich content
- Consistent implementation across the admin interface
The implementation is user-friendly and maintains consistency with other admin views.
Also applies to: 103-103, 129-136, 195-209, 247-258, 261-261
resources/views/admin/dashboard/index.php (1)
7-73: Well-organized dashboard navigation structure.The Quick Access sections are logically grouped (Content Management, Organization, Users & Settings) with consistent styling and appropriate icons. The responsive grid layout ensures good usability across device sizes.
resources/views/admin/posts/create.php (1)
91-91: Embed tool integration looks correct.The Embed plugin is properly loaded and configured with appropriate services (YouTube, Vimeo, Twitter, Instagram, Facebook, CodePen, GitHub).
Also applies to: 170-183
tests/Unit/Cms/Services/Content/EditorJsRendererTest.php (3)
562-588: Comprehensive embed block test coverage.The YouTube embed test properly validates iframe presence, domain, caption, sandbox attributes, and figure wrapper. Good coverage of the core embed functionality.
654-672: Good security test for untrusted domains.This test ensures that embeds from untrusted domains are blocked and no iframe is rendered, preventing potential XSS or content injection attacks.
715-736: XSS protection test for embed captions is well-designed.Verifies that script tags in captions are properly escaped to
<script>, preventing XSS attacks through malicious captions.resources/views/partials/media-picker-modal.php (1)
125-143: Well-structured IIFE pattern for modal functionality.The use of an IIFE prevents global namespace pollution while still exposing
openMediaPickeron the window object for external callers. State management with module-scoped variables is clean.src/Cms/Cli/Commands/Install/InstallCommand.php (1)
602-631: Well-structured email configuration flow.The email configuration follows the established pattern from Cloudinary configuration, with clear prompts, sensible defaults, and proper test-mode fallback when the user skips configuration.
resources/views/admin/media/index.php (1)
27-77: Clean responsive grid layout for media items.The media card layout with consistent styling, hover effects, and action buttons provides a good user experience. Proper use of
htmlspecialchars()for output escaping andloading="lazy"for performance.
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…into feature/image-management
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Cms/Controllers/Admin/Media.php (1)
91-112: Exception details still exposed to users despite previous review.The error logging on line 93 addresses part of the previous review comment, but line 102 still passes
$e->getMessage()directly to the view. Cloudinary exceptions may contain sensitive information such as API credentials, internal paths, configuration details, or error specifics that should not be shown to end users.🔎 Recommended fix to use generic error message
catch( \Exception $e ) { Log::error( 'Error fetching media resources: ' . $e->getMessage() ); $viewData = [ 'Title' => 'Media Library | ' . $this->getName(), 'Description' => 'Manage uploaded images', 'User' => $user, 'resources' => [], 'nextCursor' => null, 'totalCount' => 0, - 'error' => $e->getMessage() + 'error' => 'Unable to load media library. Please try again later.' ]; return $this->renderHtml( HttpResponseStatus::OK, $viewData, 'index', 'admin' ); }
🧹 Nitpick comments (3)
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (1)
288-298: Consider extracting credential-checking logic.The three new tests duplicate identical credential-checking logic. Consider extracting this into a helper method to improve maintainability:
🔎 Suggested refactor to reduce duplication
Add a helper method to the test class:
private function skipIfNoCredentials(): void { $cloudName = $this->_settings->get( 'cloudinary', 'cloud_name' ); $isTestCredentials = ($cloudName === 'test-cloud'); $hasRealCredentials = getenv( 'CLOUDINARY_URL' ) || (!$isTestCredentials && $cloudName); if( !$hasRealCredentials ) { $this->markTestSkipped( 'Cloudinary credentials not configured. Set CLOUDINARY_URL environment variable or configure real cloudinary settings to run this integration test.' ); } }Then replace the duplicated blocks in each test with:
public function testListResourcesReturnsExpectedStructure(): void { - // Skip if using test credentials (not real Cloudinary account) - $cloudName = $this->_settings->get( 'cloudinary', 'cloud_name' ); - $isTestCredentials = ($cloudName === 'test-cloud'); - $hasRealCredentials = getenv( 'CLOUDINARY_URL' ) || (!$isTestCredentials && $cloudName); - - if( !$hasRealCredentials ) - { - $this->markTestSkipped( - 'Cloudinary credentials not configured. Set CLOUDINARY_URL environment variable or configure real cloudinary settings to run this integration test.' - ); - } + $this->skipIfNoCredentials(); $uploader = new CloudinaryUploader( $this->_settings );Note: You could also apply this refactoring to the existing integration tests (lines 210-222, 241-253, 264-276) for consistency.
Also applies to: 327-337, 356-366
src/Cms/Cli/Commands/Install/InstallCommand.php (2)
623-630: Consider extracting duplicate test-mode config to a helper.The same test-mode fallback configuration array is repeated 4 times. Extracting to a private method or constant would reduce duplication and ensure consistency if the defaults change.
🔎 Proposed refactor
+ /** + * Get default test-mode email configuration + */ + private function getTestModeEmailConfig(): array + { + return [ + 'email' => [ + 'driver' => 'mail', + 'test_mode' => true, + 'from_address' => 'noreply@example.com', + 'from_name' => 'Neuron CMS' + ] + ]; + }Then replace each occurrence with:
return $this->getTestModeEmailConfig();Also applies to: 665-672, 691-698, 707-713
717-717: Consider validating port range.The port is cast to integer but not validated. Invalid ports (e.g., negative or >65535) would be stored without warning.
🔎 Proposed validation
$port = $this->input->ask( "SMTP port", "587" ); + $portInt = (int)$port; + if( $portInt < 1 || $portInt > 65535 ) + { + $this->output->warning( "Invalid port number. Using default 587." ); + $portInt = 587; + } $encryption = $this->input->choice(Then use
$portIntinstead of(int)$porton line 717.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Cms/Cli/Commands/Install/InstallCommand.phpsrc/Cms/Controllers/Admin/Media.phptests/Unit/Cms/Services/Content/EditorJsRendererTest.phptests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/Cms/Services/Content/EditorJsRendererTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (1)
src/Cms/Services/Media/CloudinaryUploader.php (2)
CloudinaryUploader(17-371)listResources(149-184)
src/Cms/Controllers/Admin/Media.php (2)
src/Cms/Controllers/Content.php (1)
Content(61-302)src/Cms/Services/Media/CloudinaryUploader.php (3)
CloudinaryUploader(17-371)__construct(28-32)listResources(149-184)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (postgres)
🔇 Additional comments (9)
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (3)
286-323: LGTM! Comprehensive structure validation.The test properly verifies the response structure from
listResources()and conditionally validates resource properties when resources are present. The approach of checking structural keys is appropriate for an integration test.
325-352: LGTM! Validates max_results option.The test correctly verifies that the
max_resultsoption is respected when listing resources.
354-398: LGTM! Pagination logic validated.The test properly exercises the pagination flow by fetching multiple pages and verifying that different resources are returned. The conditional logic handles both scenarios (with and without pagination) appropriately.
src/Cms/Cli/Commands/Install/InstallCommand.php (3)
482-486: LGTM!Clean integration of email configuration following the existing pattern established for Cloudinary.
662-714: Previous issue addressed: Validation failures now fall back to test mode.The SMTP validation failures (missing host/username/password) now correctly return a test-mode configuration instead of an empty array. This ensures email is always configured in a valid state.
756-756: LGTM!The method signature extension and email config merging follow the established pattern used for Cloudinary configuration. The default empty array preserves backward compatibility.
Also applies to: 796-800
src/Cms/Controllers/Admin/Media.php (3)
5-5: LGTM: Class hierarchy and imports updated correctly.The change to extend
Contentinstead of a baseControlleraligns with the parent class constructor pattern and provides access to rendering methods likerenderHtml(). The new imports forLogandHttpResponseStatussupport the error handling and response status functionality in theindex()method.Also applies to: 9-10, 20-20
27-32: LGTM: Constructor signature updated correctly.The constructor now accepts an optional
Applicationparameter and correctly passes it to the parentContentconstructor, matching the parent class signature shown in the relevant code snippets. The existing uploader and validator initialization logic is preserved.
55-60: LGTM: User authentication check is appropriate.The authentication check ensures a user is present before rendering the admin media library. Throwing a
RuntimeExceptionfor a missing authenticated user in an admin context is appropriate, as this represents an exceptional condition that should be caught by upstream middleware or error handlers.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Cms/Controllers/Admin/Media.php (1)
93-113: Exception details still exposed to the view.While error logging was added (good!), line 104 still passes
$e->getMessage()directly to the view. Cloudinary exceptions may contain sensitive information like API configuration details or internal paths. Consider returning a generic user-facing message instead.🔎 Recommended fix
catch( \Exception $e ) { Log::error( 'Error fetching media resources: ' . $e->getMessage() ); $viewData = [ 'Title' => 'Media Library | ' . $this->getName(), 'Description' => 'Manage uploaded images', 'User' => $user, 'resources' => [], 'nextCursor' => null, 'totalCount' => 0, - 'error' => $e->getMessage() + 'error' => 'Unable to load media library. Please try again later.' ];
🧹 Nitpick comments (4)
tests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.php (2)
6-6: Remove unused import.The
UploadApiclass is imported but never used in this test file.🔎 Proposed fix
-use Cloudinary\Api\Upload\UploadApi;
36-76: Consider extracting mock setup into a helper method.The mock creation and reflection-based injection pattern is repeated across all seven test methods. Extracting this into a private helper method would improve maintainability and reduce duplication.
💡 Example helper method
private function createUploaderWithMock(AdminApi $adminApiMock): CloudinaryUploader { $cloudinaryMock = $this->createMock(Cloudinary::class); $cloudinaryMock->method('adminApi')->willReturn($adminApiMock); $uploader = new CloudinaryUploader($this->_settings); $reflection = new \ReflectionClass($uploader); $cloudinaryProperty = $reflection->getProperty('_cloudinary'); $cloudinaryProperty->setAccessible(true); $cloudinaryProperty->setValue($uploader, $cloudinaryMock); return $uploader; }Then each test could be simplified:
public function testListResourcesWithCustomMaxResults(): void { $adminApiMock = $this->createMock(AdminApi::class); $adminApiMock->expects($this->once()) ->method('assets') ->with($this->callback(function($options) { return $options['max_results'] === 10; })) ->willReturn([ 'resources' => [], 'next_cursor' => null, 'total_count' => 0 ]); $uploader = $this->createUploaderWithMock($adminApiMock); $result = $uploader->listResources(['max_results' => 10]); $this->assertIsArray($result); }Also applies to: 93-114, 123-144, 153-174, 183-212, 232-248, 260-273
src/Cms/Controllers/Admin/Media.php (1)
124-180: Consider unifying the response structure between upload methods.The two upload methods use different response shapes:
uploadImage():success: 0|1,messagefor errorsuploadFeaturedImage():success: true|false,errorfor errorsIf both methods are consumed by different systems (Editor.js vs. custom frontend), this is fine. Otherwise, consider aligning them for consistency.
Also applies to: 190-242
tests/Unit/Cms/Controllers/MediaUploadTest.php (1)
16-16: Missing test coverage forindex()method.The new
index()method in the Media controller is not covered by this test suite. Consider adding tests for:
- Successful resource listing
- Pagination with cursor
- Error handling when Cloudinary fails
- Unauthenticated user case
Would you like me to generate test cases for the
index()method?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Cms/Controllers/Admin/Media.phptests/Unit/Cms/Controllers/MediaUploadTest.phptests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.php (1)
src/Cms/Services/Media/CloudinaryUploader.php (2)
CloudinaryUploader(17-371)listResources(149-184)
src/Cms/Controllers/Admin/Media.php (3)
src/Cms/Controllers/Content.php (1)
Content(61-302)src/Cms/Services/Media/CloudinaryUploader.php (4)
CloudinaryUploader(17-371)__construct(28-32)listResources(149-184)upload(68-89)src/Cms/Services/Media/IMediaUploader.php (1)
upload(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (postgres)
- GitHub Check: build-test (mysql)
🔇 Additional comments (7)
tests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.php (1)
33-279: Excellent test coverage for the listResources functionality.The test suite comprehensively covers all key scenarios:
- Default parameter handling and correct API invocation
- Custom parameters (max_results, next_cursor, folder)
- Result formatting and structure validation
- Empty results handling
- Error propagation
The use of reflection to inject mocks aligns with existing test patterns, and all assertions appropriately verify both the Cloudinary API interactions and the returned data structure.
src/Cms/Controllers/Admin/Media.php (2)
5-14: LGTM - Class structure and imports are well-organized.The refactor to extend
Contentprovides proper site configuration initialization. The imports align with the framework patterns used in the methods.Also applies to: 22-22
32-45: Constructor pattern is consistent with the framework.The validation of Settings type and initialization of dependencies follows the established patterns in the codebase.
tests/Unit/Cms/Controllers/MediaUploadTest.php (4)
21-55: Good test isolation practices.The setup properly backs up and restores Registry state, and tearDown cleans up global state (
$_FILES). This prevents test pollution.
57-157: Error path tests are comprehensive.Good coverage of both upload methods' error conditions: missing files and validation failures. The reflection-based mock injection works for the current implementation.
159-261: Success path tests verify expected response contracts.The tests properly mock both validator and uploader, then verify the JSON response structure matches what consumers (Editor.js, frontend) expect.
263-345: Exception tests will need updating if error messages are genericized.These tests verify the current behavior where
$e->getMessage()is returned directly. If the security recommendation to use generic error messages is implemented, update these assertions accordingly:- $this->assertEquals( 'Upload failed', $json['message'] ); + $this->assertEquals( 'Image upload failed. Please try again.', $json['message'] );
| catch( \Exception $e ) | ||
| { | ||
| $this->returnEditorJsError( $e->getMessage() ); | ||
| return $this->renderJson( | ||
| HttpResponseStatus::INTERNAL_SERVER_ERROR, | ||
| [ | ||
| 'success' => 0, | ||
| 'message' => $e->getMessage() | ||
| ] | ||
| ); | ||
| } |
There was a problem hiding this comment.
Exception message exposed in error response.
The catch block returns the raw exception message which could expose sensitive internal details to API consumers. Consider logging the full error and returning a generic message.
🔎 Recommended fix
catch( \Exception $e )
{
+ Log::error( 'Image upload failed: ' . $e->getMessage() );
+
return $this->renderJson(
HttpResponseStatus::INTERNAL_SERVER_ERROR,
[
'success' => 0,
- 'message' => $e->getMessage()
+ 'message' => 'Image upload failed. Please try again.'
]
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| catch( \Exception $e ) | |
| { | |
| $this->returnEditorJsError( $e->getMessage() ); | |
| return $this->renderJson( | |
| HttpResponseStatus::INTERNAL_SERVER_ERROR, | |
| [ | |
| 'success' => 0, | |
| 'message' => $e->getMessage() | |
| ] | |
| ); | |
| } | |
| catch( \Exception $e ) | |
| { | |
| Log::error( 'Image upload failed: ' . $e->getMessage() ); | |
| return $this->renderJson( | |
| HttpResponseStatus::INTERNAL_SERVER_ERROR, | |
| [ | |
| 'success' => 0, | |
| 'message' => 'Image upload failed. Please try again.' | |
| ] | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/Cms/Controllers/Admin/Media.php around lines 170 to 179, the catch block
returns the raw exception message to the client which can leak sensitive
internals; instead log the full exception (message, stack trace) to your server
logger and return a generic error message and code in renderJson (e.g., 'An
internal error occurred' or similar) while preserving the same HTTP status;
ensure logging uses the exception object so details are retained for debugging
but not exposed to API consumers.
| catch( \Exception $e ) | ||
| { | ||
| $this->returnError( $e->getMessage() ); | ||
| return $this->renderJson( | ||
| HttpResponseStatus::INTERNAL_SERVER_ERROR, | ||
| [ | ||
| 'success' => false, | ||
| 'error' => $e->getMessage() | ||
| ] | ||
| ); | ||
| } |
There was a problem hiding this comment.
Exception message exposed in error response.
Same issue as uploadImage() - the raw exception message is returned to the client.
🔎 Recommended fix
catch( \Exception $e )
{
+ Log::error( 'Featured image upload failed: ' . $e->getMessage() );
+
return $this->renderJson(
HttpResponseStatus::INTERNAL_SERVER_ERROR,
[
'success' => false,
- 'error' => $e->getMessage()
+ 'error' => 'Featured image upload failed. Please try again.'
]
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| catch( \Exception $e ) | |
| { | |
| $this->returnError( $e->getMessage() ); | |
| return $this->renderJson( | |
| HttpResponseStatus::INTERNAL_SERVER_ERROR, | |
| [ | |
| 'success' => false, | |
| 'error' => $e->getMessage() | |
| ] | |
| ); | |
| } | |
| catch( \Exception $e ) | |
| { | |
| Log::error( 'Featured image upload failed: ' . $e->getMessage() ); | |
| return $this->renderJson( | |
| HttpResponseStatus::INTERNAL_SERVER_ERROR, | |
| [ | |
| 'success' => false, | |
| 'error' => 'Featured image upload failed. Please try again.' | |
| ] | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Cms/Controllers/Admin/Media.php (3)
93-114: Exception details still exposed to users.The error view on line 104 includes
$e->getMessage(), which can leak sensitive information such as API credentials, internal paths, or Cloudinary configuration details. While logging on line 95 is correct, the raw exception message should not be shown to users.🔎 Recommended fix
catch( \Exception $e ) { Log::error( 'Error fetching media resources: ' . $e->getMessage() ); $viewData = [ 'Title' => 'Media Library | ' . $this->getName(), 'Description' => 'Manage uploaded images', 'User' => $user, 'resources' => [], 'nextCursor' => null, 'totalCount' => 0, - 'error' => $e->getMessage() + 'error' => 'Unable to load media library. Please try again later.' ]; return $this->renderHtml( HttpResponseStatus::OK, $viewData, 'index', 'admin' ); }
170-182: Exception details exposed in API response.Line 178 returns the raw exception message to API consumers, which can leak sensitive internal details such as configuration errors, file system paths, or API credentials.
🔎 Recommended fix
catch( \Exception $e ) { Log::error( 'uploadImage: ' . $e->getMessage() ); return $this->renderJson( HttpResponseStatus::INTERNAL_SERVER_ERROR, [ 'success' => 0, - 'message' => $e->getMessage() + 'message' => 'Image upload failed. Please try again.' ] ); }
234-246: Exception details exposed in API response.Line 242 returns the raw exception message, which can leak sensitive information to API consumers. Additionally, line 236-237 has a stray semicolon after the Log::error call (minor syntax style issue).
🔎 Recommended fix
catch( \Exception $e ) { - Log::error( 'uploadFeaturedImage: ' . $e->getMessage() ) - ; + Log::error( 'uploadFeaturedImage: ' . $e->getMessage() ); + return $this->renderJson( HttpResponseStatus::INTERNAL_SERVER_ERROR, [ 'success' => false, - 'error' => $e->getMessage() + 'error' => 'Featured image upload failed. Please try again.' ] ); }
🧹 Nitpick comments (1)
tests/Unit/Cms/Services/Media/MediaValidatorTest.php (1)
60-149: Excellent test coverage for upload error scenarios!These seven test methods provide thorough coverage of PHP's upload error constants, ensuring the
MediaValidatorhandles all error cases correctly. The tests are well-structured and the assertions appropriately verify both the validation result and error messages.Optional: Consider using a data provider to reduce duplication
While the current approach is clear and maintainable, you could reduce duplication using PHPUnit's
@dataProvider:/** * @dataProvider uploadErrorProvider */ public function testValidateReturnsFalseForUploadErrors(int $errorCode, string $expectedSubstring): void { $file = [ 'error' => $errorCode, 'tmp_name' => '' ]; $result = $this->_validator->validate($file); $this->assertFalse($result); $this->assertStringContainsString($expectedSubstring, $this->_validator->getFirstError()); } public static function uploadErrorProvider(): array { return [ 'ini_size' => [UPLOAD_ERR_INI_SIZE, 'upload_max_filesize'], 'form_size' => [UPLOAD_ERR_FORM_SIZE, 'MAX_FILE_SIZE'], 'partial' => [UPLOAD_ERR_PARTIAL, 'partially uploaded'], 'no_tmp_dir' => [UPLOAD_ERR_NO_TMP_DIR, 'temporary folder'], 'cant_write' => [UPLOAD_ERR_CANT_WRITE, 'write file to disk'], 'extension' => [UPLOAD_ERR_EXTENSION, 'extension stopped'], 'unknown' => [999, 'Unknown'], ]; }This is purely optional—the current individual methods have the advantage of clearer failure messages when a specific test breaks.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Cms/Controllers/Admin/Media.phptests/Unit/Cms/Controllers/MediaIndexTest.phptests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.phptests/Unit/Cms/Services/Media/MediaValidatorTest.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Unit/Cms/Services/Media/MediaValidatorTest.php (1)
src/Cms/Services/Media/MediaValidator.php (1)
getFirstError(188-191)
tests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.php (1)
src/Cms/Services/Media/CloudinaryUploader.php (2)
CloudinaryUploader(17-371)listResources(149-184)
tests/Unit/Cms/Controllers/MediaIndexTest.php (3)
src/Cms/Controllers/Admin/Media.php (2)
Media(22-247)index(57-114)src/Cms/Services/Media/CloudinaryUploader.php (1)
CloudinaryUploader(17-371)src/Cms/Auth/SessionManager.php (1)
set(99-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (postgres)
🔇 Additional comments (9)
tests/Unit/Cms/Controllers/MediaIndexTest.php (3)
23-45: LGTM - Proper test isolation with registry state management.The setUp method correctly stores original registry values and configures in-memory settings for Cloudinary. This ensures test isolation without side effects on the global registry.
47-56: LGTM - Proper cleanup of test state.The tearDown method correctly restores the original registry values, ensuring no test pollution.
58-70: LGTM - Test correctly verifies authentication requirement.The test properly verifies that accessing the media index without authentication throws the expected RuntimeException. The use of expectException ensures the security check is enforced.
tests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.php (3)
33-279: LGTM - Comprehensive test coverage for listResources.The test suite thoroughly covers the listResources functionality including parameter validation, pagination, custom options, result formatting, empty results, and error handling. The use of reflection to inject mocks is appropriate for testing internal dependencies.
281-348: LGTM - Complete test coverage for delete functionality.The delete tests cover all scenarios: successful deletion, resource not found, and API errors. The assertions correctly verify the return values and exception handling.
350-394: LGTM - Thorough upload test coverage with proper cleanup.The upload tests comprehensively cover default options, custom options, and transformations. Temporary file cleanup is properly handled in these tests.
Also applies to: 437-538
src/Cms/Controllers/Admin/Media.php (3)
57-92: LGTM - Proper authentication check and request handling.The index method correctly validates user authentication and uses the framework's Request object to safely retrieve the cursor parameter. The view rendering with pagination support is well-structured.
124-169: LGTM - Proper file validation and upload handling.The uploadImage method correctly validates file presence and format before uploading. The Editor.js-compatible JSON response structure is appropriate for the integration.
192-233: LGTM - Consistent validation and upload logic.The uploadFeaturedImage method follows the same validation and upload pattern as uploadImage, with an appropriately different response structure.
| public function testUploadThrowsExceptionOnApiError(): void | ||
| { | ||
| $uploadApiMock = $this->createMock( UploadApi::class ); | ||
|
|
||
| // Create a real temporary file | ||
| $tmpFile = tmpfile(); | ||
| $tmpFilePath = stream_get_meta_data( $tmpFile )['uri']; | ||
| fwrite( $tmpFile, 'fake image content' ); | ||
|
|
||
| $uploadApiMock->method( 'upload' ) | ||
| ->willThrowException( new \Exception( 'Upload failed' ) ); | ||
|
|
||
| $cloudinaryMock = $this->createMock( Cloudinary::class ); | ||
| $cloudinaryMock->method( 'uploadApi' )->willReturn( $uploadApiMock ); | ||
|
|
||
| $uploader = new CloudinaryUploader( $this->_settings ); | ||
|
|
||
| $reflection = new \ReflectionClass( $uploader ); | ||
| $cloudinaryProperty = $reflection->getProperty( '_cloudinary' ); | ||
| $cloudinaryProperty->setAccessible( true ); | ||
| $cloudinaryProperty->setValue( $uploader, $cloudinaryMock ); | ||
|
|
||
| $this->expectException( \Exception::class ); | ||
| $this->expectExceptionMessage( 'Cloudinary upload failed' ); | ||
|
|
||
| $uploader->upload( $tmpFilePath ); | ||
|
|
||
| // Clean up | ||
| fclose( $tmpFile ); | ||
| } |
There was a problem hiding this comment.
Minor resource leak in exception test.
The fclose($tmpFile) on line 434 won't be reached because the exception is thrown on line 431. While tmpfile() resources are typically cleaned up automatically when the process ends, it's better practice to ensure cleanup occurs.
🔎 Suggested fix using try-finally
public function testUploadThrowsExceptionOnApiError(): void
{
$uploadApiMock = $this->createMock( UploadApi::class );
// Create a real temporary file
$tmpFile = tmpfile();
$tmpFilePath = stream_get_meta_data( $tmpFile )['uri'];
fwrite( $tmpFile, 'fake image content' );
+ try {
$uploadApiMock->method( 'upload' )
->willThrowException( new \Exception( 'Upload failed' ) );
$cloudinaryMock = $this->createMock( Cloudinary::class );
$cloudinaryMock->method( 'uploadApi' )->willReturn( $uploadApiMock );
$uploader = new CloudinaryUploader( $this->_settings );
$reflection = new \ReflectionClass( $uploader );
$cloudinaryProperty = $reflection->getProperty( '_cloudinary' );
$cloudinaryProperty->setAccessible( true );
$cloudinaryProperty->setValue( $uploader, $cloudinaryMock );
$this->expectException( \Exception::class );
$this->expectExceptionMessage( 'Cloudinary upload failed' );
$uploader->upload( $tmpFilePath );
-
- // Clean up
+ } finally {
+ // Clean up
fclose( $tmpFile );
+ }
}🤖 Prompt for AI Agents
In tests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.php around lines 406
to 435, the temporary file created with tmpfile() is closed after the call that
throws an exception, so fclose($tmpFile) is never reached; wrap the call that
triggers the exception (the upload invocation and the
expectException/expectExceptionMessage if needed) in a try/finally block and
move fclose($tmpFile) into the finally so the temporary resource is always
closed even when the upload throws.
Summary by CodeRabbit
New Features
Tests
Documentation / Chores
✏️ Tip: You can customize this high-level summary in your review settings.