Feat/check max mail size #373
Conversation
📝 WalkthroughWalkthroughAdds message-size enforcement and content precaching: new config params and services (HTMLPurifier, MessageDataManager, MessagePrecacheService), a MessageSizeLimitExceededException, size checks in campaign handler that can suspend campaigns, and API changes to use MessageContent and subscriber-aware personalization. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Subscriber
participant Prep as MessageProcessingPreparator
participant Precache as MessagePrecacheService
participant Handler as CampaignProcessorMessageHandler
participant Mailer as RateLimitedCampaignMailer
participant Cache as CacheInterface
participant DataMgr as MessageDataManager
participant EventLog as EventLogManager
Subscriber->>Prep: processMessageLinks(campaignId, content, subscriber)
Prep->>Prep: personalize content (UserPersonalizer)
Prep->>Precache: getOrCacheBaseMessageContent(campaign)
Precache-->>Prep: base MessageContent
Prep-->>Handler: prepared MessageContent
Handler->>Mailer: composeEmail(campaign, subscriber, content)
Mailer-->>Handler: Email (rendered)
Handler->>Cache: get(sizeKey)
alt cache miss
Handler->>Handler: calculateEmailSize(Email)
else cache hit
Handler-->>Handler: use cached size
end
Handler->>Handler: compare size vs maxMailSize
alt size > maxMailSize
Handler->>DataMgr: setMessageData(campaign,"size",size)
Handler->>EventLog: record suspension/warning
Handler-->>Handler: throw MessageSizeLimitExceededException
else within limit
Handler->>Cache: set(sizeKey,size)
Handler->>EventLog: optional size info
Handler->>Mailer: proceed sending / ack
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (1)src/Domain/**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php (1)
⏰ 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)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/Domain/Subscription/Repository/SubscriberListRepository.php (1)
49-55: Implementation looks good; consider adding explicit element type to the return.The query is clean and stays read-only, which fits the domain-layer repository constraints. To help static analysis and keep consistency with
getListsByMessage(), you could document the element type of the returned array:- public function getAllActive(): array - { + /** @return SubscriberList[] */ + public function getAllActive(): array + { return $this->createQueryBuilder('l') ->where('l.active = true') ->getQuery() ->getResult(); }src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
37-37: TODO noted for HTML2Text functionality.This is a reasonable reminder. The text message is currently set directly from
getTextMessage(), but if you need auto-conversion from HTML, that'd be the place to wire it in.Want me to open an issue to track implementing HTML2Text conversion here?
src/Domain/Messaging/Service/Manager/MessageDataManager.php (1)
27-29: Potential side effect fromsession_name()call.Calling
session_name()when no session is active may return an empty string or the default name, which could lead to unexpected behavior. If the session isn't started, this check might not work as intended.Consider guarding the check or documenting the expected session state:
- if ($name === 'PHPSESSID' || $name === session_name()) { + if ($name === 'PHPSESSID' || (session_status() === PHP_SESSION_ACTIVE && $name === session_name())) { return; }src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
185-194: Minor inefficiency: redundant cache read.When the cache key doesn't exist, you calculate and cache
$size(line 186-188), but then immediately read it back from the cache on line 191. You could just reuse the local$sizevariable.+ $size = null; if (!$this->cache->has($cacheKey)) { $size = $this->calculateEmailSize($email); $this->messageDataManager->setMessageData($campaign, $sizeName, $size); $this->cache->set($cacheKey, $size); } - $size = $this->cache->get($cacheKey); + $size ??= $this->cache->get($cacheKey);
217-226: Email size calculation approach.Using
$email->toIterable()to compute the size is reasonable. The TODO comment on line 224 seems outdated sincesetMessageDatais already called incheckMessageSizeOrSuspendCampaign.foreach ($email->toIterable() as $line) { $size += strlen($line); } - // todo: setMessageData($messageid, $sizename, $mail->mailsize); return $size;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
config/PhpCodeSniffer/ruleset.xml(1 hunks)config/parameters.yml.dist(1 hunks)config/services.yml(1 hunks)config/services/managers.yml(1 hunks)config/services/repositories.yml(1 hunks)src/Domain/Messaging/Exception/MessageSizeLimitExceededException.php(1 hunks)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php(5 hunks)src/Domain/Messaging/Repository/MessageDataRepository.php(1 hunks)src/Domain/Messaging/Service/Manager/MessageDataManager.php(1 hunks)src/Domain/Messaging/Service/MessageProcessingPreparator.php(4 hunks)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php(1 hunks)src/Domain/Subscription/Repository/SubscriberListRepository.php(1 hunks)src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php(0 hunks)tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php(6 hunks)tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php(8 hunks)
💤 Files with no reviewable changes (1)
- src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Subscription/Repository/SubscriberListRepository.phpsrc/Domain/Messaging/Exception/MessageSizeLimitExceededException.phpsrc/Domain/Messaging/Repository/MessageDataRepository.phpsrc/Domain/Messaging/Service/MessageProcessingPreparator.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/Service/Manager/MessageDataManager.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (5)
src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
src/Domain/Analytics/Service/LinkTrackService.php (3)
LinkTrackService(13-98)isExtractAndSaveLinksApplicable(30-33)extractAndSaveLinks(41-80)
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
src/Domain/Messaging/Service/Manager/MessageDataManager.php (1)
MessageDataManager(11-111)
tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php (1)
src/Domain/Messaging/Service/MessageProcessingPreparator.php (2)
MessageProcessingPreparator(16-104)processMessageLinks(63-92)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (3)
src/Domain/Messaging/Exception/MessageSizeLimitExceededException.php (2)
MessageSizeLimitExceededException(9-31)__construct(11-20)src/Domain/Messaging/Service/Manager/MessageDataManager.php (3)
MessageDataManager(11-111)__construct(13-16)setMessageData(25-67)src/Domain/Messaging/Message/SubscriberConfirmationMessage.php (1)
hasHtmlEmail(39-42)
src/Domain/Messaging/Service/Manager/MessageDataManager.php (2)
src/Domain/Messaging/Repository/MessageDataRepository.php (2)
MessageDataRepository(12-20)findByIdAndName(16-19)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
__construct(42-60)
⏰ 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 (15)
config/PhpCodeSniffer/ruleset.xml (1)
33-35: Why disable the TODO comment rule?Disabling the
Generic.Commenting.Todorule (by setting severity to 0) will suppress PHP CodeSniffer warnings for TODO comments throughout the codebase. This could allow incomplete work or deferred issues to slip through code review undetected.Before merging, please clarify the rationale:
- Are there legitimate TODOs in the newly added messaging/size-limit code that require this suppression?
- If so, consider documenting why these TODOs cannot be addressed before merge.
- If not, this rule should remain enabled to catch unfinished work.
config/services.yml (1)
54-57: Service wiring looks good.The
CampaignProcessorMessageHandleris properly configured with autowiring and themaxMailSizeparameter injection. This aligns with the new size-limiting functionality.config/services/repositories.yml (1)
144-147: Repository declaration follows the established pattern.Looks consistent with the other repository definitions in this file. Good to go.
config/parameters.yml.dist (1)
90-91: Default max mail size is 200MB — verify this is intentional.The default
209715200bytes equals 200MB. That's pretty generous for email! Typical email size limits are much smaller (10-25MB for most providers). If this is meant to be a safety net rather than a strict limit, it's fine, but you might want to document the reasoning or consider a more conservative default.Also, environment variables come in as strings. Make sure the handler casts this to
intbefore size comparisons.config/services/managers.yml (1)
114-116: Manager service follows the existing pattern.Consistent with other manager declarations in this file. Nice and clean.
src/Domain/Messaging/Exception/MessageSizeLimitExceededException.php (1)
9-31: Clean exception implementation.Good use of constructor property promotion, informative error message, and getters for programmatic access to the size values. This will make debugging and logging much easier when size limits are exceeded.
tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php (3)
35-41: Smart mock setup for UserPersonalizer.Returning the input text unchanged ensures the link replacement assertions stay valid. Good thinking — this isolates the link processing behavior from personalization side effects.
132-146: Test correctly updated for Subscriber-based signature.The switch from
int $userIdtoSubscriber $subscriberis properly reflected here. The mock provides bothgetId()andgetEmail()which aligns with the implementation's usage.
175-218: Integration test covers the happy path well.The test verifies link replacement with multiple links, and the assertions for the updated endpoint format (with leading slash) match the implementation in
replaceLinks().src/Domain/Messaging/Repository/MessageDataRepository.php (1)
16-19: Read-only query method — approved.This is a clean, side-effect-free query on the composite primary key (
id+name) of theMessageDataentity. No domain purity concerns.tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (2)
64-80: Test setup properly updated for new dependencies.The constructor invocation now includes all the new dependencies (
cache,eventLogManager,messageDataManager,maxMailSize). The mocks are appropriately created and the test setup aligns with the handler's updated signature.
184-187: Method signature update correctly reflected.The test now passes a
Subscriberobject toprocessMessageLinksinstead of a plain ID, matching the updated API. Nice.src/Domain/Messaging/Service/MessageProcessingPreparator.php (2)
18-19: Good catch on the typo fix.Renaming
LINT_TRACK_ENDPOINTtoLINK_TRACK_ENDPOINT— solid correction.
63-92: Method signature update and personalization integration look good.The switch from
int $userIdtoSubscriber $subscriberprovides better type safety and enables direct access to subscriber properties for personalization. The personalization step is cleanly integrated after link replacement.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
40-60: Constructor and dependency injection look solid.The new dependencies (
cache,eventLogManager,messageDataManager) are appropriately injected. ThemaxMailSizeparameter with nullable type and default fallback to 0 is a clean pattern for optional configuration.Per coding guidelines for MessageHandlers, persistence operations are allowed here since this is the orchestration boundary.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
156-163: Size check still occurs after sending the email.The previous review flagged this: the size check on line 159 happens after
$this->mailer->send($email)on line 158. This means the first oversized email for a campaign will be delivered before suspension kicks in.If the goal is to prevent oversized emails from going out at all, move the size check before the
send()call.
🧹 Nitpick comments (3)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (3)
186-195: Potential type mismatch from cache retrieval.On line 192,
$this->cache->get($cacheKey)returnsmixed. While the cache was just set on line 189, if the cache implementation fails silently or has TTL issues, this could returnnull, leading to a comparisonnull <= $this->maxMailSizeon line 193.Consider using the value already computed or adding a type guard:
- $size = $this->cache->get($cacheKey); + $size = (int) $this->cache->get($cacheKey);Or reuse the computed value:
+ $size = null; if (!$this->cache->has($cacheKey)) { $size = $this->calculateEmailSize($email); $this->messageDataManager->setMessageData($campaign, $sizeName, $size); $this->cache->set($cacheKey, $size); } - $size = $this->cache->get($cacheKey); + $size ??= (int) $this->cache->get($cacheKey);
204-213: Consider consolidating duplicate event log entries.Two separate
eventLogManager->log()calls are made for the same suspension event. You could combine them into a single, more informative log entry:- $this->eventLogManager->log('send', sprintf( - 'Message too large (%d is over %d), suspending', - $size, - $this->maxMailSize - )); - - $this->eventLogManager->log('send', sprintf( - 'Campaign %d suspended. Message too large', - $campaign->getId() - )); + $this->eventLogManager->log('send', sprintf( + 'Campaign %d suspended: message too large (%d bytes exceeds limit of %d bytes)', + $campaign->getId(), + $size, + $this->maxMailSize + ));
218-227: Stale TODO comment.The TODO on line 225 (
// todo: setMessageData($messageid, $sizename, $mail->mailsize);) appears to reference functionality that's already implemented on line 188 incheckMessageSizeOrSuspendCampaign. Consider removing it to avoid confusion.- // todo: setMessageData($messageid, $sizename, $mail->mailsize); return $size;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.coderabbit.yaml(0 hunks)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php(5 hunks)
💤 Files with no reviewable changes (1)
- .coderabbit.yaml
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (5)
src/Domain/Messaging/Exception/MessageSizeLimitExceededException.php (2)
MessageSizeLimitExceededException(9-31)__construct(11-20)src/Domain/Messaging/Service/Manager/MessageDataManager.php (3)
MessageDataManager(11-111)__construct(13-16)setMessageData(25-67)src/Domain/Messaging/Service/MessageProcessingPreparator.php (3)
MessageProcessingPreparator(16-104)__construct(21-28)processMessageLinks(63-92)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (4)
RateLimitedCampaignMailer(13-51)__construct(17-21)composeEmail(23-40)send(45-50)src/Domain/Messaging/Message/SubscriberConfirmationMessage.php (1)
hasHtmlEmail(39-42)
⏰ 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 (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
40-60: LGTM on constructor setup.The property promotion pattern is clean, and the null coalescing to
0for$maxMailSizeeffectively disables the size check when not configured. Nice and tidy.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/Domain/Messaging/Service/Manager/MessageDataManager.php (2)
85-88: Regex backreferences won't match as intended.The
\2and\1backreferences inside character classes don't work the way you'd expect in PCRE. This was flagged in a prior review and still needs attention.
101-112: Domain-layer persistence call flagged previously.The
persist()call here was already called out in a past review. Per the coding guidelines, domain-layer code should avoid direct persistence - that belongs in the orchestration boundary (MessageHandler).src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
156-166: First oversized email still sent before suspension kicks in.This was flagged before - the size check happens after
send(), so the first oversized email goes out. If that's intentional (measure actual sent size), cool - just want to confirm that's the design choice here.
🧹 Nitpick comments (4)
src/Domain/Messaging/Service/Manager/MessageDataManager.php (1)
35-61: Consider removing commented-out code before merging.Large commented blocks like the targetlist handling add noise. If it's needed later, it can be retrieved from version control. If there's a specific reason to keep it, maybe add a TODO issue reference.
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
179-219: Size checking and suspension logic looks reasonable.The caching prevents repeated calculations, logging provides good visibility, and the exception flow handles campaign suspension cleanly. One thought: consider whether both event log entries (lines 207-216) are necessary or could be consolidated.
40-60: Constructor is getting hefty with 16 dependencies.Not blocking, but worth considering whether some of these could be grouped or if the handler is taking on too much. The PHPMD suppression hints at this already.
src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
78-78: TODO noted - worth tracking.If there's other configured data that affects mail formatting, might be worth opening an issue to track this so it doesn't get lost.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php(5 hunks)src/Domain/Messaging/Service/Manager/MessageDataManager.php(1 hunks)src/Domain/Messaging/Service/MessageProcessingPreparator.php(4 hunks)tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Manager/MessageDataManager.phpsrc/Domain/Messaging/Service/MessageProcessingPreparator.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (3)
tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php (1)
src/Domain/Messaging/Service/MessageProcessingPreparator.php (2)
MessageProcessingPreparator(16-104)processMessageLinks(63-92)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (3)
src/Domain/Messaging/Exception/MessageSizeLimitExceededException.php (2)
MessageSizeLimitExceededException(9-31)__construct(11-20)src/Domain/Messaging/Service/Manager/MessageDataManager.php (3)
MessageDataManager(12-113)__construct(14-18)setMessageData(27-69)src/Domain/Messaging/Message/SubscriberConfirmationMessage.php (1)
hasHtmlEmail(39-42)
src/Domain/Messaging/Service/Manager/MessageDataManager.php (2)
src/Domain/Messaging/Repository/MessageDataRepository.php (2)
MessageDataRepository(12-20)findByIdAndName(16-19)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
__construct(42-60)
⏰ 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 (8)
tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php (3)
35-41: Nice mock setup for the personalization passthrough.This callback approach ensures your link replacement assertions stay valid while the personalization dependency is satisfied. Clean and clear.
129-149: Test coverage for non-applicable link tracking looks solid.Properly verifies that when link tracking isn't applicable, the message is returned unchanged and no downstream methods are called.
175-218: Good integration test for the link extraction and replacement flow.The test covers the full path with multiple links, HTML content, and footer processing. Using
stringContainswith the updatedLINK_TRACK_ENDPOINTconstant is appropriate.src/Domain/Messaging/Service/Manager/MessageDataManager.php (1)
63-69: Serialization of arrays/objects usesserialize()- watch for unserialize vulnerabilities.When this data is later read and potentially unserialized, ensure proper handling to avoid object injection. Just something to keep in mind for the consuming code.
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
221-230:toIterable()returns the raw MIME stream chunks.This gives you the actual serialized email size including headers and encoding overhead, which is probably what you want for mail server limits. Just noting that this will be larger than just body content size.
src/Domain/Messaging/Service/MessageProcessingPreparator.php (3)
19-19: Typo fix in constant name - nice catch.
LINT_TRACK_ENDPOINT→LINK_TRACK_ENDPOINT. Much better.
96-100: Track URL construction looks correct now.The previous double-slash issue appears to be resolved - no extra
/prepended to the endpoint constant.
63-92: Clean refactor to use Subscriber object instead of raw ID.Passing the full
Subscriberenables both ID access for link tracking and email access for personalization without additional lookups. The personalization calls after link replacement make sense for the content flow.
TatevikGr
force-pushed
the
feat/check-max-mail-size
branch
from
1cefd9d to
6a5c209
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
156-167: Size check should happen before send, andUserMessageStatuson size error is likely wrongRight now the flow is:
$email = $this->mailer->composeEmail($processed, $subscriber); $this->mailer->send($email); $this->checkMessageSizeOrSuspendCampaign(...);which means the first oversized email is still sent before the campaign is suspended. If the max-mail-size is intended as a hard guard, this is a functional bug: oversize detection only kicks in after successfully delivering one oversize mail.
Also, in the
MessageSizeLimitExceededExceptioncatch you mark theUserMessageasSent, which will become incorrect once the check is moved beforesend()(the mail won’t have been sent at all).Concrete suggestion:
- Run the size check before sending.
- On
MessageSizeLimitExceededException, mark theUserMessageasNotSent(or introduce a dedicated status) so reporting stays truthful.For example:
try { $email = $this->mailer->composeEmail($processed, $subscriber); - $this->mailer->send($email); - $this->checkMessageSizeOrSuspendCampaign($campaign, $email, $subscriber->hasHtmlEmail()); + $this->checkMessageSizeOrSuspendCampaign($campaign, $email, $subscriber->hasHtmlEmail()); + $this->mailer->send($email); $this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent); } catch (MessageSizeLimitExceededException $e) { // stop after the first message if size is exceeded $this->updateMessageStatus($campaign, MessageStatus::Suspended); - $this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent); - - throw $e; + $this->updateUserMessageStatus($userMessage, UserMessageStatus::NotSent); + + throw $e;You may also want to revisit whether rethrowing is desired (it will surface to Messenger and potentially trigger retries/failure transport) or whether it’s enough to suspend the campaign and return normally.
🧹 Nitpick comments (4)
src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
78-78: Track or address the TODO comment.The TODO suggests there might be other configuration data needed for mail formatting or creation. This should be investigated to ensure complete functionality.
Would you like me to open an issue to track this TODO, or do you want to address it in this PR?
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (3)
8-29: DI changes andmaxMailSizedefault look reasonable, with one behavioral nuanceThe added dependencies (
CacheInterface,EventLogManager,MessageDataManager) and the new?int $maxMailSizeproperty wired through the constructor are consistent with this handler’s orchestration role and keep concerns in the right layer.One subtle behavioral point: defaulting
?int $maxMailSizeto0(treated later as “no limit”) is fine as long as config/docs clearly state “0 or null = unlimited”. If the intent is that an unset value should fall back to some other global/default limit, you may want to handle that here instead of hard-wiring 0 as the sentinel.Also applies to: 40-60
179-219: Size-check helper is solid; consider tightening cache handling and typingThe
checkMessageSizeOrSuspendCampaignhelper cleanly encapsulates the “compute once, cache per campaign & channel, log, and throw” logic. That’s a good separation of concerns for this handler.A couple of small robustness tweaks you might consider:
When reading from cache, explicitly cast to
intand guard against unexpected types/nulls (e.g., cache eviction betweenhas()andget()), so comparisons against$this->maxMailSizeare always numeric and intentional.For example, inside the method:
$size = $this->cache->get($cacheKey);if ($size <= $this->maxMailSize) {
$size = $this->cache->get($cacheKey);if (!is_int($size)) {$size = $this->calculateEmailSize($email);$this->messageDataManager->setMessageData($campaign, $sizeName, $size);$this->cache->set($cacheKey, $size);}if ($size <= $this->maxMailSize) { return; }(Or any equivalent pattern you prefer.)
- If
MessageDataManageris the canonical store for these size values, you could, in future, consider checking it first when cache is cold instead of recomputing, though that’s more of an optimization than a necessity.Overall, behavior and logging/event log entries here look on point.
221-230: Email size calculation is fine; the TODO is now misleadingThe
calculateEmailSize(Email $email)implementation usingforeach ($email->toIterable())andstrlen($line)is a reasonable approximation of raw message size in bytes given Symfony’sThe inline TODO:
// todo: setMessageData($messageid, $sizename, $mail->mailsize);is effectively already handled by
checkMessageSizeOrSuspendCampaignviaMessageDataManager::setMessageData(...). Keeping this TODO around is likely to confuse future readers into thinking something is still missing. I’d recommend removing or rewording it to reflect the current design.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php(5 hunks)src/Domain/Messaging/Service/Manager/MessageDataManager.php(1 hunks)src/Domain/Messaging/Service/MessageProcessingPreparator.php(4 hunks)tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Domain/Messaging/Service/Manager/MessageDataManager.php
- tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/MessageProcessingPreparator.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (1)
src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
src/Domain/Analytics/Service/LinkTrackService.php (3)
LinkTrackService(13-98)isExtractAndSaveLinksApplicable(30-33)extractAndSaveLinks(41-80)
⏰ 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/Domain/Messaging/Service/MessageProcessingPreparator.php (5)
21-28: Nice refactor to property promotion!The use of
private readonlywith constructor property promotion is clean and modern. This eliminates boilerplate while ensuring immutability of dependencies.
19-19: Good catch on the typo!Renaming
LINT_TRACK_ENDPOINTtoLINK_TRACK_ENDPOINTfixes the obvious typo.
80-87: Verify the ordering of link replacement and personalization.Personalization is performed after link replacement. If
UserPersonalizercould potentially insert or modify URLs as part of personalization (e.g., replacing placeholders with URLs), those new or modified URLs won't be tracked. Confirm this ordering aligns with the intended behavior.Additionally, consider whether error handling is needed around the personalization calls—if personalization fails, the entire processing flow could break.
98-98: Double slash issue resolved!The track URL is now correctly constructed without the double slash issue mentioned in previous reviews. The constant is used directly without prepending an extra
/.
63-63: All callers ofprocessMessageLinkshave been properly updated to pass theSubscriberobject. Three test cases and one production caller (CampaignProcessorMessageHandler) all correctly pass$subscriberinstead of an integer user ID. The method correctly extracts the ID via$subscriber->getId()when needed for downstream operations.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
153-154: Subscriber-basedprocessMessageLinksusage looks goodSwitching
processMessageLinksto receive the fullSubscriberobject rather than just the ID aligns with more flexible per-subscriber personalization. As long as the preparator’s signature has been updated correspondingly (as suggested by the PR summary/tests), this change is clean.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
167-222: Stub precacheService and hasHtmlEmail in tests that exercise sendingA couple of subtle mocking issues will bite once these tests hit the new code paths:
getOrCacheBaseMessageContent()returningnull
CampaignProcessorMessageHandler::__invoke()now always does:$messageContent = $this->precacheService->getOrCacheBaseMessageContent($campaign); ... $this->handleEmailSending($campaign, $subscriber, $userMessage, $messageContent);but in:
testInvokeWithValidSubscriberEmailtestInvokeWithMultipleSubscribers
$this->precacheServiceis not stubbed for those campaigns, so the default mock return isnull. BecausehandleEmailSendingrequiresMessage\MessageContentfor its 4th parameter, this will cause aTypeError.You already stub precache correctly in
testInvokeWithMailerException; do the same in the other tests, e.g.:public function testInvokeWithValidSubscriberEmail(): void { $campaign = $this->createMock(Message::class); $content = $this->createContentMock(); $campaign->method('getContent')->willReturn($content); + $this->precacheService->expects($this->once()) + ->method('getOrCacheBaseMessageContent') + ->with($campaign) + ->willReturn($content); ... }and similarly in
testInvokeWithMultipleSubscribers(likelyexpects($this->once())with the$campaignfromcreateCampaignMock()).
hasHtmlEmail()needs a concrete boolean
checkMessageSizeOrSuspendCampaign()is declared as:private function checkMessageSizeOrSuspendCampaign( Message $campaign, Email $email, bool $hasHtmlEmail ): voidand
handleEmailSendingcalls it as:$this->checkMessageSizeOrSuspendCampaign($campaign, $email, $subscriber->hasHtmlEmail());The
Subscribermocks intestInvokeWithValidSubscriberEmail,testInvokeWithMailerException, andtestInvokeWithMultipleSubscribersdon’t stubhasHtmlEmail(), so the mock will returnnull. Withstrict_types=1, passingnullinto aboolparameter will also throw aTypeError.Quick fix: stub
hasHtmlEmail()to a sensible value wherever a valid subscriber email leads to sending, e.g.:$subscriber = $this->createMock(Subscriber::class); $subscriber->method('getEmail')->willReturn('[email protected]'); $subscriber->method('getId')->willReturn(1); + $subscriber->method('hasHtmlEmail')->willReturn(true);and similarly for the other tests’ valid subscribers (text‑only cases can use
falseif you want to exercise that branch).With those adjustments, the tests should align cleanly with the new handler behavior.
Also applies to: 226-275, 279-320, 325-343
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
76-82: Don’t reuse the same mutable MessageContent instance across subscribers
getOrCacheBaseMessageContent()returns aMessageContentthatMessageProcessingPreparator::processMessageLinks()then mutates in place (link replacements + personalization):$messageContent = $this->precacheService->getOrCacheBaseMessageContent($campaign); ... foreach ($subscribers as $subscriber) { ... $this->handleEmailSending($campaign, $subscriber, $userMessage, $messageContent); }Because the same
$messageContentinstance is reused for every subscriber, later subscribers will see content that has already been link‑tracked and personalized for earlier subscribers (and link extraction will then operate on already‑tracked URLs).Given that
MessagePrecacheServiceis already caching a snapshot and reconstructing a freshMessageContenton eachgetOrCacheBaseMessageContent()call, it’s safer to fetch a new base instance per subscriber:- $messageContent = $this->precacheService->getOrCacheBaseMessageContent($campaign); - ... - foreach ($subscribers as $subscriber) { + foreach ($subscribers as $subscriber) { ... - $this->handleEmailSending($campaign, $subscriber, $userMessage, $messageContent); + $precachedContent = $this->precacheService->getOrCacheBaseMessageContent($campaign); + $this->handleEmailSending($campaign, $subscriber, $userMessage, $precachedContent);That way each subscriber starts from the same unpersonalized base, and per‑subscriber link tracking + personalization stay isolated.
Also applies to: 86-107, 155-162
♻️ Duplicate comments (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
163-174: Size check runs after send; confirm if first oversized email should be blocked or allowedRight now
handleEmailSending()does:$email = $this->mailer->composeEmail($campaign, $subscriber, $processed); $this->mailer->send($email); $this->checkMessageSizeOrSuspendCampaign($campaign, $email, $subscriber->hasHtmlEmail()); $this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent);and on
MessageSizeLimitExceededException:$this->updateMessageStatus($campaign, MessageStatus::Suspended); $this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent); throw $e;So the first oversized email is still delivered, and the corresponding
UserMessageis marked asSent; only then is the campaign suspended.If the product requirement is “never send emails larger than the configured limit”, you probably want to:
- Run
checkMessageSizeOrSuspendCampaign()beforesend(), and- Avoid marking the
UserMessageasSentin the oversize exception path.Something along these lines:
try { $email = $this->mailer->composeEmail($campaign, $subscriber, $processed); - $this->mailer->send($email); - $this->checkMessageSizeOrSuspendCampaign($campaign, $email, $subscriber->hasHtmlEmail()); - $this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent); + $this->checkMessageSizeOrSuspendCampaign($campaign, $email, $subscriber->hasHtmlEmail()); + $this->mailer->send($email); + $this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent); } catch (MessageSizeLimitExceededException $e) { - // stop after the first message if size is exceeded + // stop after the first message if size is exceeded (and don’t mark as sent) $this->updateMessageStatus($campaign, MessageStatus::Suspended); - $this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent); + $this->updateUserMessageStatus($userMessage, UserMessageStatus::NotSent); throw $e;If, on the other hand, the intended semantics are “allow the first send, then suspend further sends once size is known”, the current implementation matches that, but it might be worth documenting that behavior explicitly given how subtle it is (and that it was previously raised in review).
Also applies to: 186-237
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Domain/Analytics/Service/LinkTrackService.php(2 hunks)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php(8 hunks)src/Domain/Messaging/Service/MessagePrecacheService.php(1 hunks)src/Domain/Messaging/Service/MessageProcessingPreparator.php(2 hunks)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php(2 hunks)tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php(7 hunks)tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php(11 hunks)tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php(7 hunks)tests/Unit/Domain/Messaging/Service/RateLimitedCampaignMailerTest.php(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Analytics/Service/LinkTrackService.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/Service/MessageProcessingPreparator.phpsrc/Domain/Messaging/Service/MessagePrecacheService.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (7)
tests/Unit/Domain/Messaging/Service/RateLimitedCampaignMailerTest.php (1)
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
composeEmail(23-40)
tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php (1)
src/Domain/Analytics/Service/LinkTrackService.php (1)
extractAndSaveLinks(42-78)
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (3)
src/Domain/Messaging/Service/Manager/MessageDataManager.php (1)
MessageDataManager(13-96)src/Domain/Messaging/Service/MessagePrecacheService.php (1)
MessagePrecacheService(10-136)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
RateLimitedCampaignMailer(13-51)
tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php (1)
src/Domain/Messaging/Service/MessageProcessingPreparator.php (2)
MessageProcessingPreparator(17-107)processMessageLinks(64-95)
src/Domain/Messaging/Service/MessageProcessingPreparator.php (2)
src/Domain/Analytics/Service/LinkTrackService.php (4)
__construct(19-23)LinkTrackService(14-96)isExtractAndSaveLinksApplicable(31-34)extractAndSaveLinks(42-78)src/Domain/Messaging/Model/Message.php (2)
__construct(67-87)getId(89-92)
src/Domain/Messaging/Service/MessagePrecacheService.php (2)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
__construct(43-62)src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
__construct(22-29)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (3)
src/Domain/Messaging/Exception/MessageSizeLimitExceededException.php (1)
MessageSizeLimitExceededException(9-31)src/Domain/Messaging/Service/Manager/MessageDataManager.php (2)
MessageDataManager(13-96)setMessageData(29-71)src/Domain/Messaging/Service/MessagePrecacheService.php (2)
MessagePrecacheService(10-136)getOrCacheBaseMessageContent(21-60)
⏰ 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 (9)
src/Domain/Analytics/Service/LinkTrackService.php (1)
12-78: Refactor to MessageContent + explicit messageId looks consistentUsing
MessageContentand an explicit?int $messageIdkeeps the service focused on content while still enforcing the “message must have an ID” invariant. Text/footer handling and deduped persistence behavior are preserved, so this change looks good from a correctness and domain-purity standpoint.tests/Unit/Domain/Messaging/Service/RateLimitedCampaignMailerTest.php (1)
49-57: Tests correctly adapted to new composeEmail signaturePassing
$message->getContent()intocomposeEmailmatches the new mailer API, and the existing assertions still verify subject, from/reply-to, and bodies. No issues here.Also applies to: 73-79
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
23-40: Mailer now cleanly separates headers from contentUsing
MessageContentfor subject/text/html while still sourcing from/reply-to from theMessageoptions matches the refactored flow and keepscomposeEmailstraightforward. Looks solid.tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php (1)
39-50: Tests line up with the new content‑based LinkTrackService APIUpdating the tests to pass
MessageContentand an explicit$messageId(including the null‑ID case) keeps coverage aligned with the refactored service. Expectations around persist calls and returnedLinkTrackinstances still look correct.Also applies to: 74-79, 88-108, 116-135, 143-162, 169-179, 205-228
src/Domain/Messaging/Service/MessageProcessingPreparator.php (5)
8-8: LGTM! Clean imports and typo fix.The new imports support the refactored method signature and personalization feature. The constant rename from
LINT_TRACK_ENDPOINTtoLINK_TRACK_ENDPOINTfixes an obvious typo.Also applies to: 10-10, 12-12, 20-20
22-28: Nice refactor with property promotion.Constructor property promotion makes the code cleaner and the
readonlymodifiers prevent accidental mutation. The newUserPersonalizerdependency supports the personalization feature.
64-68: Method signature refactored for per-subscriber processing.The new signature
processMessageLinks(int $campaignId, MessageContent $content, Subscriber $subscriber)is more focused and aligns with processing content per subscriber rather than per message. This is a breaking change, but appears intentional for the size-checking feature.
101-101: Previous issue resolved.The double-slash URL issue flagged in the previous review has been correctly addressed. The track URL is now properly constructed without duplicating the leading slash.
73-73: No action needed. The parameter named$campaignIdis actually the ID of a Message entity (campaigns and messages are the same concept in phpList). Passing it toextractAndSaveLinksas themessageIdparameter is correct. This is a naming inconsistency without functional impact.
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Thanks for contributing to phpList!