Migrations #366
Migrations #366
Conversation
* .coderabbit.yaml This reverts commit 2246e49. --------- Co-authored-by: Tatevik <[email protected]>
WalkthroughThis PR adds DB migrations and an initial SQL dump, renames many ORM indexes/constraints to phplist_* prefixes, moves timestamp initialization to constructors/lifecycle hooks, refactors blacklist and administrator-token construction to constructor injection, introduces an OnlyOrmTablesFilter service, and updates CI/static-analysis configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Service / Test
participant Admin as Administrator
participant TokenNew as AdministratorToken (new)
Note over TokenNew: New flow (constructor injection)
Caller->>TokenNew: new AdministratorToken(Admin)
TokenNew->>TokenNew: generateExpiry() & generateKey() inside constructor
TokenNew-->>Caller: initialized token (associated admin)
sequenceDiagram
participant Service as SubscriberBlacklistService
participant BL as UserBlacklist
participant Manager as SubscriberBlacklistManager
participant Data as UserBlacklistData
Service->>BL: new UserBlacklist(email)
Service->>Manager: addBlacklistData(BL, name, data)
Manager->>Data: setBlacklist(BL)
Manager->>BL: persist BL (cascade where configured)
sequenceDiagram
participant EM as EntityManager
participant Metadata as Metadata
participant Filter as OnlyOrmTablesFilter
participant Asset as Asset (table/sequence)
EM->>Metadata: getAllMetadata()
Metadata-->>Filter: mapped table names & join tables
Asset->>Filter: invoke(asset)
Filter-->>Asset: allow/deny based on allowlist or sequence prefixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Areas needing extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/Domain/Analytics/Model/UserStats.php (2)
32-78: AlignlistIdtyping with the new nullable column.
Line 32 makeslistidnullable, but the property, getter, and setter still enforce non-nullableint. If Doctrine hydrates a row wherelistidisNULL, it will now throw aTypeErrorwhen assigning to the typed property or returning fromgetListId(). Please make the property and its accessors nullable (or revert the column change) to keep Doctrine hydration safe.- #[ORM\Column(name: 'listid', type: 'integer', nullable: true, options: ['default' => 0])] - private int $listId = 0; + #[ORM\Column(name: 'listid', type: 'integer', nullable: true, options: ['default' => 0])] + private ?int $listId = null; - public function getListId(): int + public function getListId(): ?int { return $this->listId; } - public function setListId(int $listId): self + public function setListId(?int $listId): self { $this->listId = $listId; return $this; }
35-85: Makevalueaccessors nullable-safe.
valueis now declared nullable in the ORM metadata (Line 35), the property is?int, yetgetValue()still promisesintandsetValue()forbidsnull. ReturningnullfromgetValue(): intwill crash immediately, and callers cannot clear the column. Please update the method signatures to accept/return?intto match the mapping.- #[ORM\Column(name: 'value', type: 'integer', nullable: true, options: ['default' => 0])] - private ?int $value = null; - - public function getValue(): int + #[ORM\Column(name: 'value', type: 'integer', nullable: true, options: ['default' => 0])] + private ?int $value = null; + + public function getValue(): ?int { return $this->value; } @@ - public function setValue(int $value): self + public function setValue(?int $value): self { $this->value = $value; return $this; }src/Domain/Subscription/Model/SubscriberList.php (1)
120-130: Fixdescriptionnullability mismatch.
setDescription(?string $description)now allowsnull, but the property is still typedstringand the column mapping remains non-nullable. CallingsetDescription(null)(which the signature invites) will immediately throw aTypeError. Either keep the API non-nullable or make the column/property truly nullable.- #[ORM\Column] - private string $description = ''; + #[ORM\Column(nullable: true)] + private ?string $description = null; - public function getDescription(): ?string + public function getDescription(): ?string { return $this->description; } - public function setDescription(?string $description): self + public function setDescription(?string $description): self { $this->description = $description;src/Domain/Subscription/Model/Subscriber.php (2)
60-202: Align uniqueId nullability with entity type.Line 60 marks the
uniqidcolumn as nullable, but the backing property and accessor signatures still use the non-nullablestringtype. Doctrine will now hydratenullfor legacy rows (the repository explicitly queries foruniqueId IS NULL) and PHP will throw aTypeErrorbecause the typed property disallows null. Please either keep the column non-nullable or, if nulls must be allowed, update the property and its accessor pairings to accept?stringso Doctrine can hydrate safely.- #[ORM\Column(name: 'uniqid', type: 'string', length: 255, nullable: true)] - private string $uniqueId = ''; + #[ORM\Column(name: 'uniqid', type: 'string', length: 255, nullable: true)] + private ?string $uniqueId = null; @@ - public function getUniqueId(): string + public function getUniqueId(): ?string { return $this->uniqueId; } - public function setUniqueId(string $uniqueId): self + public function setUniqueId(?string $uniqueId): self { $this->uniqueId = $uniqueId;
240-247: Fix extraData accessors to handle null.Line 240 returns
$this->extraData, but the property is declared?stringand the column is now nullable. When Doctrine hydrates a row whereextradataisNULL, returningnullviolates thestringreturn type and triggers a runtimeTypeError. Adjust the getter (and setter) to work with nullable values.- public function getExtraData(): string + public function getExtraData(): ?string { return $this->extraData; } - public function setExtraData(string $extraData): self + public function setExtraData(?string $extraData): self { $this->extraData = $extraData; return $this;
🧹 Nitpick comments (3)
src/Domain/Analytics/Model/UserMessageView.php (1)
15-17: Consider aligning index naming convention with other analytics models.The index names use
idxsuffix and abbreviations (msgidx,useridx,usermsgidx) that differ from the naming pattern used in related models. For instance, LinkTrack, LinkTrackUserClick, and LinkTrackMl consistently useindexsuffix withmidanduidabbreviations (e.g.,phplist_linktrack_midindex,phplist_linktrack_uidindex).For consistency across the analytics domain, consider:
-#[ORM\Index(name: 'phplist_user_message_view_msgidx', columns: ['messageid'])] -#[ORM\Index(name: 'phplist_user_message_view_useridx', columns: ['userid'])] -#[ORM\Index(name: 'phplist_user_message_view_usermsgidx', columns: ['userid', 'messageid'])] +#[ORM\Index(name: 'phplist_user_message_view_midindex', columns: ['messageid'])] +#[ORM\Index(name: 'phplist_user_message_view_uidindex', columns: ['userid'])] +#[ORM\Index(name: 'phplist_user_message_view_uidmidindex', columns: ['userid', 'messageid'])]src/Domain/Messaging/Model/Message/MessageMetadata.php (1)
19-19: Redundant initialization: property already set in constructor.The
$processedproperty is already initialized tofalsein the constructor (line 44) and the ORM column has'default' => falseoption. While this additional property-level default is not harmful, it creates triple redundancy.Consider whether all three initialization points are necessary, or if the constructor assignment and ORM default are sufficient.
.coderabbit.yaml (1)
1-9: LGTM! Configuration is valid and well-structured.The CodeRabbit configuration follows the schema correctly. The "chill" profile balances thoroughness with practicality.
Consider whether
base_branches: [".*"](line 7-8) is appropriate for your workflow. This pattern matches all branches, which means every PR to any branch will be auto-reviewed. If you primarily want reviews for PRs to specific branches (e.g.,main,dev,staging), you might want to explicitly list them instead:base_branches: - "main" - "dev"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (67)
.coderabbit.yaml(1 hunks).github/workflows/ci.yml(1 hunks)config/PHPMD/rules.xml(1 hunks)config/doctrine.yml(1 hunks)config/doctrine_migrations.yml(1 hunks)config/services.yml(1 hunks)src/Bounce/Service/SubscriberBlacklistService.php(2 hunks)src/Core/Doctrine/OnlyOrmTablesFilter.php(1 hunks)src/Domain/Analytics/Model/LinkTrack.php(3 hunks)src/Domain/Analytics/Model/LinkTrackForward.php(1 hunks)src/Domain/Analytics/Model/LinkTrackMl.php(1 hunks)src/Domain/Analytics/Model/LinkTrackUmlClick.php(1 hunks)src/Domain/Analytics/Model/LinkTrackUserClick.php(1 hunks)src/Domain/Analytics/Model/UserMessageView.php(1 hunks)src/Domain/Analytics/Model/UserStats.php(2 hunks)src/Domain/Common/Model/Interfaces/ModificationDate.php(0 hunks)src/Domain/Configuration/Model/EventLog.php(3 hunks)src/Domain/Configuration/Model/I18n.php(1 hunks)src/Domain/Configuration/Model/UrlCache.php(2 hunks)src/Domain/Identity/Command/CleanUpOldSessionTokens.php(0 hunks)src/Domain/Identity/Model/AdminLogin.php(1 hunks)src/Domain/Identity/Model/AdminPasswordRequest.php(0 hunks)src/Domain/Identity/Model/Administrator.php(5 hunks)src/Domain/Identity/Model/AdministratorToken.php(2 hunks)src/Domain/Identity/Service/SessionManager.php(1 hunks)src/Domain/Messaging/Command/ProcessQueueCommand.php(0 hunks)src/Domain/Messaging/Model/Bounce.php(2 hunks)src/Domain/Messaging/Model/BounceRegex.php(1 hunks)src/Domain/Messaging/Model/ListMessage.php(3 hunks)src/Domain/Messaging/Model/Message.php(4 hunks)src/Domain/Messaging/Model/Message/MessageFormat.php(1 hunks)src/Domain/Messaging/Model/Message/MessageMetadata.php(1 hunks)src/Domain/Messaging/Model/MessageAttachment.php(1 hunks)src/Domain/Messaging/Model/SendProcess.php(1 hunks)src/Domain/Messaging/Model/Template.php(1 hunks)src/Domain/Messaging/Model/TemplateImage.php(1 hunks)src/Domain/Messaging/Model/UserMessage.php(1 hunks)src/Domain/Messaging/Model/UserMessageBounce.php(1 hunks)src/Domain/Messaging/Model/UserMessageForward.php(1 hunks)src/Domain/Messaging/Service/Manager/ListMessageManager.php(0 hunks)src/Domain/Subscription/Model/Subscriber.php(7 hunks)src/Domain/Subscription/Model/SubscriberAttributeDefinition.php(1 hunks)src/Domain/Subscription/Model/SubscriberAttributeValue.php(1 hunks)src/Domain/Subscription/Model/SubscriberHistory.php(1 hunks)src/Domain/Subscription/Model/SubscriberList.php(6 hunks)src/Domain/Subscription/Model/Subscription.php(2 hunks)src/Domain/Subscription/Model/UserBlacklist.php(3 hunks)src/Domain/Subscription/Model/UserBlacklistData.php(1 hunks)src/Domain/Subscription/Service/Manager/SubscriberBlacklistManager.php(2 hunks)src/Migrations/Version20251028092901MySql.php(1 hunks)src/Migrations/Version20251029105320MySqlRelations.php(1 hunks)src/Migrations/Version20251030081120PostgreSqlPlatform.php(1 hunks)src/Migrations/Version20251030083621MySqlRenameIndex.php(1 hunks)src/Migrations/_template_migration.php.tpl(1 hunks)src/Migrations/initial_schema.sql(1 hunks)src/Routing/ExtraLoader.php(0 hunks)tests/Integration/Domain/Identity/Fixtures/AdministratorTokenWithAdministratorFixture.php(1 hunks)tests/Integration/Domain/Identity/Fixtures/DetachedAdministratorTokenFixture.php(3 hunks)tests/Integration/Domain/Identity/Repository/AdministratorRepositoryTest.php(3 hunks)tests/Integration/Domain/Identity/Repository/AdministratorTokenRepositoryTest.php(1 hunks)tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php(2 hunks)tests/Integration/Domain/Subscription/Fixtures/SubscriberListFixture.php(1 hunks)tests/Integration/Domain/Subscription/Service/SubscriberDeletionServiceTest.php(1 hunks)tests/Unit/Domain/Identity/Model/AdministratorTest.php(2 hunks)tests/Unit/Domain/Identity/Model/AdministratorTokenTest.php(2 hunks)tests/Unit/Domain/Subscription/Model/SubscriberTest.php(1 hunks)tests/Unit/Security/AuthenticationTest.php(2 hunks)
💤 Files with no reviewable changes (6)
- src/Domain/Common/Model/Interfaces/ModificationDate.php
- src/Domain/Identity/Command/CleanUpOldSessionTokens.php
- src/Domain/Identity/Model/AdminPasswordRequest.php
- src/Domain/Messaging/Service/Manager/ListMessageManager.php
- src/Routing/ExtraLoader.php
- src/Domain/Messaging/Command/ProcessQueueCommand.php
🧰 Additional context used
🧬 Code graph analysis (47)
src/Domain/Messaging/Model/TemplateImage.php (2)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/UserMessageView.php (1)
ORM(13-99)
src/Bounce/Service/SubscriberBlacklistService.php (2)
src/Domain/Subscription/Service/Manager/SubscriberBlacklistManager.php (1)
addEmailToBlacklist(35-54)src/Domain/Subscription/Model/UserBlacklist.php (1)
getEmail(33-36)
src/Domain/Analytics/Model/LinkTrackMl.php (3)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/LinkTrackForward.php (1)
ORM(12-86)src/Domain/Analytics/Model/LinkTrackUmlClick.php (1)
ORM(13-142)
src/Domain/Messaging/Model/Template.php (4)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/UserStats.php (1)
ORM(12-86)src/Domain/Configuration/Model/I18n.php (1)
ORM(16-67)src/Domain/Messaging/Model/BounceRegex.php (1)
ORM(12-158)
src/Domain/Subscription/Model/SubscriberHistory.php (10)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/LinkTrackForward.php (1)
ORM(12-86)src/Domain/Analytics/Model/LinkTrackMl.php (1)
ORM(12-131)src/Domain/Analytics/Model/LinkTrackUmlClick.php (1)
ORM(13-142)src/Domain/Analytics/Model/LinkTrackUserClick.php (1)
ORM(12-107)src/Domain/Analytics/Model/UserMessageView.php (1)
ORM(13-99)src/Domain/Analytics/Model/UserStats.php (1)
ORM(12-86)src/Domain/Configuration/Model/EventLog.php (2)
ORM(14-79)ORM(40-45)src/Domain/Configuration/Model/UrlCache.php (2)
ORM(13-87)ORM(75-80)src/Domain/Messaging/Model/Bounce.php (1)
ORM(13-112)
tests/Unit/Domain/Identity/Model/AdministratorTest.php (3)
src/Domain/Identity/Model/Administrator.php (4)
setLoginName(95-99)getUpdatedAt(85-88)setEmail(106-110)getPasswordHash(123-126)src/Domain/Common/Model/Interfaces/ModificationDate.php (1)
getUpdatedAt(22-22)src/TestingSupport/Traits/SimilarDatesAssertionTrait.php (1)
assertSimilarDates(24-31)
src/Domain/Messaging/Model/BounceRegex.php (2)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/LinkTrackForward.php (1)
ORM(12-86)
src/Domain/Analytics/Model/LinkTrackForward.php (1)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)
src/Domain/Messaging/Model/UserMessage.php (3)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/UserMessageView.php (1)
ORM(13-99)src/Domain/Configuration/Model/EventLog.php (2)
ORM(14-79)ORM(40-45)
src/Migrations/Version20251028092901MySql.php (2)
src/Migrations/Version20251030081120PostgreSqlPlatform.php (2)
getDescription(16-19)up(21-182)src/Migrations/Version20251029105320MySqlRelations.php (2)
getDescription(16-19)up(21-105)
src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (2)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/LinkTrackForward.php (1)
ORM(12-86)
src/Domain/Identity/Model/AdministratorToken.php (2)
src/Domain/Identity/Model/Administrator.php (1)
__construct(68-73)src/Domain/Identity/Model/AdminLogin.php (2)
__construct(43-55)getAdministrator(73-76)
src/Domain/Configuration/Model/I18n.php (3)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/UserStats.php (1)
ORM(12-86)src/Domain/Messaging/Model/BounceRegex.php (1)
ORM(12-158)
src/Domain/Messaging/Model/UserMessageBounce.php (4)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Configuration/Model/EventLog.php (2)
ORM(14-79)ORM(40-45)src/Domain/Configuration/Model/UrlCache.php (2)
ORM(13-87)ORM(75-80)src/Domain/Subscription/Model/Subscriber.php (3)
ORM(24-378)ORM(136-140)ORM(197-202)
tests/Unit/Domain/Identity/Model/AdministratorTokenTest.php (2)
src/Domain/Identity/Model/Administrator.php (1)
setLoginName(95-99)src/Domain/Identity/Model/AdministratorToken.php (1)
getAdministrator(117-120)
src/Migrations/Version20251030083621MySqlRenameIndex.php (1)
src/Migrations/Version20251030081120PostgreSqlPlatform.php (1)
up(21-182)
src/Domain/Analytics/Model/UserMessageView.php (3)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/LinkTrackMl.php (1)
ORM(12-131)src/Domain/Analytics/Model/LinkTrackUserClick.php (1)
ORM(12-107)
src/Migrations/Version20251029105320MySqlRelations.php (1)
src/Migrations/Version20251030081120PostgreSqlPlatform.php (2)
getDescription(16-19)up(21-182)
src/Domain/Subscription/Model/UserBlacklist.php (2)
src/Domain/Subscription/Model/UserBlacklistData.php (3)
ORM(11-65)getBlacklist(28-31)setBlacklist(33-37)src/Domain/Subscription/Service/Manager/SubscriberBlacklistManager.php (1)
__construct(17-23)
tests/Integration/Domain/Identity/Fixtures/DetachedAdministratorTokenFixture.php (2)
src/Domain/Identity/Model/Administrator.php (1)
setLoginName(95-99)src/TestingSupport/Traits/ModelTestTrait.php (1)
setSubjectId(25-28)
src/Domain/Messaging/Model/ListMessage.php (5)
src/Domain/Configuration/Model/EventLog.php (2)
ORM(14-79)ORM(40-45)src/Domain/Identity/Model/Administrator.php (3)
ORM(24-217)ORM(212-216)__construct(68-73)src/Domain/Messaging/Model/Message.php (3)
ORM(24-197)ORM(99-103)__construct(67-87)src/Domain/Subscription/Model/Subscriber.php (3)
ORM(24-378)ORM(136-140)__construct(112-119)src/Domain/Subscription/Model/SubscriberList.php (3)
ORM(27-243)ORM(232-237)__construct(82-91)
src/Domain/Analytics/Model/LinkTrackUmlClick.php (2)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/LinkTrackMl.php (1)
ORM(12-131)
src/Domain/Identity/Model/AdminLogin.php (1)
src/Domain/Identity/Model/Administrator.php (2)
ORM(24-217)ORM(212-216)
src/Domain/Subscription/Model/Subscription.php (4)
src/Domain/Configuration/Model/UrlCache.php (2)
ORM(13-87)ORM(75-80)src/Domain/Identity/Model/Administrator.php (2)
ORM(24-217)ORM(212-216)src/Domain/Messaging/Model/ListMessage.php (2)
ORM(16-90)ORM(85-89)src/Domain/Subscription/Model/Subscriber.php (2)
ORM(24-378)ORM(136-140)
src/Domain/Subscription/Service/Manager/SubscriberBlacklistManager.php (2)
src/Domain/Common/Repository/AbstractRepository.php (1)
persist(34-37)src/Domain/Subscription/Model/UserBlacklistData.php (1)
setBlacklist(33-37)
src/Domain/Subscription/Model/SubscriberList.php (4)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Messaging/Model/ListMessage.php (2)
ORM(16-90)ORM(85-89)src/Domain/Messaging/Model/Message.php (2)
ORM(24-197)ORM(99-103)src/Domain/Subscription/Model/Subscriber.php (3)
ORM(24-378)ORM(136-140)ORM(197-202)
src/Domain/Subscription/Model/SubscriberAttributeValue.php (2)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Subscription/Model/Subscriber.php (3)
ORM(24-378)ORM(136-140)ORM(197-202)
src/Domain/Analytics/Model/LinkTrack.php (6)
src/Domain/Configuration/Model/UrlCache.php (2)
ORM(13-87)ORM(75-80)src/Domain/Identity/Model/Administrator.php (3)
ORM(24-217)ORM(212-216)__construct(68-73)src/Domain/Messaging/Model/ListMessage.php (3)
ORM(16-90)ORM(85-89)__construct(42-46)src/Domain/Messaging/Model/Message.php (3)
ORM(24-197)ORM(99-103)__construct(67-87)src/Domain/Subscription/Model/Subscriber.php (4)
ORM(24-378)ORM(136-140)ORM(197-202)__construct(112-119)src/Domain/Messaging/Model/UserMessage.php (1)
__construct(42-47)
src/Domain/Messaging/Model/Message.php (2)
src/Domain/Identity/Model/Administrator.php (4)
ORM(24-217)ORM(212-216)getId(75-78)getUpdatedAt(85-88)src/Domain/Messaging/Model/Message/MessageMetadata.php (1)
setEntered(113-117)
tests/Unit/Domain/Subscription/Model/SubscriberTest.php (2)
src/Domain/Subscription/Model/Subscriber.php (1)
getUpdatedAt(131-134)src/Domain/Subscription/Model/SubscriberList.php (1)
getUpdatedAt(227-230)
src/Domain/Subscription/Model/UserBlacklistData.php (2)
src/Domain/Subscription/Model/UserBlacklist.php (2)
ORM(12-62)getEmail(33-36)src/Bounce/Service/SubscriberBlacklistService.php (1)
blacklist(39-74)
src/Domain/Messaging/Model/UserMessageForward.php (3)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/UserMessageView.php (1)
ORM(13-99)src/Domain/Messaging/Model/UserMessageBounce.php (1)
ORM(13-86)
src/Domain/Messaging/Model/Bounce.php (4)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/UserStats.php (1)
ORM(12-86)src/Domain/Configuration/Model/EventLog.php (2)
ORM(14-79)ORM(40-45)src/Domain/Messaging/Model/UserMessageBounce.php (1)
ORM(13-86)
tests/Integration/Domain/Subscription/Service/SubscriberDeletionServiceTest.php (1)
src/Domain/Identity/Model/Administrator.php (1)
setLoginName(95-99)
src/Domain/Subscription/Model/Subscriber.php (2)
src/Domain/Identity/Model/Administrator.php (4)
ORM(24-217)ORM(212-216)__construct(68-73)getUpdatedAt(85-88)src/Domain/Subscription/Repository/SubscriberRepository.php (1)
SubscriberRepository(20-202)
src/Domain/Configuration/Model/UrlCache.php (5)
src/Domain/Configuration/Model/EventLog.php (2)
ORM(14-79)ORM(40-45)src/Domain/Identity/Model/Administrator.php (2)
ORM(24-217)ORM(212-216)src/Domain/Messaging/Model/ListMessage.php (2)
ORM(16-90)ORM(85-89)src/Domain/Messaging/Model/Message.php (2)
ORM(24-197)ORM(99-103)src/Domain/Subscription/Model/Subscriber.php (2)
ORM(24-378)ORM(136-140)
src/Domain/Messaging/Model/MessageAttachment.php (2)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/UserMessageView.php (1)
ORM(13-99)
src/Domain/Messaging/Model/Message/MessageFormat.php (2)
src/Domain/Messaging/Model/Message.php (2)
ORM(24-197)ORM(99-103)src/Domain/Subscription/Model/Subscriber.php (2)
ORM(24-378)ORM(136-140)
tests/Integration/Domain/Identity/Fixtures/AdministratorTokenWithAdministratorFixture.php (3)
src/Domain/Identity/Model/Administrator.php (1)
setLoginName(95-99)src/TestingSupport/Traits/ModelTestTrait.php (1)
setSubjectId(25-28)src/Domain/Common/Repository/AbstractRepository.php (1)
persist(34-37)
src/Migrations/Version20251030081120PostgreSqlPlatform.php (3)
src/Migrations/Version20251028092901MySql.php (1)
up(21-33)src/Migrations/Version20251029105320MySqlRelations.php (2)
up(21-105)down(107-191)src/Migrations/Version20251030083621MySqlRenameIndex.php (2)
up(20-102)down(104-186)
src/Domain/Configuration/Model/EventLog.php (2)
src/Domain/Configuration/Model/UrlCache.php (2)
ORM(13-87)ORM(75-80)src/Domain/Subscription/Model/Subscriber.php (3)
ORM(24-378)ORM(136-140)ORM(197-202)
tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php (1)
src/Domain/Identity/Model/Administrator.php (1)
setLoginName(95-99)
src/Domain/Analytics/Model/LinkTrackUserClick.php (3)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)src/Domain/Analytics/Model/LinkTrackMl.php (1)
ORM(12-131)src/Domain/Analytics/Model/LinkTrackUmlClick.php (1)
ORM(13-142)
src/Domain/Identity/Model/Administrator.php (3)
src/Domain/Messaging/Model/Message.php (5)
ORM(24-197)ORM(99-103)__construct(67-87)getId(89-92)getUpdatedAt(94-97)src/Domain/Subscription/Model/Subscriber.php (5)
ORM(24-378)__construct(112-119)getId(121-124)getCreatedAt(126-129)getUpdatedAt(131-134)src/Domain/Identity/Model/Privileges.php (3)
Privileges(10-79)grant(61-66)revoke(68-73)
tests/Integration/Domain/Identity/Repository/AdministratorRepositoryTest.php (1)
src/Domain/Identity/Model/Administrator.php (1)
setLoginName(95-99)
tests/Integration/Domain/Subscription/Fixtures/SubscriberListFixture.php (1)
src/Domain/Identity/Model/Administrator.php (1)
setLoginName(95-99)
src/Domain/Analytics/Model/UserStats.php (1)
src/Domain/Analytics/Model/LinkTrack.php (1)
ORM(14-135)
🔇 Additional comments (43)
src/Domain/Configuration/Model/I18n.php (1)
18-19: Constraint and index names follow the prefixed naming convention.The renamed constraints are consistent with the PR objective to rename indexes to avoid duplicate names, and the new names follow the fully qualified
phplist_<entity>_<name>pattern established across the codebase (as seen in LinkTrack, BounceRegex, and UserStats).src/Domain/Messaging/Model/BounceRegex.php (1)
14-14: LGTM! Consistent naming convention.The unique constraint name has been updated from the generic "regex" to the fully-qualified "phplist_bounceregex_regex", which follows the naming pattern
phplist_{tablename}_{description}consistently used across other entities in this PR (e.g., LinkTrack, LinkTrackForward). This prevents potential naming conflicts across tables.src/Domain/Messaging/Model/UserMessageBounce.php (1)
15-18: LGTM! Index renaming is consistent and correct.The index names have been properly renamed to include the full table name prefix (
phplist_user_message_bounce_), which aligns with the PR objective to avoid duplicate index name issues across different database versions. The naming pattern is consistent with other entities in the codebase, as shown in the relevant snippets from LinkTrack, EventLog, and other domain models.src/Domain/Identity/Model/AdminLogin.php (1)
40-40: LGTM! Explicit non-nullable constraint improves schema clarity.Making the nullable constraint explicit in the ORM annotation aligns with the property's strict boolean type and default value, improving database schema documentation.
config/doctrine.yml (1)
14-14: LGTM! Savepoints enable robust nested transaction handling.Enabling savepoints improves transaction management during migrations and testing, allowing partial rollbacks within transactions on supported databases (PostgreSQL, MySQL InnoDB).
src/Domain/Messaging/Model/MessageAttachment.php (1)
13-14: LGTM! Index naming standardization prevents potential collisions.The prefixed index names (
phplist_message_attachment_*) align with the broader naming convention applied across the codebase, reducing risk of index name conflicts in shared database environments.src/Domain/Messaging/Model/Bounce.php (1)
15-16: LGTM! Index naming standardization aligns with codebase patterns.The prefixed index names follow the established
phplist_<entity>_<index>convention applied across the messaging domain.src/Domain/Subscription/Model/SubscriberHistory.php (1)
15-16: LGTM! Index naming follows established conventions.The prefixed index names align with the standardized naming pattern across the subscription domain, maintaining consistency with the broader migration effort.
config/PHPMD/rules.xml (2)
7-7: LGTM! Excluding migrations from PHPMD analysis reduces noise.Migrations often contain auto-generated or pattern-based code that doesn't benefit from PHPMD analysis. This exclusion is appropriate and aligns with similar exclusions in CI workflows.
73-76: Note: UnusedFormalParameter rule was removed from the ruleset.According to the AI summary, the
UnusedFormalParameterrule was removed from the UnusedCode rules. While this may reduce false positives (e.g., in interface implementations or callbacks), it means unused parameters will no longer be flagged by PHPMD.Verify that this removal aligns with your code quality goals. If the rule was causing too many false positives, consider whether there are specific contexts where it should still apply, or whether other tools (like PHPStan) can catch genuinely unused parameters.
.github/workflows/ci.yml (1)
78-78: LGTM! Excluding migrations from code style checks.Excluding the
Migrationsdirectories from PHP_CodeSniffer is appropriate since migration files often follow different conventions or may be auto-generated.src/Domain/Messaging/Model/Template.php (1)
16-16: LGTM! Constraint name updated to match naming convention.The unique constraint name has been updated to follow the fully-qualified, prefixed naming pattern (
phplist_template_title) consistent with the broader refactoring across other entities in this PR.src/Domain/Subscription/Model/SubscriberAttributeValue.php (1)
13-15: LGTM! Index names standardized with table prefix.The index names have been updated to use the fully-qualified naming pattern, consistent with the systematic refactoring across the codebase. The indexed columns remain unchanged, so this is purely a metadata update.
src/Domain/Messaging/Model/SendProcess.php (1)
51-54: LGTM! Lifecycle callback properly returns void.The
updateUpdatedAt()method now correctly returnsvoidinstead ofDomainModel. Doctrine lifecycle callbacks should not return values, and this change aligns with best practices. Since this is a lifecycle hook (triggered automatically by Doctrine), it should never be called manually or used in method chaining.src/Domain/Configuration/Model/EventLog.php (2)
16-17: LGTM! Index names standardized.The index names have been updated to use the
phplist_eventlog_prefix, consistent with the naming convention applied across the codebase.
18-45: LGTM! Lifecycle callback added for automatic timestamp initialization.The addition of the
#[ORM\HasLifecycleCallbacks]annotation and thesetCreatedTimestamps()method properly initializes theenteredtimestamp before persistence. The use ofDateTimeImmutableis appropriate and compatible with theDateTimeInterfaceproperty type.src/Domain/Analytics/Model/LinkTrackForward.php (1)
14-16: LGTM! Constraints and indexes renamed with table prefix.The unique constraint and index names have been updated to follow the fully-qualified naming pattern (
phplist_linktrack_forward_*), consistent with the refactoring applied to related entities likeLinkTrack.src/Domain/Messaging/Model/UserMessage.php (1)
16-20: LGTM! Index names standardized across all indexes.All five index names have been updated to use the
phplist_usermessage_prefix, maintaining consistency with the systematic refactoring of ORM metadata across messaging and analytics entities.src/Migrations/_template_migration.php.tpl (1)
1-40: LGTM! Well-structured migration template with platform checks.The template provides a solid foundation for generating platform-specific migrations with appropriate safeguards:
- Platform validation using
skipIfensures migrations only execute on the intended database platform- Supports both PostgreSQL and MySQL platforms
- Follows Doctrine migrations best practices with proper method signatures
- Clear placeholder structure for code generation
tests/Integration/Domain/Subscription/Service/SubscriberDeletionServiceTest.php (1)
55-55: LGTM! Test updated to match new initialization pattern.The Administrator initialization now explicitly sets the login name, aligning with the broader pattern of explicit field initialization across the codebase.
src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (1)
14-15: LGTM! Index naming standardized.The index names have been updated to use fully-qualified prefixed names (
phplist_user_attribute_*), which aligns with the broader pattern of standardizing ORM metadata across the codebase.tests/Unit/Domain/Identity/Model/AdministratorTest.php (3)
30-30: LGTM! Test initialization updated to match entity requirements.The Administrator is now explicitly initialized with an empty login name via
setLoginName(''), consistent with the entity's updated initialization pattern.
72-74: LGTM! Test expectations updated to match new entity lifecycle behavior.The tests now correctly reflect that:
getUpdatedAt()returns a non-null value upon initialization (Lines 72-74)- Modification timestamp is triggered by calling
setEmail()rather than an explicitupdateUpdatedAt()method (Line 79)These changes align with the entity's lifecycle callback implementation using
@PreUpdate.Also applies to: 79-79
84-86: LGTM! Password hash initialization expectation corrected.The test now correctly expects
nullinstead of an empty string for the initial password hash, matching the entity's actual behavior.tests/Integration/Domain/Subscription/Fixtures/SubscriberListFixture.php (1)
44-45: LGTM! Fixture updated to match new initialization pattern.The Administrator is now initialized using the fluent setter pattern with
setLoginName(), consistent with the entity's updated requirements.src/Bounce/Service/SubscriberBlacklistService.php (1)
43-43: LGTM! Updated to use object-based API.The code now correctly captures the
UserBlacklistobject fromaddEmailToBlacklist()(Line 43) and passes it toaddBlacklistData()(Line 53), replacing the previous string-based approach. This provides better type safety and aligns with the refactored API.Also applies to: 52-56
src/Domain/Analytics/Model/LinkTrack.php (2)
7-7: LGTM! Metadata standardization applied.The DateTime import has been added (Line 7) and all index names have been updated to use the
phplist_linktrack_prefix (Lines 16-20), aligning with the broader standardization effort across the codebase.Also applies to: 16-20
43-52: LGTM! Timestamp initialization moved to constructor.The database default for
latestClickhas been removed (Line 43) and replaced with explicit initialization in the constructor (Lines 49-52). This pattern:
- Makes timestamp behavior explicit in the application code
- Reduces database-level dependencies
- Ensures consistency across different database platforms
This aligns with the same pattern applied to other entities in this PR (e.g., Administrator, ListMessage, UserMessage).
src/Domain/Messaging/Model/UserMessageForward.php (1)
15-17: LGTM! Index naming standardized.The index names have been updated to use fully-qualified prefixed names (
phplist_user_message_forward_*), consistent with the standardization pattern applied across messaging and analytics models in this PR.tests/Unit/Domain/Subscription/Model/SubscriberTest.php (1)
59-62: LGTM! Test correctly updated to match non-nullable updatedAt.The test method name and assertion now correctly reflect that
getUpdatedAt()returns a non-nullableDateTime, aligning with the model signature shown in the relevant code snippet.tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php (2)
45-46: LGTM! Fluent initialization improves test readability.The fluent style
(new Administrator())->setLoginName('t')is concise and aligns with the broader test suite updates.
71-72: LGTM! Consistent fluent initialization for test administrators.The fluent initialization pattern is applied consistently across both administrators, improving test readability.
tests/Integration/Domain/Identity/Repository/AdministratorTokenRepositoryTest.php (1)
133-134: LGTM! Constructor-based association aligns with model refactoring.The change from no-arg constructor + setter to constructor-based Administrator injection is consistent with the broader refactoring described in the AI summary.
src/Domain/Analytics/Model/LinkTrackUmlClick.php (1)
15-18: LGTM! Index renaming prevents duplicate name conflicts.The prefixed constraint and index names align with the PR objective to avoid duplicate index name issues on some DB versions. The column definitions remain unchanged, and the naming is consistent with related entities like
LinkTrackandLinkTrackMl.tests/Integration/Domain/Identity/Repository/AdministratorRepositoryTest.php (3)
96-96: LGTM! Fluent initialization pattern applied consistently.The fluent
setLoginNamecall aligns with test suite updates for Administrator instantiation.
107-107: LGTM! Consistent fluent initialization.
168-168: LGTM! Fluent initialization for test data.tests/Unit/Security/AuthenticationTest.php (2)
39-39: LGTM! Constructor usage aligns with new AdministratorToken signature.The token is now correctly constructed with the Administrator parameter, matching the updated constructor signature.
56-56: LGTM! Mock appropriately bypasses constructor requirement.Using a mock for the token without an administrator is the correct approach for testing the null-administrator scenario, given the constructor now requires an Administrator parameter.
tests/Integration/Domain/Identity/Fixtures/DetachedAdministratorTokenFixture.php (3)
32-34: LGTM! Administrator creation required for token constructor.The Administrator instance is correctly created, assigned an ID, and persisted before being used in AdministratorToken construction.
38-40: LGTM! Defensive CSV parsing improves robustness.The explicit check for
fgetcsvreturningfalseprevents potential issues with malformed CSV data or early EOF.
43-43: LGTM! Constructor-based token initialization.The AdministratorToken is now correctly constructed with the Administrator parameter, aligning with the updated constructor signature.
tests/Unit/Domain/Identity/Model/AdministratorTokenTest.php (1)
29-29: LGTM! Constructor correctly initialized with Administrator.The AdministratorToken is now constructed with an Administrator instance, aligning with the updated constructor signature that requires this parameter.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Migrations/Version20251030081120PostgreSqlPlatform.php (1)
32-181: Critical: Primary key columns lack auto-increment mechanism.Every table with an integer primary key (e.g.,
id INT NOT NULL) is missing the auto-increment mechanism required for PostgreSQL. WithoutSERIAL,GENERATED BY DEFAULT AS IDENTITY, or an explicit sequence, INSERT operations will fail because PostgreSQL has no way to generate the next ID value automatically.Apply this pattern to all tables with single-column integer primary keys (phplist_admin, phplist_admin_login, phplist_admin_password_request, phplist_adminattribute, phplist_admintoken, phplist_attachment, phplist_bounce, phplist_bounceregex, phplist_eventlog, phplist_linktrack, phplist_linktrack_forward, phplist_linktrack_uml_click, phplist_list, phplist_listmessage, phplist_message, phplist_message_attachment, phplist_sendprocess, phplist_subscribepage, phplist_template, phplist_templateimage, phplist_urlcache, phplist_user_attribute, phplist_user_message_bounce, phplist_user_message_forward, phplist_user_message_view, phplist_user_user, phplist_user_user_history, phplist_userstats):
-$this->addSql('CREATE TABLE phplist_admin (id INT NOT NULL, ... +$this->addSql('CREATE TABLE phplist_admin (id INT GENERATED BY DEFAULT AS IDENTITY NOT NULL, ...Or use SERIAL type:
-$this->addSql('CREATE TABLE phplist_admin (id INT NOT NULL, ... +$this->addSql('CREATE TABLE phplist_admin (id SERIAL NOT NULL, ...src/Migrations/Version20251030083621MySqlRenameIndex.php (1)
21-30: Platform guards implemented correctly.The MySQL-only migration guards requested in the previous review have been properly implemented in both
up()anddown()methods. The migration will now safely skip execution on non-MySQL platforms.Also applies to: 114-123
🧹 Nitpick comments (1)
resources/Database/Schema.sql (1)
40-40: Consider standardizing charset across all tables.The schema mixes
utf8mb3(most tables) andutf8mb4(listattr tables). While this may be intentional for extended character support in specific tables, mixed charsets can cause implicit conversions during JOINs, potentially impacting performance.Consider standardizing to
utf8mb4across all tables for consistency and better Unicode support, unless there's a specific reason to useutf8mb3for certain tables.Also applies to: 405-405, 421-421, 437-437, 453-453
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
resources/Database/Schema.sql(5 hunks)src/Domain/Identity/Model/Administrator.php(5 hunks)src/Domain/Messaging/Model/Bounce.php(1 hunks)src/Domain/Messaging/Model/Message/MessageFormat.php(1 hunks)src/Migrations/Version20251030081120PostgreSqlPlatform.php(1 hunks)src/Migrations/Version20251030083621MySqlRenameIndex.php(1 hunks)src/Migrations/initial_schema.sql(1 hunks)tests/Unit/Domain/Identity/Model/AdministratorTokenTest.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Domain/Messaging/Model/Message/MessageFormat.php
- src/Domain/Messaging/Model/Bounce.php
- src/Migrations/initial_schema.sql
- src/Domain/Identity/Model/Administrator.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Unit/Domain/Identity/Model/AdministratorTokenTest.php (2)
src/Domain/Identity/Model/Administrator.php (1)
setLoginName(95-99)src/Domain/Identity/Model/AdministratorToken.php (1)
getAdministrator(117-120)
src/Migrations/Version20251030083621MySqlRenameIndex.php (3)
src/Migrations/Version20251030081120PostgreSqlPlatform.php (2)
up(21-182)down(184-256)src/Migrations/Version20251028092901MySql.php (1)
up(21-33)src/Migrations/Version20251029105320MySqlRelations.php (2)
up(21-105)down(107-191)
src/Migrations/Version20251030081120PostgreSqlPlatform.php (2)
src/Migrations/Version20251030083621MySqlRenameIndex.php (1)
up(21-112)src/Migrations/Version20251028092901MySql.php (1)
up(21-33)
🪛 GitHub Actions: phpList Core Build
resources/Database/Schema.sql
[error] 502-502: Command failed with exit code 1: mysql --host 127.0.0.1 --port 32768 -uroot -pphplist phplist < resources/Database/Schema.sql. Error: Unknown collation: 'utf8mb4_0900_ai_ci'.
🔇 Additional comments (5)
tests/Unit/Domain/Identity/Model/AdministratorTokenTest.php (2)
29-29: LGTM! Constructor usage updated correctly.The test setup properly instantiates
AdministratorTokenwith a constructor-providedAdministratorinstance, aligning with the model's updated constructor signature.
93-96: LGTM! Test method name now matches the assertion.The renamed method
testGetAdministratorReturnsConstructorProvidedAdministratoraccurately reflects that the administrator is provided via constructor and is expected to be non-null. This addresses the past review comment.src/Migrations/Version20251030081120PostgreSqlPlatform.php (2)
21-30: LGTM! Platform check follows established pattern.The PostgreSQL platform check is correctly implemented using
skipIfwith an appropriate message, consistent with other migrations in the codebase.
184-256: LGTM! Down migration correctly reverses up() operations.The down migration properly:
- Checks for PostgreSQL platform
- Drops all foreign key constraints before dropping tables
- Drops all tables created in up()
Note: The
CREATE SCHEMA publicissue mentioned in past review comments is not present in the current code.src/Migrations/Version20251030083621MySqlRenameIndex.php (1)
33-112: LGTM! Comprehensive index renaming with consistent naming convention.The migration systematically renames indexes across all tables to use a
phplist_prefix, ensuring compatibility with PostgreSQL's requirement for globally unique index names. The implementation is thorough and follows a consistent pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Migrations/Version20251028092901MySqlInit.php (1)
31-33: Split the dump into individual statements before callingaddSql.
addSql()only accepts one statement at a time. Passing the whole dump will either stop after the first statement or throwSQLSTATE[42000], so the schema never initializes. Please read the file, abort if it cannot be loaded, split it (e.g.,SqlParserUtils::split), trim, and enqueue each statement separately.+use Doctrine\DBAL\Tools\SqlParserUtils; ... - // this up() migration is auto-generated, please modify it to your needs - $this->addSql(file_get_contents(__DIR__.'/initial_schema.sql')); + $sql = file_get_contents(__DIR__ . '/initial_schema.sql'); + $this->abortIf($sql === false, 'Failed to load initial schema dump.'); + + foreach (SqlParserUtils::split($sql) as $statement) { + $statement = trim($statement); + if ($statement !== '') { + $this->addSql($statement); + } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Domain/Analytics/Model/LinkTrackForward.php(1 hunks)src/Domain/Configuration/Model/UrlCache.php(2 hunks)src/Domain/Identity/Model/AdministratorToken.php(2 hunks)src/Migrations/Version20251028092901MySqlInit.php(1 hunks)src/Migrations/Version20251028092902MySqlUpdate.php(1 hunks)src/Migrations/Version20251031072945PostGreInit.php(1 hunks)src/Migrations/_template_migration.php.tpl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Domain/Analytics/Model/LinkTrackForward.php
- src/Migrations/_template_migration.php.tpl
- src/Domain/Identity/Model/AdministratorToken.php
🧰 Additional context used
🧬 Code graph analysis (4)
src/Migrations/Version20251031072945PostGreInit.php (2)
src/Migrations/Version20251028092901MySqlInit.php (1)
up(21-33)src/Migrations/Version20251028092902MySqlUpdate.php (1)
up(19-164)
src/Migrations/Version20251028092901MySqlInit.php (1)
src/Migrations/Version20251028092902MySqlUpdate.php (2)
getDescription(14-17)up(19-164)
src/Migrations/Version20251028092902MySqlUpdate.php (1)
src/Migrations/Version20251028092901MySqlInit.php (2)
getDescription(16-19)up(21-33)
src/Domain/Configuration/Model/UrlCache.php (2)
src/Domain/Identity/Model/AdministratorToken.php (2)
ORM(21-120)ORM(72-77)src/Domain/Configuration/Model/EventLog.php (2)
ORM(14-79)ORM(40-45)
⏰ 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: phpList on PHP 8.1 [Build, Test]
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Migrations/Version20251031072945PostGreInit.php (1)
233-233: RemoveCREATE SCHEMA publicfromdown().This issue was previously identified: PostgreSQL databases already have the
publicschema, so this statement will fail withSQLSTATE[42P06], preventing the rollback from completing.Apply this diff:
- $this->addSql('CREATE SCHEMA public');
🧹 Nitpick comments (4)
src/Migrations/_template_migration.php.tpl (1)
21-24: Consider adding a<comment>placeholder for migration descriptions.The AI summary mentions
<comment>as a substitution placeholder, but thegetDescription()method returns an empty string. Using a placeholder would allow generated migrations to include meaningful descriptions automatically.Apply this diff to add the placeholder:
public function getDescription(): string { - return ''; + return '<comment>'; }src/Migrations/Version20251031072945PostGreInit.php (3)
8-8: Remove unusedMySQLPlatformimport.This migration is PostgreSQL-only; the
MySQLPlatformimport on line 8 is unused and could mislead readers.Apply this diff:
use Doctrine\DBAL\Platforms\PostgreSQLPlatform; -use Doctrine\DBAL\Platforms\MySQLPlatform; use Doctrine\Migrations\AbstractMigration;
12-18: Fix or remove MySQL-specific warning in PostgreSQL migration.The warning references "MySQL 5.7" collations and key-length errors, which are MySQL-specific concerns. Since this is a PostgreSQL-only migration, this comment is misleading.
Apply this diff to remove the misleading warning:
-/** - * ⚠️ Wizard warning: - * Doctrine will `helpfully` remove url(255) prefixes and add collations 5.7 can't read. - * Review the SQL unless you enjoy debugging key length errors at 2 AM. - * - * Ex: phplist_linktrack_forward phplist_linktrack_forward_urlindex (but there are more) - */Or update it to reflect PostgreSQL-specific concerns if any exist.
21-24: Add a meaningful description.The empty description makes it harder for developers to understand this migration's purpose at a glance.
Apply this diff:
public function getDescription(): string { - return ''; + return 'Initial PostgreSQL schema for phplist core'; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Domain/Identity/Model/AdministratorToken.php(2 hunks)src/Migrations/Version20251028092901MySqlInit.php(1 hunks)src/Migrations/Version20251028092902MySqlUpdate.php(1 hunks)src/Migrations/Version20251031072945PostGreInit.php(1 hunks)src/Migrations/_template_migration.php.tpl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Migrations/Version20251028092902MySqlUpdate.php
- src/Migrations/Version20251028092901MySqlInit.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Migrations/Version20251031072945PostGreInit.php (2)
src/Migrations/Version20251028092901MySqlInit.php (2)
getDescription(16-19)up(21-33)src/Migrations/Version20251028092902MySqlUpdate.php (2)
getDescription(14-17)up(19-164)
src/Domain/Identity/Model/AdministratorToken.php (4)
src/Domain/Identity/Model/Administrator.php (1)
__construct(68-73)src/Domain/Identity/Model/AdminPasswordRequest.php (1)
__construct(32-37)src/Domain/Identity/Service/SessionManager.php (1)
__construct(21-31)src/Domain/Identity/Model/AdminLogin.php (1)
getAdministrator(73-76)
⏰ 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: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (6)
src/Migrations/_template_migration.php.tpl (4)
7-10: Imports look good.Both platform classes are imported to support either PostgreSQL or MySQL migrations. The template will generate platform-specific migrations using the
<platform>placeholder.
12-18: Valuable warning for migration developers.This comment provides helpful context about Doctrine's automatic schema modifications that can cause issues with MySQL 5.7, particularly around URL field lengths and collations. The specific example adds practical value.
26-35: Platform guard logic is well-implemented.The
up()method correctly checks the database platform and skips the migration if it doesn't match the expected platform. The error message includes the actual platform class for debugging, which is helpful.
37-46: Platform guard logic indown()is consistent and correct.The
down()method mirrors the platform checking logic fromup(), ensuring both upgrade and rollback operations are protected. While there's duplication, it keeps the template explicit and straightforward.src/Domain/Identity/Model/AdministratorToken.php (2)
48-53: Constructor injection pattern implemented correctly.The constructor properly enforces the required administrator dependency and automatically initializes the token's expiry and key. This aligns with the dependency injection patterns used elsewhere in the codebase.
116-119: LGTM: Return type correctly reflects non-nullable property.The tightened return type is consistent with the non-nullable administrator property and improves type safety.
| #[ORM\ManyToOne(targetEntity: Administrator::class)] | ||
| #[ORM\JoinColumn(name: 'adminid', referencedColumnName: 'id', onDelete: 'CASCADE')] | ||
| private ?Administrator $administrator = null; | ||
| private Administrator $administrator; |
There was a problem hiding this comment.
🧩 Analysis chain
Good change to enforce data integrity.
Making the administrator property non-nullable is a solid improvement that ensures every token is associated with an administrator.
Verify that the database migration properly handles any existing NULL values in the adminid column before applying the NOT NULL constraint:
🏁 Script executed:
#!/bin/bash
# Description: Check if migration handles existing NULL administrators
# Search for migration that updates adminid column
rg -n "adminid" --type=php -g "*Migration*.php" -C 3
# Also check for any NOT NULL or foreign key constraints being added
rg -n "NOT NULL|onDelete.*CASCADE" --type=php -g "*Migration*.php" -C 2Length of output: 297
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Locate migration files and schema references
# Find potential migration directories
echo "=== Looking for migration directories ==="
fd -t d -E node_modules -E .git "migrat" .
echo ""
echo "=== Looking for SQL or migration files ==="
fd -E node_modules -E .git "\.sql$|migration|Migration" . | head -20
echo ""
echo "=== Search for adminid or administrator_token across entire repo ==="
rg -n "adminid|administrator_token" -C 2 | head -30
Length of output: 6799
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Check for migrations that modify admintoken to add NOT NULL
echo "=== List all migration files ordered by version ==="
ls -1 src/Migrations/Version*.php | sort
echo ""
echo "=== Search for any ALTER TABLE admintoken statements ==="
rg -n "ALTER TABLE.*admintoken|admintoken.*NOT NULL" -C 3
echo ""
echo "=== Check the current AdministratorToken entity mapping ==="
rg -n "class AdministratorToken|#\[ORM|joinColumn|nullable" src/Domain/Identity/Model/AdministratorToken.php -B 2 -A 2
Length of output: 8978
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Verify if the Doctrine JoinColumn has nullable configuration
echo "=== Check full JoinColumn configuration for administrator property ==="
rg -n "JoinColumn" src/Domain/Identity/Model/AdministratorToken.php -A 1 -B 1
echo ""
echo "=== Check if there's a nullable attribute in the JoinColumn ==="
cat src/Domain/Identity/Model/AdministratorToken.php | grep -A 3 "ManyToOne"
echo ""
echo "=== Verify the property type and check for any recent schema change migrations ==="
fd "Version.*\.php" src/Migrations/ -x grep -l "NOT NULL.*adminid\|adminid.*NOT NULL" {} \;
Length of output: 707
Database schema mismatch: adminid column allows NULL but property is non-nullable.
The Doctrine JoinColumn at line 45 lacks nullable: true, and the property type Administrator (line 46) is non-nullable. However, the database schema in both MySQL and PostgreSQL defines adminid INT DEFAULT NULL, allowing NULL values. This will cause a TypeError when Doctrine attempts to hydrate existing NULL records into the non-nullable typed property.
Either:
- Add a migration to alter the
phplist_admintokentable and setadminidtoNOT NULL(after handling existing NULL values), or - Change the
JoinColumnto includenullable: trueand make the property?Administrator(nullable type hint)
🤖 Prompt for AI Agents
In src/Domain/Identity/Model/AdministratorToken.php around line 46, the
Administrator relation is non-nullable while the DB column adminid allows NULL;
update the mapping and property to match by changing the JoinColumn to include
nullable: true and make the property nullable (change type to ?Administrator) so
Doctrine can hydrate NULL values, and adjust any constructors/getters/setters to
accept and handle nulls; alternatively, if you prefer DB non-nullability, create
and run a migration that sets all existing adminid values to valid ids (or
remove affected rows), alter the phplist_admintoken.adminid column to NOT NULL,
and keep the property and mapping non-nullable.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Migrations/Version20251031072945PostGreInit.php(1 hunks)
⏰ 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: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (7)
src/Migrations/Version20251031072945PostGreInit.php (7)
25-31: LGTM - Platform guard is correct.The platform check ensures the migration only runs on PostgreSQL.
33-60: LGTM - Sequence definitions are consistent.All sequences follow the standard PostgreSQL pattern with consistent naming.
191-210: LGTM - Foreign key constraints are properly defined.Constraints are added after table creation, and ON DELETE behaviors (CASCADE for dependents, SET NULL for optional refs) are appropriate.
213-220: LGTM - Platform guard for rollback is correct.
221-248: LGTM - Sequence drops use CASCADE correctly.All sequences are dropped with CASCADE, which handles dependent defaults.
249-268: LGTM - Foreign key constraints properly dropped before tables.
269-310: LGTM - All tables are properly dropped.Since foreign keys are dropped first, table drop order doesn't matter.
| $this->addSql('CREATE INDEX phplist_linktrack_uidindex ON phplist_linktrack (userid)'); | ||
| $this->addSql('CREATE INDEX phplist_linktrack_urlindex ON phplist_linktrack (url)'); | ||
| $this->addSql('CREATE UNIQUE INDEX phplist_linktrack_miduidurlindex ON phplist_linktrack (messageid, userid, url)'); | ||
| $this->addSql('CREATE TABLE phplist_linktrack_forward (id INT NOT NULL, url VARCHAR(2083) DEFAULT NULL, urlhash VARCHAR(32) DEFAULT NULL, uuid VARCHAR(36) DEFAULT \'\', personalise BOOLEAN DEFAULT false, PRIMARY KEY(id))'); |
There was a problem hiding this comment.
Inconsistent UUID field definitions.
UUID fields across tables have inconsistent defaults:
- Lines 92, 124:
uuid VARCHAR(36) DEFAULT ''(empty string is not a valid UUID) - Line 163:
uuid VARCHAR(36) NOT NULL(no default, will fail if not set)
Consider either making UUID fields nullable or ensuring the application always generates valid UUIDs before insert.
Apply this diff to make the fields nullable:
- $this->addSql('CREATE TABLE phplist_linktrack_forward (id INT NOT NULL, url VARCHAR(2083) DEFAULT NULL, urlhash VARCHAR(32) DEFAULT NULL, uuid VARCHAR(36) DEFAULT \'\', personalise BOOLEAN DEFAULT false, PRIMARY KEY(id))');
+ $this->addSql('CREATE TABLE phplist_linktrack_forward (id INT NOT NULL, url VARCHAR(2083) DEFAULT NULL, urlhash VARCHAR(32) DEFAULT NULL, uuid VARCHAR(36) DEFAULT NULL, personalise BOOLEAN DEFAULT false, PRIMARY KEY(id))');- $this->addSql('CREATE TABLE phplist_message (id INT NOT NULL, owner INT DEFAULT NULL, template INT DEFAULT NULL, modified TIMESTAMP(0) WITHOUT TIME ZONE NOT NULL, uuid VARCHAR(36) DEFAULT \'\', htmlformatted BOOLEAN NOT NULL, sendformat VARCHAR(20) DEFAULT NULL, astext BOOLEAN NOT NULL, ashtml BOOLEAN NOT NULL, aspdf BOOLEAN NOT NULL, astextandhtml BOOLEAN NOT NULL, astextandpdf BOOLEAN NOT NULL, repeatinterval INT DEFAULT 0, repeatuntil TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, requeueinterval INT DEFAULT 0, requeueuntil TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, embargo TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, status VARCHAR(255) DEFAULT NULL, processed BOOLEAN DEFAULT false NOT NULL, viewed INT DEFAULT 0 NOT NULL, bouncecount INT DEFAULT 0 NOT NULL, entered TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, sent TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, sendstart TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, subject VARCHAR(255) DEFAULT \'(no subject)\' NOT NULL, message TEXT DEFAULT NULL, textmessage TEXT DEFAULT NULL, footer TEXT DEFAULT NULL, fromfield VARCHAR(255) DEFAULT \'\' NOT NULL, tofield VARCHAR(255) DEFAULT \'\' NOT NULL, replyto VARCHAR(255) DEFAULT \'\' NOT NULL, userselection TEXT DEFAULT NULL, rsstemplate VARCHAR(100) DEFAULT NULL, PRIMARY KEY(id))');
+ $this->addSql('CREATE TABLE phplist_message (id INT NOT NULL, owner INT DEFAULT NULL, template INT DEFAULT NULL, modified TIMESTAMP(0) WITHOUT TIME ZONE NOT NULL, uuid VARCHAR(36) DEFAULT NULL, htmlformatted BOOLEAN NOT NULL, sendformat VARCHAR(20) DEFAULT NULL, astext BOOLEAN NOT NULL, ashtml BOOLEAN NOT NULL, aspdf BOOLEAN NOT NULL, astextandhtml BOOLEAN NOT NULL, astextandpdf BOOLEAN NOT NULL, repeatinterval INT DEFAULT 0, repeatuntil TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, requeueinterval INT DEFAULT 0, requeueuntil TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, embargo TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, status VARCHAR(255) DEFAULT NULL, processed BOOLEAN DEFAULT false NOT NULL, viewed INT DEFAULT 0 NOT NULL, bouncecount INT DEFAULT 0 NOT NULL, entered TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, sent TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, sendstart TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, subject VARCHAR(255) DEFAULT \'(no subject)\' NOT NULL, message TEXT DEFAULT NULL, textmessage TEXT DEFAULT NULL, footer TEXT DEFAULT NULL, fromfield VARCHAR(255) DEFAULT \'\' NOT NULL, tofield VARCHAR(255) DEFAULT \'\' NOT NULL, replyto VARCHAR(255) DEFAULT \'\' NOT NULL, userselection TEXT DEFAULT NULL, rsstemplate VARCHAR(100) DEFAULT NULL, PRIMARY KEY(id))');- $this->addSql('CREATE TABLE phplist_user_user (id INT NOT NULL, entered TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, modified TIMESTAMP(0) WITHOUT TIME ZONE NOT NULL, email VARCHAR(255) NOT NULL, confirmed BOOLEAN NOT NULL, blacklisted BOOLEAN NOT NULL, bouncecount INT NOT NULL, uniqid VARCHAR(255) DEFAULT NULL, htmlemail BOOLEAN NOT NULL, disabled BOOLEAN NOT NULL, extradata TEXT DEFAULT NULL, optedin BOOLEAN NOT NULL, uuid VARCHAR(36) NOT NULL, subscribepage INT DEFAULT NULL, rssfrequency VARCHAR(100) DEFAULT NULL, password VARCHAR(255) DEFAULT NULL, passwordchanged TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, foreignkey VARCHAR(100) DEFAULT NULL, PRIMARY KEY(id))');
+ $this->addSql('CREATE TABLE phplist_user_user (id INT NOT NULL, entered TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, modified TIMESTAMP(0) WITHOUT TIME ZONE NOT NULL, email VARCHAR(255) NOT NULL, confirmed BOOLEAN NOT NULL, blacklisted BOOLEAN NOT NULL, bouncecount INT NOT NULL, uniqid VARCHAR(255) DEFAULT NULL, htmlemail BOOLEAN NOT NULL, disabled BOOLEAN NOT NULL, extradata TEXT DEFAULT NULL, optedin BOOLEAN NOT NULL, uuid VARCHAR(36) DEFAULT NULL, subscribepage INT DEFAULT NULL, rssfrequency VARCHAR(100) DEFAULT NULL, password VARCHAR(255) DEFAULT NULL, passwordchanged TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL, foreignkey VARCHAR(100) DEFAULT NULL, PRIMARY KEY(id))');Also applies to: 124-124, 163-163
🤖 Prompt for AI Agents
In src/Migrations/Version20251031072945PostGreInit.php around lines 92, 124 and
163, the UUID column definitions are inconsistent (some use DEFAULT '' and one
is NOT NULL); update those column definitions to be nullable (e.g., change `uuid
VARCHAR(36) DEFAULT ''` to `uuid VARCHAR(36) DEFAULT NULL` and change `uuid
VARCHAR(36) NOT NULL` to `uuid VARCHAR(36) DEFAULT NULL`) so all UUID columns
are nullable and consistent across the migration.
…essor (#357) * Blacklist (#352) * Blacklisted * user repository methods * fix configs * add test * fix: phpmd * fix: repo configs * return a created resource --------- Co-authored-by: Tatevik <[email protected]> * Subscribepage (#353) * subscriber page manager * owner entity * test * ci fix * getByPage data --------- Co-authored-by: Tatevik <[email protected]> * Bounceregex manager (#354) * BounceRegexManager * Fix manager directory * Prop name update admin -> adminId --------- Co-authored-by: Tatevik <[email protected]> * Bounce processing command (#355) * BounceManager * Add bounce email * Move to the processor dir * SendProcess lock service * ClientIp + SystemInfo * ProcessBouncesCommand * ProcessBouncesCommand all methods * BounceProcessingService * AdvancedBounceRulesProcessor * UnidentifiedBounceReprocessor * ConsecutiveBounceHandler * Refactor * BounceDataProcessor * ClientFactory + refactor * BounceProcessorPass * Register services + phpstan fix * PhpMd * PhpMd CyclomaticComplexity * PhpCodeSniffer * Tests * Refactor * Add tests * More tests * Fix tests --------- Co-authored-by: Tatevik <[email protected]> * EventLog + translator (#356) * EventLogManager * Log failed logins + translate messages * weblate * test fix * Use translations * Fix pipeline * Weblate * Deprecate DB translation table --------- Co-authored-by: Tatevik <[email protected]> * Access level check (#358) * OwnableInterface * PermissionChecker * Check related * Register service + test * Style fix --------- Co-authored-by: Tatevik <[email protected]> * Message processor (#359) * MessageStatusEnum * Status validate * Embargo check * IspRestrictions * IspRestrictions * SendRateLimiter * UserMessageStatus * Refactor * RateLimitedCampaignMailer * RateLimitedCampaignMailerTest * Rate limit initialized from history * Check maintenance mode * Max processing time limiter --------- Co-authored-by: Tatevik <[email protected]> * Exceptions + translations (#361) * MissingMessageIdException * MailboxConnectionException * BadMethodCallException (style fix) * Translate user facing messages * Translate * Translate PasswordResetMessageHandler texts * Translate SubscriberConfirmationMessageHandler texts * MessageBuilder exceptions * BlacklistEmailAndDeleteBounceHandler * BlacklistEmailHandler - AdvancedBounceRulesProcessor * AdvancedBounceRulesProcessor * BounceStatus * CampaignProcessor * UnidentifiedBounceReprocessor * Style fix * Test fix * ConsecutiveBounceHandler --------- Co-authored-by: Tatevik <[email protected]> * Fix autowiring * Reset subscriber bounce count * Install RssFeedBundle * Update import logic, add dynamic attribute repository (#362) * Skip password and modified fields while import, do not subscribe blacklisted users * DefaultConfigProvider * Use ConfigProvider in ProcessQueueCommand * Use ConfigProvider in SubscriberCsvImporter + send email * Rename paramProvider * Test fix * AttributeValueProvider for dynamic tables * Update: config provider * Translations in default configs * Email with messageHandler * Style fix * PhpCs fix * Fix configs * replace list names * Add tests + fix * Add more tests + fix handler --------- Co-authored-by: Tatevik <[email protected]> * Refactor import add subscriber history (#363) * Add blacklisted stat to import result * Add history * Add translations * addHistory for unconfirmed * Refactor * Add changeSetDto * Do not send email on creating without any list * Flush in controller * Add test --------- Co-authored-by: Tatevik <[email protected]> * Em flush (#364) * createSubscriberList * ProcessQueueCommand * Dispatch CampaignProcessorMessage for processing * Fix tests * Fix style * CleanUpOldSessionTokens * Move bounce processing into the Bounce folder * delete/remove * Remove hardcoded TatevikGrRssBundle mapping * Fix configs * Add sync * Fix: DQL in MessageRepository.php * PhpMD * SubscriberBlacklistService, SubscriberBlacklistManager * AdministratorManager * BounceManager * SendProcessManager * TemplateManager * SubscribePageManager * Fix: tests * BounceManager * rest of flushes * save * fix test * CouplingBetweenObjects 15 --------- Co-authored-by: Tatevik <[email protected]> * Add test * Update: docs * Migrations (mysql psql) (#366) * OnlyOrmTablesFilter * Current * Admin * Init migration * In progress * Fix mapping * Fix tests * Migrate * Separate MySql * Use psql * Rename indexes * PostgreSqlPlatform * MySqlSqlPlatform rename indexes * Fix: cs * Fix: test configs * Add migration template * PR agent (#365) * .coderabbit.yaml This reverts commit 2246e49. --------- Co-authored-by: Tatevik <[email protected]> * rename template * After review 0 * After review 1 * Fix MySql migrations * After review 2 --------- Co-authored-by: Tatevik <[email protected]> * Import by foreign key (#367) * Import with a foreign key --------- Co-authored-by: Tatevik <[email protected]> * Insert initial admin command (#368) * Update: codeRabbit configs * Update: Use command for initial admin insert --------- Co-authored-by: Tatevik <[email protected]> * Update pull request template --------- Co-authored-by: Tatevik <[email protected]>
Summary
Unit test
Are your changes covered with unit tests, and do they not break anything?
You can run the existing unit tests using this command:
Code style
Have you checked that you code is well-documented and follows the PSR-2 coding
style?
You can check for this using this command:
Other Information
If there's anything else that's important and relevant to your pull
request, mention that information here. This could include benchmarks,
or other information.
If you are updating any of the CHANGELOG files or are asked to update the
CHANGELOG files by reviewers, please add the CHANGELOG entry at the top of the file.
Thanks for contributing to phpList!
Summary by CodeRabbit
New Features
Bug Fixes
Chores