Feature/calendar #23
Feature/calendar #23
Conversation
WalkthroughAdds calendar/event domain (models, migrations, repos, services, controllers, admin views, routes, widget/shortcode), Cloudinary testing docs and example env, multi‑DB CI and connection initialization, a ManagesTimestamps trait and repository adjustments, numerous migration tweaks, and expanded tests with improved cleanup and truncation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin
participant Controller as Admin Events Controller
participant Service as Event Creator Service
participant Repo as DatabaseEventRepository
participant DB as Database
Admin->>Controller: POST /admin/events (form + CSRF)
Controller->>Service: create(title,startDate,createdBy,...)
Service->>Repo: slugExists(slug)
Repo->>DB: SELECT id FROM events WHERE slug = ?
DB-->>Repo: result
alt slug exists
Repo-->>Service: true
Service-->>Controller: throw RuntimeException
Controller-->>Admin: redirect back with error flash
else slug unique
Service->>Repo: create(Event)
Repo->>DB: INSERT INTO events (...)
DB-->>Repo: lastInsertId
Repo->>DB: SELECT * FROM events WHERE id = ?
DB-->>Repo: row
Repo-->>Service: Event (hydrated)
Service-->>Controller: Event
Controller-->>Admin: redirect to /admin/events with success flash
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
versionlog.md-3-3 (1)
3-3: Fix typo in version log.Line 3 contains a typo: "compatability" should be "compatibility".
🔎 Proposed fix
-* Database compatability improvements. +* Database compatibility improvements.tests/Unit/Cms/Cli/Commands/User/CreateCommandTest.php-48-75 (1)
48-75: Add Registry cleanup to prevent test pollution.The test resets the Registry at line 51 but doesn't restore it after the test completes. If this test fails or if other tests run after it, they may encounter an empty Registry, leading to cascading test failures.
🔎 Suggested fix
Add cleanup after the assertion or in a
finallyblock:// Call the method and verify it returns null $result = $method->invoke($command); $this->assertNull($result); + + // Clean up Registry (restore state for other tests) + \Neuron\Patterns\Registry::getInstance()->reset(); }Alternatively, consider adding a
tearDown()method if multiple tests need Registry cleanup.resources/database/migrate/20251221000000_create_event_categories_table.php-21-22 (1)
21-22: Theupdated_attimestamp won't auto-update on row modifications.While
'CURRENT_TIMESTAMP'defaults work across SQLite, MySQL, and PostgreSQL at table creation, this migration has a critical issue: theupdated_atcolumn (line 22) only sets a default value and will NOT automatically update when rows are modified. For MySQL best practice, use'update' => 'CURRENT_TIMESTAMP'instead:->addColumn( 'updated_at', 'timestamp', [ 'default' => 'CURRENT_TIMESTAMP', 'update' => 'CURRENT_TIMESTAMP' ] )PostgreSQL and SQLite won't auto-update
updated_ateither with the current approach—the application must handle this explicitly.tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php-255-262 (1)
255-262: External URL dependency may cause flaky tests.
https://via.placeholder.com/150is an external service that could be unavailable or change, causing intermittent test failures.Consider using a more stable test URL or hosting a test image in your own infrastructure/CDN for integration tests.
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php-278-284 (1)
278-284: Test assumes a specificpublic_idexists in Cloudinary.The delete test uses a hardcoded
test-folder/test-imagepublic ID that may not exist, making the test result unpredictable. The delete could returnfalse(not found) ortrue(already deleted).For a reliable integration test, upload an image first, then delete it:
🔎 Proposed fix
public function testDeleteWithValidPublicId(): void { // ... skip logic ... - // Note: This test assumes a test image exists with this public_id - // In a real integration test, you would upload first, then delete $uploader = new CloudinaryUploader( $this->_settings ); - $result = $uploader->delete( 'test-folder/test-image' ); + + // Upload a test image first + $uploadResult = $uploader->uploadFromUrl( 'https://via.placeholder.com/50' ); + $this->assertNotEmpty( $uploadResult['public_id'] ); + + // Now delete it + $result = $uploader->delete( $uploadResult['public_id'] ); - $this->assertIsBool( $result ); + $this->assertTrue( $result ); }resources/views/admin/event_categories/index.php-57-60 (1)
57-60: Potential CSS injection via user-controlled color value.The
background-colorstyle uses the category color directly. Whilehtmlspecialchars()escapes HTML entities, it doesn't prevent CSS injection attacks likered; background-image: url(javascript:...)in older browsers or CSS-based data exfiltration.Consider validating the color format (e.g., hex pattern) before output:
🔎 Proposed fix
<td> - <span class="badge" style="background-color: <?= htmlspecialchars( $category->getColor() ) ?>"> + <?php + $color = $category->getColor(); + // Validate hex color format + $safeColor = preg_match( '/^#[0-9A-Fa-f]{3,6}$/', $color ) ? $color : '#808080'; + ?> + <span class="badge" style="background-color: <?= $safeColor ?>"> <?= htmlspecialchars( $category->getColor() ) ?> </span> </td>tests/Integration/IntegrationTestCase.php-101-107 (1)
101-107: SQL injection risk via string interpolation.The
$dbnamevariable is interpolated directly into the SQL query without parameterization. Although it originates fromgetDatabaseName()which reads fromSELECT DATABASE(), this pattern is fragile if the method is ever changed.🔎 Proposed fix using parameterized query
elseif( $driver === 'mysql' ) { $dbname = $this->getDatabaseName(); - $tables = $this->pdo->query( - "SELECT table_name FROM information_schema.tables WHERE table_schema = '{$dbname}' AND table_name != 'phinxlog'" - )->fetchAll( PDO::FETCH_COLUMN ); + $stmt = $this->pdo->prepare( + "SELECT table_name FROM information_schema.tables WHERE table_schema = ? AND table_name != 'phinxlog'" + ); + $stmt->execute( [ $dbname ] ); + $tables = $stmt->fetchAll( PDO::FETCH_COLUMN ); }src/Cms/Controllers/Calendar.php-122-123 (1)
122-123: Comment says "upcoming events" but fetches all events for category.The comment at line 122 states "Get upcoming events in this category," but
getByCategory()returns all published events regardless of date. Consider either updating the comment or filtering to only upcoming events.Option: Filter to upcoming events
-// Get upcoming events in this category -$events = $this->_eventRepository->getByCategory( $category->getId(), 'published' ); +// Get events in this category +$events = $this->_eventRepository->getByCategory( $category->getId(), 'published' );Or if you want only upcoming events, you may need a different repository method that combines category and date filtering.
src/Cms/Controllers/Calendar.php-44-52 (1)
44-52: Missing input validation for month/year parameters.User-supplied
monthandyearvalues are cast to integers without validation. Invalid values (e.g.,month=13,month=0,year=-1) could cause unexpected behavior or exceptions when constructingDateTimeImmutable.Proposed fix
$monthParam = $request->get( 'month', date( 'n' ) ); $yearParam = $request->get( 'year', date( 'Y' ) ); $month = (int)$monthParam; $year = (int)$yearParam; +// Validate month and year +if( $month < 1 || $month > 12 ) +{ + $month = (int)date( 'n' ); +} +if( $year < 1970 || $year > 2100 ) +{ + $year = (int)date( 'Y' ); +} + // Calculate start and end dates for the month $startDate = new DateTimeImmutable( "$year-$month-01 00:00:00" );src/Cms/Models/EventCategory.php-18-24 (1)
18-24: Uninitialized properties$_nameand$_slugmay cause issues.The properties
$_nameand$_slugare declared without default values. IfgetName()orgetSlug()is called before they are set, PHP 8 will throw an "Typed property must not be accessed before initialization" error. Consider initializing them to empty strings.🔎 Proposed fix
private ?int $_id = null; -private string $_name; -private string $_slug; +private string $_name = ''; +private string $_slug = ''; private string $_color = '#3b82f6';src/Cms/Models/EventCategory.php-26-28 (1)
26-28: Replace BelongsToMany with HasMany for the EventCategory→Event relationship.Event has a BelongsTo relationship to EventCategory using the
category_idforeign key in the events table. The inverse relationship should be HasMany, not BelongsToMany. BelongsToMany requires a separate pivot table with foreign keys from both related models, but the events table is the primary Event table with a direct foreign key to categories.-use Neuron\Orm\Attributes\{Table, BelongsToMany}; +use Neuron\Orm\Attributes\{Table, HasMany}; // Relationships -#[BelongsToMany(Event::class, pivotTable: 'events', foreignKey: 'category_id')] +#[HasMany(Event::class, foreignKey: 'category_id')] private array $_events = [];
🧹 Nitpick comments (24)
resources/views/admin/event_categories/create.php (1)
15-15: Consider using route helpers instead of hard-coded URLs.The URLs
/admin/event-categoriesare hard-coded in multiple places. If the routing structure changes, these URLs would need to be updated manually.If the framework provides route helper functions, consider using them for better maintainability:
href="<?= route('admin_event_categories_index') ?>"Also applies to: 25-25, 54-54
tests/Unit/Cms/Auth/HelpersTest.php (1)
68-83: Test case is largely redundant withtestCurrentUserIdentifierFallsBackToOsUser.This test performs the same assertions as lines 49-66. Consider consolidating or removing one of them, or differentiating the test scenarios more clearly.
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (1)
38-75: Consider using an established library likevlucas/phpdotenvinstead of custom implementation.The custom
loadEnvFile()duplicates functionality that well-tested libraries handle better, including edge cases like multiline values, variable interpolation, and export prefixes.🔎 Alternative using vlucas/phpdotenv
protected function setUp(): void { parent::setUp(); $envFile = __DIR__ . '/../../../../../.env.testing'; if( file_exists( $envFile ) ) { $dotenv = \Dotenv\Dotenv::createImmutable( dirname( $envFile ), '.env.testing' ); $dotenv->safeLoad(); } // ... rest of setup }If adding a dependency isn't desirable, the current implementation is functional for simple KEY=VALUE parsing.
tests/Integration/IntegrationTestCase.php (2)
130-140: Table names should be quoted to handle reserved words.Table names aren't quoted, which could cause issues if a table name matches a SQL reserved word or contains special characters.
🔎 Proposed fix with proper identifier quoting
// Truncate all tables foreach( $tables as $table ) { if( $driver === 'sqlite' ) { - $this->pdo->exec( "DELETE FROM {$table}" ); + $this->pdo->exec( "DELETE FROM \"{$table}\"" ); + } + elseif( $driver === 'pgsql' ) + { + $this->pdo->exec( "TRUNCATE TABLE \"{$table}\" CASCADE" ); } - else + elseif( $driver === 'mysql' ) { - $this->pdo->exec( "TRUNCATE TABLE {$table}" ); + $this->pdo->exec( "TRUNCATE TABLE `{$table}`" ); } }
10-10: Unused import:NullOutput.The
NullOutputimport is no longer used sinceConsoleOutputreplaced it inrunMigrations().🔎 Proposed fix
-use Symfony\Component\Console\Output\NullOutput;src/Cms/Cli/Commands/Install/InstallCommand.php (2)
533-597: Consider security implications of storing API secret in plain text config file.The Cloudinary API secret is stored in
config/neuron.yamlas plain text. While this is a common pattern for development, it may be a security concern in production environments. Consider documenting that users should use environment variables for production deployments.You might want to add a note after configuration:
+ $this->output->writeln( "" ); + $this->output->info( "Note: For production, consider using environment variables for sensitive credentials." ); + return [ 'cloudinary' => [
583-583: Validate max file size is a valid positive integer.The max file size input is cast to int, but no validation ensures it's a positive number. A negative or zero value could cause issues.
🔎 Proposed fix
- $maxFileSize = $this->input->ask( "Max file size in bytes", "5242880" ); // 5MB default + $maxFileSizeInput = $this->input->ask( "Max file size in bytes", "5242880" ); // 5MB default + $maxFileSize = max( 1, (int)$maxFileSizeInput ); // Ensure positive valueThen use
$maxFileSizedirectly without casting again at line 593.src/Cms/Repositories/IEventCategoryRepository.php (1)
37-43: Consider adding type hint for$idsparameter.The
findByIdsmethod accepts anarraybut the PHPDoc doesn't specify the array element type. For consistency and IDE support, consider addingint[]in the PHPDoc.🔎 Proposed improvement
/** * Find categories by IDs * - * @param array $ids + * @param int[] $ids * @return EventCategory[] */ public function findByIds( array $ids ): array;src/Cms/Repositories/DatabasePostRepository.php (1)
369-406: Code duplication between sync methods.
syncCategories/syncTagsandsyncCategoriesWithPdo/syncTagsWithPdocontain nearly identical logic, differing only in the PDO instance used. Consider refactoring to reduce duplication.🔎 Proposed refactor: delegate to PDO variants
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 ] ); - } - } + $this->syncCategoriesWithPdo( $this->_pdo, $postId, $categoryIds ); } 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 ] ); - } - } + $this->syncTagsWithPdo( $this->_pdo, $postId, $tagIds ); }Also applies to: 492-533
src/Cms/Services/Widget/WidgetRenderer.php (1)
108-117: Unused injected repository: CalendarWidget creates its own repositories.The
$_eventRepositoryproperty is checked at line 110, butCalendarWidgetinstantiated at line 115 creates its own repository instances viaRegistryin its constructor. This makes the injected repository redundant and the null check misleading—it doesn't actually gate repository availability for the widget.Consider either:
- Pass the repository to
CalendarWidgetvia constructor injection, or- Remove the repository check here since
CalendarWidgethandles its own dependencies.Option 1: Inject the repository into CalendarWidget
private function renderCalendar( array $config ): string { if( !$this->_eventRepository ) { return "<!-- Calendar widget requires EventRepository -->"; } - $widget = new CalendarWidget(); + $widget = new CalendarWidget( $this->_eventRepository ); return $widget->render( $config ); }This would require updating
CalendarWidgetto accept the repository via constructor.src/Cms/Services/Widget/CalendarWidget.php (2)
17-26: Tight coupling to concrete repository implementations.The constructor directly instantiates
DatabaseEventRepositoryandDatabaseEventCategoryRepository, making unit testing difficult without a configuredRegistry. Consider accepting repository interfaces via constructor injection for better testability and consistency with other widgets.Proposed refactor
-class CalendarWidget implements IWidget -{ - private DatabaseEventRepository $_eventRepository; - private DatabaseEventCategoryRepository $_categoryRepository; - - public function __construct() - { - $settings = Registry::getInstance()->get( 'Settings' ); - $this->_eventRepository = new DatabaseEventRepository( $settings ); - $this->_categoryRepository = new DatabaseEventCategoryRepository( $settings ); - } +class CalendarWidget implements IWidget +{ + private IEventRepository $_eventRepository; + private IEventCategoryRepository $_categoryRepository; + + public function __construct( + ?IEventRepository $eventRepository = null, + ?IEventCategoryRepository $categoryRepository = null + ) + { + if( $eventRepository && $categoryRepository ) + { + $this->_eventRepository = $eventRepository; + $this->_categoryRepository = $categoryRepository; + } + else + { + $settings = Registry::getInstance()->get( 'Settings' ); + $this->_eventRepository = new DatabaseEventRepository( $settings ); + $this->_categoryRepository = new DatabaseEventCategoryRepository( $settings ); + } + }
104-104: Remove unused$attrsparameter.The
$attrsparameter is not used inrenderTemplate(). Per static analysis hint, consider removing it or using it for future extensibility (e.g., custom CSS classes).Proposed fix
-private function renderTemplate( array $events, array $attrs ): string +private function renderTemplate( array $events ): stringAnd update the call site at line 74:
-return $this->renderTemplate( $events, $attrs ); +return $this->renderTemplate( $events );src/Cms/Services/Event/Updater.php (1)
50-66: Consider using a DTO or value object for the long parameter list.The
update()method has 15 parameters, which can make call sites error-prone and harder to maintain. Consider grouping these into a data transfer object (e.g.,EventUpdateData) for better readability and type safety.src/Cms/Repositories/DatabaseEventCategoryRepository.php (1)
100-123: Consider returning the freshly loaded entity after create.The
create()method returns the same$categoryobject with the ID set, but other repositories in the codebase (e.g.,DatabaseUserRepository,DatabaseCategoryRepository) fetch the created entity from the database to ensure all DB-set values are reflected. This is optional but would ensure consistency.Proposed change
$category->setId( (int)$this->_pdo->lastInsertId() ); -return $category; +return $this->findById( $category->getId() );tests/Integration/DatabaseCompatibilityTest.php (2)
276-276: Remove unused variable$created.Per static analysis, the
$createdvariable is assigned but never used. Since the purpose is to test rollback behavior, the assignment itself triggers the insert; the variable can be removed.Proposed fix
-$created = User::create( $user->toArray() ); +User::create( $user->toArray() );
109-136: Consider reducing sleep duration or using a different approach.Multiple tests use
sleep(1)to ensure timestamp differences, which accumulates to slow test execution. Consider using microsecond-precision timestamps if supported, or mocking the clock for faster tests.src/Cms/Services/EventCategory/Creator.php (1)
60-74: Slug generation fallback may cause collisions under high concurrency.The
uniqid()function is not guaranteed to be unique under high concurrency. While unlikely for category slugs, consider using a more robust approach if uniqueness is critical.More robust fallback
// Fallback for names with no ASCII characters if( $slug === '' ) { - $slug = 'category-' . uniqid(); + $slug = 'category-' . bin2hex( random_bytes( 8 ) ); }src/Cms/Controllers/Admin/EventCategories.php (1)
23-26: Consider using interfaces for repository type hints.The class uses
DatabaseEventCategoryRepositoryconcrete type. UsingIEventCategoryRepositorywould improve testability and allow swapping implementations.🔎 Proposed change
- private DatabaseEventCategoryRepository $_repository; + private IEventCategoryRepository $_repository;src/Cms/Controllers/Admin/Events.php (2)
25-29: Consider using interfaces for repository type hints.Same recommendation as EventCategories - using
IEventRepositoryandIEventCategoryRepositorywould improve testability.
196-200: Inconsistent permission denial handling across methods.
edit()andupdate()throwRuntimeExceptionfor unauthorized access, whiledestroy()redirects with an error message. Consider using a consistent approach - redirecting with an error message is more user-friendly.🔎 Proposed fix for edit() and update()
// Check permissions if( !$user->isAdmin() && !$user->isEditor() && $event->getCreatedBy() !== $user->getId() ) { - throw new \RuntimeException( 'Unauthorized to edit this event' ); + $this->redirect( 'admin_events', [], ['error', 'Unauthorized to edit this event'] ); }Also applies to: 241-245, 310-314
src/Cms/Repositories/DatabaseEventRepository.php (1)
350-376: N+1 query performance concern inloadRelations().
loadRelations()issues up to 2 queries per event (category + creator). When loading multiple events viaall(),getByCategory(), etc., this results in N+1 queries. For small datasets this is acceptable, but consider batch loading or lazy loading for larger scale.For future optimization, consider:
- JOIN queries to fetch related data in a single query
- Batch loading categories/creators after fetching all events
- Lazy loading relationships only when accessed
src/Cms/Models/Event.php (3)
51-54: Consider conditional initialization of created_at.The constructor always initializes
_createdAt, even when hydrating existing entities from the database. WhilefromArrayproperly overrides this, it creates an unnecessary object that's immediately discarded.🔎 Proposed refactor to avoid unnecessary object creation
public function __construct() { - $this->_createdAt = new DateTimeImmutable(); + // Created timestamp will be set by fromArray for existing entities + // or should be explicitly set for new entities }Then ensure creation services explicitly set the timestamp for new events.
127-130: Add error handling for JSON decode.The method silently falls back to default content if JSON is invalid, which could mask data corruption issues. Consider logging malformed JSON or throwing an exception.
🔎 Proposed improvement with error handling
public function getContent(): array { - return json_decode( $this->_contentRaw, true ) ?? ['blocks' => []]; + $decoded = json_decode( $this->_contentRaw, true ); + + if( json_last_error() !== JSON_ERROR_NONE ) + { + // Log the error or throw exception + error_log( "Failed to decode event content: " . json_last_error_msg() ); + return ['blocks' => []]; + } + + return $decoded ?? ['blocks' => []]; }
523-539: Consider using setter instead of direct property access.The code directly assigns to
$event->_contentRawinstead of usingsetContent()orsetContentArray(). While this works currently, it bypasses encapsulation and could cause issues if setter logic changes.🔎 Proposed refactor to use setters
// Handle content_raw if( isset( $data['content_raw'] ) ) { if( is_string( $data['content_raw'] ) ) { - $event->_contentRaw = $data['content_raw']; + $event->setContent( $data['content_raw'] ); } elseif( is_array( $data['content_raw'] ) ) { - $encoded = json_encode( $data['content_raw'] ); - if( $encoded === false ) - { - throw new \JsonException( "Failed to encode content_raw array to JSON" ); - } - $event->_contentRaw = $encoded; + $event->setContentArray( $data['content_raw'] ); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
.env.testing.example(1 hunks).github/workflows/ci.yml(2 hunks).gitignore(1 hunks)CLOUDINARY_SETUP.md(1 hunks)MIGRATIONS.md(2 hunks)TESTING.md(1 hunks)readme.md(1 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/20250113000000_create_pages_table.php(1 hunks)resources/database/migrate/20250114000000_create_categories_table.php(1 hunks)resources/database/migrate/20250115000000_create_tags_table.php(1 hunks)resources/database/migrate/20250116000000_create_posts_table.php(1 hunks)resources/database/migrate/20251221000000_create_event_categories_table.php(1 hunks)resources/database/migrate/20251221000001_create_events_table.php(1 hunks)resources/views/admin/event_categories/create.php(1 hunks)resources/views/admin/event_categories/edit.php(1 hunks)resources/views/admin/event_categories/index.php(1 hunks)src/Cms/Auth/helpers.php(7 hunks)src/Cms/Cli/Commands/Install/InstallCommand.php(4 hunks)src/Cms/Cli/Commands/Maintenance/EnableCommand.php(1 hunks)src/Cms/Controllers/Admin/EventCategories.php(1 hunks)src/Cms/Controllers/Admin/Events.php(1 hunks)src/Cms/Controllers/Blog.php(4 hunks)src/Cms/Controllers/Calendar.php(1 hunks)src/Cms/Database/ConnectionFactory.php(2 hunks)src/Cms/Maintenance/MaintenanceManager.php(2 hunks)src/Cms/Models/Event.php(1 hunks)src/Cms/Models/EventCategory.php(1 hunks)src/Cms/Repositories/DatabaseCategoryRepository.php(2 hunks)src/Cms/Repositories/DatabaseEventCategoryRepository.php(1 hunks)src/Cms/Repositories/DatabaseEventRepository.php(1 hunks)src/Cms/Repositories/DatabasePageRepository.php(2 hunks)src/Cms/Repositories/DatabasePostRepository.php(3 hunks)src/Cms/Repositories/DatabaseTagRepository.php(2 hunks)src/Cms/Repositories/DatabaseUserRepository.php(2 hunks)src/Cms/Repositories/IEventCategoryRepository.php(1 hunks)src/Cms/Repositories/IEventRepository.php(1 hunks)src/Cms/Services/Content/ShortcodeParser.php(2 hunks)src/Cms/Services/Event/Creator.php(1 hunks)src/Cms/Services/Event/Deleter.php(1 hunks)src/Cms/Services/Event/Updater.php(1 hunks)src/Cms/Services/EventCategory/Creator.php(1 hunks)src/Cms/Services/EventCategory/Deleter.php(1 hunks)src/Cms/Services/EventCategory/Updater.php(1 hunks)src/Cms/Services/Widget/CalendarWidget.php(1 hunks)src/Cms/Services/Widget/WidgetRenderer.php(4 hunks)tests/Integration/DatabaseCompatibilityTest.php(1 hunks)tests/Integration/IntegrationTestCase.php(2 hunks)tests/Unit/Cms/Auth/HelpersTest.php(1 hunks)tests/Unit/Cms/Auth/MemberAuthenticationFilterTest.php(2 hunks)tests/Unit/Cms/BlogControllerTest.php(6 hunks)tests/Unit/Cms/BlogTest.php(0 hunks)tests/Unit/Cms/Cli/Commands/Install/InstallCommandTest.php(1 hunks)tests/Unit/Cms/Cli/Commands/User/CreateCommandTest.php(1 hunks)tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php(2 hunks)versionlog.md(1 hunks)
💤 Files with no reviewable changes (1)
- tests/Unit/Cms/BlogTest.php
🧰 Additional context used
🧬 Code graph analysis (32)
resources/views/admin/event_categories/edit.php (1)
src/Cms/Models/EventCategory.php (5)
getName(55-58)getId(38-41)getSlug(72-75)getColor(89-92)getDescription(106-109)
src/Cms/Repositories/DatabaseUserRepository.php (4)
src/Cms/Repositories/DatabaseCategoryRepository.php (2)
create(77-107)findById(37-40)src/Cms/Repositories/DatabasePageRepository.php (2)
create(50-74)findById(32-36)src/Cms/Repositories/DatabasePostRepository.php (2)
create(115-169)findById(39-54)src/Cms/Repositories/DatabaseTagRepository.php (2)
create(61-91)findById(37-40)
src/Cms/Repositories/IEventCategoryRepository.php (2)
src/Cms/Repositories/DatabaseEventCategoryRepository.php (8)
all(37-43)findById(48-60)findBySlug(65-77)findByIds(82-95)create(100-123)update(128-149)delete(154-158)slugExists(163-177)src/Cms/Repositories/IEventRepository.php (7)
all(20-20)findById(28-28)findBySlug(36-36)create(89-89)update(97-97)delete(105-105)slugExists(114-114)
resources/database/migrate/20251221000000_create_event_categories_table.php (5)
resources/database/migrate/20251221000001_create_events_table.php (1)
change(13-44)src/Cms/Controllers/Admin/EventCategories.php (1)
create(82-106)src/Cms/Repositories/DatabaseEventCategoryRepository.php (1)
create(100-123)src/Cms/Repositories/IEventCategoryRepository.php (1)
create(51-51)src/Cms/Services/EventCategory/Creator.php (1)
create(32-52)
tests/Unit/Cms/Cli/Commands/Install/InstallCommandTest.php (1)
src/Cms/Cli/Commands/Install/InstallCommand.php (1)
InstallCommand(18-1104)
src/Cms/Cli/Commands/Maintenance/EnableCommand.php (1)
src/Cms/Auth/helpers.php (1)
current_user_identifier(150-170)
tests/Unit/Cms/Auth/HelpersTest.php (1)
src/Cms/Auth/helpers.php (4)
user(30-33)current_user_identifier(150-170)csrf_field(133-137)has_role(110-114)
src/Cms/Controllers/Calendar.php (3)
src/Cms/Repositories/DatabaseEventRepository.php (6)
DatabaseEventRepository(21-377)getByDateRange(173-203)all(39-50)findBySlug(75-90)incrementViewCount(340-345)getByCategory(95-111)src/Cms/Repositories/IEventRepository.php (5)
getByDateRange(73-73)all(20-20)findBySlug(36-36)incrementViewCount(122-122)getByCategory(45-45)src/Cms/Repositories/IEventCategoryRepository.php (2)
all(19-19)findBySlug(35-35)
tests/Unit/Cms/Cli/Commands/User/CreateCommandTest.php (1)
src/Cms/Cli/Commands/User/CreateCommand.php (1)
CreateCommand(14-197)
src/Cms/Services/EventCategory/Deleter.php (3)
src/Cms/Controllers/Admin/EventCategories.php (1)
__construct(32-42)src/Cms/Repositories/DatabaseEventCategoryRepository.php (2)
__construct(29-32)delete(154-158)src/Cms/Repositories/IEventCategoryRepository.php (1)
delete(67-67)
src/Cms/Controllers/Admin/EventCategories.php (5)
src/Cms/Repositories/DatabaseEventCategoryRepository.php (7)
DatabaseEventCategoryRepository(19-178)__construct(29-32)all(37-43)create(100-123)findById(48-60)update(128-149)delete(154-158)src/Cms/Services/EventCategory/Creator.php (3)
Creator(13-75)__construct(17-20)create(32-52)src/Cms/Services/EventCategory/Updater.php (3)
Updater(13-54)__construct(17-20)update(33-53)src/Cms/Services/EventCategory/Deleter.php (3)
Deleter(13-32)__construct(17-20)delete(28-31)src/Cms/Repositories/IEventCategoryRepository.php (5)
all(19-19)create(51-51)findById(27-27)update(59-59)delete(67-67)
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (1)
src/Cms/Services/Media/CloudinaryUploader.php (4)
CloudinaryUploader(16-326)upload(67-88)uploadFromUrl(98-117)delete(126-139)
src/Cms/Maintenance/MaintenanceManager.php (1)
src/Cms/Auth/helpers.php (1)
current_user_identifier(150-170)
src/Cms/Repositories/IEventRepository.php (2)
src/Cms/Repositories/DatabaseEventRepository.php (4)
all(39-50)update(272-307)slugExists(321-335)incrementViewCount(340-345)src/Cms/Models/Event.php (1)
incrementViewCount(434-438)
tests/Unit/Cms/BlogControllerTest.php (2)
src/Cms/Repositories/DatabaseUserRepository.php (1)
DatabaseUserRepository(18-241)src/Cms/Controllers/Blog.php (1)
author(133-161)
src/Cms/Services/Content/ShortcodeParser.php (3)
src/Cms/Services/Widget/CalendarWidget.php (1)
render(42-75)src/Cms/Services/Widget/WidgetRenderer.php (1)
render(36-44)src/Cms/Services/Widget/IWidget.php (1)
render(28-28)
src/Cms/Repositories/DatabaseTagRepository.php (1)
src/Cms/Models/Tag.php (5)
getCreatedAt(87-90)setCreatedAt(95-99)getUpdatedAt(104-107)setUpdatedAt(112-116)toArray(163-172)
src/Cms/Services/Event/Deleter.php (4)
src/Cms/Services/EventCategory/Deleter.php (3)
Deleter(13-32)__construct(17-20)delete(28-31)src/Cms/Controllers/Admin/Events.php (1)
__construct(35-46)src/Cms/Repositories/DatabaseEventRepository.php (2)
__construct(31-34)delete(312-316)src/Cms/Repositories/IEventRepository.php (1)
delete(105-105)
src/Cms/Repositories/DatabaseEventCategoryRepository.php (3)
src/Cms/Database/ConnectionFactory.php (2)
ConnectionFactory(17-156)createFromSettings(26-36)src/Cms/Models/EventCategory.php (9)
fromArray(182-215)setCreatedAt(131-135)setUpdatedAt(148-152)getName(55-58)getSlug(72-75)getColor(89-92)getDescription(106-109)setId(46-50)getId(38-41)src/Cms/Repositories/IEventCategoryRepository.php (8)
all(19-19)findById(27-27)findBySlug(35-35)findByIds(43-43)create(51-51)update(59-59)delete(67-67)slugExists(76-76)
src/Cms/Repositories/DatabasePostRepository.php (3)
src/Cms/Models/Post.php (9)
getCreatedAt(330-333)setCreatedAt(338-342)getUpdatedAt(347-350)setUpdatedAt(355-359)getCategories(366-369)getTags(426-429)toArray(604-621)getId(57-60)fromArray(488-597)src/Cms/Repositories/DatabaseCategoryRepository.php (2)
create(77-107)count(162-165)src/Cms/Repositories/DatabaseTagRepository.php (2)
create(61-91)count(146-149)
src/Cms/Repositories/DatabasePageRepository.php (4)
src/Cms/Models/Page.php (6)
getCreatedAt(337-340)setCreatedAt(345-349)getUpdatedAt(354-357)setUpdatedAt(362-366)toArray(449-467)getId(59-62)src/Cms/Repositories/DatabasePostRepository.php (2)
create(115-169)findById(39-54)src/Cms/Repositories/DatabaseTagRepository.php (2)
create(61-91)findById(37-40)src/Cms/Repositories/DatabaseUserRepository.php (2)
create(69-99)findById(37-40)
resources/database/migrate/20251221000001_create_events_table.php (5)
resources/database/migrate/20251221000000_create_event_categories_table.php (1)
change(13-26)src/Cms/Repositories/DatabaseEventRepository.php (1)
create(229-267)src/Cms/Repositories/IEventCategoryRepository.php (1)
create(51-51)src/Cms/Repositories/IEventRepository.php (1)
create(89-89)src/Cms/Services/Event/Creator.php (1)
create(55-107)
src/Cms/Services/EventCategory/Creator.php (3)
src/Cms/Models/EventCategory.php (6)
__construct(30-33)setName(63-67)setSlug(80-84)setColor(97-101)setDescription(114-118)getSlug(72-75)src/Cms/Repositories/DatabaseEventCategoryRepository.php (3)
__construct(29-32)create(100-123)slugExists(163-177)src/Cms/Repositories/IEventCategoryRepository.php (2)
create(51-51)slugExists(76-76)
src/Cms/Services/EventCategory/Updater.php (2)
src/Cms/Models/EventCategory.php (5)
getId(38-41)setName(63-67)setSlug(80-84)setColor(97-101)setDescription(114-118)src/Cms/Repositories/DatabaseEventCategoryRepository.php (2)
update(128-149)slugExists(163-177)
src/Cms/Services/Event/Updater.php (2)
src/Cms/Models/Event.php (14)
setTitle(84-88)setSlug(101-105)setDescription(118-122)setContent(143-147)setLocation(180-184)setStartDate(197-201)setEndDate(214-218)setAllDay(231-235)setCategoryId(248-252)setStatus(286-290)setFeaturedImage(319-323)setOrganizer(336-340)setContactEmail(353-357)setContactPhone(370-374)src/Cms/Repositories/IEventRepository.php (2)
update(97-97)slugExists(114-114)
tests/Integration/IntegrationTestCase.php (10)
tests/Unit/Cms/Auth/MemberAuthenticationFilterTest.php (1)
tearDown(40-44)tests/Unit/Cms/BlogControllerTest.php (1)
tearDown(91-99)tests/Unit/Cms/ContentControllerTest.php (1)
tearDown(52-66)tests/Unit/Cms/Services/AuthenticationTest.php (1)
tearDown(105-110)tests/Unit/BootstrapTest.php (1)
tearDown(31-40)tests/Unit/Cms/Auth/ResendVerificationThrottleTest.php (1)
tearDown(26-29)tests/Unit/Cms/Auth/SessionManagerTest.php (1)
tearDown(24-27)tests/Unit/Cms/Maintenance/MaintenanceFilterTest.php (1)
tearDown(33-37)tests/Unit/Cms/Maintenance/MaintenanceManagerTest.php (1)
tearDown(28-31)tests/Unit/Cms/Services/CsrfTokenTest.php (1)
tearDown(36-39)
src/Cms/Services/Widget/CalendarWidget.php (4)
src/Cms/Repositories/DatabaseEventRepository.php (5)
DatabaseEventRepository(21-377)findBySlug(75-90)getByCategory(95-111)getUpcoming(116-138)getPast(143-168)src/Cms/Repositories/DatabaseEventCategoryRepository.php (2)
DatabaseEventCategoryRepository(19-178)findBySlug(65-77)src/Cms/Repositories/IEventCategoryRepository.php (1)
findBySlug(35-35)src/Cms/Repositories/IEventRepository.php (4)
findBySlug(36-36)getByCategory(45-45)getUpcoming(54-54)getPast(63-63)
src/Cms/Models/Event.php (2)
src/Cms/Models/EventCategory.php (8)
getId(38-41)setId(46-50)setSlug(80-84)setDescription(114-118)setCreatedAt(131-135)setUpdatedAt(148-152)fromArray(182-215)toArray(222-233)src/Cms/Repositories/DatabaseEventRepository.php (1)
incrementViewCount(340-345)
src/Cms/Repositories/DatabaseEventRepository.php (4)
src/Cms/Database/ConnectionFactory.php (2)
ConnectionFactory(17-156)createFromSettings(26-36)src/Cms/Models/Event.php (21)
fromArray(510-602)setCreatedAt(451-455)setUpdatedAt(468-472)getTitle(76-79)getSlug(93-96)getDescription(110-113)getContentRaw(135-138)getLocation(172-175)getStartDate(189-192)getEndDate(206-209)isAllDay(223-226)getCategoryId(240-243)getStatus(278-281)getFeaturedImage(311-314)getOrganizer(328-331)getContactEmail(345-348)getContactPhone(362-365)getCreatedBy(379-382)getViewCount(417-420)setId(67-71)incrementViewCount(434-438)src/Cms/Models/EventCategory.php (6)
fromArray(182-215)setCreatedAt(131-135)setUpdatedAt(148-152)getSlug(72-75)getDescription(106-109)setId(46-50)src/Cms/Repositories/IEventRepository.php (13)
all(20-20)findById(28-28)findBySlug(36-36)getByCategory(45-45)getUpcoming(54-54)getPast(63-63)getByDateRange(73-73)getByCreator(81-81)create(89-89)update(97-97)delete(105-105)slugExists(114-114)incrementViewCount(122-122)
tests/Integration/DatabaseCompatibilityTest.php (7)
src/Cms/Repositories/DatabaseUserRepository.php (6)
DatabaseUserRepository(18-241)create(69-99)delete(135-140)findById(37-40)update(104-130)findByUsername(45-48)src/Cms/Repositories/DatabasePostRepository.php (5)
DatabasePostRepository(20-590)create(115-169)delete(212-218)findById(39-54)update(174-207)src/Cms/Repositories/DatabaseCategoryRepository.php (5)
DatabaseCategoryRepository(18-209)create(77-107)delete(143-149)findById(37-40)update(112-138)src/Cms/Repositories/DatabaseTagRepository.php (5)
DatabaseTagRepository(18-193)create(61-91)delete(127-133)findById(37-40)update(96-122)src/Cms/Repositories/DatabasePageRepository.php (5)
DatabasePageRepository(16-192)create(50-74)delete(103-108)findById(32-36)update(79-98)src/Cms/Models/User.php (4)
setUsername(86-90)setPasswordHash(120-124)setRole(137-141)setEmailVerified(211-215)src/Cms/Models/Post.php (1)
setBody(116-120)
tests/Unit/Cms/Auth/MemberAuthenticationFilterTest.php (1)
src/Cms/Models/User.php (1)
isEmailVerified(203-206)
src/Cms/Auth/helpers.php (2)
src/Cms/Auth/SessionManager.php (1)
get(108-112)src/Cms/Models/User.php (2)
getRole(129-132)getUsername(78-81)
🪛 GitHub Actions: CI
tests/Unit/Cms/BlogControllerTest.php
[error] 328-328: Test reference to undefined variable $theme
src/Cms/Controllers/Blog.php
[error] 64-64: Undefined variable $theme
🪛 PHPMD (2.15.0)
src/Cms/Controllers/Admin/EventCategories.php
47-47: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
82-82: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
src/Cms/Controllers/Admin/Events.php
51-51: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
94-94: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
src/Cms/Services/Widget/CalendarWidget.php
104-104: Avoid unused parameters such as '$attrs'. (undefined)
(UnusedFormalParameter)
tests/Integration/DatabaseCompatibilityTest.php
276-276: Avoid unused local variables such as '$created'. (undefined)
(UnusedLocalVariable)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/Unit/Cms/BlogControllerTest.php (1)
106-111: Test isolation looks good.The symmetric tearDown ensures ViewDataProvider state doesn't leak between tests. However, the same reflection-based reset concern applies here as mentioned in the setUp method review.
🧹 Nitpick comments (2)
tests/Unit/Cms/BlogControllerTest.php (2)
146-161: Consider adding foreign key constraint for author_id.The
poststable includesauthor_idbut doesn't define a foreign key constraint to theuserstable. This allows tests to create posts with invalid author IDs (like the defaultauthorId=1increateTestPostwhen no user with ID 1 exists).While this doesn't cause test failures, adding a foreign key constraint would:
- Better match production database schema
- Catch data integrity issues earlier in tests
- Make tests more realistic
Suggested enhancement
Add this foreign key constraint to the posts table creation:
CREATE TABLE posts ( id INTEGER PRIMARY KEY AUTOINCREMENT, title VARCHAR(255) NOT NULL, slug VARCHAR(255) NOT NULL UNIQUE, body TEXT NOT NULL, content_raw TEXT DEFAULT '{\"blocks\":[]}', excerpt TEXT, featured_image VARCHAR(255), author_id INTEGER NOT NULL, status VARCHAR(20) DEFAULT 'draft', published_at TIMESTAMP, view_count INTEGER DEFAULT 0, created_at TIMESTAMP NOT NULL, - updated_at TIMESTAMP + updated_at TIMESTAMP, + FOREIGN KEY (author_id) REFERENCES users(id) ON DELETE CASCADE )Then update tests to create a default user in
setUp()or ensure each test creates its required users explicitly.
80-98: Consider suggesting a public reset method for ViewDataProvider to the neuron-php/mvc maintainers.The test currently uses reflection to reset ViewDataProvider's private
_dataproperty, which couples the test to the library's internal implementation. While this approach is necessary given that ViewDataProvider doesn't expose a public reset method, it would be more maintainable if the external library provided a public API for test cleanup (e.g.,reset()orclearData()).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Unit/Cms/BlogControllerTest.php(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Unit/Cms/BlogControllerTest.php (3)
src/Cms/Repositories/DatabaseUserRepository.php (1)
DatabaseUserRepository(18-241)src/Cms/Controllers/Blog.php (2)
__construct(31-48)author(133-161)tests/Unit/Cms/Repositories/DatabaseUserRepositoryTest.php (1)
createTestUser(404-416)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test (sqlite)
🔇 Additional comments (6)
tests/Unit/Cms/BlogControllerTest.php (6)
10-10: LGTM!The new imports for
UserandDatabaseUserRepositoryare correctly added and align with the author-based filtering feature being tested.Also applies to: 14-14
29-29: LGTM!The
_userRepositoryproperty declaration follows the same pattern as the other repository properties in the test class.
248-258: LGTM!The
_userRepositoryinitialization follows the same reflection-based PDO injection pattern used for the other repositories, ensuring consistent test setup.
281-283: LGTM!The user repository injection into the Blog controller is consistent with the pattern used for other repositories and necessary for proper test isolation.
328-339: LGTM!The
createTestUserhelper is well-structured and follows the same pattern as other test data creation helpers in the class. Usingpassword_hash()ensures realistic test data even though it adds minor overhead.
413-438: Well-structured author filtering test.The test properly arranges data (two authors with their posts), mocks the request, and verifies that only the requested author's posts are included in the output.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
resources/database/migrate/20250111000000_create_users_table.php(2 hunks)resources/database/migrate/20250112000000_create_email_verification_tokens_table.php(1 hunks)resources/database/migrate/20250113000000_create_pages_table.php(2 hunks)resources/database/migrate/20250114000000_create_categories_table.php(1 hunks)resources/database/migrate/20250115000000_create_tags_table.php(1 hunks)resources/database/migrate/20250116000000_create_posts_table.php(1 hunks)resources/database/migrate/20250117000000_create_post_categories_table.php(1 hunks)resources/database/migrate/20250118000000_create_post_tags_table.php(1 hunks)resources/database/migrate/20251221000000_create_event_categories_table.php(1 hunks)resources/database/migrate/20251221000001_create_events_table.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- resources/database/migrate/20250115000000_create_tags_table.php
- resources/database/migrate/20251221000000_create_event_categories_table.php
- resources/database/migrate/20250114000000_create_categories_table.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test (sqlite)
🔇 Additional comments (13)
resources/database/migrate/20250112000000_create_email_verification_tokens_table.php (1)
17-24: LGTM!The change to unsigned
bigintegerforuser_idcorrectly aligns with theusers.idprimary key type. The foreign key constraint withCASCADEdelete ensures proper cleanup when a user is removed.resources/database/migrate/20250118000000_create_post_tags_table.php (1)
15-24: LGTM!The junction table is well-structured with a composite primary key, unsigned
bigintegerforeign keys matching the referenced tables, and appropriate indexes for query performance.resources/database/migrate/20250111000000_create_users_table.php (2)
25-25: LGTM!Good addition of
two_factor_recovery_codesas a nullable text field for storing backup codes for 2FA.
30-30: LGTM!The
timezonecolumn with a sensible'UTC'default is a good addition for user preference handling.resources/database/migrate/20250116000000_create_posts_table.php (3)
15-15: LGTM!Using
'signed' => falsefor the table's auto-increment primary key ensures consistency with the unsigned foreign key references.
22-22: LGTM!The
author_idas unsignedbigintegercorrectly matches theusers.idcolumn type.
27-27: Ensureupdated_atis explicitly managed in the post repository.Same pattern as other migrations—the timestamp no longer auto-updates. Verify the repository layer handles this.
resources/database/migrate/20250117000000_create_post_categories_table.php (1)
15-24: LGTM!The junction table structure is correct with composite primary key, properly typed unsigned
bigintegerforeign keys, and appropriate indexes.resources/database/migrate/20250113000000_create_pages_table.php (3)
15-15: LGTM!Unsigned primary key configuration is consistent with other tables.
24-24: LGTM!The
author_idcolumn correctly uses unsignedbigintegerto matchusers.id.
29-29: Ensureupdated_atis explicitly managed in the page repository.Same pattern as other migrations—verify the application layer sets this timestamp on updates.
resources/database/migrate/20251221000001_create_events_table.php (2)
15-43: Well-structured events table design.The table structure effectively supports a calendar/events system with:
- Comprehensive event metadata (dates, location, organizer, contact info)
- CMS workflow features (status, slug, featured_image, view_count)
- Editor.js integration via content_raw JSON column
- Proper indexing for query performance (slug, dates, status, foreign keys)
- Appropriate foreign key relationships with cascade behaviors
33-34: No action required. Theupdated_atcolumn is intentionally handled at the application level for database compatibility. Verification confirms that all repositories explicitly setupdated_aton every update operation (e.g.,DatabaseEventRepositorylines 283-284), ensuring consistent timestamp handling across MySQL, PostgreSQL, and SQLite. This approach was deliberately chosen to avoid database-specificON UPDATE CURRENT_TIMESTAMPsyntax, as documented in the test suite.
resources/database/migrate/20251221000001_create_events_table.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/Integration/IntegrationTestCase.php (1)
108-117: PostgreSQL: Foreign key checks not disabled during truncate.Unlike SQLite and MySQL, the PostgreSQL branch doesn't disable foreign key constraints.
TRUNCATE TABLEwill fail when tables have FK relationships. UseTRUNCATE ... CASCADEfor PostgreSQL.🔎 Proposed fix using TRUNCATE CASCADE for PostgreSQL
// Truncate all tables foreach( $tables as $table ) { if( $driver === 'sqlite' ) { $this->pdo->exec( "DELETE FROM {$table}" ); } + elseif( $driver === 'pgsql' ) + { + $this->pdo->exec( "TRUNCATE TABLE \"{$table}\" CASCADE" ); + } else { $this->pdo->exec( "TRUNCATE TABLE {$table}" ); } }
🧹 Nitpick comments (3)
tests/Integration/IntegrationTestCase.php (1)
268-280: Good error handling improvement.The switch to
ConsoleOutputand explicit exception wrapping will make migration failures much easier to debug. The original exception is preserved via the third parameter.Note: The
NullOutputimport on line 10 is now unused and can be removed.🔎 Remove unused import
use Symfony\Component\Console\Input\StringInput; -use Symfony\Component\Console\Output\NullOutput;tests/Integration/DatabaseCompatibilityTest.php (2)
140-141: Consider the test suite execution time.Multiple
sleep(1)calls across timestamp tests add 5+ seconds to the suite. This is acceptable for integration tests, but if speed becomes a concern, consider injecting a clock abstraction or using sub-second sleeps with microsecond timestamp comparisons.
293-293: Remove unused variable$created.The result of
User::create()is assigned but never used. Since the purpose is to trigger the insert (which will be rolled back), the assignment is unnecessary.🔎 Proposed fix
- $created = User::create( $user->toArray() ); + User::create( $user->toArray() );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/Integration/DatabaseCompatibilityTest.php(1 hunks)tests/Integration/IntegrationTestCase.php(3 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
tests/Integration/DatabaseCompatibilityTest.php
293-293: Avoid unused local variables such as '$created'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (mysql)
🔇 Additional comments (8)
tests/Integration/IntegrationTestCase.php (3)
66-68: LGTM!Good documentation explaining why per-test transactions are not used. This prevents "already in transaction" errors when repositories manage their own transaction lifecycle.
73-85: LGTM!The tearDown properly handles transaction cleanup before truncating tables. The
whileloop ensures all nested transactions are rolled back.
189-193: LGTM!Good addition of WAL mode and busy_timeout to match production ConnectionFactory settings, ensuring test behavior aligns with production SQLite configuration.
tests/Integration/DatabaseCompatibilityTest.php (5)
33-47: LGTM!Good test setup that ensures all repositories share the same PDO connection where migrations ran, preventing connection mismatch issues.
59-82: LGTM!The reflection-based PDO injection is a pragmatic solution for test infrastructure. The hardcoded 'sqlite' adapter in settings is intentionally a placeholder since the actual PDO connection is injected afterward.
90-118: LGTM!Good test verifying that foreign key cascade deletes work consistently across all database drivers.
324-325: Exception message assertion may be fragile.The assertion expects the exact message
'Username already exists'. If the repository or database driver returns a different message (e.g., raw PDO constraint violation), this test will fail. Consider asserting on the exception type only, or ensure the repository consistently wraps constraint violations.
342-366: LGTM!Good verification that database-specific optimizations are correctly applied. The conditional SQLite PRAGMA checks appropriately handle multi-driver scenarios.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
tests/Integration/IntegrationTestCase.php (1)
87-156: Consider identifier quoting and error handling in cleanup.The
cleanupTestData()method has two areas for improvement:
Identifier quoting: Table names (lines 98, 105, 111, 134, 139, 143) and database name (line 105) are directly interpolated into SQL. While the table names come from system tables, using proper identifier quoting is safer and more robust.
Error handling: If truncation fails, foreign key checks may remain disabled. Wrap truncation in try/finally to ensure FK checks are always re-enabled.
🔎 Proposed improvements
Example for MySQL with proper quoting and error handling:
elseif( $driver === 'mysql' ) { $dbname = $this->getDatabaseName(); $tables = $this->pdo->query( - "SELECT table_name FROM information_schema.tables WHERE table_schema = '{$dbname}' AND table_name != 'phinxlog'" + "SELECT table_name FROM information_schema.tables WHERE table_schema = " . $this->pdo->quote($dbname) . " AND table_name != 'phinxlog'" )->fetchAll( PDO::FETCH_COLUMN ); } // Disable foreign key checks temporarily if( $driver === 'sqlite' ) { $this->pdo->exec( 'PRAGMA foreign_keys = OFF' ); } elseif( $driver === 'mysql' ) { $this->pdo->exec( 'SET FOREIGN_KEY_CHECKS = 0' ); } +try { // Truncate all tables foreach( $tables as $table ) { + $quotedTable = $driver === 'pgsql' ? "\"{$table}\"" : "`{$table}`"; if( $driver === 'sqlite' ) { - $this->pdo->exec( "DELETE FROM {$table}" ); + $this->pdo->exec( "DELETE FROM \"{$table}\"" ); } elseif( $driver === 'pgsql' ) { - $this->pdo->exec( "TRUNCATE TABLE {$table} CASCADE" ); + $this->pdo->exec( "TRUNCATE TABLE \"{$table}\" CASCADE" ); } else { - $this->pdo->exec( "TRUNCATE TABLE {$table}" ); + $this->pdo->exec( "TRUNCATE TABLE `{$table}`" ); } } +} finally { // Re-enable foreign key checks if( $driver === 'sqlite' ) { $this->pdo->exec( 'PRAGMA foreign_keys = ON' ); } elseif( $driver === 'mysql' ) { $this->pdo->exec( 'SET FOREIGN_KEY_CHECKS = 1' ); } +}src/Cms/Repositories/DatabaseUserRepository.php (1)
94-103: Clean ID handling and fresh-fetch pattern.Excluding the null
idbefore callingUser::createcorrectly delegates auto-increment to the database. Fetching the user fresh from the database after creation ensures all fields—especially the generated ID—are properly populated.Optional: Add defensive null check
For extra safety, you could add a guard against an unexpected null ID:
$createdUser = User::create( $data ); // Fetch from database to get all fields -return $this->findById( $createdUser->getId() ); +$userId = $createdUser->getId(); +if( !$userId ) +{ + throw new Exception( 'Failed to retrieve user ID after creation' ); +} +return $this->findById( $userId );src/Cms/Repositories/DatabasePostRepository.php (3)
540-577: Same duplication pattern as sync methods.
loadRelationsWithPdo()duplicatesloadRelations()(lines 79-110). The same consolidation approach could apply here with an optional PDO parameter.The implementation is correct and properly loads both categories and tags within the transaction context.
554-555: Consider specifying PDO fetch mode explicitly.The
fetchAll()calls rely on the PDO's default fetch mode. SinceloadRelationsWithPdouses the ORM's PDO (which may have different defaults than$this->_pdo), consider specifyingPDO::FETCH_ASSOCexplicitly for consistency:-$stmt->execute( [ $post->getId() ] ); -$categoryRows = $stmt->fetchAll(); +$stmt->execute( [ $post->getId() ] ); +$categoryRows = $stmt->fetchAll( PDO::FETCH_ASSOC );Also applies to: 569-570
475-488: Fragile coupling to immature ORM internals via reflection—pragmatic but worth improving.Accessing the private static
_pdoproperty via reflection couples this repository to neuron-php/orm's internal implementation. While the pattern works correctly and is used consistently across tests, the 0.1.* ORM version lacks a publicgetPdo()method, forcing this workaround to access the same PDO connection used byModel::transaction()for raw SQL category sync operations.The coupling is real: renaming or restructuring
_pdoin the ORM will cause silent runtime failures. Consider:
- Opening an issue/PR on neuron-php/orm to expose a public
getPdo()method- Adding a try-catch or defensive check in case the property doesn't exist (for forward compatibility)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Cms/Repositories/DatabaseCategoryRepository.php(2 hunks)src/Cms/Repositories/DatabasePageRepository.php(2 hunks)src/Cms/Repositories/DatabasePostRepository.php(3 hunks)src/Cms/Repositories/DatabaseTagRepository.php(2 hunks)src/Cms/Repositories/DatabaseUserRepository.php(2 hunks)tests/Integration/IntegrationTestCase.php(5 hunks)tests/Integration/UserAuthenticationFlowTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Cms/Repositories/DatabaseCategoryRepository.php
🧰 Additional context used
🧬 Code graph analysis (3)
src/Cms/Repositories/DatabaseUserRepository.php (1)
src/Cms/Models/User.php (5)
getCreatedAt(345-348)setCreatedAt(353-357)getUpdatedAt(362-365)setUpdatedAt(370-374)toArray(413-433)
src/Cms/Repositories/DatabasePostRepository.php (3)
src/Cms/Models/Post.php (6)
getCreatedAt(330-333)setCreatedAt(338-342)getUpdatedAt(347-350)setUpdatedAt(355-359)toArray(604-621)getId(57-60)src/Cms/Repositories/DatabasePageRepository.php (2)
create(50-79)count(171-181)src/Cms/Repositories/DatabaseTagRepository.php (2)
create(61-96)count(151-154)
src/Cms/Repositories/DatabasePageRepository.php (4)
src/Cms/Models/Page.php (6)
getCreatedAt(337-340)setCreatedAt(345-349)getUpdatedAt(354-357)setUpdatedAt(362-366)toArray(449-467)getId(59-62)src/Cms/Repositories/DatabaseCategoryRepository.php (2)
create(77-112)findById(37-40)src/Cms/Repositories/DatabaseTagRepository.php (2)
create(61-96)findById(37-40)src/Cms/Repositories/DatabaseUserRepository.php (2)
create(69-104)findById(37-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test (mysql)
🔇 Additional comments (18)
src/Cms/Repositories/DatabasePageRepository.php (2)
58-67: LGTM!The explicit timestamp handling correctly initializes
createdAtandupdatedAtonly when not already set, using immutableDateTimeImmutable. This approach is consistent with the pattern established inDatabaseUserRepository,DatabaseTagRepository, andDatabaseCategoryRepository.
98-100: LGTM!The explicit
updatedAttimestamp update before save ensures consistent behavior across all database adapters (SQLite, MySQL, PostgreSQL) without relying on database-specific defaults or triggers.tests/Integration/IntegrationTestCase.php (6)
66-68: Good documentation of transaction handling strategy.The comment clearly explains why transactions are not managed at the test case level, preventing "already in transaction" errors.
75-82: LGTM! Proper test isolation.The tearDown method correctly handles cleanup by rolling back any lingering transactions and truncating tables for test isolation.
170-174: Good defensive handling of driver name variants.Normalizing
postgrestopgsqlensures consistent driver name handling throughout the test suite.
200-204: LGTM! SQLite optimizations align with production configuration.Enabling WAL mode and setting busy_timeout improves concurrency and lock handling, matching the ConnectionFactory configuration.
279-291: Excellent improvement to migration error visibility.Using
ConsoleOutputinstead ofNullOutputand wrapping the migration in try/catch ensures errors are visible and properly handled.
374-378: Correct PostgreSQL CASCADE handling.The addition of CASCADE for PostgreSQL is correct and necessary for truncating tables with foreign key constraints.
Note: If you apply the identifier quoting refactor suggested for
cleanupTestData(), apply it here as well for consistency.tests/Integration/UserAuthenticationFlowTest.php (1)
110-111: LGTM! Improved PostgreSQL boolean literal handling.Using
TRUEinstead of1for boolean fields is more semantically correct and aligns with PostgreSQL best practices, while remaining compatible with MySQL and SQLite.src/Cms/Repositories/DatabaseUserRepository.php (3)
83-92: Good practice: explicit timestamp handling.The database-independent timestamp initialization is well-implemented. Using
DateTimeImmutableand checking for existing values before setting ensures flexibility while maintaining consistency across database adapters.
130-132: Consistent timestamp management in update.Explicitly setting
updated_atbefore save aligns with the database-independent approach used increateand ensures timestamp consistency across all database adapters.
72-81: Database constraints onusernameandsrc/Cms/Repositories/DatabaseTagRepository.php (2)
75-84: LGTM: Database-agnostic timestamp handling.The explicit timestamp setting using
DateTimeImmutableis a good database-independent approach that ensures consistency across SQLite, MySQL, and PostgreSQL.
122-123: LGTM: Consistent timestamp handling in updates.Setting
updated_atexplicitly before saving maintains the database-independent approach and ensures the timestamp reflects the actual update time regardless of database defaults.src/Cms/Repositories/DatabasePostRepository.php (4)
123-132: LGTM!Explicit timestamp handling is consistent with the pattern used in
DatabaseTagRepositoryandDatabasePageRepository, ensuring ORM creates have proper timestamps regardless of DB adapter defaults.
136-172: Solid transaction-aware implementation.The approach correctly ensures atomicity by using the ORM's PDO connection for relation syncing within the transaction. The comment explaining why
$this->findById()can't be used (line 166-167) is helpful.Minor note: The
count($categories) > 0check at line 152 is slightly redundant sincesyncCategoriesWithPdoalready handles empty arrays gracefully, but keeping it avoids unnecessary reflection calls.
199-208: LGTM!Correctly uses the ORM's PDO connection for relation syncing within the transaction, ensuring atomicity of the update operation.
374-411: Remove dead code:syncCategoriesandsyncTagsare unused duplicates.
syncCategories()andsyncTags()(lines 374–411) are not called anywhere in the codebase. Bothcreate()andupdate()use only the PDO-parameterized versions (syncCategoriesWithPdoandsyncTagsWithPdo).Consolidate the methods by making the PDO parameter optional with
$this->_pdoas default, then remove the duplicate versions:-private function syncCategories( int $postId, array $categoryIds ): void +private function syncCategories( int $postId, array $categoryIds, ?PDO $pdo = null ): void { + $pdo = $pdo ?? $this->_pdo; // Delete existing categories - $this->_pdo->prepare( "DELETE FROM post_categories WHERE post_id = ?" ) + $pdo->prepare( "DELETE FROM post_categories WHERE post_id = ?" ) ->execute( [ $postId ] );Then remove
syncCategoriesWithPdoandsyncTagsWithPdo, updating calls to use the unified methods with explicit PDO where needed.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/Cms/Repositories/DatabaseTagRepository.php (1)
75-109: Null-safety check still missing (duplicate concern).This comment echoes the past review:
findById()returns?Tagbutcreate()returnsTag(non-nullable). Add a null check after the fetch to prevent contract violation.Additionally, this logic is duplicated across multiple repository files—consider the refactoring approach suggested in
DatabaseUserRepository.🔎 Proposed fix
$newTag->save(); - - return $this->findById( $newTag->getId() ); + + $createdTag = $this->findById( $newTag->getId() ); + if( !$createdTag ) + { + throw new Exception( 'Failed to retrieve created tag with ID: ' . $newTag->getId() ); + } + return $createdTag; }src/Cms/Repositories/DatabasePageRepository.php (1)
63-98: Null-safety check still missing (duplicate concern).This echoes the past review:
findById()returns?Pagebutcreate()returnsPage(non-nullable). Add a null check after Line 98 to ensure the contract is honored.Additionally, this timestamp-and-copy logic is duplicated across
DatabaseUserRepository,DatabaseTagRepository,DatabaseCategoryRepository, and this file—extract to shared code as suggested in theDatabaseUserRepositoryreview.🔎 Proposed fix
$newPage->save(); - - return $this->findById( $newPage->getId() ); + + $createdPage = $this->findById( $newPage->getId() ); + if( !$createdPage ) + { + throw new Exception( 'Failed to retrieve created page with ID: ' . $newPage->getId() ); + } + return $createdPage; }
🧹 Nitpick comments (2)
tests/Unit/Cms/Repositories/DatabasePageRepositoryTest.php (1)
21-46: Test setup pattern updated to match repository changes.The test now creates the repository first and retrieves the PDO connection via reflection, aligning with the repository's new initialization pattern. This approach works but exposes a minor coupling to the internal
_pdoproperty.💡 Optional improvement: Consider a test-friendly accessor
If multiple tests require PDO access, consider adding a package-private getter or test helper to avoid reflection:
// In DatabasePageRepository /** @internal For testing only */ public function getPdoForTesting(): PDO { return $this->_pdo; }Then in tests:
$this->pdo = $this->repository->getPdoForTesting();This reduces reflection brittleness while keeping the production API clean.
src/Cms/Repositories/DatabasePostRepository.php (1)
556-593: Same consolidation opportunity forloadRelationsWithPdo.This duplicates
loadRelations()(lines 79-110). Consider havingloadRelations()delegate toloadRelationsWithPdo():🔎 Proposed consolidation
private function loadRelations( Post $post ): void { - // Load categories - $stmt = $this->_pdo->prepare( - "SELECT c.* FROM categories c - INNER JOIN post_categories pc ON c.id = pc.category_id - WHERE pc.post_id = ?" - ); - // ... rest of implementation ... + $this->loadRelationsWithPdo( $this->_pdo, $post ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Cms/Repositories/DatabaseCategoryRepository.php(2 hunks)src/Cms/Repositories/DatabasePageRepository.php(5 hunks)src/Cms/Repositories/DatabasePostRepository.php(3 hunks)src/Cms/Repositories/DatabaseTagRepository.php(2 hunks)src/Cms/Repositories/DatabaseUserRepository.php(2 hunks)tests/Unit/Cms/Repositories/DatabasePageRepositoryTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/Cms/Repositories/DatabaseUserRepository.php (1)
src/Cms/Models/User.php (5)
getCreatedAt(345-348)setCreatedAt(353-357)getUpdatedAt(362-365)setUpdatedAt(370-374)toArray(413-433)
src/Cms/Repositories/DatabaseCategoryRepository.php (1)
src/Cms/Models/Category.php (5)
getCreatedAt(105-108)setCreatedAt(113-117)getUpdatedAt(122-125)setUpdatedAt(130-134)toArray(182-192)
src/Cms/Repositories/DatabasePageRepository.php (2)
src/Cms/Database/ConnectionFactory.php (2)
ConnectionFactory(17-156)createFromSettings(26-36)src/Cms/Models/Page.php (5)
getCreatedAt(337-340)setCreatedAt(345-349)getUpdatedAt(354-357)setUpdatedAt(362-366)toArray(449-467)
tests/Unit/Cms/Repositories/DatabasePageRepositoryTest.php (2)
src/Cms/Repositories/DatabasePageRepository.php (1)
DatabasePageRepository(18-217)tests/Unit/Cms/Repositories/DatabasePostRepositoryTest.php (1)
createTables(54-142)
src/Cms/Repositories/DatabaseTagRepository.php (1)
src/Cms/Models/Tag.php (5)
getCreatedAt(87-90)setCreatedAt(95-99)getUpdatedAt(104-107)setUpdatedAt(112-116)toArray(163-172)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test (sqlite)
🔇 Additional comments (8)
src/Cms/Repositories/DatabaseUserRepository.php (1)
147-148: LGTM! Database-independent timestamp handling.Explicitly setting
updatedAtensures consistent behavior across SQLite, MySQL, and PostgreSQL without relying on database-specific defaults.src/Cms/Repositories/DatabaseTagRepository.php (1)
136-137: LGTM! Database-independent timestamp handling.Explicitly setting
updatedAtensures consistent behavior across SQLite, MySQL, and PostgreSQL without relying on database-specific defaults.src/Cms/Repositories/DatabaseCategoryRepository.php (1)
152-153: LGTM! Database-independent timestamp handling.Explicitly setting
updatedAtensures consistent behavior across SQLite, MySQL, and PostgreSQL without relying on database-specific defaults.src/Cms/Repositories/DatabasePageRepository.php (2)
20-31: LGTM! Explicit PDO initialization for cross-database compatibility.Adding the
ConnectionFactoryinitialization in the constructor ensures consistent database connection handling across SQLite, MySQL, and PostgreSQL, aligning with the broader repository pattern refactor.
118-119: LGTM! Database-independent timestamp handling.Explicitly setting
updatedAtensures consistent behavior across SQLite, MySQL, and PostgreSQL without relying on database-specific defaults.src/Cms/Repositories/DatabasePostRepository.php (3)
123-132: LGTM - Explicit timestamp handling.Good approach to handle timestamps explicitly since the ORM doesn't use DB defaults. The null checks ensure existing timestamps are preserved.
143-165: Reasonable approach for working around ORM limitations.The dynamic field copying handles the type mismatch between
toArray()string output and DateTime setter expectations correctly. The explicit list of skipped fields at line 148 is clear about what's being handled separately.
215-224: Consistent transaction handling with create().Using the same PDO connection for relation syncing within the transaction ensures atomicity.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/Cms/Repositories/DatabasePostRepository.php (3)
369-406: Remove unused duplicate sync methods.
syncCategories()(lines 369-385) andsyncTags()(lines 390-406) are dead code. Thecreate()andupdate()methods exclusively callsyncCategoriesWithPdo()andsyncTagsWithPdo()instead. Keeping these unused methods increases maintenance burden.🔎 Verification and removal
Verify no other code uses these methods:
#!/bin/bash # Verify syncCategories and syncTags (without WithPdo) are unused rg -n '->syncCategories\(' --type=php -g '!DatabasePostRepository.php' rg -n '->syncTags\(' --type=php -g '!DatabasePostRepository.php'If no results, remove lines 369-406.
Based on past learnings and coding guidelines: This addresses the dead code concern flagged in previous reviews for this file.
160-167: Add null-safety check afterPost::find().Line 162 calls
Post::find()which returns?Post. CallingloadRelationsWithPdo()on line 165 without a null check will cause a TypeError if the find fails due to transaction isolation edge cases or connection issues.🔎 Proposed fix
// Use ORM's find() to get DB-set values within the same transaction // Can't use $this->findById() because it uses a different PDO connection $refreshedPost = Post::find( $post->getId() ); + if( !$refreshedPost ) + { + throw new Exception( 'Failed to retrieve created post with ID: ' . $post->getId() ); + } // Load relations manually since we're inside the transaction $this->loadRelationsWithPdo( $pdo, $refreshedPost );Based on past learnings and coding guidelines: This addresses the null-safety concern flagged in previous reviews for this file.
470-483: Add defensive checks to reflection-based PDO access.The reflection-based access to
Post::$_pdois fragile. If neuron-php/orm (version 0.1.*) changes the property name, visibility, or structure, this will fail silently or throw unclear errors.🔎 Proposed defensive implementation
private function getOrmPdo(): PDO { $reflectionClass = new \ReflectionClass( Post::class ); + if( !$reflectionClass->hasProperty( '_pdo' ) ) + { + throw new Exception( 'ORM internal structure changed: _pdo property not found' ); + } $property = $reflectionClass->getProperty( '_pdo' ); $property->setAccessible( true ); - return $property->getValue(); + $pdo = $property->getValue(); + if( !$pdo instanceof PDO ) + { + throw new Exception( 'ORM PDO connection not initialized or invalid type' ); + } + return $pdo; }Based on past learnings and coding guidelines: This addresses the reflection fragility concern flagged in previous reviews for this file.
src/Cms/Repositories/DatabaseUserRepository.php (1)
83-97: Add null-safety check and extract duplicated timestamp logic.Line 97 violates the return type contract:
findById()returns?User(nullable), butcreate()promisesUser(non-nullable). If the re-fetch fails, this causes a type error.Additionally, the timestamp-setting pattern (lines 83-92) is duplicated verbatim in
DatabaseTagRepository(lines 75-84),DatabaseCategoryRepository(lines 91-100), and other repositories, violating DRY.🔎 Immediate null-safety fix
// Save the user using ORM $user->save(); - return $this->findById( $user->getId() ); + $createdUser = $this->findById( $user->getId() ); + if( !$createdUser ) + { + throw new Exception( 'Failed to retrieve created user with ID: ' . $user->getId() ); + } + return $createdUser; }💡 Extract shared timestamp logic into a trait or base class
Consider creating a trait to eliminate the duplication:
trait ManagesTimestamps { protected function setTimestampsIfMissing( $entity ): void { $now = new \DateTimeImmutable(); if( method_exists( $entity, 'getCreatedAt' ) && method_exists( $entity, 'setCreatedAt' ) ) { if( !$entity->getCreatedAt() ) { $entity->setCreatedAt( $now ); } } if( method_exists( $entity, 'getUpdatedAt' ) && method_exists( $entity, 'setUpdatedAt' ) ) { if( !$entity->getUpdatedAt() ) { $entity->setUpdatedAt( $now ); } } } }Then use it in all repositories:
class DatabaseUserRepository implements IUserRepository { use ManagesTimestamps; public function create( User $user ): User { // ... validation ... $this->setTimestampsIfMissing( $user ); $user->save(); // ... null check and return ... } }This would eliminate 40+ lines of duplication across repositories.
Based on past learnings and coding guidelines: This addresses the recurring null-safety concern flagged in previous reviews.
src/Cms/Repositories/DatabaseTagRepository.php (1)
75-89: Add null-safety check (consistent with other repositories).Line 89 returns
findById()which can returnnull, butcreate()is declared to return non-nullableTag. This violates the method contract and is the same issue present inDatabaseUserRepositoryandDatabaseCategoryRepository.🔎 Proposed fix
// Save the tag using ORM $tag->save(); - return $this->findById( $tag->getId() ); + $createdTag = $this->findById( $tag->getId() ); + if( !$createdTag ) + { + throw new Exception( 'Failed to retrieve created tag with ID: ' . $tag->getId() ); + } + return $createdTag; }Based on past learnings and coding guidelines: This is consistent with the null-safety concerns raised in previous reviews for this repository.
src/Cms/Repositories/DatabaseCategoryRepository.php (1)
91-105: Add null-safety check (consistent with other repositories).Line 105 returns
findById()which returns?Category, butcreate()promises non-nullableCategory. This is the same type-safety violation present inDatabaseUserRepository(line 97) andDatabaseTagRepository(line 89).🔎 Proposed fix
// Save the category using ORM $category->save(); - return $this->findById( $category->getId() ); + $createdCategory = $this->findById( $category->getId() ); + if( !$createdCategory ) + { + throw new Exception( 'Failed to retrieve created category with ID: ' . $category->getId() ); + } + return $createdCategory; }Based on past learnings and coding guidelines: This addresses the null-safety concern flagged in previous reviews for this repository.
🧹 Nitpick comments (1)
src/Cms/Repositories/DatabasePostRepository.php (1)
485-533: Consider extracting common sync logic.
syncCategoriesWithPdo()(lines 492-508) andsyncTagsWithPdo()(lines 517-533) have nearly identical implementations—only the table name differs. This duplication could be reduced with a generic helper method.💡 Possible refactor
private function syncRelation( PDO $pdo, int $postId, string $tableName, string $relationColumn, array $relationIds ): void { // Delete existing relations $pdo->prepare( "DELETE FROM {$tableName} WHERE post_id = ?" ) ->execute( [ $postId ] ); // Insert new relations if( !empty( $relationIds ) ) { $stmt = $pdo->prepare( "INSERT INTO {$tableName} (post_id, {$relationColumn}, created_at) VALUES (?, ?, ?)" ); $now = ( new \DateTimeImmutable() )->format( 'Y-m-d H:i:s' ); foreach( $relationIds as $relationId ) { $stmt->execute( [ $postId, $relationId, $now ] ); } } } private function syncCategoriesWithPdo( PDO $pdo, int $postId, array $categoryIds ): void { $this->syncRelation( $pdo, $postId, 'post_categories', 'category_id', $categoryIds ); } private function syncTagsWithPdo( PDO $pdo, int $postId, array $tagIds ): void { $this->syncRelation( $pdo, $postId, 'post_tags', 'tag_id', $tagIds ); }This is optional since the methods are private and the duplication is localized.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Cms/Repositories/DatabaseCategoryRepository.php(2 hunks)src/Cms/Repositories/DatabasePageRepository.php(5 hunks)src/Cms/Repositories/DatabasePostRepository.php(3 hunks)src/Cms/Repositories/DatabaseTagRepository.php(2 hunks)src/Cms/Repositories/DatabaseUserRepository.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Cms/Repositories/DatabasePageRepository.php
🧰 Additional context used
🧬 Code graph analysis (4)
src/Cms/Repositories/DatabaseCategoryRepository.php (2)
src/Cms/Models/Category.php (5)
getCreatedAt(105-108)setCreatedAt(113-117)getUpdatedAt(122-125)setUpdatedAt(130-134)getId(37-40)src/Cms/Repositories/ICategoryRepository.php (1)
findById(17-17)
src/Cms/Repositories/DatabaseTagRepository.php (1)
src/Cms/Models/Tag.php (4)
getCreatedAt(87-90)setCreatedAt(95-99)getUpdatedAt(104-107)setUpdatedAt(112-116)
src/Cms/Repositories/DatabaseUserRepository.php (6)
src/Cms/Models/Tag.php (4)
getCreatedAt(87-90)setCreatedAt(95-99)getUpdatedAt(104-107)setUpdatedAt(112-116)src/Cms/Models/Category.php (4)
getCreatedAt(105-108)setCreatedAt(113-117)getUpdatedAt(122-125)setUpdatedAt(130-134)src/Cms/Models/User.php (4)
getCreatedAt(345-348)setCreatedAt(353-357)getUpdatedAt(362-365)setUpdatedAt(370-374)src/Cms/Repositories/DatabaseCategoryRepository.php (1)
findById(37-40)src/Cms/Repositories/DatabasePageRepository.php (1)
findById(37-41)src/Cms/Repositories/DatabaseTagRepository.php (1)
findById(37-40)
src/Cms/Repositories/DatabasePostRepository.php (1)
src/Cms/Models/Post.php (5)
getCategories(366-369)getTags(426-429)getId(57-60)setCategories(376-380)setTags(436-440)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (mysql)
🔇 Additional comments (4)
src/Cms/Repositories/DatabaseUserRepository.php (1)
124-126: LGTM: Database-independent timestamp handling.Explicitly setting
updated_atensures consistent behavior across SQLite, MySQL, and PostgreSQL without relying on database-specific triggers or defaults.src/Cms/Repositories/DatabaseTagRepository.php (1)
116-118: LGTM: Consistent timestamp handling.The database-independent approach to updating timestamps aligns with the cross-database compatibility goals mentioned in the PR summary.
src/Cms/Repositories/DatabaseCategoryRepository.php (1)
132-134: LGTM: Database-agnostic timestamp update.Setting
updated_atexplicitly ensures consistent behavior across all supported database adapters (SQLite, MySQL, PostgreSQL).src/Cms/Repositories/DatabasePostRepository.php (1)
123-132: LGTM: Consistent timestamp handling.Explicitly setting timestamps before save ensures database-independent behavior, consistent with the approach in other repositories.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/Cms/Repositories/DatabaseTagRepository.php (1)
78-85: Previous null-safety concern resolved.The
saveAndRefreshmethod from the trait now handles the null-safety check that was previously flagged. IffindByIdreturnsnullafter save, the trait throws a descriptiveRuntimeException, maintaining the non-nullable return type contract.src/Cms/Repositories/DatabaseUserRepository.php (2)
7-7: Trait adoption addresses previous DRY concerns.The
ManagesTimestampstrait eliminates the duplicated timestamp-and-copy logic that was flagged in the previous review.Also applies to: 21-22
86-93: Null-safety concern resolved via trait.The
saveAndRefreshmethod now handles the null check with aRuntimeException, addressing the critical issue flagged in the previous review.src/Cms/Repositories/DatabasePageRepository.php (1)
66-73: Previous null-safety concern resolved.The
saveAndRefreshmethod addresses the null return type violation that was flagged in the past review.src/Cms/Repositories/DatabaseCategoryRepository.php (1)
94-101: Previous null-safety and DRY concerns resolved.The
saveAndRefreshmethod addresses the critical null-safety issue flagged in the past review, and the trait adoption eliminates code duplication.src/Cms/Repositories/DatabasePostRepository.php (2)
438-491: Defensive checks properly implemented.The method now includes the requested validations: property existence check, type validation, and proper exception handling. This addresses the previous review concern about ORM structure changes.
Minor observation: Line 468 passes
nullfor instance properties, which would throw aReflectionException(caught appropriately). Since_pdois static in the ORM, this branch is effectively unreachable, but the exception handling ensures safe behavior either way.
493-541: LGTM!Clean implementation of transaction-scoped sync methods. The previous concern about duplicate non-WithPdo methods has been addressed—they've been removed as indicated by the PR summary.
🧹 Nitpick comments (2)
src/Cms/Repositories/DatabasePageRepository.php (1)
23-24: Remove unused$_pdoproperty.The
$_pdoproperty is assigned in the constructor but never used anywhere in this class. All database operations rely on the ORM methods (Page::with(), Page::query(), etc.), making the stored PDO instance redundant. Remove the property and assignment unless the "PostgreSQL workaround" comment indicates a planned use that should be documented.src/Cms/Repositories/DatabasePostRepository.php (1)
543-580: Consider consolidating withloadRelations().This method mirrors
loadRelations()(lines 83-114) but accepts a PDO parameter. The duplication is intentional for transaction isolation, but you could consolidate by makingloadRelations()accept an optional?PDO $pdo = nullparameter and defaulting to$this->_pdo.This is optional and low priority—current implementation is explicit and works correctly.
🔎 Consolidation approach
-private function loadRelations( Post $post ): void +private function loadRelations( Post $post, ?PDO $pdo = null ): void { + $pdo = $pdo ?? $this->_pdo; + // Load categories - $stmt = $this->_pdo->prepare( + $stmt = $pdo->prepare( "SELECT c.* FROM categories c INNER JOIN post_categories pc ON c.id = pc.category_id WHERE pc.post_id = ?" ); // ... rest uses $pdo instead of $this->_pdo }Then replace
loadRelationsWithPdo($pdo, $post)calls withloadRelations($post, $pdo).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
resources/database/migrate/20251221000001_create_events_table.php(1 hunks)src/Cms/Repositories/DatabaseCategoryRepository.php(4 hunks)src/Cms/Repositories/DatabasePageRepository.php(6 hunks)src/Cms/Repositories/DatabasePostRepository.php(6 hunks)src/Cms/Repositories/DatabaseTagRepository.php(4 hunks)src/Cms/Repositories/DatabaseUserRepository.php(7 hunks)src/Cms/Repositories/Traits/ManagesTimestamps.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/database/migrate/20251221000001_create_events_table.php
🧰 Additional context used
🧬 Code graph analysis (6)
src/Cms/Repositories/DatabaseCategoryRepository.php (2)
src/Cms/Repositories/Traits/ManagesTimestamps.php (2)
ensureTimestamps(20-39)saveAndRefresh(54-80)src/Cms/Models/Category.php (1)
setUpdatedAt(130-134)
src/Cms/Repositories/DatabasePageRepository.php (4)
src/Cms/Database/ConnectionFactory.php (2)
ConnectionFactory(17-156)createFromSettings(26-36)src/Cms/Repositories/Traits/ManagesTimestamps.php (2)
ensureTimestamps(20-39)saveAndRefresh(54-80)src/Cms/Models/Page.php (1)
setUpdatedAt(362-366)src/Cms/Models/Post.php (1)
setUpdatedAt(355-359)
src/Cms/Repositories/DatabaseUserRepository.php (5)
src/Cms/Repositories/Traits/ManagesTimestamps.php (2)
ensureTimestamps(20-39)saveAndRefresh(54-80)src/Cms/Models/Category.php (1)
setUpdatedAt(130-134)src/Cms/Models/Tag.php (1)
setUpdatedAt(112-116)src/Cms/Models/User.php (1)
setUpdatedAt(370-374)src/Cms/Models/Post.php (1)
setUpdatedAt(355-359)
src/Cms/Repositories/DatabaseTagRepository.php (5)
src/Cms/Repositories/Traits/ManagesTimestamps.php (2)
ensureTimestamps(20-39)saveAndRefresh(54-80)src/Cms/Models/Category.php (1)
setUpdatedAt(130-134)src/Cms/Models/Tag.php (1)
setUpdatedAt(112-116)src/Cms/Models/User.php (1)
setUpdatedAt(370-374)src/Cms/Models/Post.php (1)
setUpdatedAt(355-359)
src/Cms/Repositories/DatabasePostRepository.php (2)
src/Cms/Repositories/Traits/ManagesTimestamps.php (1)
ensureTimestamps(20-39)src/Cms/Models/Post.php (4)
getCategories(366-369)getTags(426-429)setCategories(376-380)setTags(436-440)
src/Cms/Repositories/Traits/ManagesTimestamps.php (1)
src/Cms/Services/Auth/Authentication.php (1)
id(257-265)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (postgres)
🔇 Additional comments (14)
src/Cms/Repositories/DatabaseTagRepository.php (2)
7-7: Good adoption of the ManagesTimestamps trait.The trait usage centralizes timestamp handling and null-safety checks, eliminating the duplication previously flagged across repositories.
Also applies to: 21-22
112-113: Timestamp update approach is correct.Setting
updatedAtexplicitly before save ensures database-agnostic timestamp handling.src/Cms/Repositories/DatabaseUserRepository.php (1)
163-168: Consistent timestamp updates in atomic operations.Good practice to include
updated_atin atomic increment/update operations to maintain audit trail consistency.Also applies to: 195-196, 218-221
src/Cms/Repositories/DatabasePageRepository.php (2)
6-7: Trait adoption and imports look good.Consistent with the pattern applied across other repositories.
Also applies to: 21-22
188-190: Consistent timestamp handling in atomic increment.Including
updated_atin the increment operation maintains timestamp consistency.src/Cms/Repositories/DatabaseCategoryRepository.php (2)
7-7: Consistent trait adoption.The
ManagesTimestampstrait usage is consistent with other repositories.Also applies to: 21-22
128-130: Database-independent timestamp update.Consistent with the approach used in other repositories.
src/Cms/Repositories/Traits/ManagesTimestamps.php (3)
1-12: Well-structured trait introduction.The trait cleanly encapsulates the timestamp management logic that was previously duplicated across repositories.
20-39: Solid timestamp initialization logic.Using a single
$nowinstance for both timestamps ensures consistency. The duck-typing viamethod_existsprovides flexibility for entities with varying timestamp support.
54-80: Good null-safety handling with descriptive exceptions.The method properly validates both the ID after save and the refreshed entity, throwing descriptive
RuntimeExceptionmessages. This addresses the null-safety concerns previously raised across all repositories.One consideration: if
$entity->save()throws an exception (e.g., database constraint violation), it will propagate up naturally, which is appropriate behavior. The subsequent null checks handle the edge cases where save appears to succeed but the entity cannot be retrieved.src/Cms/Repositories/DatabasePostRepository.php (4)
9-25: LGTM!Good use of the
ManagesTimestampstrait for standardized timestamp handling and proper import ofRuntimeExceptionfor explicit error handling.
119-176: Well-structured transactional creation with proper defensive checks.The null check for
$refreshedPost(lines 167-170) addresses the previous review concern. The pattern of saving original relations before the ORM save, then syncing with the transaction's PDO, ensures consistent state.
196-215: LGTM!Consistent use of transaction-scoped PDO for relation syncing, matching the pattern established in
create().
364-374: The ORM'sincrement()method supports additional column updates via the third parameter.The atomic increment implementation is correct. The third parameter allows updating additional columns, as shown in examples like
increment('points', 10, ['level' => 2]). Your usage ofincrement('view_count', 1, ['updated_at' => ...])follows the proper Laravel Eloquent pattern and will atomically update both the view count and timestamp in a single operation.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Cms/Services/Widget/CalendarWidget.php (1)
107-107: Consider removing unused parameter.The
$attrsparameter is not used withinrenderTemplate(). If there's no plan to use it for future extensibility (e.g., custom template options), consider removing it from the signature.🔎 Proposed refactor
- private function renderTemplate( array $events, array $attrs ): string + private function renderTemplate( array $events ): string {And update the call on line 77:
- return $this->renderTemplate( $events, $attrs ); + return $this->renderTemplate( $events );tests/Integration/DatabaseCompatibilityTest.php (1)
142-142: Consider reducing test execution time.Five timestamp tests use
sleep(1), adding approximately 5 seconds to the test suite. While this ensures timestamp differences at second granularity, consider alternatives such as:
- Using microsecond-precision timestamps if supported
- Mocking time in tests
- Grouping multiple timestamp assertions into fewer tests
Also applies to: 169-169, 193-193, 230-230, 267-267
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
EXCEPTION_HANDLING.mdresources/views/admin/event_categories/create.phpsrc/Cms/Controllers/Admin/EventCategories.phpsrc/Cms/Controllers/Admin/Events.phpsrc/Cms/Exceptions/AuthorizationException.phpsrc/Cms/Exceptions/CmsException.phpsrc/Cms/Exceptions/DuplicateEntityException.phpsrc/Cms/Exceptions/EntityNotFoundException.phpsrc/Cms/Exceptions/RepositoryException.phpsrc/Cms/Exceptions/SecurityException.phpsrc/Cms/Exceptions/ValidationException.phpsrc/Cms/Models/Event.phpsrc/Cms/Repositories/DatabaseEventRepository.phpsrc/Cms/Repositories/DatabaseUserRepository.phpsrc/Cms/Repositories/Traits/ManagesTimestamps.phpsrc/Cms/Services/Widget/CalendarWidget.phptests/Integration/DatabaseCompatibilityTest.phptests/Unit/Cms/Repositories/DatabaseUserRepositoryTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Cms/Repositories/Traits/ManagesTimestamps.php
- src/Cms/Repositories/DatabaseUserRepository.php
- src/Cms/Models/Event.php
🧰 Additional context used
🧬 Code graph analysis (9)
src/Cms/Exceptions/RepositoryException.php (2)
src/Cms/Exceptions/CmsException.php (3)
CmsException(12-48)__construct(22-31)shouldLog(44-47)src/Cms/Exceptions/EntityNotFoundException.php (2)
__construct(19-31)shouldLog(36-39)
src/Cms/Exceptions/EntityNotFoundException.php (2)
src/Cms/Exceptions/CmsException.php (3)
CmsException(12-48)__construct(22-31)shouldLog(44-47)src/Cms/Exceptions/DuplicateEntityException.php (2)
__construct(19-31)shouldLog(36-39)
src/Cms/Exceptions/SecurityException.php (3)
src/Cms/Exceptions/CmsException.php (3)
CmsException(12-48)__construct(22-31)shouldLog(44-47)src/Cms/Exceptions/DuplicateEntityException.php (2)
__construct(19-31)shouldLog(36-39)src/Cms/Exceptions/EntityNotFoundException.php (2)
__construct(19-31)shouldLog(36-39)
src/Cms/Exceptions/ValidationException.php (1)
src/Cms/Exceptions/CmsException.php (3)
CmsException(12-48)__construct(22-31)shouldLog(44-47)
src/Cms/Exceptions/CmsException.php (5)
src/Cms/Exceptions/DuplicateEntityException.php (2)
__construct(19-31)shouldLog(36-39)src/Cms/Exceptions/EntityNotFoundException.php (2)
__construct(19-31)shouldLog(36-39)src/Cms/Exceptions/RepositoryException.php (2)
__construct(19-36)shouldLog(41-44)src/Cms/Exceptions/SecurityException.php (2)
__construct(17-27)shouldLog(32-35)src/Cms/Exceptions/ValidationException.php (2)
__construct(20-34)shouldLog(49-52)
src/Cms/Exceptions/DuplicateEntityException.php (2)
src/Cms/Exceptions/CmsException.php (3)
CmsException(12-48)__construct(22-31)shouldLog(44-47)src/Cms/Exceptions/EntityNotFoundException.php (2)
__construct(19-31)shouldLog(36-39)
src/Cms/Exceptions/AuthorizationException.php (2)
src/Cms/Exceptions/CmsException.php (3)
CmsException(12-48)__construct(22-31)shouldLog(44-47)src/Cms/Exceptions/SecurityException.php (2)
__construct(17-27)shouldLog(32-35)
tests/Unit/Cms/Repositories/DatabaseUserRepositoryTest.php (1)
src/Cms/Exceptions/DuplicateEntityException.php (1)
DuplicateEntityException(10-40)
src/Cms/Controllers/Admin/EventCategories.php (6)
src/Cms/Repositories/DatabaseEventCategoryRepository.php (7)
DatabaseEventCategoryRepository(19-178)__construct(29-32)all(37-43)create(100-123)findById(48-60)update(128-149)delete(154-158)src/Cms/Services/EventCategory/Creator.php (2)
Creator(13-75)create(32-52)src/Cms/Services/EventCategory/Updater.php (2)
Updater(13-54)update(33-53)src/Cms/Services/EventCategory/Deleter.php (2)
Deleter(13-32)delete(28-31)src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)src/Cms/Repositories/IEventCategoryRepository.php (5)
all(19-19)create(51-51)findById(27-27)update(59-59)delete(67-67)
🪛 markdownlint-cli2 (0.18.1)
EXCEPTION_HANDLING.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 PHPMD (2.15.0)
src/Cms/Controllers/Admin/Events.php
52-52: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
95-95: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
tests/Integration/DatabaseCompatibilityTest.php
294-294: Avoid unused local variables such as '$created'. (undefined)
(UnusedLocalVariable)
src/Cms/Controllers/Admin/EventCategories.php
48-48: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
83-83: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
src/Cms/Services/Widget/CalendarWidget.php
107-107: Avoid unused parameters such as '$attrs'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (postgres)
🔇 Additional comments (22)
resources/views/admin/event_categories/create.php (2)
25-35: Great improvement - error display implemented correctly!The validation error display mechanism that was previously missing has been properly implemented. The error block shows dismissible alerts with escaped error messages, addressing the concern from the previous review.
42-60: Good UX - old input values preserved on validation failure.The form correctly repopulates field values from the
$oldarray when validation fails, preventing users from losing their input. This is a solid user experience pattern.src/Cms/Services/Widget/CalendarWidget.php (1)
46-48: Excellent fix - boolean string parsing now correct!The
filter_varwithFILTER_VALIDATE_BOOLEANproperly handles string values like"false", correctly converting them to booleanfalse. This resolves the issue from the previous review whereupcoming="false"would be treated as truthy.src/Cms/Controllers/Admin/EventCategories.php (2)
124-132: Excellent security improvement - CSRF validation now comprehensive!All three state-mutating methods (
store(),update(),destroy()) now properly validate CSRF tokens before processing any changes. The implementation:
- Validates tokens early, before any business logic
- Logs security warnings on validation failures
- Redirects with user-friendly error messages
- Follows consistent patterns across all endpoints
This fully addresses the major security concerns from previous reviews.
Also applies to: 224-232, 277-285
152-160: Good error recovery - old input preserved for user convenience.The error handling correctly stores both validation errors and the original form input in the session flash, allowing the create view to repopulate fields. This prevents users from losing their work when validation fails.
src/Cms/Controllers/Admin/Events.php (2)
134-142: Excellent security hardening - CSRF protection fully implemented!All mutation endpoints now validate CSRF tokens before any processing:
store()validates before creating eventsupdate()validates before modifying eventsdestroy()validates before deletionThe implementation includes security logging and user-friendly error messages. This comprehensively addresses the major security concerns from previous reviews.
Also applies to: 258-266, 337-345
202-211: Execution flow is safe -neverreturn type guarantees termination.The concern from previous reviews about potential null reference after
redirect()is addressed by theneverreturn type on these methods. PHP's type system guarantees thatredirect()terminates execution (viaexit()or exception), so code after the redirect calls cannot execute. The permission checks and subsequent operations are safe.Also applies to: 247-256, 326-335
src/Cms/Repositories/DatabaseEventRepository.php (1)
145-149: Critical SQL bug fixed - operator precedence now correct!The parentheses grouping in the
getPast()query has been corrected. The WHERE clause now properly evaluates as:WHERE ((date conditions) OR (date conditions)) AND status = ?This ensures the status filter applies to all events, not just those matching the second date condition. The fix resolves the critical issue from the previous review.
EXCEPTION_HANDLING.md (1)
1-410: Excellent comprehensive documentation!This exception handling guide is well-structured and provides clear guidance on:
- Exception hierarchy and types
- Usage patterns with practical examples
- Migration from generic exceptions
- Best practices for error handling
- Testing and logging integration
The before/after migration examples are particularly helpful for developers adopting this new exception hierarchy.
tests/Unit/Cms/Repositories/DatabaseUserRepositoryTest.php (1)
8-8: LGTM! Tests correctly updated for new exception hierarchy.The test updates properly reflect the migration to
DuplicateEntityException:
- Import added for the new exception class
- Exception expectations updated from generic
ExceptiontoDuplicateEntityException- Expected messages now match the standardized format: "Duplicate User: {field} '{value}' already exists"
These changes maintain test coverage while adopting the new exception handling system.
Also applies to: 232-233, 249-250, 296-297, 316-317
src/Cms/Exceptions/SecurityException.php (1)
1-36: LGTM! Well-implemented security exception.The
SecurityExceptionclass is properly structured:
- Extends
CmsExceptionwith appropriate message separation (technical vs user-friendly)- Correctly logs security violations (
shouldLog()returnstrue)- User message is appropriately generic to avoid exposing security details
- Constructor signature allows for context and exception chaining
This fits well into the overall exception hierarchy.
src/Cms/Exceptions/EntityNotFoundException.php (1)
1-40: LGTM! Flexible and well-designed not-found exception.The
EntityNotFoundExceptionclass is well-implemented:
- Flexible identifier type support (
int|string) accommodates both IDs and slugs- Clear separation of technical details (ID/slug value) from user message
- Appropriately logs not-found events for security monitoring (
shouldLog()returnstrue)- Constructor parameters provide good context for debugging
The design supports various lookup scenarios while maintaining clean user-facing messages.
src/Cms/Exceptions/CmsException.php (1)
1-48: Excellent base exception design!The
CmsExceptionbase class demonstrates solid exception design principles:
- Clean separation: Technical messages for logs vs user-friendly messages for display
- Sensible defaults:
userMessagedefaults to technical message if not provided (line 30)- Safe logging:
shouldLog()returnstrueby default, ensuring exceptions are captured unless explicitly opted out- Proper chaining: Supports exception chaining via
$previousparameter- Clear API: Simple, well-documented methods
This provides a robust foundation for the specialized exception hierarchy.
src/Cms/Exceptions/AuthorizationException.php (1)
1-41: LGTM! Security-conscious authorization exception.The
AuthorizationExceptionis well-implemented:
- Flexible constructor supports both general actions and resource-specific authorization failures
- Technical message provides context for debugging (lines 25-27)
- User message is appropriately generic to avoid leaking permission structure
- Correctly logs authorization failures (
shouldLog()returnstrue) for security auditingThe distinction between technical and user messages is particularly important for authorization failures.
src/Cms/Exceptions/RepositoryException.php (1)
1-45: LGTM! Well-structured repository exception.The
RepositoryExceptionis appropriately designed:
- Constructor parameters (
operation,entityType,details) provide comprehensive context for debugging- Optional
detailsparameter allows additional diagnostic information (lines 28-31)- User message is suitably generic to hide technical/infrastructure details from end users
- Correctly logs repository failures (
shouldLog()returnstrue)This exception properly handles infrastructure-level errors while maintaining security through message separation.
src/Cms/Exceptions/ValidationException.php (1)
1-53: LGTM! Thoughtfully designed validation exception.The
ValidationExceptiondemonstrates excellent design:
- Flexible input: Union type
string|arraysupports both single and multiple errors (line 21)- Clean normalization: Consistently stores errors as array (line 26)
- Smart messaging: User message adapts to single vs multiple errors (lines 29-31)
- Programmatic access:
getErrors()method allows controllers to access validation details- Appropriate logging:
shouldLog()returnsfalsesince validation errors are expected user input issues, not system failuresThis exception type properly handles the common validation scenario while maintaining the overall exception hierarchy pattern.
src/Cms/Exceptions/DuplicateEntityException.php (1)
1-40: LGTM! Clean exception implementation following established patterns.The implementation correctly:
- Extends
CmsExceptionand delegates to its constructor with both technical and user-facing messages- Constructs clear, informative messages for duplicate entity scenarios
- Overrides
shouldLog()to returnfalse, which is appropriate for validation errors that don't require logging- Follows the same structural pattern as
EntityNotFoundExceptionThe exception integrates well with the repository layer and provides good separation between technical details (for developers) and user-friendly messages (for end users).
tests/Integration/DatabaseCompatibilityTest.php (5)
91-119: LGTM!The test correctly verifies cascade delete behavior via foreign key constraints, which is critical for data integrity across all supported database drivers.
127-277: LGTM!The timestamp tests comprehensively verify that
updated_atis properly maintained at the application level across all entity types (User, Category, Tag, Page, Post). This is critical given the removal of database-level timestamp triggers mentioned in the docblock.
313-336: LGTM!The test properly verifies that unique constraints are enforced across all database drivers, catching duplicates at the application level.
343-367: LGTM!The test effectively validates that database-specific optimizations (SQLite PRAGMA settings) and global PDO configuration (error mode) are properly applied during initialization.
60-83: No action required—the hardcoded'sqlite'adapter does not affect multi-database testing.While line 64 hardcodes the adapter type, the resulting PDO is immediately discarded when the actual test PDO (created with the correct driver via
TEST_DB_DRIVERenvironment variable) is injected at line 77. Since repositories only create their PDO connection during construction and do not re-read the settings afterward, the hardcoded adapter value has no functional impact. ORM-only repositories useModel::setPdo(), which is set separately at line 39 with the correct test PDO. The code works as intended across SQLite, MySQL, and PostgreSQL.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
tests/Unit/Cms/Services/Widget/CalendarWidgetTest.php (1)
107-115: Consider consolidating with the default behavior test.This test duplicates the scenario covered by
test_upcoming_defaults_to_true_when_not_specified(lines 27-35), as both pass an empty attributes array and verify the same default behavior.
🧹 Nitpick comments (13)
tests/Unit/Cms/Exceptions/SecurityExceptionTest.php (1)
1-44: Well-structured test suite with strong security focus.The test suite effectively validates the critical security properties of SecurityException:
- Internal messages include diagnostic details
- User messages are generic and safe
- Security violations are always logged
The security-first approach here is commendable.
Optional: Consider adding edge case tests
For completeness, you could optionally add tests for:
- Constructor's optional parameters (
codeandprevious)- Edge cases like empty reason strings or special characters
However, the current coverage is sufficient for the core functionality.
public function test_accepts_error_code(): void { $exception = new SecurityException('reason', 403); $this->assertEquals(403, $exception->getCode()); } public function test_accepts_previous_exception(): void { $previous = new \Exception('Previous'); $exception = new SecurityException('reason', 0, $previous); $this->assertSame($previous, $exception->getPrevious()); }tests/Unit/Cms/Services/Event/DeleterTest.php (1)
13-13: Add type hint for consistency.The
$eventRepositoryproperty lacks a type hint, while$deleteron Line 12 is typed. For consistency and better IDE support, consider adding the type hint.🔎 Proposed fix
- private $eventRepository; + private IEventRepository $eventRepository;tests/Unit/Cms/Exceptions/AuthorizationExceptionTest.php (1)
8-53: Consider testing optional constructor parameters.The test suite provides good coverage of the core functionality. For completeness, you could optionally add tests for the
$codeand$previousexception parameters to verify proper exception chaining.💡 Optional additional test examples
public function test_preserves_error_code(): void { $exception = new AuthorizationException('delete', 'post', 403); $this->assertEquals(403, $exception->getCode()); } public function test_preserves_previous_exception(): void { $previous = new \Exception('Original error'); $exception = new AuthorizationException('delete', 'post', 0, $previous); $this->assertSame($previous, $exception->getPrevious()); }tests/Unit/Cms/Exceptions/EntityNotFoundExceptionTest.php (1)
1-68: Excellent test coverage for EntityNotFoundException!The test suite is well-structured and comprehensively covers the exception's functionality:
- ✅ Verifies both technical and user-facing message formats
- ✅ Confirms security-conscious behavior (identifiers hidden from user messages)
- ✅ Tests multiple identifier types (numeric IDs, string slugs)
- ✅ Validates logging behavior
- ✅ All assertions align correctly with the implementation
The tests are clear, focused, and properly isolated.
Optional: Additional test coverage ideas
If you want to be even more comprehensive, consider adding tests for:
public function test_accepts_custom_error_code(): void { $exception = new EntityNotFoundException('User', 123, 'ID', 404); $this->assertEquals(404, $exception->getCode()); } public function test_accepts_previous_exception(): void { $previous = new \Exception('Database error'); $exception = new EntityNotFoundException('User', 123, 'ID', 0, $previous); $this->assertSame($previous, $exception->getPrevious()); } public function test_extends_cms_exception(): void { $exception = new EntityNotFoundException('User', 123); $this->assertInstanceOf(\Neuron\Cms\Exceptions\CmsException::class, $exception); }tests/Unit/Cms/Repositories/DatabaseEventCategoryRepositoryTest.php (2)
127-143: Consider using$id2in an assertion for test completeness.While
$id2is intentionally created to represent a category that should not be returned, it's currently unused. Adding an assertion verifies the filtering behavior explicitly.🔎 Optional improvement
$categories = $this->repository->findByIds( [ $id1, $id3 ] ); $this->assertCount( 2, $categories ); $this->assertEquals( $id1, $categories[0]->getId() ); $this->assertEquals( $id3, $categories[1]->getId() ); + // Verify $id2 was correctly excluded + $categoryIds = array_map( fn( $c ) => $c->getId(), $categories ); + $this->assertNotContains( $id2, $categoryIds );
225-240:sleep(1)slows test execution.While acceptable for ensuring timestamp changes, this adds latency to the test suite. Consider documenting why the sleep is necessary, or for future optimization, injecting a clock abstraction.
tests/Unit/Cms/Repositories/DatabaseEventRepositoryTest.php (2)
103-118: Consider using prepared statements in helper methods for consistency.While test code with controlled inputs poses minimal risk, using prepared statements aligns with production patterns and prevents accidental issues if helper inputs become dynamic.
🔎 Optional improvement
private function createTestCategory( string $name = 'Test Category' ): int { - $this->pdo->exec( "INSERT INTO event_categories (name, slug) VALUES ('{$name}', '" . strtolower( str_replace( ' ', '-', $name ) ) . "')" ); + $slug = strtolower( str_replace( ' ', '-', $name ) ); + $stmt = $this->pdo->prepare( "INSERT INTO event_categories (name, slug) VALUES (?, ?)" ); + $stmt->execute( [ $name, $slug ] ); return (int)$this->pdo->lastInsertId(); } private function createTestUser( string $username = 'testuser' ): int { $email = $username . '@test.com'; - $this->pdo->exec( "INSERT INTO users (username, email, password_hash, role, status, email_verified) - VALUES ('{$username}', '{$email}', 'hash', 'author', 'active', 1)" ); + $stmt = $this->pdo->prepare( "INSERT INTO users (username, email, password_hash, role, status, email_verified) + VALUES (?, ?, 'hash', 'author', 'active', 1)" ); + $stmt->execute( [ $username, $email ] ); return (int)$this->pdo->lastInsertId(); }
472-489:sleep(1)slows test execution.Same observation as in
DatabaseEventCategoryRepositoryTest- acceptable for timestamp verification but adds latency.tests/Unit/Cms/Models/EventModelTest.php (1)
12-138: Consider consolidating overlapping test coverage.Both
EventModelTestandEventTestcontain tests forsetCreatorandtoArraymethods. While the duplication may be intentional (e.g., for focused regression testing), consider whether these tests could be consolidated into a single comprehensive suite to reduce maintenance overhead.tests/Unit/Cms/Models/EventTest.php (1)
451-458: Test name could be more descriptive.The test
test_is_ongoing_uses_start_date_when_no_end_datevalidates the fallback behavior but doesn't clearly convey that it expectsfalsewhen the start date is in the past. Consider a name liketest_is_ongoing_returns_false_when_past_start_date_and_no_end_datefor clarity.tests/Unit/Cms/Services/Event/CreatorTest.php (2)
16-19: Consider adding type hints to mock properties for better IDE support.The mock properties (
$eventRepository,$categoryRepository,$random) lack type declarations. AddingMockObjecttype hints would improve IDE autocomplete and static analysis.🔎 Suggested improvement
private Creator $creator; - private $eventRepository; - private $categoryRepository; - private $random; + private MockObject $eventRepository; + private MockObject $categoryRepository; + private MockObject $random;Add the import at the top:
use PHPUnit\Framework\TestCase; +use PHPUnit\Framework\MockObject\MockObject; use DateTimeImmutable;
192-225: Consider verifying title preservation.The test correctly validates the fallback slug generation for non-ASCII titles. As an optional improvement, you could also assert that the original title '日本語イベント' is preserved in
$capturedEvent->getTitle()to ensure non-ASCII characters are stored correctly.🔎 Optional additional assertion
$this->assertEquals( 'event-abc123', $capturedEvent->getSlug() ); + $this->assertEquals( '日本語イベント', $capturedEvent->getTitle() ); }tests/Unit/Cms/Services/Event/UpdaterTest.php (1)
16-17: Consider adding type hints to mock properties for better IDE support.Similar to CreatorTest, the mock properties lack type declarations. Adding
MockObjecttype hints would improve IDE autocomplete and static analysis.🔎 Suggested improvement
private Updater $updater; - private $eventRepository; - private $categoryRepository; + private MockObject $eventRepository; + private MockObject $categoryRepository;Add the import at the top:
use PHPUnit\Framework\TestCase; +use PHPUnit\Framework\MockObject\MockObject; use DateTimeImmutable;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
tests/Unit/Cms/Exceptions/AuthorizationExceptionTest.phptests/Unit/Cms/Exceptions/CmsExceptionTest.phptests/Unit/Cms/Exceptions/DuplicateEntityExceptionTest.phptests/Unit/Cms/Exceptions/EntityNotFoundExceptionTest.phptests/Unit/Cms/Exceptions/RepositoryExceptionTest.phptests/Unit/Cms/Exceptions/SecurityExceptionTest.phptests/Unit/Cms/Exceptions/ValidationExceptionTest.phptests/Unit/Cms/Models/EventCategoryTest.phptests/Unit/Cms/Models/EventModelTest.phptests/Unit/Cms/Models/EventTest.phptests/Unit/Cms/Repositories/DatabaseEventCategoryRepositoryTest.phptests/Unit/Cms/Repositories/DatabaseEventRepositoryTest.phptests/Unit/Cms/Repositories/Traits/ManagesTimestampsTest.phptests/Unit/Cms/Services/Event/CreatorTest.phptests/Unit/Cms/Services/Event/DeleterTest.phptests/Unit/Cms/Services/Event/UpdaterTest.phptests/Unit/Cms/Services/EventCategory/CreatorTest.phptests/Unit/Cms/Services/EventCategory/DeleterTest.phptests/Unit/Cms/Services/EventCategory/UpdaterTest.phptests/Unit/Cms/Services/Widget/CalendarWidgetTest.php
✅ Files skipped from review due to trivial changes (2)
- tests/Unit/Cms/Models/EventCategoryTest.php
- tests/Unit/Cms/Exceptions/CmsExceptionTest.php
🧰 Additional context used
🧬 Code graph analysis (14)
tests/Unit/Cms/Repositories/DatabaseEventCategoryRepositoryTest.php (2)
src/Cms/Repositories/DatabaseEventCategoryRepository.php (1)
DatabaseEventCategoryRepository(19-178)src/Cms/Models/EventCategory.php (2)
getColor(89-92)setColor(97-101)
tests/Unit/Cms/Models/EventModelTest.php (1)
src/Cms/Models/Event.php (3)
setCreator(404-418)getCreatedBy(379-382)setStartDate(197-201)
tests/Unit/Cms/Services/Event/UpdaterTest.php (1)
src/Cms/Models/Event.php (9)
getStartDate(189-192)getLocation(172-175)getEndDate(206-209)isAllDay(223-226)getCategoryId(240-243)getOrganizer(328-331)getContactEmail(345-348)getContactPhone(362-365)setCategoryId(248-252)
tests/Unit/Cms/Services/Widget/CalendarWidgetTest.php (1)
src/Cms/Services/Widget/CalendarWidget.php (1)
CalendarWidget(15-145)
tests/Unit/Cms/Exceptions/ValidationExceptionTest.php (2)
src/Cms/Exceptions/ValidationException.php (1)
ValidationException(10-53)src/Cms/Exceptions/CmsException.php (1)
getUserMessage(36-39)
tests/Unit/Cms/Exceptions/SecurityExceptionTest.php (2)
src/Cms/Exceptions/SecurityException.php (1)
SecurityException(10-36)src/Cms/Exceptions/CmsException.php (1)
getUserMessage(36-39)
tests/Unit/Cms/Exceptions/DuplicateEntityExceptionTest.php (3)
src/Cms/Exceptions/DuplicateEntityException.php (1)
DuplicateEntityException(10-40)src/Cms/Exceptions/CmsException.php (1)
getUserMessage(36-39)tests/Unit/Cms/Exceptions/CmsExceptionTest.php (1)
test_includes_error_code(34-39)
tests/Unit/Cms/Models/EventTest.php (1)
src/Cms/Models/Event.php (25)
getLocation(172-175)setLocation(180-184)setStartDate(197-201)getStartDate(189-192)getEndDate(206-209)setEndDate(214-218)isAllDay(223-226)setAllDay(231-235)getCategoryId(240-243)setCategoryId(248-252)getCategory(257-260)setCategory(265-273)getOrganizer(328-331)setOrganizer(336-340)getContactEmail(345-348)setContactEmail(353-357)getContactPhone(362-365)setContactPhone(370-374)getCreatedBy(379-382)setCreatedBy(387-391)getCreator(396-399)setCreator(404-418)isUpcoming(483-487)isPast(492-497)isOngoing(502-507)
tests/Unit/Cms/Exceptions/EntityNotFoundExceptionTest.php (2)
src/Cms/Exceptions/EntityNotFoundException.php (1)
EntityNotFoundException(10-40)src/Cms/Exceptions/CmsException.php (1)
getUserMessage(36-39)
tests/Unit/Cms/Services/EventCategory/UpdaterTest.php (1)
src/Cms/Models/EventCategory.php (2)
setColor(97-101)getColor(89-92)
tests/Unit/Cms/Services/EventCategory/CreatorTest.php (1)
src/Cms/Models/EventCategory.php (1)
getColor(89-92)
tests/Unit/Cms/Exceptions/AuthorizationExceptionTest.php (2)
src/Cms/Exceptions/AuthorizationException.php (1)
AuthorizationException(10-41)src/Cms/Exceptions/CmsException.php (1)
getUserMessage(36-39)
tests/Unit/Cms/Repositories/Traits/ManagesTimestampsTest.php (2)
src/Cms/Exceptions/RepositoryException.php (1)
RepositoryException(10-45)src/Cms/Repositories/Traits/ManagesTimestamps.php (3)
ensureTimestamps(21-40)saveAndRefresh(55-85)createEntity(104-108)
tests/Unit/Cms/Services/EventCategory/DeleterTest.php (1)
tests/Unit/Cms/Services/Event/DeleterTest.php (5)
DeleterTest(10-65)setUp(15-20)test_delete_returns_true_on_success(22-36)test_delete_returns_false_on_failure(38-51)test_delete_calls_repository_delete(53-64)
🪛 PHPMD (2.15.0)
tests/Unit/Cms/Repositories/DatabaseEventCategoryRepositoryTest.php
133-133: Avoid unused local variables such as '$id2'. (undefined)
(UnusedLocalVariable)
tests/Unit/Cms/Repositories/Traits/ManagesTimestampsTest.php
108-108: Avoid unused parameters such as '$id'. (undefined)
(UnusedFormalParameter)
149-149: Avoid unused parameters such as '$id'. (undefined)
(UnusedFormalParameter)
163-163: Avoid unused parameters such as '$id'. (undefined)
(UnusedFormalParameter)
180-180: Avoid unused parameters such as '$id'. (undefined)
(UnusedFormalParameter)
196-196: Avoid unused parameters such as '$id'. (undefined)
(UnusedFormalParameter)
220-220: Avoid unused parameters such as '$id'. (undefined)
(UnusedFormalParameter)
250-250: Avoid unused parameters such as '$id'. (undefined)
(UnusedFormalParameter)
271-271: Avoid unused parameters such as '$id'. (undefined)
(UnusedFormalParameter)
295-295: Avoid unused parameters such as '$id'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (postgres)
🔇 Additional comments (55)
tests/Unit/Cms/Exceptions/SecurityExceptionTest.php (4)
10-18: LGTM! Proper validation of internal message formatting.The test correctly verifies that the internal exception message includes the security violation prefix, which is important for logging and debugging.
20-28: Excellent security practice: generic user messages.This test validates that the user-facing message is generic and doesn't expose internal security details. This is critical for preventing information disclosure to potential attackers.
30-36: Good defensive testing against information leakage.While this test overlaps with the generic message test, it provides valuable defense-in-depth by explicitly verifying that specific security-related terms don't leak into user messages. This would catch issues if the implementation were changed to dynamically construct user messages.
38-43: LGTM! Validates security violations are logged.Correctly verifies that security exceptions are always logged, which is essential for security monitoring and incident response.
tests/Unit/Cms/Exceptions/DuplicateEntityExceptionTest.php (1)
10-58: Excellent test coverage for DuplicateEntityException.The test suite comprehensively validates all aspects of the exception class:
- Message construction with entity type, field, and value
- Technical and user-facing message formats
- Field name capitalization in user messages
- Non-logging behavior
- Error code propagation
All assertions correctly match the implementation and test names are clear and descriptive.
tests/Unit/Cms/Services/Event/DeleterTest.php (3)
22-36: LGTM!The test correctly verifies that the
Deleter::delete()method returns true when the repository succeeds. The mock setup and assertions are appropriate.
38-51: LGTM!The test correctly verifies that the
Deleter::delete()method returns false when the repository fails. The mock setup and assertions are appropriate.
53-64: LGTM!The test correctly verifies that the repository's
deletemethod is invoked with the provided event instance. The use ofidenticalTo()ensures strict object identity checking.tests/Unit/Cms/Exceptions/RepositoryExceptionTest.php (6)
1-9: LGTM!The file structure, namespace, and imports are correct and follow PHPUnit testing conventions.
10-18: LGTM!The test correctly verifies that the exception constructs a message in the format "Failed to {operation} {entityType}" when no details are provided.
20-32: LGTM!The test correctly verifies that technical details are appended to the message with the expected format when provided.
34-42: LGTM!The test correctly verifies that the user-facing message remains generic and doesn't expose technical details to end users.
44-54: LGTM!The test correctly verifies that specific technical details (SQL keywords and line numbers) don't leak into user-facing messages. This is an important security practice.
56-61: LGTM!The test correctly verifies that repository exceptions are logged by default, which aligns with the documented behavior.
tests/Unit/Cms/Exceptions/AuthorizationExceptionTest.php (6)
1-9: LGTM! Clean test class structure.The namespace, imports, and class declaration are correctly set up for unit testing the AuthorizationException.
10-18: LGTM! Correctly tests message construction.The test properly verifies that both action and resource are included in the exception message.
20-28: LGTM! Correctly tests action-only scenario.The test properly verifies message construction when only the action parameter is provided.
30-38: LGTM! Verifies generic user-facing message.The test correctly confirms that the user message is generic and doesn't expose internal details.
40-45: LGTM! Important security verification.This test ensures that sensitive resource details are not leaked in user-facing messages—a critical security property for authorization exceptions.
47-52: LGTM! Verifies logging behavior.The test correctly confirms that authorization exceptions are always logged by default, which is important for security auditing.
tests/Unit/Cms/Exceptions/ValidationExceptionTest.php (2)
1-9: LGTM! Clean test structure.The imports, namespace, and class declaration follow PHPUnit conventions correctly.
10-66: Excellent test coverage for ValidationException.The test suite comprehensively covers all public methods and validates both single and multiple error scenarios. The assertions correctly verify:
- Error storage and retrieval via
getErrors()- Technical message format with "Validation failed" prefix
- User-friendly messages with appropriate prompts
- Non-logging default behavior
All test methods align perfectly with the implementation in
ValidationException.php.tests/Unit/Cms/Services/Widget/CalendarWidgetTest.php (2)
16-25: LGTM!Clean setup following PHPUnit best practices. Both repository dependencies are properly mocked for unit testing.
27-95: Excellent coverage of boolean parsing variations.The test suite comprehensively validates the
upcomingattribute parsing across multiple formats (boolean, string '0'/'1', string 'true'/'false', and default behavior), ensuring robust handling of user input.tests/Unit/Cms/Repositories/Traits/ManagesTimestampsTest.php (1)
14-24: Good approach for testing traits with anonymous classes.Using an anonymous class with trait aliasing to expose protected methods is an idiomatic PHPUnit pattern for trait testing.
tests/Unit/Cms/Services/EventCategory/DeleterTest.php (1)
10-65: Well-structured test suite for the Deleter service.The tests appropriately cover success, failure, and verification scenarios. The structure aligns with the existing
Event\DeleterTestpattern, ensuring consistency across the codebase.tests/Unit/Cms/Services/EventCategory/UpdaterTest.php (1)
10-205: Comprehensive test coverage for the Updater service.Tests thoroughly cover the update scenarios including slug uniqueness validation with ID exclusion, color changes, and description handling. The test structure is consistent with other service tests in the codebase.
tests/Unit/Cms/Repositories/DatabaseEventCategoryRepositoryTest.php (1)
20-53: Well-structured test setup with in-memory SQLite and reflection-based PDO injection.The setup pattern using in-memory SQLite with schema creation and reflection-based PDO injection is consistent with other repository tests (e.g.,
DatabaseEventRepositoryTest), ensuring isolation and reproducibility.tests/Unit/Cms/Services/EventCategory/CreatorTest.php (1)
10-251: Thorough test coverage for the Creator service.The tests comprehensively cover slug generation scenarios including custom slugs, non-ASCII name handling, duplicate detection, color defaults, and character normalization. The callback capture pattern for inspecting entities passed to the repository mock is well-implemented.
tests/Unit/Cms/Repositories/DatabaseEventRepositoryTest.php (3)
23-98: Comprehensive test setup matching the pattern from other repository tests.The setup correctly creates all required tables with foreign key relationships and uses reflection-based PDO injection, consistent with the
DatabaseEventCategoryRepositoryTestpattern shown in the relevant code snippets.
350-365: Good edge case: testing events that span the query date range.Testing spanning events ensures the
getByDateRangequery correctly handles events wherestart_date < range_startandend_date > range_end.
592-604: Good coverage of null relation handling.Testing that
getCategory()returnsnullwhen no category is assigned whilegetCreator()still works ensures the relation loading logic handles partial data correctly.tests/Unit/Cms/Models/EventModelTest.php (1)
1-139: LGTM! Well-structured tests for Event model behavior.The test suite provides focused coverage of
setCreatorandtoArraymethods with clear edge cases:
- Creator synchronization with
created_byfield in various states- Proper null handling for optional relationships
- Serialization behavior for date formatting and field inclusion/exclusion
The tests follow PHPUnit best practices with descriptive names and focused assertions.
tests/Unit/Cms/Models/EventTest.php (5)
14-152: LGTM! Comprehensive coverage of constructor and basic properties.The tests validate:
- Constructor initialization of
created_atasDateTimeImmutable- All basic getter/setter pairs with appropriate null handling
- Proper use of immutable date objects
61-119: LGTM! Thorough content handling validation.The tests properly validate Editor.js content handling including:
- Default empty blocks structure
- JSON string and array conversions
- Error handling for invalid UTF-8 with
JsonExceptionThe invalid UTF-8 test case at line 113 is a good edge case for ensuring robust error handling.
170-337: LGTM! Relationship handling is well-tested.The tests validate proper synchronization between:
categoryentity andcategory_idfieldcreator(User) entity andcreated_byfieldGood coverage of edge cases including null relationships and entities without IDs.
381-458: LGTM! Date status helpers are thoroughly tested.The tests validate
isUpcoming,isPast, andisOngoingmethods with various date scenarios including:
- Future and past events
- Events with and without end dates
- Ongoing events with proper boundary conditions
All assertions align with the expected behavior based on the Event model implementation.
460-624: LGTM! Comprehensive serialization/deserialization coverage.The
fromArrayandtoArraytests validate:
- Complete field mapping in both directions
- Date string parsing and formatting ('Y-m-d H:i:s')
- Relationship population (category, creator)
- Proper null handling for optional fields
- Conditional
idexclusion when nullExcellent coverage of serialization edge cases.
tests/Unit/Cms/Services/Event/CreatorTest.php (8)
21-32: LGTM!The setUp method correctly initializes all mocks and dependencies for the Creator service.
34-68: LGTM!The test properly validates basic event creation, including automatic slug generation from the title and all required field assignments.
70-100: LGTM!The test correctly verifies that custom slugs are used when provided instead of auto-generating from the title.
161-190: LGTM!The test effectively validates slug generation from titles with special characters, ensuring proper normalization.
227-252: LGTM!The test properly validates error handling when an invalid category ID is provided, ensuring appropriate exceptions are thrown.
254-273: LGTM!The test correctly validates that duplicate slugs are prevented, maintaining data integrity.
275-309: LGTM!The test correctly validates all-day event creation with the
isAllDayflag.
311-349: LGTM!The test properly validates event creation without a category, ensuring the category lookup is skipped and
categoryIdremains null.tests/Unit/Cms/Services/Event/UpdaterTest.php (9)
19-28: LGTM!The setUp method correctly initializes the Updater service with properly mocked dependencies.
30-60: LGTM!The test correctly validates that basic updates don't trigger slug validation when no new slug is provided, and the original slug is preserved.
62-89: LGTM!The test properly validates slug updates, including the correct exclusion of the current event ID when checking for duplicates.
91-146: Verify content field assertion.Similar to CreatorTest, this test provides a
contentparameter (line 124) but doesn't assert that it was set correctly. If the Event model supports content updates, consider adding$this->assertEquals('{"blocks":[{"type":"paragraph","data":{"text":"Updated content"}}]}', $event->getContent());for completeness.The verification script from CreatorTest applies here as well.
148-173: LGTM!The test correctly validates that slug uniqueness is enforced across different events, preventing updates with duplicate slugs.
175-206: LGTM!The test properly validates error handling when attempting to associate an event with a non-existent category.
208-235: LGTM!The test correctly validates that an event can retain its current slug during updates, with proper self-exclusion in the duplicate check.
237-268: LGTM!The test properly validates category removal, ensuring the category lookup is skipped and the association is cleared when null is provided.
270-291: LGTM!The test correctly validates status transitions, ensuring events can be updated from draft to published status.
| $this->expectException( RepositoryException::class ); | ||
| $this->expectExceptionMessage( 'Failed to save TestEntity' ); | ||
| $this->expectExceptionMessage( 'Entity ID is null' ); | ||
|
|
||
| $this->repository->saveAndRefresh( $entity, $finder, 'TestEntity' ); | ||
| } |
There was a problem hiding this comment.
Multiple expectExceptionMessage() calls - only the last one is effective.
PHPUnit only checks the last expectExceptionMessage() call. If you need to verify the message contains multiple substrings, use expectExceptionMessageMatches() with a regex pattern.
🔎 Proposed fix
$this->expectException( RepositoryException::class );
- $this->expectExceptionMessage( 'Failed to save TestEntity' );
- $this->expectExceptionMessage( 'Entity ID is null' );
+ $this->expectExceptionMessageMatches( '/Failed to save TestEntity.*Entity ID is null/s' );
$this->repository->saveAndRefresh( $entity, $finder, 'TestEntity' );🤖 Prompt for AI Agents
In tests/Unit/Cms/Repositories/Traits/ManagesTimestampsTest.php around lines 165
to 170, the test uses multiple expectExceptionMessage() calls but only the last
one is effective; replace the multiple calls with a single expectation that
matches both substrings (e.g. use expectExceptionMessageMatches() with a regex
that asserts both "Failed to save TestEntity" and "Entity ID is null" are
present) or combine into one explicit message string so the test verifies both
pieces of text in the thrown exception.
| $this->expectException( RepositoryException::class ); | ||
| $this->expectExceptionMessage( 'Failed to retrieve TestEntity' ); | ||
| $this->expectExceptionMessage( '123' ); | ||
|
|
||
| $this->repository->saveAndRefresh( $entity, $finder, 'TestEntity' ); | ||
| } |
There was a problem hiding this comment.
Same issue: multiple expectExceptionMessage() calls.
Apply the same fix pattern here.
🔎 Proposed fix
$this->expectException( RepositoryException::class );
- $this->expectExceptionMessage( 'Failed to retrieve TestEntity' );
- $this->expectExceptionMessage( '123' );
+ $this->expectExceptionMessageMatches( '/Failed to retrieve TestEntity.*123/' );
$this->repository->saveAndRefresh( $entity, $finder, 'TestEntity' );📝 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.
| $this->expectException( RepositoryException::class ); | |
| $this->expectExceptionMessage( 'Failed to retrieve TestEntity' ); | |
| $this->expectExceptionMessage( '123' ); | |
| $this->repository->saveAndRefresh( $entity, $finder, 'TestEntity' ); | |
| } | |
| $this->expectException( RepositoryException::class ); | |
| $this->expectExceptionMessageMatches( '/Failed to retrieve TestEntity.*123/' ); | |
| $this->repository->saveAndRefresh( $entity, $finder, 'TestEntity' ); | |
| } |
🤖 Prompt for AI Agents
In tests/Unit/Cms/Repositories/Traits/ManagesTimestampsTest.php around lines
182-187 there are two expectExceptionMessage() calls which is invalid; replace
them with a single expectation that matches both pieces of text (use
expectExceptionMessageMatches with a regex that ensures the message contains
"Failed to retrieve TestEntity" and "123"). Remove the two
expectExceptionMessage() calls and add one
expectExceptionMessageMatches('/Failed to retrieve TestEntity.*123/'); so the
test asserts both substrings in a single expectation.
| $this->expectException( RepositoryException::class ); | ||
| $this->expectExceptionMessage( 'Failed to save TestEntity' ); | ||
| $this->expectExceptionMessage( 'Entity ID is null' ); | ||
|
|
||
| $this->repository->createEntity( $entity, $finder, 'TestEntity' ); | ||
| } |
There was a problem hiding this comment.
Same issue: multiple expectExceptionMessage() calls in test_create_entity_throws_exception_when_id_is_null.
🔎 Proposed fix
$this->expectException( RepositoryException::class );
- $this->expectExceptionMessage( 'Failed to save TestEntity' );
- $this->expectExceptionMessage( 'Entity ID is null' );
+ $this->expectExceptionMessageMatches( '/Failed to save TestEntity.*Entity ID is null/s' );
$this->repository->createEntity( $entity, $finder, 'TestEntity' );📝 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.
| $this->expectException( RepositoryException::class ); | |
| $this->expectExceptionMessage( 'Failed to save TestEntity' ); | |
| $this->expectExceptionMessage( 'Entity ID is null' ); | |
| $this->repository->createEntity( $entity, $finder, 'TestEntity' ); | |
| } | |
| $this->expectException( RepositoryException::class ); | |
| $this->expectExceptionMessageMatches( '/Failed to save TestEntity.*Entity ID is null/s' ); | |
| $this->repository->createEntity( $entity, $finder, 'TestEntity' ); | |
| } |
🤖 Prompt for AI Agents
In tests/Unit/Cms/Repositories/Traits/ManagesTimestampsTest.php around lines
273–278, the test calls expectExceptionMessage() twice which the second call
overwrites the first; replace the two expectExceptionMessage() calls with a
single expectation that matches the actual exception message (either a single
expectExceptionMessage('Failed to save TestEntity: Entity ID is null') if the
exception message is combined, or use expectExceptionMessageMatches('/Failed to
save TestEntity.*Entity ID is null/') to assert both parts appear).
| $this->expectException( RepositoryException::class ); | ||
| $this->expectExceptionMessage( 'Failed to retrieve TestEntity' ); | ||
| $this->expectExceptionMessage( '456' ); | ||
|
|
||
| $this->repository->createEntity( $entity, $finder, 'TestEntity' ); | ||
| } |
There was a problem hiding this comment.
Same issue: multiple expectExceptionMessage() calls in test_create_entity_throws_exception_when_finder_returns_null.
🔎 Proposed fix
$this->expectException( RepositoryException::class );
- $this->expectExceptionMessage( 'Failed to retrieve TestEntity' );
- $this->expectExceptionMessage( '456' );
+ $this->expectExceptionMessageMatches( '/Failed to retrieve TestEntity.*456/' );
$this->repository->createEntity( $entity, $finder, 'TestEntity' );🤖 Prompt for AI Agents
In tests/Unit/Cms/Repositories/Traits/ManagesTimestampsTest.php around lines 297
to 302, the test uses two expectExceptionMessage() calls which only preserves
the last message; replace them with a single expectation that asserts both
pieces are present — e.g. remove the two expectExceptionMessage() calls and
instead call expectExceptionMessageMatches() with a regex that ensures "Failed
to retrieve TestEntity" and "456" both appear (for example '/Failed to retrieve
TestEntity.*456/'), then keep the expectException(RepositoryException::class)
and the call to $this->repository->createEntity(...).
| public function test_create_with_all_optional_fields(): void | ||
| { | ||
| $startDate = new DateTimeImmutable( '2025-06-15 10:00:00' ); | ||
| $endDate = new DateTimeImmutable( '2025-06-15 17:00:00' ); | ||
|
|
||
| $category = new EventCategory(); | ||
| $category->setId( 3 ); | ||
|
|
||
| $this->categoryRepository->expects( $this->once() ) | ||
| ->method( 'findById' ) | ||
| ->with( 3 ) | ||
| ->willReturn( $category ); | ||
|
|
||
| $this->eventRepository->expects( $this->once() ) | ||
| ->method( 'slugExists' ) | ||
| ->willReturn( false ); | ||
|
|
||
| $capturedEvent = null; | ||
| $this->eventRepository->expects( $this->once() ) | ||
| ->method( 'create' ) | ||
| ->with( $this->callback( function( Event $event ) use ( &$capturedEvent ) { | ||
| $capturedEvent = $event; | ||
| return true; | ||
| })) | ||
| ->willReturnCallback( function( Event $event ) { | ||
| $event->setId( 1 ); | ||
| return $event; | ||
| }); | ||
|
|
||
| $this->creator->create( | ||
| 'Tech Conference', | ||
| $startDate, | ||
| 5, | ||
| Event::STATUS_PUBLISHED, | ||
| 'tech-conf', | ||
| 'A great tech event', | ||
| '{"blocks":[{"type":"paragraph","data":{"text":"Hello"}}]}', | ||
| 'Convention Center', | ||
| $endDate, | ||
| false, | ||
| 3, | ||
| '/images/tech.jpg', | ||
| 'Tech Org', | ||
| 'info@tech.com', | ||
| '555-1234' | ||
| ); | ||
|
|
||
| $this->assertEquals( 'tech-conf', $capturedEvent->getSlug() ); | ||
| $this->assertEquals( 'A great tech event', $capturedEvent->getDescription() ); | ||
| $this->assertEquals( 'Convention Center', $capturedEvent->getLocation() ); | ||
| $this->assertEquals( $endDate, $capturedEvent->getEndDate() ); | ||
| $this->assertFalse( $capturedEvent->isAllDay() ); | ||
| $this->assertEquals( 3, $capturedEvent->getCategoryId() ); | ||
| $this->assertEquals( '/images/tech.jpg', $capturedEvent->getFeaturedImage() ); | ||
| $this->assertEquals( 'Tech Org', $capturedEvent->getOrganizer() ); | ||
| $this->assertEquals( 'info@tech.com', $capturedEvent->getContactEmail() ); | ||
| $this->assertEquals( '555-1234', $capturedEvent->getContactPhone() ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Event model file
find . -type f -name "*.php" | xargs grep -l "class Event" | grep -v test | grep -v vendor | head -5
# Find Creator service file
find . -type f -name "*.php" | xargs grep -l "class Creator" | grep -v test | grep -v vendor | head -5Repository: Neuron-PHP/cms
Length of output: 389
🏁 Script executed:
#!/bin/bash
# Check Event model for getContent method and content field
echo "=== Event Model ==="
cat ./src/Cms/Models/Event.php
echo -e "\n=== Event Creator Service ==="
cat ./src/Cms/Services/Event/Creator.phpRepository: Neuron-PHP/cms
Length of output: 16239
Add assertion for content field.
The test provides a content parameter with Editor.js JSON on line 138, but lacks an assertion to verify it was properly set on $capturedEvent. Add $this->assertEquals(['blocks' => [['type' => 'paragraph', 'data' => ['text' => 'Hello']]]], $capturedEvent->getContent()); to verify the content matches, maintaining consistency with other field assertions.
🤖 Prompt for AI Agents
In tests/Unit/Cms/Services/Event/CreatorTest.php around lines 102–159, the test
passes Editor.js JSON as the content parameter but does not assert it was set on
the created Event; add an assertion that verifies the parsed content equals the
expected array. Insert a line after the other field assertions (e.g., after the
description or slug assertions) that asserts $capturedEvent->getContent() equals
['blocks' => [['type' => 'paragraph', 'data' => ['text' => 'Hello']]]]; this
ensures the content field is validated like the other fields.
| <?php | ||
|
|
||
| namespace Tests\Unit\Cms\Services\Widget; | ||
|
|
||
| use Neuron\Cms\Services\Widget\CalendarWidget; | ||
| use Neuron\Cms\Repositories\DatabaseEventRepository; | ||
| use Neuron\Cms\Repositories\DatabaseEventCategoryRepository; | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| class CalendarWidgetTest extends TestCase | ||
| { | ||
| private $eventRepository; | ||
| private $categoryRepository; | ||
| private $widget; | ||
|
|
||
| protected function setUp(): void | ||
| { | ||
| $this->eventRepository = $this->createMock( DatabaseEventRepository::class ); | ||
| $this->categoryRepository = $this->createMock( DatabaseEventCategoryRepository::class ); | ||
|
|
||
| $this->widget = new CalendarWidget( | ||
| $this->eventRepository, | ||
| $this->categoryRepository | ||
| ); | ||
| } | ||
|
|
||
| public function test_upcoming_defaults_to_true_when_not_specified(): void | ||
| { | ||
| $this->eventRepository->expects( $this->once() ) | ||
| ->method( 'getUpcoming' ) | ||
| ->with( 5, 'published' ) | ||
| ->willReturn( [] ); | ||
|
|
||
| $this->widget->render( [] ); | ||
| } | ||
|
|
||
| public function test_upcoming_true_calls_get_upcoming(): void | ||
| { | ||
| $this->eventRepository->expects( $this->once() ) | ||
| ->method( 'getUpcoming' ) | ||
| ->with( 5, 'published' ) | ||
| ->willReturn( [] ); | ||
|
|
||
| $this->widget->render( ['upcoming' => true] ); | ||
| } | ||
|
|
||
| public function test_upcoming_false_string_calls_get_past(): void | ||
| { | ||
| $this->eventRepository->expects( $this->once() ) | ||
| ->method( 'getPast' ) | ||
| ->with( 5, 'published' ) | ||
| ->willReturn( [] ); | ||
|
|
||
| $this->widget->render( ['upcoming' => 'false'] ); | ||
| } | ||
|
|
||
| public function test_upcoming_false_boolean_calls_get_past(): void | ||
| { | ||
| $this->eventRepository->expects( $this->once() ) | ||
| ->method( 'getPast' ) | ||
| ->with( 5, 'published' ) | ||
| ->willReturn( [] ); | ||
|
|
||
| $this->widget->render( ['upcoming' => false] ); | ||
| } | ||
|
|
||
| public function test_upcoming_zero_string_calls_get_past(): void | ||
| { | ||
| $this->eventRepository->expects( $this->once() ) | ||
| ->method( 'getPast' ) | ||
| ->with( 5, 'published' ) | ||
| ->willReturn( [] ); | ||
|
|
||
| $this->widget->render( ['upcoming' => '0'] ); | ||
| } | ||
|
|
||
| public function test_upcoming_one_string_calls_get_upcoming(): void | ||
| { | ||
| $this->eventRepository->expects( $this->once() ) | ||
| ->method( 'getUpcoming' ) | ||
| ->with( 5, 'published' ) | ||
| ->willReturn( [] ); | ||
|
|
||
| $this->widget->render( ['upcoming' => '1'] ); | ||
| } | ||
|
|
||
| public function test_upcoming_true_string_calls_get_upcoming(): void | ||
| { | ||
| $this->eventRepository->expects( $this->once() ) | ||
| ->method( 'getUpcoming' ) | ||
| ->with( 5, 'published' ) | ||
| ->willReturn( [] ); | ||
|
|
||
| $this->widget->render( ['upcoming' => 'true'] ); | ||
| } | ||
|
|
||
| public function test_limit_is_respected(): void | ||
| { | ||
| $this->eventRepository->expects( $this->once() ) | ||
| ->method( 'getUpcoming' ) | ||
| ->with( 10, 'published' ) | ||
| ->willReturn( [] ); | ||
|
|
||
| $this->widget->render( ['limit' => 10] ); | ||
| } | ||
|
|
||
| public function test_limit_defaults_to_5(): void | ||
| { | ||
| $this->eventRepository->expects( $this->once() ) | ||
| ->method( 'getUpcoming' ) | ||
| ->with( 5, 'published' ) | ||
| ->willReturn( [] ); | ||
|
|
||
| $this->widget->render( [] ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test coverage for category filtering functionality.
The CalendarWidget implementation supports category filtering via the category attribute, but this functionality is not tested. Based on the implementation, the following scenarios should be covered:
-
Valid category: When a valid category slug is provided, verify that:
findBySlug()is called with the correct sluggetByCategory()is called with the category ID and 'published' status- The limit is properly applied to the filtered results
-
Missing category: When a category slug doesn't exist, verify that:
findBySlug()returns null- The render method returns an HTML comment indicating the missing category
-
Category with limit: Verify that the manual
array_slicelogic correctly limits category-filtered events
Do you want me to generate test methods for the category filtering scenarios?
🤖 Prompt for AI Agents
In tests/Unit/Cms/Services/Widget/CalendarWidgetTest.php around lines 1 to 116,
add three unit tests to cover category filtering: (1) Valid category: mock
categoryRepository->findBySlug to return a category object with an id, assert it
is called with the provided slug, mock eventRepository->getByCategory to be
called with that category id and 'published' status and return a list, call
widget->render(['category'=>'the-slug','limit'=>N]) and assert getByCategory was
invoked and the returned events respect the provided limit; (2) Missing
category: mock categoryRepository->findBySlug to return null, call
widget->render(['category'=>'missing-slug']) and assert the returned HTML is the
expected comment indicating the missing category; (3) Category with limit
slicing: mock findBySlug to return a category id, mock getByCategory to return
an array longer than the limit, call widget->render with a smaller 'limit' and
assert the final rendered output contains only the first N events (verifying the
array_slice/limit behavior). Ensure each test sets expectations on repository
method calls and uses the same patterns as existing tests for mocking and
willReturn values.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
Behavior
✏️ Tip: You can customize this high-level summary in your review settings.