Dev #159
Dev #159
Conversation
* blacklist controller * blacklist request * UserBlacklistNormalizer * add in development flags * phpcs fix * use blacklist branch * fix swagger * use core dev branch --------- Co-authored-by: Tatevik <[email protected]>
* SubscribePageController * SubscribePageNormalizer * SubscribePageCreateRequest * Get/set page data, update/delete page, add tests --------- Co-authored-by: Tatevik <[email protected]>
* Add BounceRegexController * Use code dev branch --------- Co-authored-by: Tatevik <[email protected]>
* Fix composer json * Add flush after delete * Fix: test * SubscriberBlacklistManager * listMessageManager * AdminController flush * TemplateController flush * SubscribePageController flush * save * fix test --------- Co-authored-by: Tatevik <[email protected]>
📝 WalkthroughWalkthroughThis PR adds bounce-regex, blacklist, and subscribe-page REST endpoints, new request DTOs and normalizers, and many integration/unit tests and fixtures. It reorganizes several messaging services under a Manager namespace, swaps direct campaign processing for Symfony Messenger dispatch, removes SubscriberService in favor of manager usage, and injects Doctrine EntityManager with explicit flush() calls across numerous controllers. Composer gains a VCS repository and tatevikgr/rss-feed dependency. Several validator attributes were adjusted to trim inputs. Sequence DiagramsequenceDiagram
autonumber
actor Client
participant API as REST Controller
participant Manager
participant DB as EntityManager
participant Bus as MessageBus
participant Handler
rect rgba(120,180,120,0.08)
Note over Client,API: Typical synchronous CRUD (new controllers)
Client->>API: HTTP create/update/delete
API->>Manager: call manager (business logic)
Manager->>DB: persist/remove
DB-->>API: ack (after flush)
API-->>Client: HTTP response (200/201/204)
end
rect rgba(120,160,220,0.06)
Note over Client,Bus: Campaign send flow moved to Messenger
Client->>API: sendMessage()
API->>Bus: dispatch(SyncCampaignProcessorMessage)
Bus->>Handler: handle()
Handler->>Manager: process()
Manager->>DB: persist
DB-->>Handler: ack
Handler-->>API: result (sync)
API-->>Client: HTTP response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/Subscription/Controller/SubscriberAttributeValueController.php (2)
344-361: Critical bug: GET endpoint deletes data instead of reading it.This method has multiple critical issues:
Data corruption: Lines 354-355 delete the attribute and flush the deletion, but this is a GET endpoint that should only read data. Every call to this endpoint will permanently delete the attribute being fetched.
Type mismatch: Line 346 incorrectly types
$subscriberasSubscriberAttributeDefinitionwhen it should beSubscriber.Apply this diff to fix both issues:
public function getAttributeDefinition( Request $request, - #[MapEntity(mapping: ['subscriberId' => 'id'])] ?SubscriberAttributeDefinition $subscriber, + #[MapEntity(mapping: ['subscriberId' => 'id'])] ?Subscriber $subscriber, #[MapEntity(mapping: ['definitionId' => 'id'])] ?SubscriberAttributeDefinition $definition, ): JsonResponse { $this->requireAuthentication($request); if (!$definition || !$subscriber) { throw $this->createNotFoundException('Subscriber attribute not found.'); } $attribute = $this->attributeManager->getSubscriberAttribute($subscriber->getId(), $definition->getId()); - $this->attributeManager->delete($attribute); - $this->entityManager->flush(); + if ($attribute === null) { + throw $this->createNotFoundException('Subscriber attribute not found.'); + } return $this->json( $this->normalizer->normalize($attribute), Response::HTTP_OK ); }
122-129: Add flush call aftercreateOrUpdateto persist changes.The
createOrUpdatemethod on line 122–126 is missing$this->entityManager->flush()required to persist the attribute. This is inconsistent with the delete operation (line 198) and with the identicalcreateOrUpdatecall inAdminAttributeValueController(line 129), which includes the flush. Per the PR objective to move flush calls to controllers, add the flush after line 126.$attributeDefinition = $this->attributeManager->createOrUpdate( subscriber:$subscriber, definition: $definition, value: $request->toArray()['value'] ?? null ); $this->entityManager->flush(); $json = $this->normalizer->normalize($attributeDefinition, 'json');src/Messaging/Controller/ListMessageController.php (2)
326-343: Missing EntityManager flush after disassociation.The
disassociateMessageFromListmethod removes an association but doesn't call$this->entityManager->flush()to persist the change, unlikeassociateMessageWithListat line 267. This inconsistency could result in data loss if the application terminates before an implicit flush occurs.Apply this diff to ensure persistence:
$this->listMessageManager->removeAssociation($message, $subscriberList); + $this->entityManager->flush(); return $this->json(null, Response::HTTP_NO_CONTENT);
390-402: Missing EntityManager flush after removing all associations.The
removeAllListAssociationsForMessagemethod removes all list associations but doesn't call$this->entityManager->flush()to persist the changes, unlikeassociateMessageWithListat line 267. This inconsistency could result in data loss.Apply this diff to ensure persistence:
$this->listMessageManager->removeAllListAssociationsForMessage($message); + $this->entityManager->flush(); return $this->json(null, Response::HTTP_NO_CONTENT);src/Identity/Controller/AdminAttributeValueController.php (2)
345-365: Critical: GET endpoint deletes data.The
getAttributeDefinitionmethod is mapped to a GET route but callsdelete()on line 358. GET requests must be idempotent and should never mutate state. This will cause data loss when clients retrieve an attribute.Remove the delete call:
$attribute = $this->attributeManager->getAdminAttribute( adminId: $admin->getId(), attributeDefinitionId: $definition->getId() ); - $this->attributeManager->delete($attribute); - $this->entityManager->flush(); return $this->json( $this->normalizer->normalize($attribute), Response::HTTP_OK );
347-347: Critical: Wrong type hint for $admin parameter.Line 347 maps the
adminIdroute parameter toAdminAttributeDefinition, but it should map toAdministrator. This will cause entity resolution to fail.Fix the type hint:
- #[MapEntity(mapping: ['adminId' => 'id'])] ?AdminAttributeDefinition $admin, + #[MapEntity(mapping: ['adminId' => 'id'])] ?Administrator $admin,
🧹 Nitpick comments (7)
src/Subscription/Controller/SubscriberAttributeDefinitionController.php (1)
84-96: Consider explicit transaction management for better atomicity.Currently, flush occurs before normalization completes. If normalization or response generation fails, the database changes are already committed. For stronger guarantees, consider wrapping each operation in an explicit transaction:
$this->entityManager->beginTransaction(); try { $attributeDefinition = $this->definitionManager->create($definitionRequest->getDto()); $this->entityManager->flush(); $json = $this->normalizer->normalize($attributeDefinition, 'json'); $this->entityManager->commit(); return $this->json($json, Response::HTTP_CREATED); } catch (\Throwable $e) { $this->entityManager->rollback(); throw $e; }Alternatively, consider using a Doctrine event listener or Symfony middleware to flush only after successful request completion.
Also applies to: 144-164, 206-219
tests/Integration/Subscription/Controller/SubscriberControllerTest.php (1)
75-76: Remove unnecessary blank lines.The extra blank line doesn't add value and should be removed to maintain consistent spacing between test methods.
src/Identity/Controller/AdminAttributeDefinitionController.php (1)
93-95: Remove extra blank line.The blank line at line 95 is unnecessary and should be removed for consistency.
src/Subscription/Request/SubscribePageRequest.php (1)
12-14: Consider renamingtitleto clarify its email constraint.The field name
titlecombined with#[Assert\Email]validation is semantically confusing. Consider renaming toApply this diff to improve clarity:
- #[Assert\NotBlank] - #[Assert\Email] - public string $title; + #[Assert\NotBlank] + #[Assert\Email] + public string $email;src/Subscription/Serializer/SubscribePageNormalizer.php (1)
23-25: Consider consistency with other normalizers.This normalizer returns an empty array for non-SubscribePage objects, whereas
AdministratorNormalizerthrows anInvalidArgumentException. While both approaches work, consider aligning error handling for consistency unless there's a specific reason for the difference.tests/Integration/Subscription/Fixtures/SubscribePageFixture.php (1)
35-58: Prefer conventional while loop.The
do-while(true)with conditional break is less idiomatic than a standardwhileloop. Consider refactoring for clarity.Apply this diff:
- do { - $data = fgetcsv($handle); - if ($data === false) { - break; - } + while (($data = fgetcsv($handle)) !== false) { $row = array_combine($headers, $data); // ... rest of loop body - } while (true); + }src/Subscription/Serializer/UserBlacklistNormalizer.php (1)
30-30: Use DateTimeInterface::ATOM constant for clarity.The date format string
'Y-m-d\TH:i:sP'is equivalent toDateTimeInterface::ATOM. Using the constant improves readability and aligns with the pattern inAdministratorNormalizer.Apply this diff:
- 'added' => $object->getAdded()?->format('Y-m-d\TH:i:sP'), + 'added' => $object->getAdded()?->format(DateTimeInterface::ATOM),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/Integration/Messaging/Fixtures/Message.csvis excluded by!**/*.csvtests/Integration/Subscription/Fixtures/SubscribePage.csvis excluded by!**/*.csv
📒 Files selected for processing (48)
composer.json(3 hunks)config/services/managers.yml(1 hunks)config/services/messenger_handlers.yml(1 hunks)config/services/normalizers.yml(1 hunks)src/Identity/Controller/AdminAttributeDefinitionController.php(5 hunks)src/Identity/Controller/AdminAttributeValueController.php(5 hunks)src/Identity/Controller/AdministratorController.php(5 hunks)src/Identity/Controller/PasswordResetController.php(5 hunks)src/Identity/Controller/SessionController.php(4 hunks)src/Messaging/Controller/BounceRegexController.php(1 hunks)src/Messaging/Controller/CampaignController.php(7 hunks)src/Messaging/Controller/ListMessageController.php(3 hunks)src/Messaging/Controller/TemplateController.php(4 hunks)src/Messaging/OpenApi/SwaggerSchemasResponse.php(1 hunks)src/Messaging/Request/CreateBounceRegexRequest.php(1 hunks)src/Messaging/Request/Message/MessageMetadataRequest.php(1 hunks)src/Messaging/Serializer/BounceRegexNormalizer.php(1 hunks)src/Messaging/Serializer/MessageNormalizer.php(1 hunks)src/Messaging/Service/CampaignService.php(3 hunks)src/Subscription/Controller/BlacklistController.php(1 hunks)src/Subscription/Controller/SubscribePageController.php(1 hunks)src/Subscription/Controller/SubscriberAttributeDefinitionController.php(5 hunks)src/Subscription/Controller/SubscriberAttributeValueController.php(4 hunks)src/Subscription/Controller/SubscriberController.php(8 hunks)src/Subscription/Controller/SubscriberImportController.php(2 hunks)src/Subscription/Controller/SubscriberListController.php(4 hunks)src/Subscription/Controller/SubscriptionController.php(4 hunks)src/Subscription/OpenApi/SwaggerSchemasResponse.php(1 hunks)src/Subscription/Request/AddToBlacklistRequest.php(1 hunks)src/Subscription/Request/SubscribePageDataRequest.php(1 hunks)src/Subscription/Request/SubscribePageRequest.php(1 hunks)src/Subscription/Serializer/SubscribePageNormalizer.php(1 hunks)src/Subscription/Serializer/UserBlacklistNormalizer.php(1 hunks)src/Subscription/Service/SubscriberService.php(0 hunks)tests/Integration/Messaging/Controller/BounceRegexControllerTest.php(1 hunks)tests/Integration/Messaging/Controller/CampaignControllerTest.php(1 hunks)tests/Integration/Messaging/Fixtures/MessageFixture.php(1 hunks)tests/Integration/Subscription/Controller/BlacklistControllerTest.php(1 hunks)tests/Integration/Subscription/Controller/SubscribePageControllerTest.php(1 hunks)tests/Integration/Subscription/Controller/SubscriberControllerTest.php(1 hunks)tests/Integration/Subscription/Fixtures/SubscribePageFixture.php(1 hunks)tests/Unit/Messaging/Request/CreateBounceRegexRequestTest.php(1 hunks)tests/Unit/Messaging/Serializer/BounceRegexNormalizerTest.php(1 hunks)tests/Unit/Messaging/Serializer/MessageNormalizerTest.php(2 hunks)tests/Unit/Messaging/Service/CampaignServiceTest.php(2 hunks)tests/Unit/Subscription/Serializer/SubscribePageNormalizerTest.php(1 hunks)tests/Unit/Subscription/Serializer/UserBlacklistNormalizerTest.php(1 hunks)tests/Unit/Subscription/Service/SubscriberServiceTest.php(0 hunks)
💤 Files with no reviewable changes (2)
- src/Subscription/Service/SubscriberService.php
- tests/Unit/Subscription/Service/SubscriberServiceTest.php
🧰 Additional context used
🧬 Code graph analysis (22)
src/Subscription/Request/SubscribePageDataRequest.php (1)
src/Subscription/Request/SubscribePageRequest.php (1)
getDto(19-22)
tests/Unit/Messaging/Serializer/BounceRegexNormalizerTest.php (1)
src/Messaging/Serializer/BounceRegexNormalizer.php (3)
BounceRegexNormalizer(10-41)supportsNormalization(37-40)normalize(15-32)
src/Subscription/Request/SubscribePageRequest.php (1)
src/Subscription/Request/SubscribePageDataRequest.php (1)
getDto(17-20)
src/Subscription/Serializer/UserBlacklistNormalizer.php (1)
src/Subscription/Controller/BlacklistController.php (1)
__construct(30-41)
src/Identity/Controller/AdminAttributeDefinitionController.php (1)
src/Common/Service/Provider/PaginatedDataProvider.php (1)
PaginatedDataProvider(17-56)
src/Subscription/Controller/SubscriberAttributeDefinitionController.php (1)
src/Common/Service/Provider/PaginatedDataProvider.php (1)
PaginatedDataProvider(17-56)
src/Subscription/Controller/SubscribePageController.php (5)
src/Subscription/OpenApi/SwaggerSchemasResponse.php (1)
OA(9-156)src/Common/Controller/BaseController.php (2)
BaseController(15-41)requireAuthentication(28-40)src/Subscription/Request/SubscribePageDataRequest.php (1)
SubscribePageDataRequest(10-21)src/Subscription/Request/SubscribePageRequest.php (1)
SubscribePageRequest(10-23)src/Subscription/Serializer/SubscribePageNormalizer.php (3)
SubscribePageNormalizer(11-42)__construct(13-16)normalize(21-33)
src/Subscription/Controller/BlacklistController.php (3)
src/Common/Controller/BaseController.php (2)
BaseController(15-41)requireAuthentication(28-40)src/Subscription/Request/AddToBlacklistRequest.php (1)
AddToBlacklistRequest(10-23)src/Subscription/Serializer/UserBlacklistNormalizer.php (3)
UserBlacklistNormalizer(11-42)__construct(13-15)normalize(20-33)
tests/Unit/Subscription/Serializer/UserBlacklistNormalizerTest.php (1)
src/Subscription/Serializer/UserBlacklistNormalizer.php (3)
UserBlacklistNormalizer(11-42)supportsNormalization(38-41)normalize(20-33)
src/Subscription/Controller/SubscriberAttributeValueController.php (1)
src/Common/Service/Provider/PaginatedDataProvider.php (1)
PaginatedDataProvider(17-56)
tests/Integration/Messaging/Controller/BounceRegexControllerTest.php (1)
tests/Integration/Identity/Fixtures/AdministratorFixture.php (1)
AdministratorFixture(14-59)
src/Identity/Controller/AdministratorController.php (1)
src/Common/Service/Provider/PaginatedDataProvider.php (1)
PaginatedDataProvider(17-56)
src/Subscription/Controller/SubscriberListController.php (3)
src/Subscription/Controller/SubscriberController.php (1)
__construct(35-45)src/Subscription/Controller/SubscriptionController.php (1)
__construct(34-45)src/Common/Service/Provider/PaginatedDataProvider.php (1)
PaginatedDataProvider(17-56)
src/Subscription/Controller/SubscriptionController.php (3)
src/Identity/Controller/AdminAttributeValueController.php (1)
__construct(33-46)src/Subscription/Controller/SubscriberController.php (1)
__construct(35-45)src/Subscription/Controller/SubscriberListController.php (1)
__construct(38-51)
src/Messaging/Controller/BounceRegexController.php (4)
src/Messaging/OpenApi/SwaggerSchemasResponse.php (1)
OA(9-140)src/Common/Controller/BaseController.php (2)
BaseController(15-41)requireAuthentication(28-40)src/Messaging/Request/CreateBounceRegexRequest.php (1)
CreateBounceRegexRequest(10-42)src/Messaging/Serializer/BounceRegexNormalizer.php (2)
BounceRegexNormalizer(10-41)normalize(15-32)
src/Messaging/Controller/CampaignController.php (1)
src/Messaging/Service/CampaignService.php (4)
__construct(23-29)CampaignService(21-93)createMessage(47-56)updateMessage(58-78)
tests/Unit/Subscription/Serializer/SubscribePageNormalizerTest.php (2)
src/Identity/Serializer/AdministratorNormalizer.php (1)
AdministratorNormalizer(12-41)src/Subscription/Serializer/SubscribePageNormalizer.php (3)
SubscribePageNormalizer(11-42)supportsNormalization(38-41)normalize(21-33)
tests/Unit/Messaging/Request/CreateBounceRegexRequestTest.php (1)
src/Messaging/Request/CreateBounceRegexRequest.php (2)
CreateBounceRegexRequest(10-42)getDto(31-41)
src/Subscription/Serializer/SubscribePageNormalizer.php (2)
src/Identity/Serializer/AdministratorNormalizer.php (1)
AdministratorNormalizer(12-41)src/Subscription/Controller/SubscribePageController.php (1)
__construct(27-35)
tests/Integration/Subscription/Controller/SubscribePageControllerTest.php (2)
tests/Integration/Identity/Fixtures/AdministratorFixture.php (1)
AdministratorFixture(14-59)tests/Integration/Subscription/Fixtures/SubscribePageFixture.php (1)
SubscribePageFixture(14-62)
src/Identity/Controller/AdminAttributeValueController.php (4)
src/Identity/Controller/AdminAttributeDefinitionController.php (1)
__construct(31-43)src/Subscription/Controller/SubscriberAttributeValueController.php (1)
__construct(32-44)src/Subscription/Controller/SubscriberListController.php (1)
__construct(38-51)src/Common/Service/Provider/PaginatedDataProvider.php (1)
PaginatedDataProvider(17-56)
src/Subscription/Controller/SubscriberController.php (3)
src/Common/Controller/BaseController.php (2)
BaseController(15-41)requireAuthentication(28-40)src/Subscription/Service/SubscriberHistoryService.php (2)
SubscriberHistoryService(18-53)getSubscriberHistory(26-52)src/Subscription/Controller/SubscriberImportController.php (2)
Route(22-158)Route(36-157)
⏰ 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). (4)
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
🔇 Additional comments (45)
src/Subscription/Controller/SubscriberImportController.php (1)
10-10: LGTM!The import statement is correctly added to support the new exception handling below.
tests/Integration/Messaging/Fixtures/MessageFixture.php (2)
61-99: Excellent refactoring to named parameters.The migration from positional to named constructor arguments significantly improves readability and maintainability across all domain model instantiations (MessageSchedule, MessageMetadata, MessageContent, MessageOptions, and Message).
69-69: Verify MessageStatus enum cases are accessible and contain the CSV value.The fixture code loads a MessageStatus enum value from CSV using
MessageStatus::from($row['status'])at line 69. The CSV file contains a single status value:0. While I can confirm MessageStatus is a backed enum from the externalphplist/corepackage, I cannot access the enum definition in the sandbox environment to verify that0maps to a valid enum case.Please manually confirm:
- The MessageStatus enum in phplist/core contains a case with backing value
0- Or update the CSV to use a valid status value if
0is not supportedIf the value is invalid,
MessageStatus::from()will throw aValueErrorat runtime.src/Subscription/Controller/SubscriberAttributeDefinitionController.php (5)
7-7: LGTM!The import is correctly added to support the EntityManager dependency.
31-43: LGTM!The EntityManager dependency is correctly injected using constructor property promotion with
readonlyfor immutability.
144-164: LGTM!The explicit flush after the update operation correctly persists changes before normalization and response generation, consistent with the PR's architectural change.
206-219: LGTM!The explicit flush after the delete operation correctly persists the removal before the response is sent.
84-96: Manual verification required: AttributeDefinitionManager class not found in codebase.The
AttributeDefinitionManagerclass referenced in the controller cannot be located via code search. While the controller's dependency injection clearly expects this class, its definition is not accessible. This may indicate the class is new in this PR, generated, or located outside the searchable codebase.Without access to the
create()method implementation, I cannot verify whether it internally callsflush(). Please confirm:
- The manager class exists and its
create()method does not independently call$this->entityManager->flush()src/Subscription/Controller/SubscriberAttributeValueController.php (2)
32-44: LGTM! EntityManager dependency properly injected.The addition of
EntityManagerInterfaceas a constructor dependency follows best practices and supports the explicit flush pattern being introduced throughout controllers.
197-198: LGTM! Explicit flush ensures deletion is persisted.The flush call after delete ensures the deletion is immediately persisted to the database, which is appropriate for this operation.
src/Identity/Controller/AdministratorController.php (1)
7-7: LGTM! Consistent persistence pattern.The EntityManagerInterface injection and flush calls after create, update, and delete operations ensure immediate persistence and align with the broader pattern applied across controllers in this PR.
Also applies to: 39-40, 154-154, 261-261, 310-310
src/Identity/Controller/SessionController.php (1)
7-7: LGTM! Consistent persistence for session operations.The EntityManagerInterface injection and flush calls after session creation and deletion ensure changes are immediately persisted, consistent with the broader pattern in this PR.
Also applies to: 38-38, 101-101, 169-169
src/Subscription/Request/SubscribePageDataRequest.php (1)
1-21: LGTM! Clean DTO implementation.The request class follows the established pattern with appropriate validation constraints and typed properties.
src/Identity/Controller/PasswordResetController.php (2)
7-7: Consistent persistence pattern for password reset operations.The EntityManagerInterface injection and flush calls after token generation and password updates ensure changes are immediately persisted, consistent with the broader pattern in this PR.
Also applies to: 33-33, 79-79, 176-176
117-126: Verify whether validation modifies state in the external phplist/core dependency.The
validatePasswordResetTokenmethod is from the externalphplist/corepackage and its implementation cannot be inspected in this repository. Without confirming whether this method modifies entity state (e.g., marking tokens as validated, updating timestamps), the flush at line 123 cannot be verified as necessary. Check the phplist/core source or documentation to confirm whether this validation method has side effects requiring persistence.src/Identity/Controller/AdminAttributeDefinitionController.php (1)
7-7: LGTM! Consistent persistence pattern.The EntityManagerInterface injection and flush calls after create, update, and delete operations ensure immediate persistence and align with the broader pattern applied across controllers in this PR.
Also applies to: 36-37, 94-94, 163-163, 219-219
src/Identity/Controller/AdminAttributeValueController.php (3)
33-46: LGTM: EntityManager injection follows project-wide pattern.The EntityManager injection and initialization are consistent with the broader refactoring across controllers in this PR.
124-133: LGTM: Explicit flush after create/update.The flush call correctly persists the attribute value changes immediately after the createOrUpdate operation.
196-204: LGTM: Explicit flush after delete.The flush call correctly persists the deletion immediately.
src/Subscription/Controller/SubscriberListController.php (3)
38-51: LGTM: EntityManager injection follows project pattern.The EntityManager dependency injection is consistent with the broader refactoring across controllers in this PR.
229-233: LGTM: Explicit flush after delete.The flush call ensures the list deletion is immediately persisted.
280-284: LGTM: Explicit flush after create.The flush call ensures the newly created list is immediately persisted.
src/Messaging/Request/Message/MessageMetadataRequest.php (1)
16-24: LGTM: Status string converted to enum.The change correctly converts the string status to a
MessageStatusenum usingfrom(). The@SuppressWarningsannotation appropriately handles the PHPMD static access warning for enum conversion.src/Messaging/Serializer/MessageNormalizer.php (1)
42-42: LGTM: Enum serialized as scalar value.The change correctly serializes the
MessageStatusenum as its scalar value rather than the enum object, which is appropriate for JSON output.tests/Unit/Messaging/Serializer/MessageNormalizerTest.php (2)
49-49: LGTM: Test updated for enum-based status.The test correctly constructs
MessageMetadatawith theMessageStatus::Draftenum, aligning with the enum-based status handling in the production code.
83-83: LGTM: Assertion validates enum value serialization.The assertion correctly validates that the status is serialized as the enum's scalar value.
tests/Unit/Messaging/Request/CreateBounceRegexRequestTest.php (1)
1-46: LGTM: Comprehensive unit tests for CreateBounceRegexRequest.The tests appropriately validate both explicit field population and default values in the DTO extraction.
src/Subscription/OpenApi/SwaggerSchemasResponse.php (1)
145-153: LGTM: SubscribePage schema added.The OpenAPI schema definition for
SubscribePageis well-structured and includes appropriate property types and references.tests/Integration/Messaging/Controller/CampaignControllerTest.php (1)
97-106: Verify that Message ID 2 is in a sendable state.The
MessageFixtureloads from Message.csv, which contains Message ID 2 with statussubmitted. However, theMessageStatusenum definition could not be located in the repository to confirm whethersubmittedis a valid sendable status. Review theMessageStatusenum (likely in the core package) and ensure thatsubmittedis an acceptable status for the send operation, or update the fixture if a different status is required.tests/Unit/Messaging/Serializer/BounceRegexNormalizerTest.php (1)
1-60: LGTM!The unit tests are well-structured and provide comprehensive coverage of the
BounceRegexNormalizerbehavior, including positive cases, type checking, and error handling.tests/Unit/Subscription/Serializer/SubscribePageNormalizerTest.php (1)
1-71: LGTM!The test properly validates the normalizer's behavior, including correct delegation to
AdministratorNormalizerfor nested owner data.src/Subscription/Request/AddToBlacklistRequest.php (1)
1-23: LGTM!The DTO is well-defined with appropriate validation constraints for adding entries to the blacklist.
src/Messaging/OpenApi/SwaggerSchemasResponse.php (1)
123-137: LGTM!The OpenAPI schema definition for
BounceRegexis complete and correctly mirrors the normalizer output structure.config/services/normalizers.yml (1)
97-107: LGTM!The new normalizer services are correctly registered following the established pattern used by existing normalizers.
src/Subscription/Controller/SubscriptionController.php (2)
7-7: LGTM!EntityManager injection follows proper dependency injection patterns and aligns with the broader refactoring across controllers in this PR.
Also applies to: 32-32, 39-39, 44-44
134-134: LGTM!The explicit flush calls after mutations ensure proper persistence and align with the systematic approach across this PR to manage transaction boundaries in controllers.
Also applies to: 201-201
config/services/messenger_handlers.yml (1)
1-5: LGTM!The message handler service registration follows Symfony best practices and correctly configures the handler for the messaging workflow.
src/Subscription/Request/SubscribePageRequest.php (1)
16-17: LGTM!The
activefield is properly defined with appropriate type validation.src/Messaging/Serializer/BounceRegexNormalizer.php (1)
10-41: LGTM!The normalizer follows the established pattern consistently, with proper type checking and comprehensive field mapping.
src/Messaging/Controller/TemplateController.php (2)
265-266: LGTM!Good refactor to store the created template before flushing. This ensures the normalized response reflects the actual persisted entity.
325-325: LGTM!The EntityManager flush call after deletion is consistent with the PR's objective to move persistence control to controllers.
tests/Unit/Messaging/Service/CampaignServiceTest.php (1)
40-45: LGTM!The test setup correctly reflects the updated CampaignService constructor. Using named arguments improves readability and makes the parameter mapping explicit.
tests/Unit/Subscription/Serializer/UserBlacklistNormalizerTest.php (1)
16-56: LGTM!Comprehensive test coverage validating normalization behavior, type support checks, and edge cases. The tests properly mock dependencies and verify interactions.
src/Subscription/Serializer/UserBlacklistNormalizer.php (1)
26-26: No issues found. The manager call is necessary.The verification confirms that UserBlacklist only exposes
getEmail()andgetAdded()methods (visible in the test mocks). ThegetBlacklistReason()call to the manager is not redundant—it's the intended design pattern where the manager provides business logic for fetching the reason, while the object provides only its direct attributes.src/Subscription/Serializer/SubscribePageNormalizer.php (1)
31-31: Verify whetherSubscribePage.getOwner()can returnnullin the phplist/core domain model.The concern is valid:
AdministratorNormalizer::normalize()throwsInvalidArgumentExceptionfor non-Administrator objects (including null). However, the review comment cannot be definitively resolved without confirming the nullability of thegetOwner()return type in the external phplist/core package.Evidence supporting the concern:
- No null safety check at line 31 before passing result to
adminNormalizer->normalize()- Test suite lacks a null-owner test case
- Similar pattern in
AdminAttributeValueNormalizer(line 28) also lacks null handling- Codebase shows awareness of nullable fields elsewhere (e.g.,
getValue() ?? ...)If
getOwner()can be null, add a null check or use a ternary:'owner' => $object->getOwner() ? $this->adminNormalizer->normalize($object->getOwner()) : null,
tests/Integration/Subscription/Controller/SubscribePageControllerTest.php
Show resolved
Hide resolved
tests/Integration/Subscription/Fixtures/SubscribePageFixture.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/Integration/Identity/Controller/SessionControllerTest.php(1 hunks)tests/Integration/Identity/Fixtures/AdministratorFixture.php(1 hunks)tests/Integration/Identity/Fixtures/AdministratorTokenFixture.php(1 hunks)tests/Integration/Subscription/Fixtures/SubscriberListFixture.php(2 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). (4)
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
🔇 Additional comments (4)
tests/Integration/Identity/Controller/SessionControllerTest.php (1)
103-103: Confirm missing test coverage for valid user with invalid password scenario.The change shifted this test from "valid user with wrong password" to "non-existent user with wrong password." Based on the test file, the "valid username + invalid password" scenario is not tested anywhere else—all other uses of
'john.doe'pair it with the valid password'Bazinga!'.This removes coverage for an important authentication edge case. Verify if this was intentional, or add a dedicated test for the "valid user + incorrect password" scenario to maintain security test coverage.
tests/Integration/Identity/Fixtures/AdministratorFixture.php (1)
32-33: LGTM! Good idempotency pattern for fixtures.The fixture now checks for existing Administrator entities before creating new ones, making it safe to run multiple times. This is a solid approach for test fixtures.
Also applies to: 41-45
tests/Integration/Identity/Fixtures/AdministratorTokenFixture.php (2)
45-46: LGTM! Good defensive practice.The deterministic login name pattern prevents fixture conflicts and makes the fixture more robust for repeated loads or when combined with other fixtures.
51-51: Constructor injection successfully implemented across the codebase.Verification confirms that all instantiations of
AdministratorTokenhave been updated to use the new constructor signature. The only direct instantiation found (line 51 in the fixture) correctly passes the$adminparameter, and no legacysetAdministrator()calls remain in the codebase.
tests/Integration/Subscription/Fixtures/SubscriberListFixture.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/Integration/Subscription/Controller/SubscribePageControllerTest.php (1)
82-89: Load auth fixtures before usingauthenticatedJsonRequest.This test uses an authenticated request but doesn’t load
AdministratorTokenFixture, causing intermittent 403s when state is clean. Please load the token fixture.- $this->loadFixtures([AdministratorFixture::class, SubscribePageFixture::class]); + $this->loadFixtures([ + AdministratorFixture::class, + AdministratorTokenFixture::class, + SubscribePageFixture::class, + ]);
🧹 Nitpick comments (10)
src/Messaging/Request/CreateBounceRegexRequest.php (4)
19-21: Ensure listOrder is non-negative.Guard against negative ordering with a dedicated constraint. Also, nice job resolving the earlier nullable/default mismatch.
#[Assert\Type('integer')] + #[Assert\PositiveOrZero] public int $listOrder = 0;
22-23: Constrain admin identifier to a positive integer.If this maps to an existing admin id, restrict to > 0 while still allowing null.
#[Assert\Type('integer')] + #[Assert\Positive] public ?int $admin = null;
31-41: Consider omitting nulls from the DTO payload.If consumers interpret “missing” vs “explicit null” differently, filter out nulls to reduce ambiguity.
- return [ + $data = [ 'regex' => $this->regex, 'action' => $this->action, 'listOrder' => $this->listOrder, 'admin' => $this->admin, 'comment' => $this->comment, 'status' => $this->status, - ]; + ]; + return array_filter($data, static fn($v) => $v !== null);If explicit nulls have meaning for your create flow, keep current behavior.
12-29: Minor: Type constraints may be redundant with typed props.Given strict_types and typed properties, some Assert\Type checks are defensive duplication. Keep them if you rely on serializer casting/validation for incoming scalars; otherwise, consider dropping to reduce noise.
src/Subscription/Controller/SubscribePageController.php (4)
98-106: OpenAPI: add explicittype: 'object'to request bodies.Swagger-php infers objects from
properties, but being explicit improves tooling.- requestBody: new OA\RequestBody( + requestBody: new OA\RequestBody( required: true, - content: new OA\JsonContent( + content: new OA\JsonContent( + type: 'object', properties: [ new OA\Property(property: 'title', type: 'string'), new OA\Property(property: 'active', type: 'boolean', nullable: true), ] ) ), @@ - content: new OA\JsonContent( + content: new OA\JsonContent( + type: 'object', properties: [ - new OA\Property(property: 'title', type: 'string', nullable: true), + new OA\Property(property: 'title', type: 'string', nullable: true), new OA\Property(property: 'active', type: 'boolean', nullable: true), ] ) ),Also applies to: 158-164
390-400: OpenAPI: includetype: 'object'for the 200 response schema.Minor clarity improvement for generators.
- new OA\Response( + new OA\Response( response: 200, description: 'Success', - content: new OA\JsonContent( + content: new OA\JsonContent( + type: 'object', properties: [ new OA\Property(property: 'id', type: 'integer'), new OA\Property(property: 'name', type: 'string'), new OA\Property(property: 'data', type: 'string', nullable: true), ], - type: 'object' ) ),
279-279: 204 should not include a body.
$this->json(null, 204)can emit anullbody. Prefer an empty response for 204.- return $this->json(null, Response::HTTP_NO_CONTENT); + return new Response('', Response::HTTP_NO_CONTENT);
151-164: Update DTO vs. docs: are fields optional on update?
updatePage()uses the same DTO as create; OpenAPI markstitle/activenullable (optional), but the DTO likely requirestitle. Either split DTOs (Create vs Update with optional fields) or marktitlenon-nullable in the update docs.Option A (recommended): introduce Update DTO with optional fields and use it in
updatePage():-use PhpList\RestBundle\Subscription\Request\SubscribePageRequest; +use PhpList\RestBundle\Subscription\Request\SubscribePageRequest; +use PhpList\RestBundle\Subscription\Request\SubscribePageUpdateRequest; @@ - /** @var SubscribePageRequest $updateRequest */ - $updateRequest = $this->validator->validate($request, SubscribePageRequest::class); + /** @var SubscribePageUpdateRequest $updateRequest */ + $updateRequest = $this->validator->validate($request, SubscribePageUpdateRequest::class);New DTO (src/Subscription/Request/SubscribePageUpdateRequest.php):
<?php declare(strict_types=1); namespace PhpList\RestBundle\Subscription\Request; use Symfony\Component\Validator\Constraints as Assert; final class SubscribePageUpdateRequest implements RequestInterface { #[Assert\Type('string')] #[Assert\Length(min: 1, max: 255)] public ?string $title = null; #[Assert\Type('bool')] public ?bool $active = null; public function getDto(): self { return $this; } }Also applies to: 200-225
tests/Integration/Subscription/Controller/SubscribePageControllerTest.php (2)
83-86: Align test payloads with intendedtitlesemantics.If you switch
titleto free text (recommended), update payloads to human-friendly titles.- $payload = json_encode([ - 'title' => '[email protected]', - 'active' => true, - ], JSON_THROW_ON_ERROR); + $payload = json_encode([ + 'title' => 'Welcome Page', + 'active' => true, + ], JSON_THROW_ON_ERROR); @@ - $payload = json_encode([ - 'title' => '[email protected]', - 'active' => false, - ], JSON_THROW_ON_ERROR); + $payload = json_encode([ + 'title' => 'Updated Welcome Page', + 'active' => false, + ], JSON_THROW_ON_ERROR);Also applies to: 125-127
240-253: Consider adding a create/update validation test for missing/invalidtitle.You already cover set-data 422. Add analogous tests for create/update to lock the contract. I can draft them if helpful.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.coderabbit.yaml(1 hunks)src/Messaging/Request/CreateBounceRegexRequest.php(1 hunks)src/Subscription/Controller/SubscribePageController.php(1 hunks)tests/Integration/Identity/Fixtures/AdminAttributeDefinitionFixture.php(1 hunks)tests/Integration/Identity/Fixtures/AdminAttributeValueFixture.php(1 hunks)tests/Integration/Identity/Fixtures/AdministratorFixture.php(1 hunks)tests/Integration/Identity/Fixtures/AdministratorTokenFixture.php(1 hunks)tests/Integration/Messaging/Fixtures/MessageFixture.php(2 hunks)tests/Integration/Messaging/Fixtures/TemplateFixture.php(1 hunks)tests/Integration/Subscription/Controller/SubscribePageControllerTest.php(1 hunks)tests/Integration/Subscription/Fixtures/SubscribePageFixture.php(1 hunks)tests/Integration/Subscription/Fixtures/SubscriberFixture.php(1 hunks)tests/Integration/Subscription/Fixtures/SubscriberListFixture.php(3 hunks)tests/Integration/Subscription/Fixtures/SubscriptionFixture.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Integration/Subscription/Fixtures/SubscribePageFixture.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Integration/Subscription/Fixtures/SubscriberListFixture.php (2)
tests/Integration/Identity/Fixtures/AdministratorFixture.php (1)
AdministratorFixture(14-70)tests/Integration/Identity/Fixtures/AdminAttributeValueFixture.php (1)
getDependencies(68-74)
src/Subscription/Controller/SubscribePageController.php (5)
src/Subscription/OpenApi/SwaggerSchemasResponse.php (1)
OA(9-156)src/Common/Controller/BaseController.php (2)
BaseController(15-41)requireAuthentication(28-40)src/Subscription/Request/SubscribePageDataRequest.php (1)
SubscribePageDataRequest(10-21)src/Subscription/Request/SubscribePageRequest.php (1)
SubscribePageRequest(10-23)src/Subscription/Serializer/SubscribePageNormalizer.php (3)
SubscribePageNormalizer(11-42)__construct(13-16)normalize(21-33)
tests/Integration/Subscription/Controller/SubscribePageControllerTest.php (3)
tests/Integration/Identity/Fixtures/AdministratorFixture.php (1)
AdministratorFixture(14-70)tests/Integration/Identity/Fixtures/AdministratorTokenFixture.php (1)
AdministratorTokenFixture(15-65)tests/Integration/Subscription/Fixtures/SubscribePageFixture.php (1)
SubscribePageFixture(14-65)
⏰ 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). (4)
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
🔇 Additional comments (17)
.coderabbit.yaml (1)
1-16: Configuration looks solid—schema-compliant YAML for the CodeRabbit reviewer.All properties conform to the CodeRabbit configuration schema. The
chillreview profile with high-level summaries and collapsed walkthroughs makes sense for the PR guidance to keep things relaxed and focused.One tiny note: Line 3 sets
tone_instructions: "chill", which is valid but pretty minimal. Since you've already specifiedreviews.profile: "chill"(line 5), thetone_instructionscould either be removed or expanded into a more descriptive guideline if you want to override or augment the predefined profile behavior. The schema allows up to 250 characters, so you could add something like"Be relaxed and supportive in review tone, focusing on high-impact items."if that aligns with your intent.Also, line 15
base_branches: [".*"]will enable auto-review on all branches—confirm that's intentional.tests/Integration/Messaging/Fixtures/MessageFixture.php (2)
48-50: Nice defensive check for CSV integrity.Catching the
array_combinefailure here prevents cryptic errors downstream if the CSV data is malformed. The error message is clear and helpful.
65-103: Solid refactor to named arguments.Switching from positional to named arguments across all the Message-related constructors makes this code much easier to read and maintain. The explicit
MessageStatus::from()conversion is also a nice touch. No concerns here.tests/Integration/Identity/Fixtures/AdministratorTokenFixture.php (3)
40-42: Nice defensive check!Catching the
array_combinefailure upfront with a clear error message will save debugging time if the CSV ever gets malformed.
48-49: Good call on the deterministic naming.The
admin_{id}pattern makes fixture data predictable and the comment clearly explains why. Works well for avoiding collisions.
54-54: Constructor injection FTW.Passing
$admindirectly to the constructor enforces the relationship at creation time—cleaner than relying on a separate setter call. Assuming other usages across the PR got the same treatment.tests/Integration/Identity/Fixtures/AdminAttributeValueFixture.php (1)
43-45: Good defensive validation.The header/data length mismatch check prevents downstream errors when CSV rows are malformed. This pattern is consistently applied across all fixtures in the PR.
tests/Integration/Identity/Fixtures/AdministratorFixture.php (1)
32-32: Excellent addition of idempotency.Fetching the repository and checking for existing administrators prevents duplicate creation when fixtures are re-run. Combined with the CSV validation, this makes the fixture more robust.
Also applies to: 40-48
tests/Integration/Subscription/Fixtures/SubscriberListFixture.php (2)
9-9: Past concern resolved with fixture dependencies.Implementing
DependentFixtureInterfaceand declaringAdministratorFixtureas a dependency ensures administrators exist with correct IDs before this fixture runs. This addresses the ID conflation concern from the previous review.Also applies to: 14-14, 17-17, 80-85
36-36: Nice optimization with admin caching.The
adminsByIdcache reduces repeated repository lookups for the same owner. The deterministicowner_Xlogin name pattern and CSV validation both improve robustness.Also applies to: 44-59
tests/Integration/Messaging/Fixtures/TemplateFixture.php (1)
38-40: Consistent validation pattern.The CSV validation aligns with the pattern applied across all fixtures in this PR, preventing malformed data from causing issues downstream.
tests/Integration/Identity/Fixtures/AdminAttributeDefinitionFixture.php (1)
38-40: Solid validation check.This validation ensures CSV integrity and matches the approach used throughout the fixture improvements in this PR.
tests/Integration/Subscription/Fixtures/SubscriptionFixture.php (1)
44-46: Robust CSV validation.The header/data mismatch check is a good defensive measure, consistent with the improvements across all fixtures in this PR.
tests/Integration/Subscription/Fixtures/SubscriberFixture.php (1)
39-41: Clean validation implementation.This completes the consistent CSV validation pattern applied across all fixtures, improving error handling for malformed test data.
src/Messaging/Request/CreateBounceRegexRequest.php (1)
16-18: Verify allowed values for action and status before adding Choice constraints.The suggestion to use
Assert\Choiceis sound—similar request classes in the codebase (e.g.,SubscribersExportRequest) use it. However, tests show only single examples:action='delete'andstatus='active'. Without comprehensive coverage of all valid values, or access to the core domain model (PhpList\Core\Domain\Messaging\Model\BounceRegex), the exact choice list cannot be determined from this repository alone.Before implementing, confirm with your domain or documentation:
- All valid
actionvalues (e.g., 'delete', 'bounce', 'report', etc.)- All valid
statusvalues (e.g., 'active', 'inactive', etc.)Then apply:
#[Assert\Choice(choices: ['delete', ...], message: 'Invalid action.')] public ?string $action = null; #[Assert\Choice(choices: ['active', ...], message: 'Invalid status.')] public ?string $status = null;src/Subscription/Controller/SubscribePageController.php (2)
145-149: Nice: flush moved into controllers after mutating ops.Flush after create/update/delete/setData aligns with the PR goal and fixes the previously flagged persistence gap. Looks good.
Also applies to: 222-224, 276-279, 430-433
216-221: Verify owner reassignment behavior is intentional.The
owner: $adminparameter is always passed unconditionally to the manager'supdatePage()method (line 220). While theSubscribePageManageris imported from an external package (PhpList\Core\Domain\Subscription\Service\Manager), confirm this unconditional reassignment aligns with business logic—if ownership should remain unchanged during updates, passnullor conditionally set the owner instead.
| #[Assert\NotBlank] | ||
| #[Assert\Type('string')] | ||
| public string $regex; | ||
|
|
There was a problem hiding this comment.
Trim and validate the regex input.
- Trim blanks so " " fails validation.
- Add a compile check to avoid invalid PCRE patterns causing runtime warnings later.
Apply:
- #[Assert\NotBlank]
+ #[Assert\NotBlank(normalizer: 'trim')]
#[Assert\Type('string')]
public string $regex;Add this class-level callback (and import ExecutionContextInterface) to ensure the pattern compiles:
use Symfony\Component\Validator\Context\ExecutionContextInterface;
#[Assert\Callback('validateRegexPattern')]
public function validateRegexPattern(ExecutionContextInterface $context): void
{
if (!isset($this->regex)) {
return;
}
set_error_handler(static function () { /* suppress preg warnings */ });
$ok = @preg_match($this->regex, '');
restore_error_handler();
if ($ok === false) {
$context->buildViolation('Invalid regular expression pattern.')
->atPath('regex')
->addViolation();
}
}🤖 Prompt for AI Agents
In src/Messaging/Request/CreateBounceRegexRequest.php around lines 12-15, the
regex field currently only has NotBlank/Type assertions but needs trimming and a
compile-time check; add the Assert\Callback attribute on the class and import
Symfony\Component\Validator\Context\ExecutionContextInterface and
Assert\Callback, then implement a validateRegexPattern(ExecutionContextInterface
$context): void method that trims $this->regex (so values like " " become
empty and trigger NotBlank), returns if regex is not set/empty, and otherwise
suppresses preg warnings (set_error_handler/restore_error_handler or @) and
attempts preg_match on the pattern; if preg_match returns false add a violation
on 'regex' with message "Invalid regular expression pattern." to prevent invalid
PCRE from reaching runtime.
| properties: [ | ||
| new OA\Property(property: 'title', type: 'string'), | ||
| new OA\Property(property: 'active', type: 'boolean', nullable: true), | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Contract mismatch: title validated as email vs docs/examples as human title.
Docs show a human-readable title, but the DTO (SubscribePageRequest) uses Assert\Email; tests send email-like strings to pass validation. Pick one and align DTO, docs, and tests. Recommended: treat title as free text.
Proposed change (in src/Subscription/Request/SubscribePageRequest.php):
class SubscribePageRequest implements RequestInterface
{
- #[Assert\NotBlank]
- #[Assert\Email]
- public string $title;
+ #[Assert\NotBlank]
+ #[Assert\Type('string')]
+ #[Assert\Length(min: 1, max: 255)]
+ public string $title;If you adopt this, also update the tests to use human titles (see test comments), and keep the OpenAPI examples as-is.
Also applies to: 159-163
🤖 Prompt for AI Agents
In src/Subscription/Controller/SubscribePageController.php around lines 101-105
and 159-163, the OpenAPI/docs expect a human-readable page title but the DTO
validation currently treats `title` as an email; change the DTO
(src/Subscription/Request/SubscribePageRequest.php) to validate `title` as free
text (e.g., NotBlank and/or Length constraints) instead of Assert\Email, keep
the OA\Property type as string, and update any tests to send human-readable
titles (not email strings) so docs, DTO and tests are consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/Integration/Subscription/Fixtures/SubscribePageFixture.php (1)
40-40: Add validation for array_combine return value.The
array_combine()call can returnfalsewhen header and data arrays have mismatched lengths, which would break the next line. This was flagged previously but the fix doesn't appear in the current code.Apply this diff to handle the case:
$row = array_combine($headers, $data); + if ($row === false) { + throw new RuntimeException('Malformed CSV data: header/data length mismatch.'); + }
🧹 Nitpick comments (2)
tests/Integration/Subscription/Fixtures/SubscribePageFixture.php (2)
51-57: Consider adding an idempotency check.Unlike
AdministratorFixture, this doesn't check if a SubscribePage with the given ID already exists. If the fixture loads multiple times in a test suite, you might get duplicate key errors or duplicate data. Adding a check would make this more robust.Here's a pattern from AdministratorFixture you could adapt:
+ $pageRepository = $manager->getRepository(SubscribePage::class); + + // Make fixture idempotent: skip if page with this ID already exists + $existing = $pageRepository->find($row['id']); + if ($existing instanceof SubscribePage) { + continue; + } + $page = new SubscribePage(); $this->setSubjectId($page, (int)$row['id']); $page->setTitle($row['title']); $page->setActive(filter_var($row['active'], FILTER_VALIDATE_BOOLEAN)); $page->setOwner($owner); $manager->persist($page);
42-49: Cache administrator lookups for better performance.If multiple pages share the same owner, you'll hit the database for each row.
SubscriberListFixturecaches admins in an$adminsByIdarray to avoid repeated queries. Not critical for small test fixtures, but nice for consistency.Pattern from SubscriberListFixture:
$adminRepository = $manager->getRepository(Administrator::class); + $adminsById = []; do { $data = fgetcsv($handle); if ($data === false) { break; } $row = array_combine($headers, $data); - $owner = $adminRepository->find($row['owner']); + $ownerId = (int)$row['owner']; + $owner = $adminsById[$ownerId] ?? $adminRepository->find($ownerId); if ($owner === null) { $owner = new Administrator(); - $this->setSubjectId($owner, (int)$row['owner']); + $this->setSubjectId($owner, $ownerId); $owner->setLoginName('subscribepage_owner_' . $ownerId); $owner->setSuperUser(true); $owner->setDisabled(false); $manager->persist($owner); + $adminsById[$ownerId] = $owner; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/PULL_REQUEST_TEMPLATE.md(1 hunks)tests/Integration/Identity/Fixtures/AdministratorFixture.php(1 hunks)tests/Integration/Identity/Fixtures/AdministratorTokenFixture.php(1 hunks)tests/Integration/Messaging/Fixtures/MessageFixture.php(2 hunks)tests/Integration/Subscription/Fixtures/SubscribePageFixture.php(1 hunks)tests/Integration/Subscription/Fixtures/SubscriberListFixture.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Integration/Identity/Fixtures/AdministratorFixture.php
- tests/Integration/Subscription/Fixtures/SubscriberListFixture.php
- tests/Integration/Identity/Fixtures/AdministratorTokenFixture.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Integration/Subscription/Fixtures/SubscribePageFixture.php (3)
tests/Integration/Identity/Fixtures/AdministratorFixture.php (1)
load(17-66)tests/Integration/Identity/Fixtures/AdministratorTokenFixture.php (1)
load(18-61)tests/Integration/Subscription/Fixtures/SubscriberListFixture.php (1)
load(20-75)
⏰ 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). (4)
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
🔇 Additional comments (3)
.github/PULL_REQUEST_TEMPLATE.md (1)
1-1: PR template looks stripped down—was this intentional?Line 1 contains just a CodeRabbit AI prompt (
@coderabbitai summary), whereas PR templates typically provide structured guidance for contributors (e.g., description, type of change, testing, checklists). The drastic simplification suggests this might be an incomplete or accidental change.If the goal is to simplify the template, consider whether minimal guidance is still helpful. Otherwise, this should probably be restored to provide contributor clarity.
tests/Integration/Messaging/Fixtures/MessageFixture.php (2)
62-100: Nice refactor to named arguments!The shift from positional to named constructor arguments across all value objects and the
Messageentity makes the fixture data mapping explicit and easier to maintain. This reduces the risk of subtle bugs from argument reordering.
49-50: Verify null safety for admin and template lookups.If the CSV references IDs that don't exist,
find()returns null, which gets passed to theMessageconstructor. TheMessageclass is imported from an external package (PhpList\Core), so I couldn't inspect its constructor signature to verify whether it accepts nullable values. Ensure the constructor can handle null values or that your fixture CSV always contains valid IDs for existing admin and template entities.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/Integration/Subscription/Fixtures/SubscribePageFixture.php (1)
42-49: Set loginName before persisting fallback Administrator.When we create a fallback
PhpList\Core\Domain\Identity\Model\Administrator, we still never assign aloginName. Doctrine will try to persist a row with a null login, tripping the not-null/unique constraint and blowing up the fixture load. Please mirror the other fixtures and give it a deterministic value beforepersist().if ($owner === null) { $owner = new Administrator(); $this->setSubjectId($owner, (int)$row['owner']); + $owner->setLoginName('subscribepage_owner_' . $row['owner']); $owner->setSuperUser(true); $owner->setDisabled(false); $manager->persist($owner); }
🧹 Nitpick comments (5)
src/Subscription/Request/SubscribePageRequest.php (1)
16-17: Clarify the nullable design of theactiveproperty.The
?booltype hint with a default value offalseis inconsistent. With the default, this property will never actually benullunless explicitly set tonull, making the nullable type hint unnecessary.Consider one of these approaches:
- If
activeshould always have a boolean value when not provided:public bool $active = false;- If
activeshould be distinguishable as "not set" vs "set to false":public ?bool $active = null;Apply this diff if
activeshould always be a boolean:- #[Assert\Type(type: 'bool')] - public ?bool $active = false; + #[Assert\Type(type: 'bool')] + public bool $active = false;Or apply this diff if you need to distinguish "not provided" from "false":
#[Assert\Type(type: 'bool')] - public ?bool $active = false; + public ?bool $active = null;src/Messaging/Request/CreateTemplateRequest.php (1)
15-17: Good addition of the trim normalizer.Trimming whitespace on the title field before validation is a sensible improvement.
You might consider whether the
contentfield (line 19) should also usenormalizer: 'trim'for consistency, though preserving whitespace in content might be intentional.src/Messaging/Request/CreateBounceRegexRequest.php (2)
13-13: Consider adding trim normalizer to prevent whitespace-only regex.A past review suggested adding
normalizer: 'trim'to theNotBlankassertion. Without it, whitespace-only strings like" "would pass validation even though they're not meaningful regex patterns.Apply this diff:
- #[Assert\NotBlank] + #[Assert\NotBlank(normalizer: 'trim')]Based on past review comments.
50-52: Optional: Error handler doesn't need to return true.The error handler here suppresses warnings, and returning
trueworks fine, but conventionally these handlers just return nothing or have an empty body. It's a minor style preference.If you prefer, you could simplify to:
- set_error_handler(static function () { - return true; - }); + set_error_handler(static function () { + // Suppress preg_match warnings + });tests/Unit/Messaging/Request/CreateBounceRegexRequestTest.php (1)
49-61: Optional: Remove redundant assertion.The meaningful assertion here is the mock's
expects($this->never())expectation on line 55. Line 60's$this->assertTrue(true)is a no-op that doesn't add value.You could remove line 60 entirely, or if you prefer an explicit success marker, use:
- // if no exception and no violation calls, the test passes - $this->assertTrue(true); + // Test passes if buildViolation was never called
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Identity/Request/CreateSessionRequest.php(1 hunks)src/Identity/Request/UpdateAttributeDefinitionRequest.php(1 hunks)src/Identity/Request/ValidateTokenRequest.php(1 hunks)src/Messaging/Request/CreateBounceRegexRequest.php(1 hunks)src/Messaging/Request/CreateTemplateRequest.php(1 hunks)src/Subscription/Controller/SubscriberImportController.php(2 hunks)src/Subscription/Request/SubscribePageRequest.php(1 hunks)tests/Integration/Identity/Fixtures/AdministratorFixture.php(1 hunks)tests/Integration/Identity/Fixtures/AdministratorTokenFixture.php(1 hunks)tests/Integration/Messaging/Fixtures/MessageFixture.php(2 hunks)tests/Integration/Subscription/Fixtures/SubscribePageFixture.php(1 hunks)tests/Integration/Subscription/Fixtures/SubscriberListFixture.php(4 hunks)tests/Unit/Messaging/Request/CreateBounceRegexRequestTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Integration/Messaging/Fixtures/MessageFixture.php
- tests/Integration/Identity/Fixtures/AdministratorFixture.php
🧰 Additional context used
🧬 Code graph analysis (5)
tests/Unit/Messaging/Request/CreateBounceRegexRequestTest.php (1)
src/Messaging/Request/CreateBounceRegexRequest.php (2)
CreateBounceRegexRequest(11-63)getDto(32-42)
src/Subscription/Controller/SubscriberImportController.php (1)
tests/Helpers/DummyAnalyticsController.php (1)
json(12-15)
src/Subscription/Request/SubscribePageRequest.php (3)
src/Identity/Request/CreateSessionRequest.php (1)
getDto(20-23)src/Identity/Request/ValidateTokenRequest.php (1)
getDto(16-19)src/Subscription/Request/SubscribePageDataRequest.php (1)
getDto(17-20)
tests/Integration/Subscription/Fixtures/SubscribePageFixture.php (2)
tests/Integration/Identity/Fixtures/AdministratorTokenFixture.php (1)
load(18-61)tests/Integration/Subscription/Fixtures/SubscriberListFixture.php (1)
load(20-75)
tests/Integration/Subscription/Fixtures/SubscriberListFixture.php (2)
tests/Integration/Identity/Fixtures/AdministratorFixture.php (1)
AdministratorFixture(14-67)tests/Integration/Identity/Fixtures/AdminAttributeValueFixture.php (1)
getDependencies(65-71)
⏰ 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). (4)
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
🔇 Additional comments (13)
src/Subscription/Controller/SubscriberImportController.php (2)
10-10: LGTM – Import looks good.The addition of the
CouldNotReadUploadedFileExceptionimport is necessary for the specific catch block below. Nice and clean.
148-151: Nice fix – specific exception handling added.Good addition of the specific catch for
CouldNotReadUploadedFileExceptionbefore the genericException. This provides clearer error handling and a more informative response.Also noting that the spacing issue from the previous review has been addressed – the concatenation now correctly includes a space after "file."
src/Subscription/Request/SubscribePageRequest.php (1)
19-22: LGTM!The
getDto()implementation follows the established pattern from other request classes in the codebase.src/Identity/Request/UpdateAttributeDefinitionRequest.php (1)
13-13: Good addition of trim normalizer.Trimming whitespace before validation prevents whitespace-only names from passing validation. This is a sensible improvement.
src/Identity/Request/CreateSessionRequest.php (1)
12-18: Trimming credentials improves validation.Adding the trim normalizer to both
loginNameandpasswordprevents whitespace-only values from passing validation, which is standard practice for authentication fields.src/Identity/Request/ValidateTokenRequest.php (1)
12-12: Trimming tokens is sensible.Adding the trim normalizer prevents accidental whitespace from causing token validation failures. Good consistency with the other request DTOs.
src/Messaging/Request/CreateTemplateRequest.php (3)
9-9: Nice cleanup!Adding the import statement improves readability and avoids repeating the fully-qualified namespace throughout the file.
19-21: LGTM!Using the imported constraint instead of the fully-qualified name is cleaner and consistent with the new import.
23-24: Looks good!The refactor to use the imported constraint is consistent with the rest of the file. The constraint on the nullable
textfield should work fine since Symfony validators skip null values by default.src/Messaging/Request/CreateBounceRegexRequest.php (1)
32-42: LGTM!The DTO mapping is straightforward and includes all fields with their current values.
tests/Unit/Messaging/Request/CreateBounceRegexRequestTest.php (3)
14-32: LGTM!Comprehensive test that validates all DTO fields are correctly mapped.
34-47: LGTM!Good test coverage of default values for optional fields.
63-79: LGTM!Thorough test that validates the violation is properly built and added for invalid regex patterns.
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
Documentation
Tests