Feature/blog editor upgrade #22
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds EditorJS content support (storage, editor UI, renderer), Cloudinary-backed media upload + validation and admin endpoints, many namespace updates to Neuron\Data\Settings, ORM adoption across repositories and tests, new events and emissions for auth/maintenance flows, upgrade tooling/CLI, migrations, CI/docs updates, and assorted view/test changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as User (Browser)
participant Editor as EditorJS
participant Form as Admin Form
participant AdminCtrl as Admin Media/Post Controller
participant Validator as MediaValidator
participant Uploader as CloudinaryUploader
participant PostModel as Post Model
participant DB as Database
participant Renderer as EditorJsRenderer
participant View as Blog View
Browser->>Editor: edit blocks / add image
Editor->>Form: populate hidden content-json
Browser->>Form: submit (content-json + file)
Form->>AdminCtrl: POST /admin/upload/image or save post
AdminCtrl->>Validator: validate(file)
Validator-->>AdminCtrl: result (ok / errors)
alt valid image upload
AdminCtrl->>Uploader: upload(tempFile)
Uploader-->>AdminCtrl: {url, public_id,...}
AdminCtrl-->>Editor: return EditorJS success JSON
end
AdminCtrl->>PostModel: setContent(content-json)
PostModel->>PostModel: decode content_raw, extract plain text into _body
PostModel->>DB: persist post with content_raw
DB-->>PostModel: saved
PostModel->>Renderer: render(content_raw)
Renderer-->>View: rendered HTML
View-->>Browser: display formatted post
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Cms/Services/Auth/Authentication.php (1)
226-249: Fix infinite recursion on stale sessions inuser()/logout()In
user():if( !$user ) { // Clear stale session if user no longer exists $this->logout(); }If the session contains a
user_idfor a user that has been deleted, this callslogout(), which in turn calls$this->check()and$this->user(), triggering the same path again and causing infinite recursion (now with additional event logic on each loop).Instead of delegating to
logout(), clear the stale session inline:- $user = $this->_userRepository->findById( $userId ); - - if( !$user ) - { - // Clear stale session if user no longer exists - $this->logout(); - } - - return $user; + $user = $this->_userRepository->findById( $userId ); + + if( !$user ) + { + // Clear stale session and remember-me cookie without recursion + $this->_sessionManager->destroy(); + + if( isset( $_COOKIE['remember_token'] ) ) + { + setcookie( 'remember_token', '', time() - 3600, '/', '', true, true ); + } + + return null; + } + + return $user;This preserves the intended behavior (dropping an invalid session and remember-me cookie) without risking a stack overflow or runaway event emissions when the underlying user record is missing.
src/Cms/Controllers/Blog.php (1)
87-93: "Not found" post will have empty content for rendering.When a post is not found, a dummy Post object is created with
setBody()but the new rendering pipeline usesgetContent()which will return the default empty EditorJS structure{"blocks":[]}. This means the "not found" message won't be displayed.Update the "not found" post to use the new content structure:
if( !$post ) { $post = new Post(); $post->setTitle( 'Article Not Found' ); - $post->setBody( 'The requested article does not exist.' ); + $post->setContent( json_encode([ + 'blocks' => [ + [ + 'type' => 'paragraph', + 'data' => ['text' => 'The requested article does not exist.'] + ] + ] + ]) ); $post->setSlug( $slug ); }
🧹 Nitpick comments (16)
resources/views/http_codes/500.php (2)
8-20: Inline styles are appropriate for error pages.Using inline styles ensures the page displays correctly even if external stylesheets fail to load, which is a good practice for error pages.
Optional: Consider adding responsive design for smaller screens:
.centered { text-align: center; - margin-top: 100px; /* Adjust this value as needed */ + margin-top: 10vh; + padding: 0 15px; } .centered-text { text-align: center; margin-top: 20px; font-size: 16px; color: #666; + padding: 0 15px; }
1-6: Consider adding accessibility attributes and user escape hatches.The error page would benefit from:
- ARIA landmark roles for screen readers
- A link back to the home page or contact information
-<div class="row"> - <div class="col"> +<div class="row" role="main"> + <div class="col"> <h1 class="centered">Something went wrong on our end.</h1> <p class="centered-text">The server encountered an unexpected error. Please try again later.</p> + <p class="centered-text"><a href="/" style="color: #0066cc;">Return to Home</a></p> </div> </div>src/Cms/Repositories/DatabaseCategoryRepository.php (1)
211-217: GROUP BY may break on PostgreSQL
allWithPostCount()selectsc.*but groups only byc.id; PostgreSQL’s stricter GROUP BY rules will reject this query. If you intend this repository to work against PostgreSQL (per the class doc), you’ll need to either:
- Group by all selected
categoriescolumns, or- Select only grouped/aggregated columns.
Please confirm whether PostgreSQL is a supported target in your deployments; if so, consider adjusting the SELECT/GROUP BY to be portable.
composer.json (1)
15-27: Dependency updates align with the Settings refactor; confirm version rangesThe move to
neuron-php/mvc: "0.9.*"and addition ofneuron-php/data: "0.9.*"andneuron-php/scaffolding: "0.8.*"fit the new Settings namespace and tooling story.One nuance:
"0.9.*"can allow earlier 0.9.x releases than^0.9.5would have; in practice Composer will usually pick the latest matching version, but if you depend on features/bugfixes introduced in later 0.9.x, you may prefer a caret or a higher minimum.Please make sure
composer.lockis updated and CI (including migrations/tests that touch Settings) passes with these new constraints. If you want to avoid accidentally downgrading, consider tightening the constraints (e.g.,^0.9.5/^0.9.0) before merging.src/Cms/Repositories/DatabasePostRepository.php (1)
84-103: Verify handling of the newcontent_rawcolumn in postsThe new migration adds a non‑nullable
content_rawcolumn intended for Editor.js JSON, but this repository currently:
- Inserts/updates only
body/excerpt(nocontent_rawin the column/value lists), and- Does not include
content_rawin the$dataarray passed toPost::fromArray().Because the DB default will populate
content_raw, this won’t break inserts, but any Editor.js JSON stored on the entity may not be persisted or hydrated unless other layers handle it.Please double‑check how the
Postmodel exposes raw Editor.js content (e.g.,getContentRaw()/setContentRaw()or similar), and consider extending:
- the INSERT/UPDATE statements, and
mapRowToPost()’s$dataarray
to read/writecontent_rawif that field is meant to be first‑class in the domain.Also applies to: 141-168, 440-455
src/Cms/Services/Auth/PasswordResetter.php (1)
106-110: LGTM! Event emissions enhance observability.The addition of
PasswordResetRequestedEventandPasswordResetCompletedEventprovides valuable audit trails for password reset workflows. The fallback to'unknown'for$_SERVER['REMOTE_ADDR']appropriately handles CLI and testing contexts.Consider importing the event classes at the top of the file rather than using fully qualified names for improved readability:
+use Neuron\Cms\Events\PasswordResetRequestedEvent; +use Neuron\Cms\Events\PasswordResetCompletedEvent; +use Neuron\Application\CrossCutting\Event;Then use:
- \Neuron\Application\CrossCutting\Event::emit( new \Neuron\Cms\Events\PasswordResetRequestedEvent( + Event::emit( new PasswordResetRequestedEvent(Also applies to: 175-179
resources/views/admin/posts/edit.php (2)
24-31: Harden handling of existing EditorJS content JSON before initializationCurrent parsing only falls back on JSON syntax errors; valid-but-unexpected JSON (e.g.
"null"or an object withoutblocks) will flow intodataand may confuse EditorJS.You can defensively ensure a valid
blocksarray after parsing:-let existingContent; -try { - existingContent = JSON.parse(existingContentJson); -} catch (error) { - console.error('Failed to parse existing content:', error); - existingContent = { blocks: [] }; -} +let existingContent; +try { + existingContent = JSON.parse(existingContentJson); + + // Ensure we always pass a valid Editor.js data structure + if( !existingContent || !Array.isArray( existingContent.blocks ) ) + { + existingContent = { blocks: [] }; + } +} catch (error) { + console.error('Failed to parse existing content:', error); + existingContent = { blocks: [] }; +}This keeps the editor resilient to legacy or malformed
content_rawvalues while preserving your existing fallback behavior.Also applies to: 93-103, 104-109
82-90: Consider pinning EditorJS CDN versions and reducingsave()frequencyThe EditorJS integration and submit flow look solid. Two optional robustness points:
- The
@latestCDN URLs can introduce breaking changes when a new major EditorJS (or tool) version ships. Consider pinning to a specific major/minor version (e.g.@2.xor an exact version) and upgrading deliberately.onChangecurrently callseditor.save()on every change, which is relatively heavy. If this becomes a performance issue for long posts, you could debounce/throttle the save into#content-jsonor rely solely on the submit handler to serialize content.Both are optional, but would make the editor behavior more predictable in production.
Also applies to: 145-149, 151-163
tests/Cms/Services/Post/CreatorTest.php (1)
45-69: Centralize EditorJS content JSON in a helper/fixture for test readabilityThe tests correctly assert both
getContentRaw()and the derivedgetBody(). The same EditorJS JSON pattern ({"blocks":[{"type":"paragraph","data":{"text":"Body"}}]}and variants) is repeated across multiple tests, though.To keep things DRY and easier to tweak later, consider extracting small helpers/constants, e.g.:
private function makeEditorJsContent(string $text = 'Body'): string { return json_encode([ 'blocks' => [ [ 'type' => 'paragraph', 'data' => ['text' => $text], ], ], ]); }Then use
makeEditorJsContent('Test body content')/makeEditorJsContent()throughout the tests instead of hardcoding JSON strings.Also applies to: 90-95, 118-123, 148-153, 177-182, 218-223, 263-273, 297-305
src/Cms/Maintenance/MaintenanceManager.php (1)
61-82: Emit maintenance events without risking core enable/disable operationsThe new enabled/disabled events are wired in cleanly, and the optional
$disabledByargument keeps the public API backward compatible.One robustness refinement to consider: if any event listener hooked into
MaintenanceModeEnabledEvent/MaintenanceModeDisabledEventthrows, callers ofenable()/disable()will see an exception even though the maintenance file has already been written or deleted. To keep state changes reliable, you might wrap the emits:- $result = $this->writeMaintenanceFile( $data ); - - // Emit maintenance mode enabled event - if( $result ) - { - \Neuron\Application\CrossCutting\Event::emit( new \Neuron\Cms\Events\MaintenanceModeEnabledEvent( - $data['enabled_by'], - $message - ) ); - } + $result = $this->writeMaintenanceFile( $data ); + + // Emit maintenance mode enabled event (non-fatal) + if( $result ) + { + try + { + \Neuron\Application\CrossCutting\Event::emit( + new \Neuron\Cms\Events\MaintenanceModeEnabledEvent( + $data['enabled_by'], + $message + ) + ); + } + catch( \Throwable $e ) + { + // Optional: log but do not affect enable() result + } + }and similarly in
disable(). That keeps maintenance mode toggling deterministic even if downstream listeners misbehave.Also applies to: 87-107
src/Cms/Services/Auth/Authentication.php (1)
41-59: Event emissions for login attempts look good; consider small refactorsThe new
UserLoginFailedEvent,UserLoginEvent, andUserLogoutEventhooks are well-placed and give you good observability over auth flows. A couple of low-friction refinements:
- The four failure branches emit
UserLoginFailedEventwith only thereasondiffering. You could extract a small private helper likeemitLoginFailed(string $username, string $reason): voidto reduce duplication and keep changes localized if you later enrich the payload.- In
login(), you callmicrotime( true )both when settinglogin_timein the session and when emittingUserLoginEvent. Reusing a single timestamp would keep those values perfectly aligned:- // Store user ID in session - $this->_sessionManager->set( 'user_id', $user->getId() ); - $this->_sessionManager->set( 'user_role', $user->getRole() ); - $this->_sessionManager->set( 'login_time', microtime( true ) ); + // Store user ID in session + $this->_sessionManager->set( 'user_id', $user->getId() ); + $this->_sessionManager->set( 'user_role', $user->getRole() ); + $loginTime = microtime( true ); + $this->_sessionManager->set( 'login_time', $loginTime ); @@ - // Emit user login event - \Neuron\Application\CrossCutting\Event::emit( new \Neuron\Cms\Events\UserLoginEvent( - $user, - $_SERVER['REMOTE_ADDR'] ?? 'unknown', - microtime( true ) - ) ); + // Emit user login event + \Neuron\Application\CrossCutting\Event::emit( new \Neuron\Cms\Events\UserLoginEvent( + $user, + $_SERVER['REMOTE_ADDR'] ?? 'unknown', + $loginTime + ) );Overall the behavior is sound; these are just to keep things DRY and metrics-friendly.
Also applies to: 61-85, 87-110, 134-156, 161-201
tests/Cms/Services/Post/UpdaterTest.php (1)
39-42: Mirror CreatorTest by centralizing EditorJS JSON for updatesThese tests correctly validate that
Updater::update()preserves both the raw EditorJS JSON (getContentRaw()) and the derived body text (getBody()), and that other fields (slug, status, categories, tags, etc.) behave as expected.As in
CreatorTest, you might want to centralize the repeated JSON snippets into a small helper (on this class or a shared trait) so any future changes to your EditorJS block shape only need to be made in one place.Also applies to: 54-70, 97-99, 127-128, 171-172, 218-219, 254-255, 285-286, 321-322
resources/views/admin/posts/create.php (1)
131-142: Consider potential issues with programmatic form submission.The submit handler prevents the default form submission, saves editor content, then calls
e.target.submit(). This bypasses normal HTML5 validation and could cause issues if there are other submit handlers or validation logic.Consider using FormData to submit the form programmatically while maintaining validation:
-document.getElementById('post-form').addEventListener('submit', async (e) => { - e.preventDefault(); - - try { - const savedData = await editor.save(); - document.getElementById('content-json').value = JSON.stringify(savedData); - e.target.submit(); - } catch (error) { - console.error('Error saving editor content:', error); - alert('Error preparing content. Please try again.'); - } -}); +document.getElementById('post-form').addEventListener('submit', async (e) => { + // Don't prevent default initially - let HTML5 validation run first + if (!e.target.checkValidity()) { + return; // Let browser handle validation + } + + e.preventDefault(); + + try { + const savedData = await editor.save(); + document.getElementById('content-json').value = JSON.stringify(savedData); + e.target.submit(); + } catch (error) { + console.error('Error saving editor content:', error); + alert('Error preparing content. Please try again.'); + } +});src/Cms/Controllers/Blog.php (1)
106-108: Add error handling for content rendering.The renderer should handle malformed JSON or rendering errors gracefully to prevent the page from breaking when post content is invalid.
Wrap the rendering in a try-catch block:
- // Render content from Editor.js JSON - $content = $post->getContent(); - $renderedContent = $this->_renderer->render( $content ); + // Render content from Editor.js JSON + $content = $post->getContent(); + try { + $renderedContent = $this->_renderer->render( $content ); + } catch (\Exception $e) { + error_log("Error rendering post content for slug '{$slug}': " . $e->getMessage()); + $renderedContent = '<p>Content temporarily unavailable.</p>'; + }src/Cms/Models/Post.php (2)
125-128: Add error handling for malformed JSON in getContent().If
_contentRawcontains invalid JSON,json_decode()will returnnulland the??operator will correctly return the default, but this happens silently. Consider logging the error for debugging.public function getContent(): array { - return json_decode( $this->_contentRaw, true ) ?? ['blocks' => []]; + $decoded = json_decode( $this->_contentRaw, true ); + if ($decoded === null && json_last_error() !== JSON_ERROR_NONE) { + error_log("Invalid JSON in Post content_raw (ID: {$this->_id}): " . json_last_error_msg()); + } + return $decoded ?? ['blocks' => []]; }
607-646: Consider XSS risks in extractPlainText() for 'raw' block type.Line 633 includes raw HTML from
rawblocks without sanitization when extracting plain text. Whilestrip_tags()is applied at line 640, malicious HTML could still be present in the intermediate processing.Since this is meant to be plain text extraction, consider whether
rawblocks should be included at all, or strip tags immediately:$blockText = match( $block['type'] ) { 'paragraph', 'header' => $block['data']['text'] ?? '', 'list' => isset( $block['data']['items'] ) && is_array( $block['data']['items'] ) ? implode( "\n", $block['data']['items'] ) : '', 'quote' => $block['data']['text'] ?? '', 'code' => $block['data']['code'] ?? '', - 'raw' => $block['data']['html'] ?? '', + 'raw' => strip_tags($block['data']['html'] ?? ''), // Strip tags immediately for raw HTML default => '' }; if( $blockText !== '' ) { // Strip HTML tags from text $blockText = strip_tags( $blockText ); $text[] = trim( $blockText ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (61)
composer.json(2 hunks)resources/app/Initializers/AuthInitializer.php(1 hunks)resources/app/Initializers/MaintenanceInitializer.php(1 hunks)resources/app/Initializers/PasswordResetInitializer.php(1 hunks)resources/app/Initializers/RegistrationInitializer.php(1 hunks)resources/database/migrate/20251119224525_add_content_raw_to_posts.php(1 hunks)resources/views/admin/posts/create.php(3 hunks)resources/views/admin/posts/edit.php(3 hunks)resources/views/blog/show.php(2 hunks)resources/views/http_codes/500.php(1 hunks)src/Bootstrap.php(1 hunks)src/Cms/Cli/Commands/Generate/EmailCommand.php(0 hunks)src/Cms/Cli/Commands/Generate/stubs/email.stub(0 hunks)src/Cms/Cli/Commands/Install/InstallCommand.php(1 hunks)src/Cms/Cli/Commands/Maintenance/EnableCommand.php(1 hunks)src/Cms/Cli/Commands/Queue/InstallCommand.php(0 hunks)src/Cms/Cli/Provider.php(0 hunks)src/Cms/Controllers/Admin/Categories.php(1 hunks)src/Cms/Controllers/Admin/Profile.php(1 hunks)src/Cms/Controllers/Admin/Users.php(1 hunks)src/Cms/Controllers/Blog.php(4 hunks)src/Cms/Controllers/Content.php(4 hunks)src/Cms/Database/ConnectionFactory.php(1 hunks)src/Cms/Email/helpers.php(1 hunks)src/Cms/Events/EmailVerifiedEvent.php(1 hunks)src/Cms/Events/MaintenanceModeDisabledEvent.php(1 hunks)src/Cms/Events/MaintenanceModeEnabledEvent.php(1 hunks)src/Cms/Events/PasswordResetCompletedEvent.php(1 hunks)src/Cms/Events/PasswordResetRequestedEvent.php(1 hunks)src/Cms/Events/UserLoginEvent.php(1 hunks)src/Cms/Events/UserLoginFailedEvent.php(1 hunks)src/Cms/Events/UserLogoutEvent.php(1 hunks)src/Cms/Maintenance/MaintenanceConfig.php(1 hunks)src/Cms/Maintenance/MaintenanceManager.php(1 hunks)src/Cms/Models/Post.php(5 hunks)src/Cms/Repositories/DatabaseCategoryRepository.php(1 hunks)src/Cms/Repositories/DatabaseEmailVerificationTokenRepository.php(1 hunks)src/Cms/Repositories/DatabasePageRepository.php(1 hunks)src/Cms/Repositories/DatabasePasswordResetTokenRepository.php(1 hunks)src/Cms/Repositories/DatabasePostRepository.php(1 hunks)src/Cms/Repositories/DatabaseTagRepository.php(1 hunks)src/Cms/Repositories/DatabaseUserRepository.php(1 hunks)src/Cms/Services/Auth/Authentication.php(5 hunks)src/Cms/Services/Auth/EmailVerifier.php(2 hunks)src/Cms/Services/Auth/PasswordResetter.php(3 hunks)src/Cms/Services/Email/Sender.php(1 hunks)src/Cms/Services/Member/RegistrationService.php(1 hunks)src/Cms/Services/Post/Creator.php(3 hunks)src/Cms/Services/Post/Updater.php(3 hunks)tests/Cms/BlogControllerTest.php(1 hunks)tests/Cms/Cli/Commands/Install/InstallCommandTest.php(1 hunks)tests/Cms/ContentControllerTest.php(1 hunks)tests/Cms/Maintenance/MaintenanceConfigTest.php(1 hunks)tests/Cms/Repositories/DatabaseEmailVerificationTokenRepositoryTest.php(1 hunks)tests/Cms/Repositories/DatabaseUserRepositoryTest.php(1 hunks)tests/Cms/Services/AuthenticationTest.php(1 hunks)tests/Cms/Services/EmailVerifierTest.php(1 hunks)tests/Cms/Services/PasswordResetterTest.php(1 hunks)tests/Cms/Services/Post/CreatorTest.php(9 hunks)tests/Cms/Services/Post/UpdaterTest.php(10 hunks)tests/Cms/Services/RegistrationServiceTest.php(1 hunks)
💤 Files with no reviewable changes (4)
- src/Cms/Cli/Provider.php
- src/Cms/Cli/Commands/Generate/EmailCommand.php
- src/Cms/Cli/Commands/Queue/InstallCommand.php
- src/Cms/Cli/Commands/Generate/stubs/email.stub
🧰 Additional context used
🧬 Code graph analysis (15)
src/Cms/Events/PasswordResetRequestedEvent.php (2)
src/Cms/Events/EmailVerifiedEvent.php (2)
__construct(29-33)getName(35-38)src/Cms/Events/PasswordResetCompletedEvent.php (2)
__construct(30-35)getName(37-40)
resources/views/admin/posts/create.php (1)
src/Cms/View/helpers.php (1)
route_path(70-79)
src/Cms/Events/MaintenanceModeDisabledEvent.php (2)
src/Cms/Events/EmailVerifiedEvent.php (2)
__construct(29-33)getName(35-38)src/Cms/Events/MaintenanceModeEnabledEvent.php (2)
__construct(30-35)getName(37-40)
src/Cms/Events/UserLoginEvent.php (3)
src/Cms/Events/EmailVerifiedEvent.php (2)
__construct(29-33)getName(35-38)src/Cms/Events/UserLoginFailedEvent.php (2)
__construct(31-38)getName(40-43)src/Cms/Events/UserLogoutEvent.php (2)
__construct(30-35)getName(37-40)
src/Cms/Events/UserLoginFailedEvent.php (3)
src/Cms/Events/EmailVerifiedEvent.php (2)
__construct(29-33)getName(35-38)src/Cms/Events/UserLoginEvent.php (2)
__construct(31-37)getName(39-42)src/Cms/Events/UserLogoutEvent.php (2)
__construct(30-35)getName(37-40)
src/Cms/Maintenance/MaintenanceManager.php (2)
src/Cms/Events/MaintenanceModeEnabledEvent.php (1)
MaintenanceModeEnabledEvent(24-41)src/Cms/Events/MaintenanceModeDisabledEvent.php (1)
MaintenanceModeDisabledEvent(24-39)
tests/Cms/Services/Post/CreatorTest.php (2)
src/Cms/Controllers/Content.php (1)
getTitle(141-144)src/Cms/Models/Post.php (2)
getTitle(74-77)getContentRaw(133-136)
src/Cms/Events/MaintenanceModeEnabledEvent.php (2)
src/Cms/Events/EmailVerifiedEvent.php (2)
__construct(29-33)getName(35-38)src/Cms/Events/MaintenanceModeDisabledEvent.php (2)
__construct(29-33)getName(35-38)
src/Cms/Services/Post/Creator.php (1)
src/Cms/Models/Post.php (1)
setContent(142-147)
src/Cms/Services/Auth/EmailVerifier.php (2)
src/Cms/Events/EmailVerifiedEvent.php (1)
EmailVerifiedEvent(24-39)src/Cms/Services/Auth/Authentication.php (1)
user(226-249)
resources/views/admin/posts/edit.php (2)
src/Cms/View/helpers.php (1)
route_path(70-79)src/Cms/Models/Post.php (2)
getId(57-60)getContentRaw(133-136)
src/Cms/Services/Auth/Authentication.php (6)
src/Cms/Events/UserLoginFailedEvent.php (1)
UserLoginFailedEvent(23-44)src/Cms/Models/User.php (2)
isLockedOut(332-340)isActive(187-190)src/Cms/Auth/SessionManager.php (1)
set(99-103)src/Cms/Events/UserLoginEvent.php (1)
UserLoginEvent(24-43)src/Cms/Controllers/Auth/Login.php (1)
logout(139-143)src/Cms/Events/UserLogoutEvent.php (1)
UserLogoutEvent(24-41)
resources/database/migrate/20251119224525_add_content_raw_to_posts.php (1)
src/Cms/Services/Post/Updater.php (1)
update(48-83)
tests/Cms/Services/Post/UpdaterTest.php (2)
src/Cms/Models/Post.php (5)
setContent(142-147)getTitle(74-77)getContentRaw(133-136)getBody(108-111)getStatus(246-249)src/Cms/Repositories/DatabasePostRepository.php (1)
update(127-187)
src/Cms/Services/Post/Updater.php (1)
src/Cms/Models/Post.php (1)
setContent(142-147)
🔇 Additional comments (54)
src/Cms/Maintenance/MaintenanceConfig.php (1)
5-5: Namespace migration is complete and verified.The import change from
Neuron\Data\SettingtoNeuron\Data\Settingson line 5 is correct. Verification confirms no remaining old namespace references exist in the codebase, and the new namespace is consistently used across 20 files (production code and tests).src/Cms/Cli/Commands/Maintenance/EnableCommand.php (1)
8-8: Yaml settings source namespace update looks correctThe Yaml import now targets
Neuron\Data\Settings\Source\Yaml, which matches how it’s used inloadConfiguration()and aligns with the Settings namespace migration; no further changes needed here as long as the installedneuron-php/dataversion exposes this namespace.src/Cms/Services/Email/Sender.php (1)
7-7: SettingManager import aligned with Settings namespaceSwitching to
Neuron\Data\Settings\SettingManageris consistent with the property/ctor type hints and the rest of the Settings migration; the email config access remains unchanged.src/Cms/Services/Member/RegistrationService.php (1)
9-9: RegistrationService now correctly targets Settings\SettingManagerThe import of
Neuron\Data\Settings\SettingManagermatches the constructor and property types and keeps the member settings lookups intact.src/Cms/Controllers/Admin/Categories.php (1)
11-11: Categories controller import updated to Settings\SettingManagerThe controller now imports
Neuron\Data\Settings\SettingManager, keeping it consistent with the rest of the Settings namespace migration in the project.tests/Cms/Maintenance/MaintenanceConfigTest.php (1)
6-6: Test Yaml source import matches new Settings namespaceUsing
Neuron\Data\Settings\Source\Yamlin this test is consistent with the production MaintenanceConfig usage and the broader Settings namespace move.src/Cms/Repositories/DatabaseEmailVerificationTokenRepository.php (1)
7-7: Repository constructor now depends on Settings\SettingManagerThe repository correctly imports
Neuron\Data\Settings\SettingManagerand passes it toConnectionFactory::createFromSettings(), keeping database configuration behavior unchanged under the new namespace.tests/Cms/Repositories/DatabaseEmailVerificationTokenRepositoryTest.php (1)
8-8: Test mock updated to Settings\SettingManagerThe test now mocks
Neuron\Data\Settings\SettingManager, matching the repository’s constructor signature and ensuring the database config wiring remains valid.tests/Cms/ContentControllerTest.php (1)
6-6: In-memory settings source switched to Settings\Source\MemoryThe test now uses
Neuron\Data\Settings\Source\Memory, which is consistent with the Settings namespace refactor and how the controller reads settings via the Registry.src/Cms/Repositories/DatabaseUserRepository.php (1)
7-7: Repository constructor now aligned withSettingsnamespaceThe repository’s dependency on
SettingManageris correctly switched to theNeuron\Data\Settingsnamespace, keeping the constructor type-hint andConnectionFactory::createFromSettings()usage consistent.Please run your user-related tests (e.g.,
DatabaseUserRepositoryTest, auth flows) to confirm there are no remaining references to the oldNeuron\Data\Setting\SettingManagertype.src/Cms/Repositories/DatabasePageRepository.php (1)
8-8: Page repositorySettingManagerimport migrated cleanlyThe constructor’s
SettingManagerdependency now points atNeuron\Data\Settings\SettingManager, matching the rest of the PR’s namespace migration without altering any repository behavior.Confirm that any code instantiating
DatabasePageRepository(controllers/services) is also using the updatedSettings-namespaceSettingManagerinstance from the container/registry.src/Cms/Repositories/DatabaseTagRepository.php (1)
7-7: Tag repository settings dependency updated to new namespace
DatabaseTagRepositorynow correctly importsNeuron\Data\Settings\SettingManagerwhile keeping the constructor contract and internals the same, so behavior should be unaffected.As part of your test run, ensure any install/seed or tag-related CLI flows still resolve the correct
SettingManagerimplementation.tests/Cms/Cli/Commands/Install/InstallCommandTest.php (1)
8-9: Install command test imports aligned withSettingsnamespaceThe test now imports
Neuron\Data\Settings\SettingManagerandNeuron\Data\Settings\Source\Yaml, matching the production code’s updated namespaces and avoiding mismatch with the newer data/settings package layout.If these imports are no longer needed by the test, they can be safely removed in a later cleanup pass once static analysis (e.g., phpstan/psalm) is green.
src/Cms/Controllers/Admin/Users.php (1)
13-13: Admin Users controller now consistent withSettings-based repositoriesThe controller’s import of
Neuron\Data\Settings\SettingManagermatches the updated constructor dependency ofDatabaseUserRepository, keeping the Registry-provided'Settings'instance aligned with the repository’s expected type.Please verify that the Registry is registering an instance of the new
Neuron\Data\Settings\SettingManagerclass under the'Settings'key in your bootstrap/initializer code.src/Bootstrap.php (1)
4-5: Bootstrap imports updated to new data library namespaces
VersionandYamlnow import fromNeuron\Data\Objects\VersionandNeuron\Data\Settings\Source\Yaml, which matches the reorganized data library structure without changing theboot()logic.After updating composer dependencies, run a full application bootstrap (and any bootstrap-related tests) to confirm there are no unresolved references to the old
Neuron\Data\Object\VersionorNeuron\Data\Setting\Source\Yamlclasses.src/Cms/Controllers/Admin/Profile.php (1)
10-10: Admin Profile controller settings import matches repository expectationsThe profile controller’s import now targets
Neuron\Data\Settings\SettingManager, aligning its dependency graph with the updatedDatabaseUserRepositoryand the broader Settings namespace migration.As with the Users controller, double‑check that the Registry’s
'Settings'entry is an instance of the newSettings\SettingManagerto avoid subtle type mismatches at runtime.src/Cms/Database/ConnectionFactory.php (1)
5-5: Namespace migration verified as completeThe import change to
Neuron\Data\Settings\SettingManageris correct and actively used. Verification confirms:
- No lingering references to the old
Neuron\Data\Settingnamespace exist- The type hint in
createFromSettings()(line 26) properly references the newSettingManagerclass- The method implementation correctly delegates to
createFromConfig()as expectedThe migration is consistent and safe to merge.
src/Cms/Repositories/DatabaseCategoryRepository.php (1)
7-7: Settings namespace update is consistentSwitching to
Neuron\Data\Settings\SettingManagerhere matches the broader Settings namespace refactor and should be a no‑op at runtime as long as the updated neuron‑php/data dependency is installed.Please run the test suite after
composer updatewith the new dependencies to confirm there are no autoloading or wiring issues aroundSettingManager.tests/Cms/Repositories/DatabaseUserRepositoryTest.php (1)
8-8: Updated SettingManager import in tests looks goodUsing
Neuron\Data\Settings\SettingManagerin the mocks keeps the test suite aligned with the refactored Settings namespace; behavior remains unchanged because the class is mocked.After updating dependencies, please run this test class to ensure the new namespace resolves correctly under your autoloader.
src/Cms/Repositories/DatabasePostRepository.php (1)
10-10: Settings namespace import updated correctlyReferencing
Neuron\Data\Settings\SettingManagerhere keeps this repository in sync with the Settings refactor and ConnectionFactory usage; the constructor contract remains the same.After dependency updates, please run a small smoke test that instantiates
DatabasePostRepositoryvia your usual bootstrap path to confirm the new namespace resolves as expected.src/Cms/Email/helpers.php (1)
4-4: Email helpers now reference the new Settings namespaceUpdating the type to
Neuron\Data\Settings\ISettingSourcekeeps these helpers compatible with the refactored Settings package without changing their runtime behavior.Please ensure the updated Settings package is present in all environments where these helpers are used (including CLI contexts) and that any existing call sites still pass a compatible settings source.
resources/database/migrate/20251119224525_add_content_raw_to_posts.php (1)
13-22: Migration forcontent_rawis straightforward; verify adapter support for optionsAdding
content_rawas non‑nullable TEXT with a default empty Editor.js document is reasonable and should keep existing rows valid.The use of
'after' => 'body'and'comment' => 'Editor.js JSON content'is typically MySQL‑specific in Phinx; depending on how strictly you target SQLite/PostgreSQL as well, those options may be ignored or could need adapter‑specific handling.Please confirm that this migration runs cleanly against all database engines you support (especially in CI and local dev). If you see adapter‑specific issues, you may want to conditionally set
after/commentor drop them for non‑MySQL adapters.src/Cms/Events/EmailVerifiedEvent.php (1)
24-38: EmailVerifiedEvent implementation looks solidThe event cleanly encapsulates the verified
Uservia a readonly promoted property and exposes a clear, stable name ('email.verified') for subscribers. The design is consistent with the other event classes in this PR.src/Cms/Events/MaintenanceModeEnabledEvent.php (1)
24-40: MaintenanceModeEnabledEvent is consistent with the event modelReadonly constructor‑promoted properties for
enabledByandmessageplus a stable name'maintenance.enabled'match the existing disabled‑event pattern and give listeners all relevant context.src/Cms/Events/UserLoginEvent.php (1)
24-42: UserLoginEvent structure is consistent and ready for consumersThe event payload (User, IP, timestamp) and name
user.loginalign with the other event types you’ve added and give downstream listeners enough context without extra coupling. No issues spotted.src/Cms/Controllers/Content.php (2)
3-4: Version loading via factory is a clean upgradeSwitching to
Factories\Version::fromFile("../.version.json")while keeping the broad\Exceptioncatch preserves existing behavior and centralizes version deserialization in the data library. As long as the working directory is stable for web/CLI entry points, this looks good.Also applies to: 51-55, 85-94
201-213: Defaulting markdown page route toindeximproves resilienceUsing
$request->getRouteParameter('page') ?? 'index'avoids failures when the route omitspageand matches the typical “root markdown page” expectation. Just confirm your router/front-controller has anindex.md(or equivalent) to back this default.src/Cms/Repositories/DatabasePasswordResetTokenRepository.php (1)
5-8: Settings namespace refactor is consistentUsing
Neuron\Data\Settings\SettingManagerhere matches the broader Settings namespace migration and keeps the existing constructor contract intact. No repository behavior changes introduced.Also applies to: 23-32
tests/Cms/Services/PasswordResetterTest.php (1)
11-13: Test settings imports now align with production namespacesUpdating to
Settings\Source\MemoryandSettings\SettingManagerkeeps the test configuration in sync with the data layer refactor. The in-memory setup logic remains the same and should continue to exercise PasswordResetter as before.Also applies to: 34-42
tests/Cms/Services/EmailVerifierTest.php (1)
10-12: EmailVerifier tests correctly track the Settings namespace changeThe move to
Neuron\Data\Settings\Source\MemoryandSettings\SettingManagermirrors the production code and leaves the test behavior intact (test-mode email, same keys). Looks good.Also applies to: 31-39
resources/app/Initializers/RegistrationInitializer.php (1)
31-38: Registration initializer now guards against the correct Settings manager typeSwitching the instanceof check to
\Neuron\Data\Settings\SettingManagermatches the new Settings namespace and preserves the early-exit behavior when configuration isn’t wired. No additional issues here.src/Cms/Services/Auth/EmailVerifier.php (1)
10-11: I'll help you verify the review comment about email verification event emission safety. Let me start by exploring the codebase to understand how the Event::emit() method handles missing emitters and listener failures.
<function_calls>
#!/bin/bashFind the Event class definition
rg -n "class Event" --type=php
Find emit method definitions
rg -n "function emit|public function emit" --type=php -A 5
</function_calls>src/Cms/Services/Post/Creator.php (1)
35-40: Confirmed: API change is complete and well-implemented internally, but lacks external documentationThe migration is thorough within the repository:
- Post model correctly implements
setContent()with EditorJS JSON, auto-extracting plain text to_bodyfor backward compatibility- Creator service properly uses
setContent()- Admin form is fully updated with Editor.js frontend that serializes to EditorJS JSON
- Database schema includes
content_rawcolumn for JSON storage- Tests pass EditorJS JSON correctly
However, this package is publicly distributed via Composer (
"neuron-php/cms"library type), making Creator::create() a public API. The breaking change—requiring EditorJS JSON instead of rendered body strings—lacks a migration guide or changelog. No backward compatibility wrapper exists.tests/Cms/Services/RegistrationServiceTest.php (1)
10-11: LGTM! Namespace migration applied correctly.The import paths have been updated to use the new
Neuron\Data\Settingsnamespace, aligning with the project-wide refactoring.resources/app/Initializers/PasswordResetInitializer.php (1)
9-9: LGTM! Namespace migration applied correctly.The import path has been updated to use the new
Neuron\Data\Settings\SettingManagernamespace.src/Cms/Cli/Commands/Install/InstallCommand.php (1)
9-10: LGTM! Namespace migration applied correctly.The import paths have been updated to use the new
Neuron\Data\Settingsnamespace.tests/Cms/Services/AuthenticationTest.php (1)
11-11: LGTM! Namespace migration applied correctly.The import path has been updated to use the new
Neuron\Data\Settings\SettingManagernamespace.tests/Cms/BlogControllerTest.php (1)
13-14: LGTM! Namespace migration applied correctly.The import paths have been updated to use the new
Neuron\Data\Settingsnamespace.resources/app/Initializers/AuthInitializer.php (1)
32-32: LGTM! Type check updated for namespace migration.The instanceof check has been updated to reference the new
Neuron\Data\Settings\SettingManagernamespace.resources/views/blog/show.php (2)
51-160: LGTM! Comprehensive styling for EditorJS content.The CSS additions provide proper styling for post content and EditorJS widgets, including headings, lists, blockquotes, code blocks, images, and custom widgets.
39-39: I'll help you verify the concerns raised in the review comment. Let me search the codebase for the Blog controller and examine how$renderedContentis set.
<function_calls>
#!/bin/bashFind the Blog controller
fd -t f -i "blog" --type=php | head -20
</function_calls>Let me search more specifically for the Blog controller:
<function_calls>
#!/bin/bashSearch for Blog controller class definition
rg -n "class Blog" --type=php -A 5
</function_calls>Now let me search for where the show view is rendered:
<function_calls>
#!/bin/bashSearch for show view rendering and renderedContent variable
rg -n "renderedContent|show.php|blog/show" --type=php -B 3 -A 3
</function_calls>Let me also check the structure of the resources/views/blog directory:
<function_calls>
#!/bin/bashList files in blog views directory
fd -t f "show.php" && echo "---" && fd -t f "blog" resources/views/ 2>/dev/null | head -10
</function_calls>Let me check the full repository structure to understand the layout:
<function_calls>
#!/bin/bashGet an overview of the repository structure
git ls-files | grep -E "(controller|blog)" | head -20
</function_calls>src/Cms/Services/Auth/PasswordResetter.php (1)
10-10: LGTM! Namespace migration applied correctly.The import path has been updated to use the new
Neuron\Data\Settings\SettingManagernamespace.resources/app/Initializers/MaintenanceInitializer.php (1)
45-59: Updated Settings manager namespace check looks correctSwitching the instanceof check to
\Neuron\Data\Settings\SettingManageraligns this initializer with the new Settings namespace without changing runtime behavior. No further changes needed here.src/Cms/Events/PasswordResetCompletedEvent.php (1)
24-40: PasswordResetCompletedEvent structure and naming are consistentReadonly payload (User + IP) and the
password.reset_completedname match the existing event style in this module. Good to go.src/Cms/Events/PasswordResetRequestedEvent.php (1)
24-40: PasswordResetRequestedEvent aligns with the existing event modelReadonly User + IP payload and the
password.reset_requestedname follow the same pattern as your other events. Looks good as a foundation for security monitoring and auditing.src/Cms/Events/UserLogoutEvent.php (1)
1-41: LGTM!The event class is well-structured with clear documentation, follows the established event pattern, and properly uses constructor property promotion. The inclusion of
sessionDurationis a valuable addition for analytics.src/Cms/Controllers/Blog.php (1)
40-44: LGTM! Well-structured dependency initialization.The renderer initialization properly chains dependencies (WidgetRenderer → ShortcodeParser → EditorJsRenderer), creating a clean content processing pipeline.
src/Cms/Events/UserLoginFailedEvent.php (1)
1-44: LGTM!The event class is well-designed for security monitoring and audit trails. The inclusion of
reason,ip, andtimestampprovides comprehensive context for failed login attempts, which is essential for security analysis and compliance.src/Cms/Events/MaintenanceModeDisabledEvent.php (1)
8-39: LGTM! Event structure is appropriate.The event class is well-documented and follows the established pattern. Using a string for
$disabledBy(username/identifier) is consistent with the EnabledEvent and provides flexibility.src/Cms/Services/Post/Updater.php (1)
39-62: LGTM! Clean migration to EditorJS content handling.The parameter rename from
$bodyto$contentand the corresponding method call change fromsetBody()tosetContent()properly reflect the shift to EditorJS JSON format. The updated docblock clearly documents this change.src/Cms/Models/Post.php (3)
156-169: LGTM! Good error handling for JSON encoding.The
setContentArray()method properly handles JSON encoding failures with an explicit exception, which is better than silent failures. The use of\JsonExceptionis appropriate.
501-523: Well-designed content handling in fromArray().The method properly handles both
content_rawandcontentkeys, and supports both string and array formats for each. This provides good flexibility for different data sources while maintaining backward compatibility.
625-635: Use match expression correctly handles EditorJS block types.The match expression properly extracts text from various EditorJS block types. The handling of list items with
implode()and the fallback to empty string for unknown types is appropriate.resources/views/admin/posts/create.php (1)
104-108: The image upload endpoint handler for/admin/upload/imageis not implemented in the codebase.While the codebase has a robust CSRF protection system in place (CsrfFilter and CsrfToken service), the actual route handler for the
/admin/upload/imageendpoint referenced in the frontend editor configuration (create.php and edit.php) could not be located. The endpoint is configured in the views but no corresponding PHP route handler exists insrc/Cms/Controllers/or elsewhere.Recommendation: Implement the
/admin/upload/imageroute handler and ensure it is protected with the CsrfFilter to validate CSRF tokens before processing uploads.
|
|
||
| namespace Neuron\Cms\Events; | ||
|
|
||
| use Neuron\Cms\Models\User; |
There was a problem hiding this comment.
Remove unused import.
The User class is imported but not used in this event class. The $disabledBy property is a string, not a User object.
-use Neuron\Cms\Models\User;
use Neuron\Events\IEvent;📝 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.
| use Neuron\Cms\Models\User; | |
| use Neuron\Events\IEvent; |
🤖 Prompt for AI Agents
In src/Cms/Events/MaintenanceModeDisabledEvent.php around line 5, the file
imports Neuron\Cms\Models\User but never uses it (the $disabledBy property is a
string). Remove the unused import line and confirm there are no remaining
references to the User class in this file.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Cms/Repositories/DatabaseUserRepository.php (1)
20-32: PDO connection is created but never used.The
$_pdoproperty is initialized viaConnectionFactory::createFromSettings()but no method in this class uses it. This wastes a database connection. Either remove the PDO initialization or add a comment explaining if it's needed for future use or external access.- private ?PDO $_pdo = null; - /** * Constructor * * @param SettingManager $settings Settings manager with database configuration * @throws Exception if database configuration is missing or adapter is unsupported */ public function __construct( SettingManager $settings ) { - // Keep PDO property for backwards compatibility with tests - $this->_pdo = ConnectionFactory::createFromSettings( $settings ); + // ORM is initialized in Bootstrap - no direct PDO needed }src/Cms/Repositories/DatabasePostRepository.php (1)
260-282:getByCategoryandgetByTagdon't load relations.Unlike
findByIdandfindBySlug, these methods return posts without loading categories and tags. This inconsistency may cause issues when consuming code expects relations to be populated.- return array_map( fn( $row ) => Post::fromArray( $row ), $rows ); + $posts = array_map( fn( $row ) => Post::fromArray( $row ), $rows ); + foreach( $posts as $post ) + { + $this->loadRelations( $post ); + } + return $posts;Apply to both
getByCategoryandgetByTag. Note: this introduces N+1 queries; consider batching if performance is critical.Also applies to: 287-309
♻️ Duplicate comments (1)
resources/views/admin/posts/create.php (1)
99-106: Plugin load check is incomplete (same issue as edit.php).Similar to
edit.php, the plugin availability check only validatesEditorJS,Header, andList, but the configuration uses additional plugins (ImageTool,Quote,CodeTool,Delimiter,RawTool).Apply the same fix suggested for
edit.phpto ensure all required plugins are validated before initialization.
🧹 Nitpick comments (17)
resources/config/neuron.yaml.example (4)
79-90: Cloudinary config is clear; add a note about handling credentials securelyThe structure and defaults look good and match typical Cloudinary usage. To avoid people committing real secrets into VCS, it’s worth explicitly calling that out in the example.
Consider this small tweak:
cloudinary: cloud_name: your-cloud-name # Your Cloudinary cloud name (required) api_key: your-api-key # API key from Cloudinary console (required) - api_secret: your-api-secret # API secret from Cloudinary console (required) + api_secret: your-api-secret # API secret from Cloudinary console (required) + # NOTE: In production, do not commit real Cloudinary credentials to version control. + # Prefer environment-specific config or environment variables for secrets. folder: neuron-cms/images # Default folder for uploads (optional) max_file_size: 5242880 # Maximum file size in bytes (5MB default) allowed_formats: [jpg, jpeg, png, gif, webp] # Allowed image formatsAlso double‑check that
max_file_sizeandallowed_formatsalign with whatever validation the uploader middleware/enforcer is using so the UX is consistent.
91-101: Database example looks fine; verify adapter strings match actual driver identifiersThe keys and comments are straightforward and useful. The only potential footgun is the
adapterstring values and how SQLite is meant to be configured.
- Confirm that
mysql,pgsql, andsqliteexactly match what your DB layer expects (nopdo_mysql/pdo_pgsqlvs custom names mismatch).- For
sqlite,host,port,user, andpassare typically ignored; consider adding a brief comment or a second example to show what a SQLite config should look like to avoid confusion.
110-128: Email/PHPMailer config is well documented; double‑check driver mapping and test mode behaviorThe email section is nicely annotated and gives practical SMTP examples. The main risk is drift between these keys and the actual mailer implementation.
- Ensure that
driver: mail|sendmail|smtpmaps cleanly to the PHPMailer setup (or equivalent) so users don’t hit silent fallbacks.- Confirm that
test_modebehaves exactly as described (log but do not send) across all drivers.- Optionally, you might add a short note that Gmail typically requires an app password and 2FA, which you already hint at via
your-app-passwordbut could call out explicitly in a comment.
129-138: Maintenance config is solid; consider clarifying IP/CIDR supportDefaults here look good (localhost allowlist, clear retry semantics, and optional custom view).
- If the implementation supports CIDR ranges and IPv6 ranges, it may be worth turning the commented
192.168.1.0/24example into a one‑line explanation in the comment to make that capability explicit.- Double‑check that
retry_afteris indeed interpreted as seconds (matching HTTPRetry-After) and thatcustom_viewpaths are resolved consistently with the rest of the view system..github/workflows/ci.yml (1)
10-11: Consider updatingactions/checkoutto v4.
actions/checkout@v3is functional but v4 is the current version with performance improvements and Node.js 20 runtime.- name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4resources/database/migrate/20251205000000_add_two_factor_and_timezone_to_users.php (1)
45-45:update()is called unconditionally.When both columns already exist,
update()is called without any pending changes. While harmless, you could wrap it in a conditional to avoid the no-op database call.- $table->update(); + if( !$table->hasColumn( 'two_factor_recovery_codes' ) || !$table->hasColumn( 'timezone' ) ) + { + $table->update(); + }Alternatively, track whether any column was added and only call
update()if needed.src/Cms/Repositories/DatabasePageRepository.php (1)
24-27: Constructor parameter is unused.The
$settingsparameter is no longer used since ORM initialization moved to Bootstrap. Consider either removing the parameter (breaking change) or documenting why it's retained for interface compatibility.src/Cms/Repositories/DatabasePostRepository.php (1)
411-426: Duplicate code:attachCategories/attachTagsoverlap with sync methods.The attach methods duplicate the INSERT logic from
syncCategories/syncTags. Consider refactoring to have sync call a shared insert helper, or remove the attach methods if sync is the canonical approach.Also applies to: 442-457
resources/views/admin/posts/edit.php (1)
119-126: Plugin load check is incomplete.The interval only checks for
EditorJS,Header, andList, but the editor configuration also usesImageTool,Quote,CodeTool,Delimiter, andRawTool. If any of these fail to load, the editor will initialize but those tools will be unavailable or cause errors.Consider extending the check to include all required plugins:
if (typeof EditorJS !== 'undefined' && typeof Header !== 'undefined' && - typeof List !== 'undefined') { + typeof List !== 'undefined' && + typeof ImageTool !== 'undefined' && + typeof Quote !== 'undefined' && + typeof CodeTool !== 'undefined' && + typeof Delimiter !== 'undefined' && + typeof RawTool !== 'undefined') {resources/views/admin/posts/create.php (1)
118-194: Consider extracting shared EditorJS initialization to a reusable script.The EditorJS initialization logic is nearly identical to
edit.php. While acceptable for view files, extracting the shared initialization code to a dedicated JavaScript file (e.g.,/js/admin/editor-init.js) would reduce duplication and simplify maintenance.src/Cms/Models/Post.php (1)
156-169: Proper error handling in setContentArray.Using
json_last_error_msg()and throwingJsonExceptionprovides clear feedback when JSON encoding fails. However, using theJSON_THROW_ON_ERRORflag withjson_encode()would be more idiomatic for PHP 7.3+.public function setContentArray( array $content ): self { - $encoded = json_encode( $content ); - - if( $encoded === false ) - { - $error = json_last_error_msg(); - throw new \JsonException( "Failed to encode content array to JSON: {$error}" ); - } + $encoded = json_encode( $content, JSON_THROW_ON_ERROR ); $this->_contentRaw = $encoded; $this->_body = $this->extractPlainText( $encoded ); return $this; }src/Cms/Services/Media/MediaValidator.php (2)
108-150: MIME type allowlist is hardcoded and doesn't align with configurableallowed_formats.The
allowed_formatssetting is configurable and used for extension validation, but the$allowedMimeTypesarray is hardcoded. If someone configures additional formats (e.g.,svg,bmp), the MIME check will reject them even though the extension check passes. Also,image/jpgis not a valid MIME type (should only beimage/jpeg).Consider deriving the MIME types from the allowed formats or making both configurable:
- $allowedMimeTypes = [ - 'image/jpeg', - 'image/jpg', - 'image/png', - 'image/gif', - 'image/webp' - ]; + $mimeTypeMap = [ + 'jpg' => 'image/jpeg', + 'jpeg' => 'image/jpeg', + 'png' => 'image/png', + 'gif' => 'image/gif', + 'webp' => 'image/webp' + ]; + + $allowedMimeTypes = array_values( array_intersect_key( $mimeTypeMap, array_flip( $allowedFormats ) ) );
81-99: Zero-byte check should precede max-size check.A zero-byte file will trigger both the
$size > $maxSizecondition (false) and then the$size === 0condition. While logically correct, checking for zero-byte first provides a more specific error message and is slightly more efficient.private function validateFileSize( int $size ): bool { + if( $size === 0 ) + { + $this->_errors[] = 'File is empty'; + return false; + } + $maxSize = $this->_settings->get( 'cloudinary', 'max_file_size' ) ?? 5242880; // 5MB default if( $size > $maxSize ) { $maxSizeMB = round( $maxSize / 1048576, 2 ); $this->_errors[] = "File size exceeds maximum allowed size of {$maxSizeMB}MB"; return false; } - if( $size === 0 ) - { - $this->_errors[] = 'File is empty'; - return false; - } - return true; }tests/Cms/Services/Media/MediaValidatorTest.php (1)
163-167: Consider adding a test for valid image upload and MIME type mismatch.The comment acknowledges that full validation with real image files is complex, but you could use a minimal valid JPEG/PNG byte sequence to test the happy path. Additionally, testing a file with a valid extension but invalid MIME type (e.g., a
.jpgfile containing text) would improve coverage.Would you like me to provide example code for creating a minimal valid image in vfsStream for the happy path test?
src/Cms/Controllers/Admin/Media.php (2)
133-144: Usingexitmakes the controller difficult to unit test.The
exitcalls prevent proper testing and don't align well with MVC patterns. Consider returning early or throwing an exception that the framework catches, or using a response object pattern.If refactoring is not feasible now, you could at least extract the exit to a protected method that can be mocked in tests:
+ /** + * Terminate the response (can be overridden in tests) + */ + protected function terminate(): void + { + exit; + } + private function returnEditorJsSuccess( array $result ): void { echo json_encode( [ 'success' => 1, 'file' => [ 'url' => $result['url'], 'width' => $result['width'], 'height' => $result['height'] ] ] ); - exit; + $this->terminate(); }Also applies to: 168-175
49-82: Consider extracting common upload logic to reduce duplication.Both
uploadImageanduploadFeaturedImageshare the same validation and upload logic, differing only in response format. This could be refactored to a private helper.private function processUpload(): array { if( !isset( $_FILES['image'] ) ) { throw new \InvalidArgumentException( 'No file was uploaded' ); } $file = $_FILES['image']; if( !$this->_validator->validate( $file ) ) { throw new \InvalidArgumentException( $this->_validator->getFirstError() ); } return $this->_uploader->upload( $file['tmp_name'] ); }Also applies to: 92-125
src/Cms/Cli/Commands/Install/UpgradeCommand.php (1)
424-437:runMigrations()is misleading - it doesn't run migrations.The method name suggests it runs migrations, but it only outputs instructions. Either rename it to
displayMigrationInstructions()or implement actual migration execution.- private function runMigrations(): bool + private function displayMigrationInstructions(): void { - $this->output->writeln( "Running migrations...\n" ); - - // For now, instruct user to run migrations manually - // In future, could integrate with MigrationManager - $this->output->info( "Run: php neuron db:migrate" ); - - return true; }Update the call site at line 145 accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
.github/workflows/ci.yml(1 hunks).gitignore(1 hunks)MIGRATIONS.md(1 hunks)UPGRADE_NOTES.md(1 hunks)composer.json(2 hunks)readme.md(1 hunks)resources/.cms-manifest.json(1 hunks)resources/config/database.yaml.example(0 hunks)resources/config/email.yaml.example(0 hunks)resources/config/maintenance.yaml.example(0 hunks)resources/config/neuron.yaml.example(1 hunks)resources/config/routes.yaml(1 hunks)resources/database/migrate/20250111000000_create_users_table.php(1 hunks)resources/database/migrate/20251205000000_add_two_factor_and_timezone_to_users.php(1 hunks)resources/views/admin/posts/create.php(3 hunks)resources/views/admin/posts/edit.php(3 hunks)src/Cms/Cli/Commands/Install/InstallCommand.php(2 hunks)src/Cms/Cli/Commands/Install/UpgradeCommand.php(1 hunks)src/Cms/Cli/Provider.php(1 hunks)src/Cms/Controllers/Admin/Media.php(1 hunks)src/Cms/Dtos/MediaUploadDto.yaml(1 hunks)src/Cms/Models/Post.php(5 hunks)src/Cms/Repositories/DatabaseCategoryRepository.php(7 hunks)src/Cms/Repositories/DatabasePageRepository.php(5 hunks)src/Cms/Repositories/DatabasePostRepository.php(13 hunks)src/Cms/Repositories/DatabaseTagRepository.php(6 hunks)src/Cms/Repositories/DatabaseUserRepository.php(5 hunks)src/Cms/Services/Media/CloudinaryUploader.php(1 hunks)src/Cms/Services/Media/IMediaUploader.php(1 hunks)src/Cms/Services/Media/MediaValidator.php(1 hunks)tests/Cms/BlogControllerTest.php(3 hunks)tests/Cms/Repositories/DatabaseCategoryRepositoryTest.php(2 hunks)tests/Cms/Repositories/DatabasePostRepositoryTest.php(3 hunks)tests/Cms/Repositories/DatabaseTagRepositoryTest.php(2 hunks)tests/Cms/Repositories/DatabaseUserRepositoryTest.php(3 hunks)tests/Cms/Services/AuthenticationTest.php(3 hunks)tests/Cms/Services/Media/CloudinaryUploaderTest.php(1 hunks)tests/Cms/Services/Media/MediaValidatorTest.php(1 hunks)
💤 Files with no reviewable changes (3)
- resources/config/maintenance.yaml.example
- resources/config/email.yaml.example
- resources/config/database.yaml.example
✅ Files skipped from review due to trivial changes (2)
- resources/.cms-manifest.json
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
🧰 Additional context used
🧬 Code graph analysis (13)
resources/views/admin/posts/edit.php (2)
src/Cms/View/helpers.php (1)
route_path(70-79)src/Cms/Models/Post.php (1)
getContentRaw(133-136)
src/Cms/Services/Media/IMediaUploader.php (2)
src/Cms/Controllers/Admin/Media.php (1)
Media(18-192)src/Cms/Services/Media/CloudinaryUploader.php (3)
upload(67-88)uploadFromUrl(98-120)delete(129-142)
src/Cms/Services/Media/MediaValidator.php (2)
src/Cms/Controllers/Admin/Media.php (2)
Media(18-192)__construct(26-39)src/Cms/Services/Media/CloudinaryUploader.php (1)
__construct(27-31)
src/Cms/Controllers/Admin/Media.php (3)
src/Cms/Services/Media/CloudinaryUploader.php (3)
CloudinaryUploader(16-197)__construct(27-31)upload(67-88)src/Cms/Services/Media/MediaValidator.php (3)
MediaValidator(14-192)__construct(24-27)getFirstError(188-191)src/Cms/Services/Media/IMediaUploader.php (1)
upload(22-22)
tests/Cms/Services/Media/CloudinaryUploaderTest.php (2)
src/Cms/Services/Media/CloudinaryUploader.php (3)
CloudinaryUploader(16-197)upload(67-88)uploadFromUrl(98-120)src/Cms/Services/Media/IMediaUploader.php (2)
upload(22-22)uploadFromUrl(32-32)
src/Cms/Repositories/DatabasePostRepository.php (5)
src/Cms/Models/Post.php (9)
fromArray(488-585)getId(57-60)setCategories(376-380)setTags(436-440)toArray(592-609)setId(65-69)getCategories(366-369)getTags(426-429)incrementViewCount(321-325)src/Cms/Models/Category.php (4)
fromArray(143-175)getId(37-40)toArray(182-192)setId(45-49)src/Cms/Models/Tag.php (4)
fromArray(125-156)getId(36-39)toArray(163-172)setId(44-48)src/Cms/Models/User.php (4)
fromArray(438-489)getId(61-64)toArray(413-433)setId(69-73)src/Cms/Repositories/IPostRepository.php (9)
findBySlug(22-22)create(27-27)count(104-104)delete(37-37)all(47-47)getByAuthor(56-56)getByCategory(65-65)getByTag(74-74)incrementViewCount(109-109)
src/Cms/Services/Media/CloudinaryUploader.php (3)
src/Cms/Controllers/Admin/Media.php (2)
Media(18-192)__construct(26-39)src/Cms/Services/Media/MediaValidator.php (1)
__construct(24-27)src/Cms/Services/Media/IMediaUploader.php (3)
upload(22-22)uploadFromUrl(32-32)delete(41-41)
src/Cms/Repositories/DatabaseUserRepository.php (4)
src/Cms/Repositories/DatabaseCategoryRepository.php (4)
create(77-98)delete(131-137)all(142-145)count(150-153)src/Cms/Repositories/DatabasePostRepository.php (4)
create(115-144)delete(212-218)all(223-240)count(338-348)src/Cms/Repositories/DatabaseTagRepository.php (4)
create(61-82)delete(115-121)all(126-129)count(134-137)src/Cms/Models/User.php (3)
toArray(413-433)setId(69-73)getId(61-64)
src/Cms/Repositories/DatabaseCategoryRepository.php (2)
src/Cms/Repositories/DatabaseTagRepository.php (4)
create(61-82)all(126-129)count(134-137)allWithPostCount(142-163)src/Cms/Models/Category.php (3)
toArray(182-192)setId(45-49)getId(37-40)
src/Cms/Cli/Commands/Install/UpgradeCommand.php (1)
src/Cms/Cli/Commands/Install/InstallCommand.php (2)
__construct(25-32)getName(37-40)
tests/Cms/Services/Media/MediaValidatorTest.php (1)
src/Cms/Services/Media/MediaValidator.php (4)
MediaValidator(14-192)validate(35-73)getErrors(178-181)getFirstError(188-191)
src/Cms/Repositories/DatabaseTagRepository.php (1)
src/Cms/Models/Tag.php (3)
toArray(163-172)setId(44-48)getId(36-39)
resources/views/admin/posts/create.php (1)
src/Cms/View/helpers.php (1)
route_path(70-79)
🪛 actionlint (1.7.9)
.github/workflows/ci.yml
27-27: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 LanguageTool
UPGRADE_NOTES.md
[style] ~44-~44: The double modal “Required Run” is nonstandard (only accepted in certain dialects). Consider “to be Run”.
Context: ... Changes - None ### Action Required 1. Run php neuron cms:upgrade to copy new mi...
(NEEDS_FIXED)
🪛 PHPMD (2.15.0)
src/Cms/Cli/Commands/Install/UpgradeCommand.php
61-61: Avoid unused parameters such as '$parameters'. (undefined)
(UnusedFormalParameter)
406-406: Avoid unused local variables such as '$installedMigrations'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (55)
src/Cms/Cli/Commands/Install/InstallCommand.php (2)
9-10: Namespace migration verified as complete.No references to the old
Neuron\Data\Setting\...namespace remain in the codebase. The migration toNeuron\Data\Settings\...is consistently applied across all importing files, including multiple repositories, services, and tests. The imports at lines 9-10 are correct and aligned with the rest of the codebase.
185-186: The upload directories are unused; files are uploaded to Cloudinary, not stored locally.The
/storage/uploadsand/storage/uploads/tempdirectories are created during installation but are never used. The actual upload flow validates files viaMediaValidator(which checks MIME type, extension, and image integrity) and then uploads directly to Cloudinary. No local file storage, caching, or temporary handling occurs.The security concerns and cleanup mechanisms mentioned in the original review are not applicable since files don't reside on disk. If these directories are intended for future local storage fallback or caching, that capability should be implemented and explicitly documented.
Likely an incorrect or invalid review comment.
resources/config/neuron.yaml.example (1)
102-108: Migrations config: ensure paths/table name match the migration tool’s expectationsThe
migrationsblock is sensible and clearly documented. Correctness depends on how your migration tooling is wired.
- Verify that
path,seeds_path, andschema_fileare resolved relative to project root in the same way the migration runner expects.- Confirm that
table: phinx_logis consistent with how your migration library is configured elsewhere (many Phinx setups usephinxlog), or that you’ve intentionally standardized on this custom name.- Make sure
auto_dump_schemais wired into the migration command so toggling it here has the intended effect.readme.md (1)
2-2: LGTM!The Codecov badge addition aligns well with the CI workflow changes that now upload coverage reports.
.github/workflows/ci.yml (1)
13-24: LGTM!The PHP setup with pcov for coverage and the PHPUnit configuration are well-structured. Using
--coverage-filter srcensures only source code is measured.src/Cms/Repositories/DatabaseCategoryRepository.php (8)
37-40: LGTM!Clean ORM usage for the
findByIdmethod.
45-56: LGTM!The
findBySlugandfindByNamemethods correctly use the ORM's fluent query builder pattern.
64-72: LGTM!Good defensive check for empty array before querying, and proper use of
whereInfor batch lookups.
103-126: LGTM!The update method properly handles duplicate detection while excluding the current record, and uses the ORM's
save()method correctly.
131-137: LGTM!The delete method correctly uses the query builder and returns a meaningful boolean result.
142-153: LGTM!The
all()andcount()methods are concise and leverage the ORM effectively.
158-179: LGTM!The raw SQL approach for the JOIN + aggregation query is consistent with
DatabaseTagRepository::allWithPostCount(). The TODO comment appropriately documents the ORM limitation.
77-98: The code is correct as-is. WhiletoArray()does return dates as'Y-m-d H:i:s'strings, the Category model'sfromArray()method explicitly handles string-to-DateTimeImmutable conversion (lines 142-158), which the ORM'screate()method uses internally. Tests confirm this works correctly without any changes needed.src/Cms/Repositories/DatabasePageRepository.php (1)
32-36: LGTM on eager loading for author.Using
Page::with('author')for eager loading is the correct approach to avoid N+1 queries when accessing author data.src/Cms/Repositories/DatabaseUserRepository.php (2)
133-136: ORM query pattern looks correct.The
User::orderBy('created_at', 'DESC')->all()pattern is consistent with other repositories and provides expected ordering.
83-88: No action needed; password hashing is handled correctly.The password is already hashed in the service layer (via
PasswordHasher) before being set on the User model throughsetPasswordHash(). The User model'ssetPasswordHash()method (line 120-124) simply stores the hash value without modification. WhenUser::create($user->toArray())is called, it persists the pre-computed hash as-is; there is no re-hashing mechanism in the model or ORM layer.src/Cms/Repositories/DatabaseTagRepository.php (3)
142-163: Raw SQL forallWithPostCount()is appropriate.Keeping raw SQL for the JOIN with aggregation is the right pragmatic choice. The TODO comment clearly indicates future work.
108-109: Ensuresave()is called on an ORM-hydrated instance.If
$tagwas instantiated vianew Tag()rather than fetched from the database,save()may not work correctly depending on ORM implementation. Verify this path is only reached with ORM-loaded entities.
75-79: This pattern is standard and working across all models in the codebase.The concern about passing null
idintoArray()tocreate()is unfounded. The exact same pattern (Model::create($entity->toArray())) is used consistently across all repositories (User, Post, Page, Category, Tag) and is functioning properly. The Neuron ORM (v0.1.*) handles null values in mass assignment gracefully, treatingnullvalues as absent fields that don't override database defaults like auto-increment IDs. No special filtering is needed.Likely an incorrect or invalid review comment.
src/Cms/Repositories/DatabasePostRepository.php (2)
163-196: Raw SQL UPDATE with ORM CREATE is intentional but worth documenting.The comment on lines 163-164 explains why raw SQL is used for updates. This asymmetry (ORM for create, raw SQL for update) is acceptable given the ORM limitation, but consider adding a TODO to revisit when the ORM supports dirty tracking.
76-110:loadRelationshelper is well-structured.The method cleanly separates loading categories and tags via JOINs. Good use of
array_mapwith arrow functions.resources/database/migrate/20251205000000_add_two_factor_and_timezone_to_users.php (1)
26-43: Theaftercolumn references are safe—remove unnecessary idempotency checks.The columns
two_factor_secretandlast_login_atare created in the initial migration, so theafterplacement is valid. However,two_factor_recovery_codesandtimezoneare already defined in20250111000000_create_users_table.php. ThehasColumn()guards make this migration safe for existing installations, but the column definitions are redundant for new installs. Either consolidate these columns into the initial migration, or clarify in code comments why this migration exists as a separate patch.resources/database/migrate/20250111000000_create_users_table.php (1)
25-25: LGTM! Verify recovery codes are encrypted at the application layer.The
two_factor_recovery_codescolumn is appropriately defined as nullable TEXT. Since recovery codes are sensitive authentication credentials, ensure they are encrypted before storage at the application layer.tests/Cms/Repositories/DatabaseTagRepositoryTest.php (1)
8-8: LGTM! ORM initialization follows consistent test pattern.The ORM initialization with
Model::setPdo()in setUp is consistent with the broader migration to ORM-based data access across repository tests.Also applies to: 33-34
tests/Cms/Repositories/DatabaseCategoryRepositoryTest.php (1)
8-8: LGTM! ORM initialization follows consistent test pattern.The ORM initialization matches the pattern used in other repository tests, supporting the migration to ORM-based data access.
Also applies to: 33-34
tests/Cms/Repositories/DatabaseUserRepositoryTest.php (1)
8-9: LGTM! Test updates align with ORM migration and 2FA support.The changes correctly reflect:
- Namespace migration for SettingManager
- ORM initialization consistent with other repository tests
- Schema update for
two_factor_recovery_codesmatching the migrationAlso applies to: 47-48, 66-66
src/Cms/Cli/Provider.php (1)
21-30: LGTM! Upgrade command registration follows CLI conventions.The new
cms:upgradecommand is properly registered using the same pattern as existing commands. The comment update to plural form is appropriate.src/Cms/Dtos/MediaUploadDto.yaml (1)
1-19: LGTM! Validation rules are appropriate for media uploads.The DTO configuration correctly defines:
- Required file field with NotEmpty validation
- Optional folder field with reasonable 255-character limit
tests/Cms/BlogControllerTest.php (3)
13-16: LGTM! ORM initialization and Settings migration applied correctly.The test setup now properly initializes the ORM and uses the migrated Settings namespace, consistent with other test files.
Also applies to: 54-55
100-120: LGTM! Users table creation supports blog author functionality.The addition of the users table in test setup is appropriate if the Blog controller needs to access author information. The schema correctly includes the new 2FA fields.
129-129: LGTM! Editor.js default structure is correctly formatted.The
content_rawcolumn default value'{"blocks":[]}'is a valid empty Editor.js document structure, supporting the block-based editor upgrade.tests/Cms/Services/Media/CloudinaryUploaderTest.php (3)
17-29: LGTM! Test setup correctly initializes Cloudinary configuration.The in-memory settings with test Cloudinary credentials are appropriate for unit testing the uploader service.
31-61: LGTM! Unit tests appropriately cover error paths.The tests correctly validate exception handling for:
- Missing configuration
- Non-existent files
- Invalid URLs
68-113: LGTM! Integration tests appropriately marked incomplete.Marking integration tests as incomplete is correct since they require actual Cloudinary credentials and external resources. The commented examples provide clear guidance for future integration testing.
UPGRADE_NOTES.md (1)
1-151: Well-structured upgrade documentation.The upgrade notes provide clear instructions for the upgrade workflow, version-specific changes, and troubleshooting guidance. The section headers and formatting are appropriate for developer documentation.
One consideration: ensure the version numbers (2025.12.5, 2025.11.7) align with actual release tags in the repository to avoid confusion.
tests/Cms/Repositories/DatabasePostRepositoryTest.php (2)
35-36: LGTM - ORM initialization in test setup.Initializing the ORM with the test PDO connection ensures the repository tests can properly exercise ORM-backed data access.
56-85: Schema updates align with migration requirements.The test schema correctly includes the new
two_factor_recovery_codes,timezonecolumns for users, andcontent_rawfor posts. The default'{"blocks":[]}'matches the Editor.js empty content structure used in the Post model.resources/views/admin/posts/edit.php (2)
93-103: Good XSS prevention in content loading.Using
JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOTflags ensures the JSON output is safe for embedding in a script context, preventing injection attacks via malicious post content.
199-211: Form submission pattern is correct.Calling
e.target.submit()aftere.preventDefault()correctly bypasses the event listener and performs a native form submission, avoiding an infinite loop.tests/Cms/Services/AuthenticationTest.php (2)
11-12: LGTM - Updated imports for ORM and Settings namespace.The namespace migration from
Neuron\Data\SettingtoNeuron\Data\Settingsand the addition of ORM initialization align with the broader refactoring in this PR.
50-51: ORM initialization added correctly.Initializing the ORM with the test PDO connection before creating tables ensures proper ORM functionality during authentication tests.
resources/views/admin/posts/create.php (1)
75-82: CDN versions are now pinned - previous feedback addressed.The script versions are pinned to specific versions (e.g.,
@2.29.1,@2.8.1), which addresses the previous review concern about using@latest.src/Cms/Services/Media/IMediaUploader.php (1)
12-42: Well-designed media uploader interface.The
IMediaUploaderinterface provides a clean contract for media operations with appropriate PHPDoc documentation. The return array structure (url,public_id,width,height,format) is well-documented and aligns with theCloudinaryUploaderimplementation shown in the relevant snippets.src/Cms/Models/Post.php (2)
21-22: Good design for dual content storage.Maintaining both
_body(plain text) and_contentRaw(Editor.js JSON) provides backward compatibility for code that expects plain text while supporting the new block-based editor format.
499-533: Content initialization in fromArray is well-structured.The logic correctly handles multiple input formats (
content_rawas string/array,contentas string/array) and properly falls back to extracting plain text whenbodyis not explicitly provided.src/Cms/Services/Media/MediaValidator.php (1)
14-73: LGTM!The overall validation flow is well-structured with proper early returns, error accumulation, and comprehensive checks including upload errors, file existence, size, and type validation. The use of
is_uploaded_file()could provide additional security for production environments, butfile_exists()is acceptable for general validation.tests/Cms/Services/Media/MediaValidatorTest.php (2)
94-111: Test relies on$file['size']from array, not actual file size.The test creates a file with
'fake content'(12 bytes) but passessize => 10485760in the array. This works becauseMediaValidatortrusts$file['size']from the$_FILESsuperglobal. This is correct behavior for testing the validator, but consider adding a comment clarifying that PHP populates$_FILES['size']at upload time.
20-34: LGTM!Good test setup with virtual filesystem and in-memory settings. The configuration matches the validator's expected defaults, enabling consistent test behavior.
src/Cms/Controllers/Admin/Media.php (1)
26-39: LGTM!Good defensive check for Settings in the constructor with a clear exception message. The dependency injection via Registry is appropriate for this framework.
src/Cms/Services/Media/CloudinaryUploader.php (3)
39-57: LGTM!Good validation of required Cloudinary configuration with a clear error message. The exception during construction ensures fast-fail behavior.
184-196: LGTM!Clean normalization of Cloudinary response with sensible defaults for missing fields. Preferring
secure_urloverurlis the correct approach.
67-88: LGTM!Good file existence validation before upload attempt, and proper exception chaining preserves the original error context.
src/Cms/Cli/Commands/Install/UpgradeCommand.php (3)
61-61: Unused$parametersis acceptable for interface compliance.The static analysis flags
$parametersas unused, but this is a common pattern for CLI command implementations where the method signature is dictated by the parent class. No action needed.
21-28: LGTM!Constructor follows the same pattern as
InstallCommand, usinggetcwd()for project path and relative path traversal for component path.
284-290: LGTM!Clean use of
array_diffto identify new migrations by comparing package and installed lists.
| # Database Migrations Guide | ||
|
|
||
| This document provides comprehensive guidance for working with database migrations in Neuron CMS. | ||
|
|
||
| ## Table of Contents | ||
|
|
||
| 1. [Core Principles](#core-principles) | ||
| 2. [Migration Workflow](#migration-workflow) | ||
| 3. [Common Scenarios](#common-scenarios) | ||
| 4. [Upgrade Path Considerations](#upgrade-path-considerations) | ||
| 5. [Best Practices](#best-practices) | ||
| 6. [Troubleshooting](#troubleshooting) | ||
|
|
||
| ## Core Principles | ||
|
|
||
| ### Never Modify Existing Migrations | ||
|
|
||
| **CRITICAL RULE: Once a migration has been committed to the repository, NEVER modify it.** | ||
|
|
||
| **Why?** | ||
| - Phinx tracks which migrations have been executed using a `phinxlog` table | ||
| - Existing installations have already run the original migration | ||
| - Modifying an existing migration will NOT update those installations | ||
| - This creates schema drift between installations | ||
|
|
||
| **Example of What NOT to Do:** | ||
|
|
||
| ```php | ||
| // ❌ WRONG: Editing cms/resources/database/migrate/20250111000000_create_users_table.php | ||
| // to add a new column after it's already been committed | ||
| public function change() | ||
| { | ||
| $table = $this->table( 'users' ); | ||
| $table->addColumn( 'username', 'string' ) | ||
| ->addColumn( 'email', 'string' ) | ||
| ->addColumn( 'new_column', 'string' ) // DON'T ADD THIS HERE! | ||
| ->create(); | ||
| } | ||
| ``` | ||
|
|
||
| ### Always Create New Migrations for Schema Changes | ||
|
|
||
| **Correct Approach:** Create a new migration file with a new timestamp. | ||
|
|
||
| ```php | ||
| // ✅ CORRECT: Create cms/resources/database/migrate/20251205000000_add_new_column_to_users.php | ||
| use Phinx\Migration\AbstractMigration; | ||
|
|
||
| class AddNewColumnToUsers extends AbstractMigration | ||
| { | ||
| public function change() | ||
| { | ||
| $table = $this->table( 'users' ); | ||
| $table->addColumn( 'new_column', 'string', [ 'null' => true ] ) | ||
| ->update(); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ## Migration Workflow | ||
|
|
||
| ### Creating a New Migration | ||
|
|
||
| 1. **Generate migration file with timestamp:** | ||
| ```bash | ||
| # Format: YYYYMMDDHHMMSS_description_of_change.php | ||
| # Example: 20251205143000_add_timezone_to_users.php | ||
| ``` | ||
|
|
||
| 2. **Use descriptive names:** | ||
| - `add_[column]_to_[table].php` - Adding columns | ||
| - `remove_[column]_from_[table].php` - Removing columns | ||
| - `create_[table]_table.php` - Creating new tables | ||
| - `rename_[old]_to_[new]_in_[table].php` - Renaming columns | ||
|
|
||
| 3. **Place migrations in the correct location:** | ||
| - CMS component: `cms/resources/database/migrate/` | ||
| - Test installations: `testing/*/db/migrate/` | ||
|
|
||
| ### Implementing the Migration | ||
|
|
||
| ```php | ||
| <?php | ||
|
|
||
| use Phinx\Migration\AbstractMigration; | ||
|
|
||
| /** | ||
| * Brief description of what this migration does | ||
| */ | ||
| class AddTimezoneToUsers extends AbstractMigration | ||
| { | ||
| /** | ||
| * Change Method. | ||
| * | ||
| * Write your reversible migrations using this method. | ||
| * | ||
| * For more information see documentation: | ||
| * https://book.cakephp.org/phinx/0/en/migrations.html | ||
| */ | ||
| public function change() | ||
| { | ||
| $table = $this->table( 'users' ); | ||
|
|
||
| $table->addColumn( 'timezone', 'string', [ | ||
| 'limit' => 50, | ||
| 'default' => 'UTC', | ||
| 'null' => false, | ||
| 'after' => 'last_login_at' // Optional: specify column position | ||
| ]) | ||
| ->update(); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Testing the Migration | ||
|
|
||
| 1. **Test in development environment:** | ||
| ```bash | ||
| php neuron db:migrate | ||
| ``` | ||
|
|
||
| 2. **Test rollback (if applicable):** | ||
| ```bash | ||
| php neuron db:rollback | ||
| ``` | ||
|
|
||
| 3. **Verify schema changes:** | ||
| ```bash | ||
| # SQLite | ||
| sqlite3 storage/database.sqlite3 "PRAGMA table_info(users);" | ||
|
|
||
| # MySQL | ||
| mysql -u user -p -e "DESCRIBE users;" database_name | ||
| ``` | ||
|
|
||
| ## Common Scenarios | ||
|
|
||
| ### Adding a Column to an Existing Table | ||
|
|
||
| ```php | ||
| class AddRecoveryCodeToUsers extends AbstractMigration | ||
| { | ||
| public function change() | ||
| { | ||
| $table = $this->table( 'users' ); | ||
| $table->addColumn( 'two_factor_recovery_codes', 'text', [ | ||
| 'null' => true, | ||
| 'comment' => 'JSON-encoded recovery codes for 2FA' | ||
| ]) | ||
| ->update(); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Adding Multiple Columns | ||
|
|
||
| ```php | ||
| class AddUserPreferences extends AbstractMigration | ||
| { | ||
| public function change() | ||
| { | ||
| $table = $this->table( 'users' ); | ||
| $table->addColumn( 'timezone', 'string', [ 'limit' => 50, 'default' => 'UTC' ] ) | ||
| ->addColumn( 'language', 'string', [ 'limit' => 10, 'default' => 'en' ] ) | ||
| ->addColumn( 'theme', 'string', [ 'limit' => 20, 'default' => 'light' ] ) | ||
| ->update(); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Renaming a Column | ||
|
|
||
| ```php | ||
| class RenamePasswordHashInUsers extends AbstractMigration | ||
| { | ||
| public function change() | ||
| { | ||
| $table = $this->table( 'users' ); | ||
| $table->renameColumn( 'password_hash', 'hashed_password' ) | ||
| ->update(); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Adding an Index | ||
|
|
||
| ```php | ||
| class AddTimezoneIndexToUsers extends AbstractMigration | ||
| { | ||
| public function change() | ||
| { | ||
| $table = $this->table( 'users' ); | ||
| $table->addIndex( [ 'timezone' ], [ 'name' => 'idx_users_timezone' ] ) | ||
| ->update(); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Modifying a Column (Breaking Change) | ||
|
|
||
| When you need to change a column's type or constraints: | ||
|
|
||
| ```php | ||
| class ModifyEmailColumnInUsers extends AbstractMigration | ||
| { | ||
| public function change() | ||
| { | ||
| $table = $this->table( 'users' ); | ||
|
|
||
| // Phinx doesn't directly support changeColumn in all cases | ||
| // You may need to use raw SQL for complex changes | ||
| $table->changeColumn( 'email', 'string', [ | ||
| 'limit' => 320, // Changed from 255 to support longer emails | ||
| 'null' => false | ||
| ]) | ||
| ->update(); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Creating a New Table (with Foreign Keys) | ||
|
|
||
| ```php | ||
| class CreateSessionsTable extends AbstractMigration | ||
| { | ||
| public function change() | ||
| { | ||
| $table = $this->table( 'sessions' ); | ||
|
|
||
| $table->addColumn( 'user_id', 'integer', [ 'null' => false ] ) | ||
| ->addColumn( 'token', 'string', [ 'limit' => 64 ] ) | ||
| ->addColumn( 'ip_address', 'string', [ 'limit' => 45, 'null' => true ] ) | ||
| ->addColumn( 'user_agent', 'string', [ 'limit' => 255, 'null' => true ] ) | ||
| ->addColumn( 'expires_at', 'timestamp', [ 'null' => false ] ) | ||
| ->addColumn( 'created_at', 'timestamp', [ 'default' => 'CURRENT_TIMESTAMP' ] ) | ||
| ->addIndex( [ 'token' ], [ 'unique' => true ] ) | ||
| ->addIndex( [ 'user_id' ] ) | ||
| ->addForeignKey( 'user_id', 'users', 'id', [ | ||
| 'delete' => 'CASCADE', | ||
| 'update' => 'CASCADE' | ||
| ]) | ||
| ->create(); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ## Upgrade Path Considerations | ||
|
|
||
| ### Problem: Schema Drift Between Installations | ||
|
|
||
| When you update the CMS code via `composer update`, the code changes (like Model classes expecting new columns) but the database schema doesn't automatically update. | ||
|
|
||
| **Symptoms:** | ||
| - `SQLSTATE[HY000]: General error: 1 no such column: column_name` | ||
| - Model methods reference columns that don't exist in older installations | ||
|
|
||
| ### Solution: Migration-Based Upgrades | ||
|
|
||
| 1. **Update the initial migration for NEW installations:** | ||
| - Edit the `create_*_table.php` migration in development | ||
| - This ensures new installations get the complete schema | ||
|
|
||
| 2. **Create an ALTER migration for EXISTING installations:** | ||
| - Create `add_*_to_*.php` migration with the same changes | ||
| - This updates installations that already ran the original migration | ||
|
|
||
| **Example Workflow:** | ||
|
|
||
| ```bash | ||
| # Step 1: User model now needs 'timezone' column | ||
| # Don't edit: 20250111000000_create_users_table.php (old installations already ran this) | ||
|
|
||
| # Step 2: Create new migration | ||
| touch cms/resources/database/migrate/20251205000000_add_timezone_to_users.php | ||
|
|
||
| # Step 3: Implement the migration | ||
| # (see examples above) | ||
|
|
||
| # Step 4: Document in versionlog.md | ||
| # Version X.Y.Z | ||
| # - Added timezone column to users table (Migration: 20251205000000) | ||
|
|
||
| # Step 5: Users upgrade via composer and run: | ||
| php neuron db:migrate | ||
| ``` | ||
|
|
||
| ### cms:install Command Behavior | ||
|
|
||
| The `cms:install` command (`src/Cms/Cli/Commands/Install/InstallCommand.php`): | ||
|
|
||
| 1. Copies ALL migration files from `cms/resources/database/migrate/` to project | ||
| 2. **Skips** migrations that already exist (by filename) | ||
| 3. Optionally runs migrations | ||
|
|
||
| **Limitation:** When you run `composer update`, new migrations in the CMS package don't automatically copy to existing installations. | ||
|
|
||
| **Workaround:** Manually copy new migrations or run `cms:install` with reinstall option (will overwrite files). | ||
|
|
||
| **Future Enhancement:** Create `cms:upgrade` command to: | ||
| - Detect new migrations in CMS package | ||
| - Copy them to installation | ||
| - Optionally run them | ||
|
|
||
| ## Best Practices | ||
|
|
||
| ### 1. Use Descriptive Migration Names | ||
| ``` | ||
| ✅ 20251205120000_add_two_factor_recovery_codes_to_users.php | ||
| ❌ 20251205120000_update_users.php | ||
| ``` | ||
|
|
||
| ### 2. Include Comments in Migration Code | ||
| ```php | ||
| /** | ||
| * Add two-factor authentication recovery codes to users table | ||
| * | ||
| * This migration adds support for 2FA recovery codes, allowing users | ||
| * to regain access if they lose their authenticator device. | ||
| */ | ||
| class AddTwoFactorRecoveryCodesToUsers extends AbstractMigration | ||
| ``` | ||
|
|
||
| ### 3. Always Test Rollbacks | ||
| ```php | ||
| // Make migrations reversible when possible | ||
| public function change() | ||
| { | ||
| // Phinx can automatically reverse addColumn, addIndex, etc. | ||
| $table = $this->table( 'users' ); | ||
| $table->addColumn( 'timezone', 'string' )->update(); | ||
| } | ||
|
|
||
| // For complex migrations, implement up/down explicitly | ||
| public function up() | ||
| { | ||
| // Migration code | ||
| } | ||
|
|
||
| public function down() | ||
| { | ||
| // Rollback code | ||
| } | ||
| ``` | ||
|
|
||
| ### 4. Handle NULL Values Appropriately | ||
|
|
||
| When adding columns to tables with existing data: | ||
|
|
||
| ```php | ||
| // Good: Allow NULL or provide default value | ||
| $table->addColumn( 'timezone', 'string', [ | ||
| 'default' => 'UTC', | ||
| 'null' => false | ||
| ]); | ||
|
|
||
| // Alternative: Allow NULL, update later | ||
| $table->addColumn( 'timezone', 'string', [ 'null' => true ] ); | ||
| ``` | ||
|
|
||
| ### 5. Document Breaking Changes | ||
|
|
||
| If a migration requires manual intervention: | ||
|
|
||
| ```php | ||
| /** | ||
| * BREAKING CHANGE: Removes legacy authentication method | ||
| * | ||
| * BEFORE RUNNING: | ||
| * 1. Ensure all users have migrated to new auth system | ||
| * 2. Backup the database | ||
| * 3. Review docs at: docs/auth-migration.md | ||
| */ | ||
| class RemoveLegacyAuthColumns extends AbstractMigration | ||
| ``` | ||
|
|
||
| ### 6. Version Documentation | ||
|
|
||
| Update `versionlog.md` with migration information: | ||
|
|
||
| ```markdown | ||
| ## Version 2.1.0 - 2025-12-05 | ||
|
|
||
| ### Database Changes | ||
| - Added `two_factor_recovery_codes` column to users table | ||
| - Added `timezone` column to users table with default 'UTC' | ||
| - Migration files: 20251205000000_add_two_factor_and_timezone_to_users.php | ||
|
|
||
| ### Upgrade Notes | ||
| Run `php neuron db:migrate` to apply schema changes. | ||
| ``` | ||
|
|
||
| ## Troubleshooting | ||
|
|
||
| ### Migration Already Exists Error | ||
|
|
||
| **Problem:** Migration file exists in both CMS package and installation, but with different content. | ||
|
|
||
| **Solution:** | ||
| - Check which version ran (look at installation's file modification date) | ||
| - Create a new migration to reconcile differences | ||
| - Never overwrite the existing migration | ||
|
|
||
| ### Column Already Exists | ||
|
|
||
| **Problem:** Migration tries to add a column that already exists. | ||
|
|
||
| ``` | ||
| SQLSTATE[HY000]: General error: 1 duplicate column name | ||
| ``` | ||
|
|
||
| **Solution:** | ||
| ```php | ||
| public function change() | ||
| { | ||
| $table = $this->table( 'users' ); | ||
|
|
||
| // Check if column exists before adding | ||
| if( !$table->hasColumn( 'timezone' ) ) | ||
| { | ||
| $table->addColumn( 'timezone', 'string', [ 'default' => 'UTC' ] ) | ||
| ->update(); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Migration Tracking Out of Sync | ||
|
|
||
| **Problem:** Phinx thinks a migration ran, but the schema change isn't present. | ||
|
|
||
| **Solution:** | ||
| ```bash | ||
| # Check migration status | ||
| php neuron db:status | ||
|
|
||
| # If needed, manually fix phinxlog table | ||
| sqlite3 storage/database.sqlite3 | ||
| > DELETE FROM phinxlog WHERE version = '20251205000000'; | ||
| > .quit | ||
|
|
||
| # Re-run migration | ||
| php neuron db:migrate | ||
| ``` | ||
|
|
||
| ### Data Loss Prevention | ||
|
|
||
| **Always backup before:** | ||
| - Dropping columns | ||
| - Renaming columns | ||
| - Changing column types | ||
| - Dropping tables | ||
|
|
||
| ```bash | ||
| # SQLite backup | ||
| cp storage/database.sqlite3 storage/database.sqlite3.backup | ||
|
|
||
| # MySQL backup | ||
| mysqldump -u user -p database_name > backup.sql | ||
| ``` | ||
|
|
||
| ## Additional Resources | ||
|
|
||
| - [Phinx Documentation](https://book.cakephp.org/phinx/0/en/migrations.html) | ||
| - [Neuron CMS Installation Guide](README.md) | ||
| - Project-wide migration guidelines: `/CLAUDE.md` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the referenced CLAUDE.md file exists
fd -t f 'CLAUDE.md' 2>/dev/null || echo "CLAUDE.md not found"
# Also check for any other file references in MIGRATIONS.md
echo "=== Checking referenced files in MIGRATIONS.md ==="
rg '(README\.md|/CLAUDE\.md)' MIGRATIONS.mdRepository: Neuron-PHP/cms
Length of output: 203
🏁 Script executed:
#!/bin/bash
# Verify README.md exists
fd -t f 'README.md' | head -5Repository: Neuron-PHP/cms
Length of output: 87
Remove or update broken reference to non-existent CLAUDE.md file.
The documentation is comprehensive and well-structured, but the "Additional Resources" section references /CLAUDE.md which does not exist in the repository. Either remove this reference or verify it should point to an existing file.
🤖 Prompt for AI Agents
MIGRATIONS.md around lines 1 to 464: the "Additional Resources" section
references a non-existent /CLAUDE.md; remove that entry or replace it with the
correct, existing document path (e.g., CONTRIBUTING.md, README.md, or another
repository doc) after verifying via repo search, then update the Additional
Resources list and commit the change.
| # Media Upload Routes | ||
| admin_media_upload: | ||
| method: POST | ||
| route: /admin/upload/image | ||
| controller: Neuron\Cms\Controllers\Admin\Media@uploadImage | ||
| filter: auth | ||
|
|
||
| admin_media_featured_upload: | ||
| method: POST | ||
| route: /admin/upload/featured-image | ||
| controller: Neuron\Cms\Controllers\Admin\Media@uploadFeaturedImage | ||
| filter: auth |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the Media controller
find . -type f -name "*.php" | grep -i media | head -20Repository: Neuron-PHP/cms
Length of output: 335
🏁 Script executed:
# Search for the Media controller class definition
rg -l "class Media" --type=phpRepository: Neuron-PHP/cms
Length of output: 183
🏁 Script executed:
# Check for CSRF protection patterns in the codebase
rg -n "csrf|CSRF|VerifyCsrfToken|CsrfToken" --type=php --type=yaml | head -30Repository: Neuron-PHP/cms
Length of output: 2706
🏁 Script executed:
# Look for middleware configuration and auth filter implementation
rg -n "filter.*auth|auth.*filter" resources/config/routes.yaml | head -20Repository: Neuron-PHP/cms
Length of output: 467
🏁 Script executed:
# Read the Media controller to check for CSRF validation
wc -l ./src/Cms/Controllers/Admin/Media.phpRepository: Neuron-PHP/cms
Length of output: 99
🏁 Script executed:
# Get the Media controller content
cat -n ./src/Cms/Controllers/Admin/Media.phpRepository: Neuron-PHP/cms
Length of output: 5155
🏁 Script executed:
# Check for auth filter implementation
find . -type f -name "*.php" -path "*/filters/*" -o -name "*Filter*" -o -name "*Auth*" | grep -i filter | head -20Repository: Neuron-PHP/cms
Length of output: 366
🏁 Script executed:
# Search for where CSRF is actually applied - middleware or filter
rg -l "VerifyCsrfToken|csrf.*middleware|middleware.*csrf" --type=phpRepository: Neuron-PHP/cms
Length of output: 40
🏁 Script executed:
# Read the AuthenticationFilter to see if it handles CSRF
cat -n ./src/Cms/Auth/Filters/AuthenticationFilter.phpRepository: Neuron-PHP/cms
Length of output: 2272
🏁 Script executed:
# Read the CsrfFilter to understand how CSRF is handled
cat -n ./src/Cms/Auth/Filters/CsrfFilter.phpRepository: Neuron-PHP/cms
Length of output: 2589
🏁 Script executed:
# Check the routes configuration to see which filters are applied globally or to upload routes
cat -n resources/config/routes.yaml | sed -n '240,280p'Repository: Neuron-PHP/cms
Length of output: 1248
CSRF protection missing: Apply CsrfFilter to media upload endpoints.
The routes use filter: auth but should also include CSRF protection for POST endpoints. A CsrfFilter exists in the codebase that validates CSRF tokens on state-changing requests, but these upload routes do not apply it. Add csrf filter to both routes (e.g., filter: auth csrf or configure a composite filter) to protect against Cross-Site Request Forgery attacks on file uploads.
🤖 Prompt for AI Agents
In resources/config/routes.yaml around lines 254 to 265 the POST routes for
admin_media_upload and admin_media_featured_upload only apply the auth filter
but lack CSRF protection; update both route definitions to include the
CsrfFilter by adding the csrf filter alongside auth (e.g., "filter: auth csrf"
or equivalent composite filter) so POST upload endpoints validate CSRF tokens
before processing file uploads.
| $packageManifestJson = file_get_contents( $packageManifestPath ); | ||
| $this->_packageManifest = json_decode( $packageManifestJson, true ); | ||
|
|
||
| if( !$this->_packageManifest ) | ||
| { | ||
| $this->output->error( "Failed to parse package manifest" ); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
JSON decode failure not properly detected.
json_decode returns null on parse error, but an empty JSON object {} or null literal also decodes to falsy values. Use json_last_error() for reliable error detection.
$packageManifestJson = file_get_contents( $packageManifestPath );
$this->_packageManifest = json_decode( $packageManifestJson, true );
- if( !$this->_packageManifest )
+ if( json_last_error() !== JSON_ERROR_NONE )
{
- $this->output->error( "Failed to parse package manifest" );
+ $this->output->error( "Failed to parse package manifest: " . json_last_error_msg() );
return false;
}Apply similar fix at line 186 for installed manifest parsing.
📝 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.
| $packageManifestJson = file_get_contents( $packageManifestPath ); | |
| $this->_packageManifest = json_decode( $packageManifestJson, true ); | |
| if( !$this->_packageManifest ) | |
| { | |
| $this->output->error( "Failed to parse package manifest" ); | |
| return false; | |
| } | |
| $packageManifestJson = file_get_contents( $packageManifestPath ); | |
| $this->_packageManifest = json_decode( $packageManifestJson, true ); | |
| if( json_last_error() !== JSON_ERROR_NONE ) | |
| { | |
| $this->output->error( "Failed to parse package manifest: " . json_last_error_msg() ); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/Cms/Cli/Commands/Install/UpgradeCommand.php around lines 171-178, the
code treats any falsy return from json_decode as a parse failure; instead, check
json_last_error() after json_decode to reliably detect parse errors and only
treat it as a failure when json_last_error() !== JSON_ERROR_NONE. Update the
error handling to call json_last_error_msg() (or include json_last_error()) in
the output error for better diagnostics. Also apply the same change at the
installed manifest parsing around line 186 so both manifest parses validate JSON
via json_last_error() rather than truthiness.
| private function updateInstalledManifest(): bool | ||
| { | ||
| $manifestPath = $this->_projectPath . '/.cms-manifest.json'; | ||
|
|
||
| // Merge new migrations into installed list | ||
| $installedMigrations = $this->_installedManifest['migrations'] ?? []; | ||
| $packageMigrations = $this->_packageManifest['migrations'] ?? []; | ||
|
|
||
| $this->_installedManifest['version'] = $this->_packageManifest['version']; | ||
| $this->_installedManifest['updated_at'] = date( 'Y-m-d H:i:s' ); | ||
| $this->_installedManifest['migrations'] = $packageMigrations; | ||
|
|
||
| $json = json_encode( $this->_installedManifest, JSON_PRETTY_PRINT ); | ||
|
|
||
| if( file_put_contents( $manifestPath, $json ) === false ) | ||
| { | ||
| $this->output->warning( "Failed to update manifest file" ); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused $installedMigrations variable.
As flagged by static analysis, $installedMigrations is assigned but never used. The manifest is updated to use $packageMigrations directly.
private function updateInstalledManifest(): bool
{
$manifestPath = $this->_projectPath . '/.cms-manifest.json';
- // Merge new migrations into installed list
- $installedMigrations = $this->_installedManifest['migrations'] ?? [];
$packageMigrations = $this->_packageManifest['migrations'] ?? [];
$this->_installedManifest['version'] = $this->_packageManifest['version'];
$this->_installedManifest['updated_at'] = date( 'Y-m-d H:i:s' );
$this->_installedManifest['migrations'] = $packageMigrations;🧰 Tools
🪛 PHPMD (2.15.0)
406-406: Avoid unused local variables such as '$installedMigrations'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In src/Cms/Cli/Commands/Install/UpgradeCommand.php around lines 401 to 422, the
local variable $installedMigrations is assigned but never used; remove the
unused assignment and any related dead code so the function only gathers
$packageMigrations and writes the manifest, ensuring no other references to
$installedMigrations remain in this method.
| 'list' => isset( $block['data']['items'] ) && is_array( $block['data']['items'] ) | ||
| ? implode( "\n", $block['data']['items'] ) | ||
| : '', |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Editor.js List plugin v1.9 nested items data structure content property
💡 Result:
Short answer: In Editor.js List (v1.9 / nested-list family) each list item is an object { content: string, meta: {...}, items: Item[] } — the content property is a plain string containing the item's text/HTML (not an array of blocks). Nested items are stored in the items array (recursive Item objects). Example:
{
"type": "list",
"data": {
"style": "unordered",
"items": [
{
"content": "Apples",
"meta": {},
"items": [
{ "content": "Red", "meta": {}, "items": [] }
]
}
]
}
}
Sources: Editor.js List README (item shape & examples). [1][2]
References
[1] editor-js/list (README) — GitHub.
[2] @editorjs/nested-list (README) — npm.
🏁 Script executed:
# Locate and examine the Post.php file
fd "Post.php" -t fRepository: Neuron-PHP/cms
Length of output: 81
🏁 Script executed:
# Search for list handling in the codebase
rg "extractListItems|extractPlainText" -t php -B 3 -A 10Repository: Neuron-PHP/cms
Length of output: 2868
🏁 Script executed:
# Check for any tests related to list extraction
rg "list.*items|nested.*list" -t php --max-count 20Repository: Neuron-PHP/cms
Length of output: 167
🏁 Script executed:
# Get the full Post.php file with line numbers to see context around lines 638-640
wc -l src/Cms/Models/Post.phpRepository: Neuron-PHP/cms
Length of output: 85
🏁 Script executed:
# Read the extractPlainText method - estimate lines 600-700
sed -n '600,700p' src/Cms/Models/Post.php | cat -nRepository: Neuron-PHP/cms
Length of output: 1995
List item extraction will produce "Array" strings for nested lists.
The Editor.js List plugin (v1.9+) stores list items as objects with content and items properties for nested structures, not simple strings. The current implode() call at lines 639-640 will convert nested item objects to the literal string "Array", losing the actual content.
Consider handling nested list structures:
'list' => isset( $block['data']['items'] ) && is_array( $block['data']['items'] )
- ? implode( "\n", $block['data']['items'] )
+ ? $this->extractListItems( $block['data']['items'] )
: '',With a helper method:
private function extractListItems( array $items ): string
{
$texts = [];
foreach( $items as $item )
{
if( is_string( $item ) )
{
$texts[] = $item;
}
elseif( is_array( $item ) && isset( $item['content'] ) )
{
$texts[] = $item['content'];
if( isset( $item['items'] ) && is_array( $item['items'] ) )
{
$texts[] = $this->extractListItems( $item['items'] );
}
}
}
return implode( "\n", $texts );
}🤖 Prompt for AI Agents
In src/Cms/Models/Post.php around lines 638 to 640, the list handling uses
implode() on $block['data']['items'] which will convert nested item arrays to
the string "Array"; replace this with a recursive extractor that walks items and
nested 'items' arrays, concatenates their 'content' or string values with "\n",
and return that string, then call that helper instead of implode(). Ensure the
helper accepts an array, checks for string items, array items with 'content' and
optional 'items', recurses for nested lists, and returns the joined string.
| public function incrementViewCount( int $id ): bool | ||
| { | ||
| $stmt = $this->_pdo->prepare( "UPDATE pages SET view_count = view_count + 1 WHERE id = ?" ); | ||
| $stmt->execute( [ $id ] ); | ||
|
|
||
| return $stmt->rowCount() > 0; | ||
| } | ||
|
|
||
| /** | ||
| * Map database row to Page object | ||
| * | ||
| * @param array $row Database row | ||
| * @return Page | ||
| */ | ||
| private function mapRowToPage( array $row ): Page | ||
| { | ||
| $data = [ | ||
| 'id' => (int)$row['id'], | ||
| 'title' => $row['title'], | ||
| 'slug' => $row['slug'], | ||
| 'content' => $row['content'], | ||
| 'template' => $row['template'], | ||
| 'meta_title' => $row['meta_title'], | ||
| 'meta_description' => $row['meta_description'], | ||
| 'meta_keywords' => $row['meta_keywords'], | ||
| 'author_id' => (int)$row['author_id'], | ||
| 'status' => $row['status'], | ||
| 'view_count' => (int)$row['view_count'], | ||
| 'published_at' => $row['published_at'] ?? null, | ||
| 'created_at' => $row['created_at'], | ||
| 'updated_at' => $row['updated_at'] ?? null, | ||
| ]; | ||
|
|
||
| $page = Page::fromArray( $data ); | ||
|
|
||
| // Load relationships | ||
| $page->setAuthor( $this->loadAuthor( $page->getAuthorId() ) ); | ||
|
|
||
| return $page; | ||
| } | ||
|
|
||
| /** | ||
| * Load author for a page | ||
| * | ||
| * @param int $authorId | ||
| * @return User|null | ||
| */ | ||
| private function loadAuthor( int $authorId ): ?User | ||
| { | ||
| try | ||
| { | ||
| $stmt = $this->_pdo->prepare( "SELECT * FROM users WHERE id = ? LIMIT 1" ); | ||
| $stmt->execute( [ $authorId ] ); | ||
| $row = $stmt->fetch(); | ||
|
|
||
| if( !$row ) | ||
| { | ||
| return null; | ||
| } | ||
| $page = Page::find( $id ); | ||
|
|
||
| return User::fromArray( $row ); | ||
| } | ||
| catch( \PDOException $e ) | ||
| if( !$page ) | ||
| { | ||
| // Users table may not exist in test environments | ||
| return null; | ||
| return false; | ||
| } | ||
|
|
||
| $page->incrementViewCount(); | ||
| return $page->save(); | ||
| } |
There was a problem hiding this comment.
Race condition in incrementViewCount.
The fetch-increment-save pattern is not atomic. Under concurrent requests, view counts can be lost. Consider using a direct SQL UPDATE posts SET view_count = view_count + 1 WHERE id = ? for atomicity.
public function incrementViewCount( int $id ): bool
{
- $page = Page::find( $id );
-
- if( !$page )
- {
- return false;
- }
-
- $page->incrementViewCount();
- return $page->save();
+ // Atomic increment to avoid race conditions
+ $affected = Page::query()
+ ->where( 'id', $id )
+ ->increment( 'view_count' );
+ return $affected > 0;
}If the ORM doesn't support increment(), use raw SQL as a fallback.
📝 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.
| public function incrementViewCount( int $id ): bool | |
| { | |
| $stmt = $this->_pdo->prepare( "UPDATE pages SET view_count = view_count + 1 WHERE id = ?" ); | |
| $stmt->execute( [ $id ] ); | |
| return $stmt->rowCount() > 0; | |
| } | |
| /** | |
| * Map database row to Page object | |
| * | |
| * @param array $row Database row | |
| * @return Page | |
| */ | |
| private function mapRowToPage( array $row ): Page | |
| { | |
| $data = [ | |
| 'id' => (int)$row['id'], | |
| 'title' => $row['title'], | |
| 'slug' => $row['slug'], | |
| 'content' => $row['content'], | |
| 'template' => $row['template'], | |
| 'meta_title' => $row['meta_title'], | |
| 'meta_description' => $row['meta_description'], | |
| 'meta_keywords' => $row['meta_keywords'], | |
| 'author_id' => (int)$row['author_id'], | |
| 'status' => $row['status'], | |
| 'view_count' => (int)$row['view_count'], | |
| 'published_at' => $row['published_at'] ?? null, | |
| 'created_at' => $row['created_at'], | |
| 'updated_at' => $row['updated_at'] ?? null, | |
| ]; | |
| $page = Page::fromArray( $data ); | |
| // Load relationships | |
| $page->setAuthor( $this->loadAuthor( $page->getAuthorId() ) ); | |
| return $page; | |
| } | |
| /** | |
| * Load author for a page | |
| * | |
| * @param int $authorId | |
| * @return User|null | |
| */ | |
| private function loadAuthor( int $authorId ): ?User | |
| { | |
| try | |
| { | |
| $stmt = $this->_pdo->prepare( "SELECT * FROM users WHERE id = ? LIMIT 1" ); | |
| $stmt->execute( [ $authorId ] ); | |
| $row = $stmt->fetch(); | |
| if( !$row ) | |
| { | |
| return null; | |
| } | |
| $page = Page::find( $id ); | |
| return User::fromArray( $row ); | |
| } | |
| catch( \PDOException $e ) | |
| if( !$page ) | |
| { | |
| // Users table may not exist in test environments | |
| return null; | |
| return false; | |
| } | |
| $page->incrementViewCount(); | |
| return $page->save(); | |
| } | |
| public function incrementViewCount( int $id ): bool | |
| { | |
| // Atomic increment to avoid race conditions | |
| $affected = Page::query() | |
| ->where( 'id', $id ) | |
| ->increment( 'view_count' ); | |
| return $affected > 0; | |
| } |
🤖 Prompt for AI Agents
In src/Cms/Repositories/DatabasePageRepository.php around lines 169 to 180, the
current fetch-increment-save pattern causes a race condition and lost view
counts under concurrency; replace it with an atomic update by using the
model/query builder's increment method (e.g. Page::where('id',
$id)->increment('view_count')) or, if unavailable, execute a raw SQL update
("UPDATE pages SET view_count = view_count + 1 WHERE id = ?") and then check the
affected rows to return true/false; ensure you handle the case where no row
matches the id and return false in that case.
| public function incrementViewCount( int $id ): bool | ||
| { | ||
| $stmt = $this->_pdo->prepare( "UPDATE posts SET view_count = view_count + 1 WHERE id = ?" ); | ||
| $stmt->execute( [ $id ] ); | ||
| $post = Post::find( $id ); | ||
|
|
||
| return $stmt->rowCount() > 0; | ||
| if( !$post ) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| $post->incrementViewCount(); | ||
| return $post->save(); | ||
| } |
There was a problem hiding this comment.
Race condition in incrementViewCount.
Same issue as in DatabasePageRepository: the fetch-increment-save pattern loses increments under concurrency. Use an atomic SQL update.
public function incrementViewCount( int $id ): bool
{
- $post = Post::find( $id );
-
- if( !$post )
- {
- return false;
- }
-
- $post->incrementViewCount();
- return $post->save();
+ $stmt = $this->_pdo->prepare( "UPDATE posts SET view_count = view_count + 1 WHERE id = ?" );
+ $stmt->execute( [ $id ] );
+ return $stmt->rowCount() > 0;
}📝 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.
| public function incrementViewCount( int $id ): bool | |
| { | |
| $stmt = $this->_pdo->prepare( "UPDATE posts SET view_count = view_count + 1 WHERE id = ?" ); | |
| $stmt->execute( [ $id ] ); | |
| $post = Post::find( $id ); | |
| return $stmt->rowCount() > 0; | |
| if( !$post ) | |
| { | |
| return false; | |
| } | |
| $post->incrementViewCount(); | |
| return $post->save(); | |
| } | |
| public function incrementViewCount( int $id ): bool | |
| { | |
| $stmt = $this->_pdo->prepare( "UPDATE posts SET view_count = view_count + 1 WHERE id = ?" ); | |
| $stmt->execute( [ $id ] ); | |
| return $stmt->rowCount() > 0; | |
| } |
🤖 Prompt for AI Agents
In src/Cms/Repositories/DatabasePostRepository.php around lines 353 to 364, the
current fetch-increment-save flow causes lost increments under concurrency;
replace it with an atomic SQL increment. Use the query builder/Eloquent's
incremental update (e.g., Post::where('id', $id)->increment('view_count')) so
the database performs the increment atomically, then return true if the affected
row count is > 0 (false otherwise); remove the find-and-save sequence to avoid
the race condition.
| private function syncCategories( int $postId, array $categoryIds ): void | ||
| { | ||
| // Delete existing categories | ||
| $this->_pdo->prepare( "DELETE FROM post_categories WHERE post_id = ?" ) | ||
| ->execute( [ $postId ] ); | ||
|
|
||
| // Insert new categories | ||
| if( !empty( $categoryIds ) ) | ||
| { | ||
| $stmt = $this->_pdo->prepare( "INSERT INTO post_categories (post_id, category_id, created_at) VALUES (?, ?, ?)" ); | ||
| $now = ( new \DateTimeImmutable() )->format( 'Y-m-d H:i:s' ); | ||
| foreach( $categoryIds as $categoryId ) | ||
| { | ||
| $stmt->execute( [ $postId, $categoryId, $now ] ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Sync tags for a post (removes old, adds new) | ||
| */ | ||
| private function syncTags( int $postId, array $tagIds ): void | ||
| { | ||
| // Delete existing tags | ||
| $this->_pdo->prepare( "DELETE FROM post_tags WHERE post_id = ?" ) | ||
| ->execute( [ $postId ] ); | ||
|
|
||
| // Insert new tags | ||
| if( !empty( $tagIds ) ) | ||
| { | ||
| $stmt = $this->_pdo->prepare( "INSERT INTO post_tags (post_id, tag_id, created_at) VALUES (?, ?, ?)" ); | ||
| $now = ( new \DateTimeImmutable() )->format( 'Y-m-d H:i:s' ); | ||
| foreach( $tagIds as $tagId ) | ||
| { | ||
| $stmt->execute( [ $postId, $tagId, $now ] ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Sync operations are not atomic.
syncCategories and syncTags perform DELETE followed by INSERTs without a transaction. If the process fails mid-way, you'll have a post with no categories/tags. Wrap in a transaction for consistency.
private function syncCategories( int $postId, array $categoryIds ): void
{
+ $this->_pdo->beginTransaction();
+ try
+ {
// Delete existing categories
$this->_pdo->prepare( "DELETE FROM post_categories WHERE post_id = ?" )
->execute( [ $postId ] );
// Insert new categories
if( !empty( $categoryIds ) )
{
$stmt = $this->_pdo->prepare( "INSERT INTO post_categories (post_id, category_id, created_at) VALUES (?, ?, ?)" );
$now = ( new \DateTimeImmutable() )->format( 'Y-m-d H:i:s' );
foreach( $categoryIds as $categoryId )
{
$stmt->execute( [ $postId, $categoryId, $now ] );
}
}
+ $this->_pdo->commit();
+ }
+ catch( \Exception $e )
+ {
+ $this->_pdo->rollBack();
+ throw $e;
+ }
}Apply the same pattern to syncTags.
| public function uploadFromUrl( string $url, array $options = [] ): array | ||
| { | ||
| // Validate URL | ||
| if( !filter_var( $url, FILTER_VALIDATE_URL ) ) | ||
| { | ||
| throw new \Exception( "Invalid URL: {$url}" ); | ||
| } | ||
|
|
||
| // Merge with default options from config | ||
| $uploadOptions = $this->buildUploadOptions( $options ); | ||
|
|
||
| try | ||
| { | ||
| $uploadApi = $this->_cloudinary->uploadApi(); | ||
| $result = $uploadApi->upload( $url, $uploadOptions ); | ||
|
|
||
| return $this->formatResult( $result ); | ||
| } | ||
| catch( \Exception $e ) | ||
| { | ||
| throw new \Exception( "Cloudinary upload from URL failed: " . $e->getMessage(), 0, $e ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential SSRF vulnerability in uploadFromUrl.
The FILTER_VALIDATE_URL check doesn't prevent SSRF attacks. An attacker could provide URLs to internal services (e.g., http://localhost:8080/admin, http://169.254.169.254/ for cloud metadata). Consider restricting to HTTPS only and/or validating against an allowlist of domains.
public function uploadFromUrl( string $url, array $options = [] ): array
{
// Validate URL
if( !filter_var( $url, FILTER_VALIDATE_URL ) )
{
throw new \Exception( "Invalid URL: {$url}" );
}
+ // Security: Only allow HTTPS URLs from external hosts
+ $parsedUrl = parse_url( $url );
+ $host = strtolower( $parsedUrl['host'] ?? '' );
+
+ if( ( $parsedUrl['scheme'] ?? '' ) !== 'https' )
+ {
+ throw new \Exception( "Only HTTPS URLs are allowed" );
+ }
+
+ // Block internal/private IP ranges
+ $ip = gethostbyname( $host );
+ if( filter_var( $ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE ) === false )
+ {
+ throw new \Exception( "URL points to a private or reserved IP range" );
+ }🤖 Prompt for AI Agents
In src/Cms/Services/Media/CloudinaryUploader.php around lines 98-120, the
uploadFromUrl method only uses FILTER_VALIDATE_URL which allows SSRF to internal
addresses; update validation to only accept HTTPS URLs and enforce a safe
destination by resolving the URL host to an IP and rejecting addresses in
private, loopback, link-local or cloud metadata ranges (e.g., 10.0.0.0/8,
172.16.0.0/12, 192.168.0.0/16, 127.0.0.0/8, 169.254.0.0/16, ::1, fc00::/7,
fe80::/10), or alternatively compare the host against a configured allowlist; if
the URL fails these checks throw an exception; keep existing option merging and
upload logic unchanged after validation.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
26-33: Reconsiderfail_ci_if_error: truefor codecov upload.The codecov-action has been updated to v5 (addressing the prior review concern), but the
fail_ci_if_errorsetting differs from the previously suggested value. Setting this totruemeans your CI will fail if the codecov service experiences outages, network issues, or slowdowns—external factors outside your control.Industry best practice is to set this to
falseto prevent codecov availability issues from blocking merges. Consider reverting unless you have a specific requirement for codecov success to be mandatory.Apply this diff to align with best practices:
- name: Upload coverage to Codecov uses: codecov/codecov-action@v5 with: token: ${{ secrets.CODECOV_TOKEN }} files: ./coverage.xml flags: cms slug: Neuron-PHP/cms - fail_ci_if_error: true + fail_ci_if_error: false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)readme.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- readme.md
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
13-24: Modern CI setup with solid coverage configuration.The workflow updates to use
shivammathur/setup-php@v2with PHP 8.4 is a clean improvement over container-based execution. The Composer install flags (--optimize-autoloader, --no-interaction) and PHPUnit coverage command (with --coverage-clover and --coverage-filter src) are both well-configured and follow CI best practices.
Upgrades the blog editor to be a block based editor.
A ton of refactoring.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Chores
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.