Refactor/update message sendin #374
Conversation
📝 WalkthroughWalkthroughThis pull request implements a comprehensive messaging refactoring that introduces precache-based campaign processing, placeholder resolution architecture, and modularized email construction. It replaces inline content handling with data-loading and precaching flows, adds 50+ new domain services, introduces a placeholder resolver pattern with 20+ resolver implementations, and reorganizes email building through base and specialized builders with support for attachments, sizing, and image embedding. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
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.
| 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.
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
Outdated
Show resolved
Hide resolved
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.
| $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.
New Features
PDF generation for messages, per-subscriber remote-content fetching, tracking-pixel user tracking, and richer attachment handling with downloadable copies.
Improvements
Unified email builder flow with consistent composition and multi-format output (HTML/Text/PDF); expanded, context-aware placeholder personalization (many URL/list resolvers); improved remote-content precaching and output formatting; new configurable parameters and translations.
---------
Co-authored-by: Tatevik <tatevikg1@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Domain/Messaging/Service/EmailService.php (1)
67-70: Consider guarding against empty recipients.Line 69 accesses
$email->getTo()[0]without checking if the array is non-empty. If somehow an email without aToaddress makes it here, you'd get an undefined index error.Might be worth a quick guard or assertion, though if callers always set recipients this could be fine in practice.
Suggested defensive check
+ $toAddresses = $email->getTo(); + if (empty($toAddresses)) { + throw new \InvalidArgumentException('Email must have at least one recipient'); + } + $envelope = new Envelope( sender: new Address($this->bounceEmail, 'PHPList Bounce'), - recipients: [new Address($email->getTo()[0]->getAddress())] + recipients: [new Address($toAddresses[0]->getAddress())] );tests/Unit/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandlerTest.php (1)
63-70: Fix test inconsistency: second test should mockfind()instead ofgetListNames().Both tests need to mock the same repository method. The handler's private
getListNames()method loops through list IDs and calls$subscriberListRepository->find($listId)individually—it never uses the repository's batchgetListNames()method. The first test (line 63-70) correctly mocksfind(), but the second test at line 128 should do the same instead of mockinggetListNames().Suggested fix
- $listRepo->method('getListNames')->with([42])->willReturn([]); + $listRepo->method('find') + ->willReturnCallback(function (int $id) { + return null; + });src/Domain/Messaging/MessageHandler/PasswordResetMessageHandler.php (1)
24-27: Minor: DocBlock mentions "subscriber confirmation" but this handles password reset.The comment says "Process a subscriber confirmation message" but the handler processes
PasswordResetMessage. Small copy-paste oversight.📝 Suggested fix
/** - * Process a subscriber confirmation message by sending the confirmation email + * Process a password reset message by sending the reset email */ public function __invoke(PasswordResetMessage $message): void
🤖 Fix all issues with AI agents
In `@resources/translations/messages.en.xlf`:
- Around line 777-780: Remove the duplicate translation trans-unit by keeping a
single canonical entry for the source "Insufficient memory to add attachment to
campaign %campaignId% %totalSize% - %memLimit%" and deleting the redundant
trans-unit (either id="QLlIQQh" or id="28lDkjt"); ensure only one trans-unit
remains with that source/target text and update any tooling or references that
expect that id if you choose to keep the other id.
In `@src/Domain/Common/FileHelper.php`:
- Around line 36-59: The writeFileToDirectory method can leave the original temp
file orphaned when you append an extension and currently does not check
fopen/fwrite results; update writeFileToDirectory to write to the temp file
returned by tempnam (use file_put_contents($tempPath, $contents, LOCK_EX) or
check fwrite/fflush returns) and verify the write succeeded, then if an
extension is required perform a rename($tempPath, $targetPath) and check its
return value; on any failure (write or rename) unlink the temp file and return
null, and only return $fullPath when write+rename (or write alone when no rename
needed) have succeeded. Target symbols: writeFileToDirectory, tempnam, rename,
unlink, file_put_contents/fopen/fwrite/fflush/fclose.
In `@src/Domain/Common/RemotePageFetcher.php`:
- Around line 65-70: RemotePageFetcher is performing persistence side effects by
calling $this->entityManager->remove(...) and then
$this->urlCacheRepository->persist(...); remove direct EntityManager usage from
the Domain class and push the logic into the repository/infrastructure layer:
add a single repository method (e.g., UrlCacheRepository::replaceCacheForUrl or
purgeAndSave) that encapsulates getByUrl, removal of existing UrlCache entities,
and persisting the new UrlCache, then update RemotePageFetcher to call that
repository method (passing $url, $content, $lastModified) and delete any
EntityManager references from RemotePageFetcher.
- Around line 111-124: The prepareUrl method is converting the final URL to
ISO-8859-1 which breaks percent-encoded UTF-8 sequences; remove the
mb_convert_encoding($url, 'ISO-8859-1', 'UTF-8') call in prepareUrl so the URL
remains UTF-8 after rawurlencode(), and return the result via expandUrl($url)
unchanged; ensure no other code in prepareUrl replaces or re-encodes the URL.
In `@src/Domain/Configuration/Service/Placeholder/ContactUrlValueResolver.php`:
- Around line 22-28: In ContactUrlValueResolver::__invoke, avoid calling
$this->config->getValue(ConfigOption::VCardUrl) twice; fetch it once into a
local variable (e.g., $url = $this->config->getValue(ConfigOption::VCardUrl) ??
''), then return $url for text contexts and htmlspecialchars($url) for non-text
contexts, preserving the existing logic with a single config lookup.
In
`@src/Domain/Configuration/Service/Placeholder/ForwardMessageIdValueResolver.php`:
- Around line 37-43: The current code in ForwardMessageIdValueResolver casts
$forwardMessage to (int) producing 0 for non-numeric inputs; before casting,
validate $forwardMessage is an integer string (e.g., use
ctype_digit($forwardMessage) or preg_match('/^\d+$/')) and if it fails return ''
(and optionally emit a debug/warning via the resolver's logger if present)
instead of proceeding to create $messageId = 0; update the block around
$forwardMessage/$messageId to perform this check and only cast when validation
passes.
In `@src/Domain/Configuration/Service/Placeholder/ForwardUrlValueResolver.php`:
- Around line 30-38: In ForwardUrlValueResolver.php inside the HTML branch
(where $ctx->isHtml() is true), ensure the user UID is HTML-escaped (use
htmlspecialchars on $ctx->getUser()->getUniqueId()) and avoid emitting mid=0
when $ctx->messageId() is null by only appending the mid parameter when
messageId() !== null (build the query fragment conditionally rather than always
using '%d' in sprintf). Update the sprintf/concatenation accordingly so $url,
$sep and the escaped uid are consistently escaped and mid is included only when
present.
In `@src/Domain/Configuration/Service/Placeholder/JumpoffValueResolver.php`:
- Around line 25-37: In JumpoffValueResolver::__invoke, when appending the jo=1
flag to the URL produced by $this->urlBuilder->withUid(...) you must preserve
any URL fragment (the part after '#') by inserting the query parameter before
the fragment; split the URL on '#' into $basePart and $fragment, append the
parameter to $basePart (using '?' or '&' depending on whether $basePart already
contains a query string), then reattach '#'.$fragment if present; keep all
existing query parameters intact and return the reconstructed URL instead of
naively concatenating '&jo=1'.
In
`@src/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolver.php`:
- Around line 22-39: The PreferencesUrlValueResolver::__invoke currently
concatenates the raw UID into the URL; update it to URL-encode the UID and to
HTML-escape the separator with explicit flags: use
rawurlencode($ctx->getUser()->getUniqueId()) when building both plain and HTML
URLs, and in the HTML branch call htmlspecialchars($sep, ENT_QUOTES |
ENT_SUBSTITUTE, 'UTF-8') (and ensure the already-escaped $url keeps its existing
flags). This fixes potential injection from arbitrary UIDs while keeping proper
HTML escaping for the separator; locate PlaceholderContext usage and
ConfigOption::PreferencesUrl handling inside __invoke to apply the changes.
In `@src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php`:
- Around line 238-254: The code assumes buildPhplistEmail() returns an Email and
dereferences $email->getTo()[0], which can be null and mask the original
exception; update CampaignProcessorMessageHandler to check the return of
systemEmailBuilder->buildPhplistEmail(...) (and that getTo() has at least one
recipient) before constructing Envelope and calling mailer->send; if
buildPhplistEmail() returns null or no recipients, log a clear error (including
$e->getMessage() context) or rethrow the original exception instead of
proceeding to send so the original failure is not hidden.
In `@src/Domain/Messaging/Service/AttachmentAdder.php`:
- Around line 59-62: In AttachmentAdder (in the method handling the
OutputFormat::Text case) avoid directly accessing $email->getTo()[0]; first
check that $email->getTo() is non-empty (e.g. empty() or count()>0) before
calling ->getAddress(); if no recipients exist, choose a safe fallback (empty
string, null, or skip building the $viewUrl / log and return/throw as
appropriate) so $att->getId() is not combined with an undefined index—update the
branch that builds $viewUrl to use this defensive check and create the URL only
when a valid $userEmail is available.
In `@src/Domain/Messaging/Service/Builder/BaseEmailBuilder.php`:
- Around line 48-55: The current newline checks in BaseEmailBuilder (the
preg_match calls that validate $to and $subject) only match "\n" and miss "\r",
allowing header-injection via "\r\n"; update both validations to use a regex
that matches either CR or LF (e.g. /[\r\n]/ or /(\r|\n)/) so any CR or LF in $to
or $subject triggers the existing $this->eventLogManager->log(...) and returns
false, leaving the rest of the logic unchanged.
- Around line 126-139: The code calls
$this->configProvider->getValue(ConfigOption::UnsubscribeUrl) into $removeUrl
and immediately uses str_contains($removeUrl, '?') which will throw if null;
update BaseEmailBuilder to guard the value: check that $removeUrl is a non-empty
string before calling str_contains and before adding the 'List-Unsubscribe'
header (e.g. skip header creation when getValue returns null/empty), and
reference the same symbols ($removeUrl, getValue, ConfigOption::UnsubscribeUrl,
str_contains, List-Unsubscribe) so the header logic only runs when a valid
unsubscribe URL exists.
In `@src/Domain/Messaging/Service/Builder/EmailBuilder.php`:
- Line 122: The code assigns $adminAddress from the wrong config option; change
the call in EmailBuilder where $adminAddress is set (currently
$this->configProvider->getValue(ConfigOption::UnsubscribeUrl)) to use the admin
address config key (e.g. ConfigOption::AdminAddress or the correct enum value)
so the value used when building the List-Owner header comes from the intended
config; update the lookup in the EmailBuilder class (the $adminAddress
assignment) to use the AdminAddress config option and keep the fallback behavior
unchanged.
In `@src/Domain/Messaging/Service/Builder/SystemEmailBuilder.php`:
- Around line 93-109: The applyContentAndFormatting method calls Email::text()
with nullable $textMessage which can cause a runtime error; guard against null
by ensuring a string is passed to Email::text() (for example coalesce
$textMessage to an empty string or derive plain text from $htmlMessage after
embedding via $this->templateImageEmbedder) before calling $email->text(),
keeping the existing html handling via ($this->templateImageEmbedder)(html:
$htmlMessage, messageId: $messageId) and only passing a non-null string to
Email::text().
In `@src/Domain/Messaging/Service/MessagePrecacheService.php`:
- Around line 188-196: The log message is referencing the wrong URL variable
which is misleading: when ($this->remotePageFetcher) is called with $regs[1] and
returns falsy (the $remoteContent check in MessagePrecacheService), update the
$this->eventLogManager->log call to reference the actual fetched URL ($regs[1])
instead of $loadedMessageData['sendurl'] — optionally include both values for
context (fetched URL $regs[1] and original sendurl
$loadedMessageData['sendurl']) so the log clearly shows which URL failed; adjust
the log entry in the remoteContent falsy branch accordingly.
In `@src/Domain/Subscription/Repository/SubscriberRepository.php`:
- Around line 209-216: getDataById currently calls getArrayResult()[0] which
will trigger an undefined offset/TypeError when no record is found; modify
getDataById to store the result of
createQueryBuilder(...)->getQuery()->getArrayResult() in a variable, check if
the array is empty, and then either return an empty array or throw a specific
not-found exception (e.g., SubscriberNotFoundException) to preserve the declared
return type and avoid notices; update the method (getDataById) to handle the
empty result path before indexing [0].
In
`@tests/Unit/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolverTest.php`:
- Around line 65-80: The test's expected string uses htmlspecialchars() with
default flags while PreferencesUrlValueResolver uses ENT_QUOTES | ENT_SUBSTITUTE
and 'UTF-8'; update the test in testHtmlEscapesUrlAndAppendsUid to call
htmlspecialchars($raw, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') and
htmlspecialchars('&', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') so the escaping
behavior matches PreferencesUrlValueResolver and the asserted string is produced
correctly.
In
`@tests/Unit/Domain/Configuration/Service/Placeholder/UserDataSupportingResolverTest.php`:
- Around line 24-38: The makeCtx helper has a parameter typed as Subscriber but
defaulted to null which is deprecated; update the function signature of makeCtx
to accept a nullable Subscriber (e.g., ?Subscriber $user = null) and keep the
existing null-coalescing logic that constructs a Subscriber when $user is null;
ensure any related docblock/type hints reflect the nullable type so tools and
PHP 8.1+ no longer warn.
In
`@tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php`:
- Around line 371-375: Replace the deprecated withConsecutive usage on the mock
by switching to a willReturnCallback closure that keeps a local call counter;
inside the callback validate each invocation's arguments against the expected
values for that call (e.g., check arg0==1, arg1==$precached, arg2==$subscriber1
for call 0, then subscriber2 for call 1) and return the corresponding predefined
response (or pull from an array of return values, similar to
willReturnOnConsecutiveCalls). Concretely, change the mock setup that used
withConsecutive(...) ->willReturnOnConsecutiveCalls(...) to a single
->willReturnCallback(function(...$args) use (&$i, $expectedArgs, $returns) { /*
assert args match $expectedArgs[$i], $ret=$returns[$i]; $i++; return $ret; */
}), referencing the same expected values ($precached, $subscriber1,
$subscriber2) and returns to preserve behavior.
♻️ Duplicate comments (6)
resources/translations/messages.en.xlf (1)
741-812: Previous feedback regarding "__" prefix still applies.The new entries correctly follow the existing pattern in this file, but as noted in the previous review, the
__prefix typically marks auto-generated placeholder translations in Symfony. Since this is an English-to-English file, these should probably have the prefix removed before release. Deferring to the prior comment on this.src/Domain/Common/RemotePageFetcher.php (1)
51-56: Caching logic may always miss: querying withlastModified = time().Setting
$lastModified = time()then callingfindByUrlAndLastModified($url, $lastModified)will look for cache entries modified after the current timestamp - essentially future-dated entries that won't exist. This likely causes the cache lookup to always fail.If the intent is to check for existing cached content regardless of timestamp, consider using a simpler
findByUrl($url)query here, or rethink the freshness logic.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
223-224: Move the size check before sending the email.
Right now the oversized message is already sent before the checker runs.♻️ Suggested fix
- $this->rateLimitedCampaignMailer->send($email); - ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail()); + ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail()); + $this->rateLimitedCampaignMailer->send($email);src/Domain/Messaging/Service/Constructor/SystemMailContentBuilder.php (1)
18-66: Guard against null config values for powered‑by content.
ConfigProvider::getValue()can return null;preg_replace()andHtml2Textexpect strings and will throwTypeErrorin PHP 8+. Coalesce to a safe default.🛠️ Proposed fix
public function __construct( private readonly Html2Text $html2Text, private readonly ConfigProvider $configProvider, private readonly TemplateRepository $templateRepository, private readonly TemplateImageManager $templateImageManager, private readonly bool $poweredByPhplist = false, ) { - $this->poweredByText = $configProvider->getValue(ConfigOption::PoweredByText); + $this->poweredByText = $configProvider->getValue(ConfigOption::PoweredByText) ?? ''; } @@ if (!$this->poweredByPhplist) { + $poweredByImage = $this->configProvider->getValue(ConfigOption::PoweredByImage) ?? ''; $phpListPowered = preg_replace( '/src=".*power-phplist.png"/', 'src="powerphplist.png"', - $this->configProvider->getValue(ConfigOption::PoweredByImage), + $poweredByImage, ); } else { $phpListPowered = $this->poweredByText; }#!/bin/bash # Confirm defaults for PoweredByText/PoweredByImage to see if null is possible. rg -n "PoweredByText|PoweredByImage" -S src/Domain/Configurationtests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php (1)
153-160: Replace deprecatedwithConsecutive.
PHPUnit 10 removes this API; a callback keeps the intent without relying on deprecated behavior.🛠️ Suggested refactor
- $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'; + });src/Domain/Messaging/Service/MessagePrecacheService.php (1)
91-95: Null safety issue withparseLogoPlaceholderscalls.
$messagePrecacheDto->templatecan benull(set inapplyTemplateonly when template exists), butparseLogoPlaceholdersexpects astring. This will cause aTypeErrorunder strict types.🔧 Suggested fix with null checks
- $messagePrecacheDto->content = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->content); - $messagePrecacheDto->template = $this->templateImageManager - ->parseLogoPlaceholders($messagePrecacheDto->template); - $messagePrecacheDto->htmlFooter = $this->templateImageManager - ->parseLogoPlaceholders($messagePrecacheDto->htmlFooter); + $messagePrecacheDto->content = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->content); + if ($messagePrecacheDto->template !== null) { + $messagePrecacheDto->template = $this->templateImageManager + ->parseLogoPlaceholders($messagePrecacheDto->template); + } + $messagePrecacheDto->htmlFooter = $this->templateImageManager + ->parseLogoPlaceholders($messagePrecacheDto->htmlFooter);
🧹 Nitpick comments (34)
src/Domain/Subscription/Service/SubscriberCsvImporter.php (1)
192-192: Consider tracking this TODO as a follow-up task.
If you want, I can help draft the refactor plan or implement the grace‑timeisBlacklistedlogic swap.resources/translations/messages.en.xlf (1)
745-752: Inconsistent placeholder style between similar entries.Two entries describe essentially the same notification but use different placeholder conventions:
- Line 746:
%s(printf-style)- Line 750:
%subject%(Symfony-style named placeholder)If both are intentionally used in different contexts (e.g., one for logging, one for templates), this is fine. Otherwise, consider standardizing on the named
%subject%style for consistency with Symfony conventions and better readability.tests/Unit/Domain/Identity/Service/SessionManagerTest.php (1)
35-44: Heads up onwithConsecutive()for future PHPUnit upgrades.The
withConsecutive()method used here was deprecated in PHPUnit 9.6 and removed in PHPUnit 10. Since you're on PHPUnit 9.5.0, this works fine for now. Just something to keep in mind if you upgrade later — the pattern would need to shift to callback-based expectations or separate test cases.No action needed right now, just a friendly FYI. 🙂
config/services/parameters.yml (1)
2-10: Avoid drift between flat and nested parameters.You now have two copies of the same values. Consider YAML anchors/aliases so updates stay in sync.
♻️ Suggested change (YAML anchors)
- app.config.message_from_address: 'news@example.com' - app.config.default_message_age: 15768000 + app.config.message_from_address: &message_from_address 'news@example.com' + app.config.default_message_age: &default_message_age 15768000 @@ - app.config: - message_from_address: 'news@example.com' + app.config: + message_from_address: *message_from_address admin_address: 'admin@example.com' - default_message_age: 15768000 + default_message_age: *default_message_agesrc/Domain/Common/PdfGenerator.php (1)
11-18: Make compression configurable to avoid bloated PDFs.Compression is always disabled right now. That’s great for tests, but could increase attachment size in production. Consider a flag so callers can opt in.
♻️ Example tweak (optional flag)
- public function createPdfBytes(string $text): string + public function createPdfBytes(string $text, bool $compress = false): string { $pdf = new FPDF(); - // Disable compression to ensure plain text and metadata are visible in output (helps testing) + // Allow callers to toggle compression (useful for tests vs prod) if (method_exists($pdf, 'SetCompression')) { - $pdf->SetCompression(false); + $pdf->SetCompression($compress); }tests/Unit/Domain/Common/PdfGeneratorTest.php (1)
12-30: Heads up on non-ASCII characters in the test.The test string includes Japanese and Cyrillic characters (
'Hello PDF! こんにちは Привет'), but FPDF's default Arial font doesn't support these glyphs - they'll likely render as blanks or cause issues. The test will pass (PDF structure is valid), but it's worth noting this doesn't validate actual multi-language rendering.If multi-language PDF support is needed down the road, you'd want to look at FPDF's Unicode extension or TCPDF.
src/Domain/Common/OnceCacheGuard.php (1)
18-27: Minor: Potential race condition betweenhas()andset().There's a small TOCTOU window where two concurrent processes could both see
has()return false and both returntrue. For the intended use case (throttling attachment logging per the AI summary), this is probably fine - worst case you'd get a couple extra log entries during the race window.If stricter "exactly once" semantics are ever needed, you'd want an atomic operation, but PSR-16 doesn't expose
setIfNotExists. This is more of an FYI than something to change.tests/Unit/Domain/Identity/Service/PasswordManagerTest.php (1)
214-216: Heads up onwithConsecutive()deprecation.This works fine with PHPUnit 9.5, but
withConsecutive()was deprecated in PHPUnit 10 and removed in PHPUnit 11. If you upgrade later, you'll need to refactor to usewillReturnCallback()with invocation tracking.src/Domain/Messaging/MessageHandler/SubscriberConfirmationMessageHandler.php (1)
23-23: Minor formatting nit: missing space after attribute.For consistency with other handlers (e.g.,
EmailService.phpuses#[Autowire('%app.mailer_from%')] private readonly), add a space after the closing bracket.Suggested fix
- #[Autowire('%app.confirmation_url%')]private readonly string $confirmationUrl + #[Autowire('%app.confirmation_url%')] private readonly string $confirmationUrlsrc/Domain/Common/RemotePageFetcher.php (1)
81-93: Silent exception swallowing makes debugging tricky.The catch block returns an empty string without logging. Consider at least logging at debug level so failures don't vanish into the void.
💡 Suggested improvement
} catch (Throwable $e) { + // Consider injecting LoggerInterface and logging: + // $this->logger->debug('Failed to fetch URL', ['url' => $url, 'error' => $e->getMessage()]); return ''; }src/Domain/Configuration/Model/ConfigOption.php (1)
18-18: Resolve the TODO or link it to a tracked task.
Leaving TODOs in config enums tends to age poorly—can we either remove it or reference a tracking issue?If you want, I can help track down where
domainis defined and suggest the right fix.src/Domain/Configuration/Service/Placeholder/SignatureValueResolver.php (1)
31-35: Consider non-greedy regex forsrcreplacement.The
.*in the pattern is greedy and could over-match if the HTML contains multiple attributes before the target filename. A non-greedy.*?would be safer:♻️ Suggested tweak
return preg_replace( - '/src=".*power-phplist.png"/', + '/src=".*?power-phplist.png"/', 'src="powerphplist.png"', $this->config->getValue(ConfigOption::PoweredByImage) ?? '' );src/Domain/Configuration/Model/Dto/PlaceholderContext.php (1)
43-46: Minor:getUser()is redundant with public$userproperty.Since
$userispublic readonlyon line 14, consumers can access it directly as$ctx->user. ThegetUser()method works fine but adds API surface without benefit. Could remove it or make$userprivate if you want to enforce getter usage.src/Domain/Configuration/Service/Placeholder/ConfirmationUrlValueResolver.php (2)
30-32: Inconsistent HTML escaping compared to sibling resolvers.This resolver only escapes the separator (
&→&), leaving the base URL and uid unescaped. Other resolvers likeBlacklistUrlValueResolverandForwardUrlValueResolverescape the entire URL withhtmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8').If the configured
ConfirmationUrlcontains characters like<,>, or", they'll pass through unescaped in HTML context.♻️ Suggested fix for consistent escaping
if ($ctx->isHtml()) { - return sprintf('%s%suid=%s', $url, htmlspecialchars($sep), $ctx->getUser()->getUniqueId()); + $fullUrl = sprintf('%s%suid=%s', $url, $sep, $ctx->getUser()->getUniqueId()); + return htmlspecialchars($fullUrl, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); }
22-35: Consider using LegacyUrlBuilder for consistency.Other resolvers like
UnsubscribeValueResolverandBlacklistUrlValueResolveruseLegacyUrlBuilder->withUid()to append the uid parameter. This resolver manually builds the query string. UsingLegacyUrlBuilderwould handle edge cases (existing query params, URL encoding) consistently across all resolvers.src/Domain/Configuration/Service/Placeholder/ForwardMessageIdValueResolver.php (1)
52-66: Same escaping inconsistency:uidis not HTML-escaped.The URL and separator are escaped individually (lines 55-56), but
$uidon line 57 is passed raw. For consistency and safety, consider building the full URL first, then escaping it all at once (same pattern suggested forForwardUrlValueResolver).♻️ Suggested fix
if ($ctx->isHtml()) { - $forwardUrl = sprintf( - '%s%suid=%s&mid=%d', - htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'), - htmlspecialchars($sep, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'), - $uid, - $messageId - ); + $rawUrl = sprintf('%s%suid=%s&mid=%d', $url, $sep, $uid, $messageId); + $forwardUrl = htmlspecialchars($rawUrl, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); return sprintf( '<a href="%s">%s</a>',src/Domain/Configuration/Service/Placeholder/PreferencesValueResolver.php (1)
38-45: UnescapeduniqueIdin HTML attribute context.The
uniqueIdvalue from$ctx->getUser()->getUniqueId()is concatenated directly into thehrefattribute without escaping. While UIDs are typically alphanumeric, if one ever contains special characters, this could break the HTML or introduce XSS.Suggested fix
return '<a href="' . $safeUrl - . htmlspecialchars($sep) + . $sep . 'uid=' - . $ctx->getUser()->getUniqueId() + . htmlspecialchars($ctx->getUser()->getUniqueId(), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . '">' . $safeLabel . '</a> ';src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php (2)
33-39: Potential nullmessageIdand unescapeduniqueId.Two things to consider:
$ctx->messageId()can returnnull(per PlaceholderContext), which would producem=with no value in the URL.- The
uniqueIdis directly concatenated without HTML escaping.Suggested hardening
return '<img src="' . $base . '/ut.php?u=' - . $ctx->getUser()->getUniqueId() + . htmlspecialchars($ctx->getUser()->getUniqueId(), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . '&m=' - . $ctx->messageId() + . ($ctx->messageId() ?? 0) . '" width="1" height="1" border="0" alt="" />';
27-31: Base URL isn't escaped before embedding in HTML.If the configured domain or
restApiDomaincontains special characters, this could break the HTML. Consider escaping$basebefore concatenation.src/Domain/Configuration/Service/Placeholder/ForwardValueResolver.php (1)
36-46: UnescapeduniqueIdin HTML href attribute.The
uniqueIdis concatenated directly without escaping on line 40. For consistency with how the URL and label are escaped, this should also be escaped.Also,
$ctx->messageId()can returnnull. The text path (line 48) uses%dwhich converts null to 0, but the HTML path concatenates it directly.Suggested fix
return '<a href="' . htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') - . htmlspecialchars($sep) + . $sep . 'uid=' - . $ctx->getUser()->getUniqueId() + . htmlspecialchars($ctx->getUser()->getUniqueId(), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . '&mid=' - . $ctx->messageId() + . ($ctx->messageId() ?? 0) . '">' . htmlspecialchars($label, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . '</a> ';src/Domain/Configuration/Service/Placeholder/UserDataSupportingResolver.php (1)
10-51: Consider memoizing subscriber data per resolver instance.
resolve()hits the repository on every placeholder; if several user‑data placeholders are used, this can repeat the same query. A small cache keyed by user id would avoid N+1 behavior.♻️ Suggested memoization
class UserDataSupportingResolver implements SupportingPlaceholderResolverInterface { private array $supportedKeys = [ 'CONFIRMED', 'BLACKLISTED', 'OPTEDIN', 'BOUNCECOUNT', 'ENTERED', 'MODIFIED', 'UNIQID', 'UUID', 'HTMLEMAIL', 'SUBSCRIBEPAGE', 'RSSFREQUENCY', 'DISABLED', 'FOREIGNKEY', ]; + private array $userDataCache = []; @@ public function resolve(string $key, PlaceholderContext $ctx): ?string { $canon = strtoupper($key); - $data = $this->subscriberRepository->getDataById($ctx->getUser()->getId()); + $userId = $ctx->getUser()->getId(); + if (!isset($this->userDataCache[$userId])) { + $this->userDataCache[$userId] = $this->subscriberRepository->getDataById($userId); + } + $data = $this->userDataCache[$userId];src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php (3)
97-129: Heads up: DTO mutation as a side effect.The
replaceUserSpecificRemoteContentmethod modifies$messagePrecacheDto->contentand->htmlFormatteddirectly. While this works, it's a hidden side effect that could surprise callers who don't expect their input DTO to be mutated.Consider either:
- Documenting this behavior clearly
- Returning a modified copy instead
Not a blocker, just something to be aware of.
131-131: Minor typo: "Formating" → "Formatting".Just a small spelling fix for the method name.
✏️ Suggested fix
- private function ensureHtmlFormating(string $content, bool $addDefaultStyle): string + private function ensureHtmlFormatting(string $content, bool $addDefaultStyle): stringAnd update the call site on line 91:
- $htmlMessage = $this->ensureHtmlFormating(content: $htmlMessage, addDefaultStyle: $addDefaultStyle); + $htmlMessage = $this->ensureHtmlFormatting(content: $htmlMessage, addDefaultStyle: $addDefaultStyle);
75-94: Placeholder processing flow looks solid.Good separation of text vs HTML processing paths, with format-specific placeholder resolution. The TODO on line 92 about CLICKTRACK is noted - might be worth creating an issue to track.
Want me to open an issue to track the CLICKTRACK TODO?
src/Domain/Configuration/Service/PlaceholderResolver.php (3)
43-49: Handle potentialpreg_replace_callbacknull return.
preg_replace_callbackcan returnnullon error (e.g., malformed pattern). Currently this would silently set$valueto null, which could cause issues downstream.🔧 Suggested defensive fix
foreach ($this->patternResolvers as $resolver) { - $value = preg_replace_callback( + $result = preg_replace_callback( $resolver['pattern'], fn(array $match) => (string) ($resolver['resolver'])($context, $match), $value ); + if ($result !== null) { + $value = $result; + } }
51-56: Same null-safety concern here.The main placeholder resolution regex could also return null on error.
🔧 Suggested fix
- return preg_replace_callback( + $result = preg_replace_callback( '/\[([^\]%%]+)(?:%%([^\]]+))?\]/i', fn(array $matches) => $this->resolveSinglePlaceholder($matches, $context), $value ); + return $result ?? $value;
73-74: Minor: Double normalization.
normalizePlaceholderKeyis called twice here - once for$keyNormalizedand once inside thestrtoupperfor$canon. Could cache the result, but it's probably not a hot path so NBD.$keyNormalized = $this->normalizePlaceholderKey($rawKey); $canon = strtoupper($keyNormalized); // reuse instead of re-normalizingtests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
229-244: Using Reflection to access private properties is a bit fragile.Accessing
campaignEmailBuildervia Reflection works but couples the test to internal implementation details. If the property name changes, these tests break silently.Consider making the builder injectable or using a test-specific factory. Not urgent since it works, just something to keep in mind for maintainability.
src/Domain/Configuration/Service/MessagePlaceholderProcessor.php (2)
99-107: Case-sensitive placeholder detection.The
str_containschecks for[FOOTER]and[SIGNATURE]are case-sensitive. If templates might use variations like[Footer]or[footer], this could lead to duplicate placeholders being appended. If case variations are expected, consider usingstriposinstead.
50-50: Consider clarifying theFORWARDFORMplaceholder's purpose.
FORWARDFORMis registered to always return an empty string (line 50). UnlikeFORWARDEDBYwhich has an actual implementation, this placeholder appears incomplete. Either add a comment explaining why it returns empty (e.g., "deprecated", "reserved for future use"), or implement its intended functionality.src/Domain/Messaging/Service/AttachmentAdder.php (1)
198-219: Inconsistent behavior: exception first time, silent pass after.The first copy failure throws
AttachmentCopyException, but subsequent failures for the same key silently continue. This asymmetry might be unexpected for callers. Consider documenting this behavior or returning a consistent signal (e.g., always return false, log only once).tests/Unit/Domain/Messaging/Service/AttachmentAdderTest.php (1)
160-233: Consider adding tests for edge cases.Good exception testing! Consider adding coverage for:
- Memory limit exceeded scenario (the
hasMemoryForAttachmentreturns false path)- Subsequent failures for the same key (should not throw per current implementation)
src/Domain/Messaging/Service/MessagePrecacheService.php (1)
21-41: Large constructor acknowledged with suppression.The parameter count is high but the suppression annotation shows awareness. If this grows further, consider introducing a configuration DTO or smaller collaborator services.
src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
219-266: PDF format methods have similar structure.
applyPdfFormatandapplyTextAndPdfFormatshare a similar pattern. If more PDF variants are added, consider extracting common logic. For now, the duplication is acceptable.
| <trans-unit id="QLlIQQh" resname="Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%"> | ||
| <source>Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</source> | ||
| <target>__Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</target> | ||
| </trans-unit> |
There was a problem hiding this comment.
Duplicate translation entry detected.
Lines 777-780 and 797-800 define trans-units with the exact same source and target text:
Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%
These have different IDs (QLlIQQh vs 28lDkjt) but identical content. One should be removed to avoid confusion and maintain a single source of truth.
Suggested fix: Remove the duplicate entry
- <trans-unit id="28lDkjt" resname="Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%">
- <source>Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</source>
- <target>__Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</target>
- </trans-unit>Also applies to: 797-800
🤖 Prompt for AI Agents
In `@resources/translations/messages.en.xlf` around lines 777 - 780, Remove the
duplicate translation trans-unit by keeping a single canonical entry for the
source "Insufficient memory to add attachment to campaign %campaignId%
%totalSize% - %memLimit%" and deleting the redundant trans-unit (either
id="QLlIQQh" or id="28lDkjt"); ensure only one trans-unit remains with that
source/target text and update any tooling or references that expect that id if
you choose to keep the other id.
| public function writeFileToDirectory(string $directory, string $originalFilename, string $contents): ?string | ||
| { | ||
| $pathInfo = pathinfo($originalFilename); | ||
| $name = $pathInfo['filename'] === '' ? 'file' : $pathInfo['filename']; | ||
| $ext = $pathInfo['extension'] ?? ''; | ||
|
|
||
| $newFile = tempnam($directory, $name); | ||
| if ($newFile === false) { | ||
| return null; | ||
| } | ||
|
|
||
| if ($ext !== '') { | ||
| $newFile .= '.' . $ext; | ||
| } | ||
| $relativeName = basename($newFile); | ||
|
|
||
| $fullPath = $directory . '/' . $relativeName; | ||
|
|
||
| $fileHandle = fopen($fullPath, 'w'); | ||
|
|
||
| fwrite($fileHandle, $contents); | ||
| fclose($fileHandle); | ||
|
|
||
| return $fullPath; |
There was a problem hiding this comment.
Handle temp-file rename and write failures.
Appending an extension leaves the original temp file orphaned, and fopen/fwrite failures aren’t checked (so the method can return a path that wasn’t actually written).
🛠️ Suggested fix
- $newFile = tempnam($directory, $name);
- if ($newFile === false) {
- return null;
- }
-
- if ($ext !== '') {
- $newFile .= '.' . $ext;
- }
- $relativeName = basename($newFile);
-
- $fullPath = $directory . '/' . $relativeName;
-
- $fileHandle = fopen($fullPath, 'w');
-
- fwrite($fileHandle, $contents);
- fclose($fileHandle);
-
- return $fullPath;
+ $newFile = tempnam($directory, $name);
+ if ($newFile === false) {
+ return null;
+ }
+
+ if ($ext !== '') {
+ $newWithExt = $newFile . '.' . $ext;
+ if (!rename($newFile, $newWithExt)) {
+ `@unlink`($newFile);
+ return null;
+ }
+ $newFile = $newWithExt;
+ }
+
+ $fileHandle = fopen($newFile, 'wb');
+ if ($fileHandle === false) {
+ `@unlink`($newFile);
+ return null;
+ }
+
+ if (fwrite($fileHandle, $contents) === false) {
+ fclose($fileHandle);
+ `@unlink`($newFile);
+ return null;
+ }
+ fclose($fileHandle);
+
+ return $newFile;🤖 Prompt for AI Agents
In `@src/Domain/Common/FileHelper.php` around lines 36 - 59, The
writeFileToDirectory method can leave the original temp file orphaned when you
append an extension and currently does not check fopen/fwrite results; update
writeFileToDirectory to write to the temp file returned by tempnam (use
file_put_contents($tempPath, $contents, LOCK_EX) or check fwrite/fflush returns)
and verify the write succeeded, then if an extension is required perform a
rename($tempPath, $targetPath) and check its return value; on any failure (write
or rename) unlink the temp file and return null, and only return $fullPath when
write+rename (or write alone when no rename needed) have succeeded. Target
symbols: writeFileToDirectory, tempnam, rename, unlink,
file_put_contents/fopen/fwrite/fflush/fclose.
| $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 concern: direct EntityManager usage in Domain layer.
Line 67 uses $this->entityManager->remove($cache) directly in src/Domain/**. Per the coding guidelines, domain services shouldn't perform persistence side effects (this isn't DynamicListAttr-related).
Consider moving the cache purging and persistence logic to an Application/Infrastructure layer component, or have the repository handle the remove/persist internally without exposing the EntityManager here.
Based on coding guidelines, Domain services should not perform persistence operations.
🤖 Prompt for AI Agents
In `@src/Domain/Common/RemotePageFetcher.php` around lines 65 - 70,
RemotePageFetcher is performing persistence side effects by calling
$this->entityManager->remove(...) and then
$this->urlCacheRepository->persist(...); remove direct EntityManager usage from
the Domain class and push the logic into the repository/infrastructure layer:
add a single repository method (e.g., UrlCacheRepository::replaceCacheForUrl or
purgeAndSave) that encapsulates getByUrl, removal of existing UrlCache entities,
and persisting the new UrlCache, then update RemotePageFetcher to call that
repository method (passing $url, $content, $lastModified) and delete any
EntityManager references from RemotePageFetcher.
| private function prepareUrl(string $url, array $userData): string | ||
| { | ||
| //# fix the Editor replacing & with & | ||
| $url = str_ireplace('&', '&', $url); | ||
|
|
||
| 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); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ISO-8859-1 encoding URLs PHP non-Latin characters issues
💡 Result:
Short answer: modern URLs/IRIs should be encoded as UTF-8 then percent-encoded; ISO‑8859‑1 percent-encoding will misinterpret non‑Latin characters in clients/servers expecting UTF‑8. Use UTF‑8 throughout (or convert on input) and use rawurlencode() (RFC‑3986) or idn_to_ascii() for hostnames. [1][2][3]. (rfc-editor.org)
Common causes
- You percent‑encode ISO‑8859‑1 byte values (e.g. %E9) but consumers expect UTF‑8 (%C3%A9), so characters become garbled. [1][4]. (rfc-editor.org)
Fixes (concise)
- Prefer UTF‑8 everywhere: store strings as UTF‑8 and percent‑encode them. Use rawurlencode() to follow RFC‑3986 (spaces → %20). [2]. (php.net)
- If input is ISO‑8859‑1, convert to UTF‑8 first, then rawurlencode():
- PHP example:
$utf8 = mb_convert_encoding($iso_string, 'UTF-8', 'ISO-8859-1');
$url_safe = rawurlencode($utf8);
- PHP example:
- If you only have single‑byte ISO data and want a quick conversion: utf8_encode($iso_string) then rawurlencode(), but prefer mb_convert_encoding for clarity.
- For query strings from forms, ensure pages use charset=UTF-8 and send so browsers submit UTF‑8. [3]. (stackoverflow.com)
- For internationalized domain names (non‑ASCII host), use IDNA: idn_to_ascii() in PHP. [2]. (php.net)
If you want, tell me how you currently build/receive the URL (PHP version, example string) and I’ll give the exact code to fix it.
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc3986?utm_source=openai
- 2: https://www.rfc-editor.org/rfc/rfc3986?utm_source=openai
- 3: https://www.php.net/manual/en/function.urlencode.php?utm_source=openai
- 4: https://stackoverflow.com/questions/4683081/is-it-advisable-to-have-non-ascii-characters-in-the-url?utm_source=openai
- 5: https://www.php.net/manual/en/function.urlencode.php?utm_source=openai
🌐 Web query:
modern web standards UTF-8 vs ISO-8859-1 URL encoding
💡 Result:
Short answer: use UTF‑8 everywhere. Modern web standards and browser/URL specs expect UTF‑8 for document encoding, form submission and percent-encoding of non‑ASCII characters; ISO‑8859‑1 (or Windows‑1252) is legacy and causes interoperability/garbling issues.
Key points and sources
- HTML/Forms: HTML5/WHATWG encourage/assume UTF‑8; form submission (application/x‑www-form-urlencoded) and URL-related APIs use the document’s encoding (and in practice UTF‑8). Use or UTF‑8 on the HTTP Content-Type. (html.spec.whatwg.org)
- URL percent‑encoding: URIs are sequences of octets; modern URL standards and implementations percent‑encode the UTF‑8 octets for non‑ASCII characters (WHATWG URL / browser APIs follow UTF‑8). (rfc-editor.org)
- Legacy (ISO‑8859‑1): Historically some HTTP text/* defaults and legacy servers used ISO‑8859‑1 (or locale defaults). Relying on ISO‑8859‑1 risks mis-decoding and security/usability problems; browsers do encoding sniffing, so explicitly declaring UTF‑8 is required for correctness. (stackoverflow.com)
- Practical guidance: always serve HTML with Content-Type charset=utf-8, include early in the head, and percent-encode URL parts using UTF‑8 (use encodeURIComponent / URL / URLSearchParams in JS). (html.spec.whatwg.org)
If you want, I can list exact spec quotes and links for implementation detail (percent‑encode sets, form encoding algorithm, HTTP charset rules).
Citations:
- 1: https://html.spec.whatwg.org/document-metadata.html?utm_source=openai
- 2: https://www.rfc-editor.org/rfc/rfc3986?utm_source=openai
- 3: https://stackoverflow.com/questions/52351400/why-its-necessary-to-specify-the-character-encoding-in-an-html5-document-if-the?utm_source=openai
- 4: https://html.spec.whatwg.org/document-metadata.html?utm_source=openai
🌐 Web query:
RFC 3986 URL encoding character set requirements
💡 Result:
Quick summary of RFC 3986 requirements for characters and percent-encoding in URIs:
-
Character classes:
- unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~". These need not be percent-encoded; percent-encoded octets corresponding to unreserved characters should be decoded by normalizers. [1]
- reserved = gen-delims / sub-delims. These may be used as delimiters and therefore normally must be percent-encoded when used as data. [1]
- gen-delims = ":" / "/" / "?" / "#" / "[" / "]" / "@". [1]
- sub-delims = "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "=". [1]
-
Percent-encoding syntax and rules:
- pct-encoded = "%" HEXDIG HEXDIG (two hexadecimal digits). Uppercase hex is recommended for consistency; uppercase and lowercase hex are equivalent. The "%" character itself must be encoded as "%25" when used as data. Implementations must not percent-encode or decode the same string more than once. Decode percent-encoded octets corresponding to unreserved characters when normalizing. [2]
Sources: RFC 3986 (Sections 2.1–2.4, 2.2–2.3). [1][2]
References:
[1] RFC 3986 (rfc-editor).
[2] RFC 3986 (IETF datatracker).
🏁 Script executed:
cat -n src/Domain/Common/RemotePageFetcher.php | head -150Repository: phpList/core
Length of output: 5213
Remove the ISO-8859-1 conversion; it breaks percent-encoded URLs.
Line 121 converts the entire URL to ISO-8859-1, but this conflicts with RFC 3986 and modern standards. The rawurlencode() on line 118 correctly percent-encodes userData values as UTF-8 octets (e.g., é → %C3%A9), but then the ISO-8859-1 conversion causes HTTP clients expecting UTF-8 to misinterpret these bytes, mangling non-Latin characters. Delete the mb_convert_encoding() call on line 121 and keep everything UTF-8 end-to-end.
🤖 Prompt for AI Agents
In `@src/Domain/Common/RemotePageFetcher.php` around lines 111 - 124, The
prepareUrl method is converting the final URL to ISO-8859-1 which breaks
percent-encoded UTF-8 sequences; remove the mb_convert_encoding($url,
'ISO-8859-1', 'UTF-8') call in prepareUrl so the URL remains UTF-8 after
rawurlencode(), and return the result via expandUrl($url) unchanged; ensure no
other code in prepareUrl replaces or re-encodes the URL.
| public function __invoke(PlaceholderContext $ctx): string | ||
| { | ||
| if ($ctx->isText()) { | ||
| return $this->config->getValue(ConfigOption::VCardUrl) ?? ''; | ||
| } | ||
|
|
||
| return htmlspecialchars($this->config->getValue(ConfigOption::VCardUrl) ?? ''); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHP htmlspecialchars default flags ENT_QUOTES single quotes escaping behavior
💡 Result:
Short answer: Since PHP 8.1 the default flags for htmlspecialchars are ENT_QUOTES | ENT_SUBSTITUTE | ENT_HTML401, so single quotes are converted by default. With ENT_QUOTES a single quote becomes ' for HTML4 (or ' for XML/XHTML/HTML5). [1][2]
References:
- PHP manual: htmlspecialchars (shows default flags and replacements). [1]
- PHP.watch explanation of the PHP 8.1 change (ENT_COMPAT → ENT_QUOTES|ENT_SUBSTITUTE|ENT_HTML401). [2]
🏁 Script executed:
find . -maxdepth 2 -type f \( -name "composer.json" -o -name ".php-version" -o -name ".tool-versions" \) -exec cat {} \;Repository: phpList/core
Length of output: 5758
Avoid redundant config fetch.
The config value is fetched twice; store it in a variable once and reuse it.
🔧 Suggested fix
+ $url = $this->config->getValue(ConfigOption::VCardUrl) ?? '';
+ if ($url === '') {
+ return '';
+ }
if ($ctx->isText()) {
- return $this->config->getValue(ConfigOption::VCardUrl) ?? '';
+ return $url;
}
- return htmlspecialchars($this->config->getValue(ConfigOption::VCardUrl) ?? '');
+ return htmlspecialchars($url);🤖 Prompt for AI Agents
In `@src/Domain/Configuration/Service/Placeholder/ContactUrlValueResolver.php`
around lines 22 - 28, In ContactUrlValueResolver::__invoke, avoid calling
$this->config->getValue(ConfigOption::VCardUrl) twice; fetch it once into a
local variable (e.g., $url = $this->config->getValue(ConfigOption::VCardUrl) ??
''), then return $url for text contexts and htmlspecialchars($url) for non-text
contexts, preserving the existing logic with a single config lookup.
| $remoteContent = ($this->remotePageFetcher)($regs[1], []); | ||
| if (!$remoteContent) { | ||
| $this->eventLogManager->log( | ||
| page: 'unknown page', | ||
| entry: 'Error fetching URL: ' . $loadedMessageData['sendurl'] . ' cannot proceed', | ||
| ); | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Error message references wrong URL variable.
Line 192 logs $loadedMessageData['sendurl'] but the actual URL being fetched is $regs[1] (extracted from the content). This could be misleading in logs.
🔧 Suggested fix
$remoteContent = ($this->remotePageFetcher)($regs[1], []);
if (!$remoteContent) {
$this->eventLogManager->log(
page: 'unknown page',
- entry: 'Error fetching URL: ' . $loadedMessageData['sendurl'] . ' cannot proceed',
+ entry: 'Error fetching URL: ' . $regs[1] . ' cannot proceed',
);
return false;
}🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/MessagePrecacheService.php` around lines 188 -
196, The log message is referencing the wrong URL variable which is misleading:
when ($this->remotePageFetcher) is called with $regs[1] and returns falsy (the
$remoteContent check in MessagePrecacheService), update the
$this->eventLogManager->log call to reference the actual fetched URL ($regs[1])
instead of $loadedMessageData['sendurl'] — optionally include both values for
context (fetched URL $regs[1] and original sendurl
$loadedMessageData['sendurl']) so the log clearly shows which URL failed; adjust
the log entry in the remoteContent falsy branch accordingly.
| public function getDataById(int $subscriberId): array | ||
| { | ||
| return $this->createQueryBuilder('s') | ||
| ->select('s') | ||
| ->where('s.id = :subscriberId') | ||
| ->setParameter('subscriberId', $subscriberId) | ||
| ->getQuery() | ||
| ->getArrayResult()[0]; |
There was a problem hiding this comment.
Handle “not found” to avoid undefined offset/TypeError.
getArrayResult()[0] will throw a notice and return null when no record exists, violating the array return type.
🐛 Safer return handling
- public function getDataById(int $subscriberId): array
+ public function getDataById(int $subscriberId): ?array
{
- return $this->createQueryBuilder('s')
+ $result = $this->createQueryBuilder('s')
->select('s')
->where('s.id = :subscriberId')
->setParameter('subscriberId', $subscriberId)
->getQuery()
- ->getArrayResult()[0];
+ ->getArrayResult();
+
+ return $result[0] ?? null;
}📝 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.
| public function getDataById(int $subscriberId): array | |
| { | |
| return $this->createQueryBuilder('s') | |
| ->select('s') | |
| ->where('s.id = :subscriberId') | |
| ->setParameter('subscriberId', $subscriberId) | |
| ->getQuery() | |
| ->getArrayResult()[0]; | |
| public function getDataById(int $subscriberId): ?array | |
| { | |
| $result = $this->createQueryBuilder('s') | |
| ->select('s') | |
| ->where('s.id = :subscriberId') | |
| ->setParameter('subscriberId', $subscriberId) | |
| ->getQuery() | |
| ->getArrayResult(); | |
| return $result[0] ?? null; | |
| } |
🤖 Prompt for AI Agents
In `@src/Domain/Subscription/Repository/SubscriberRepository.php` around lines 209
- 216, getDataById currently calls getArrayResult()[0] which will trigger an
undefined offset/TypeError when no record is found; modify getDataById to store
the result of createQueryBuilder(...)->getQuery()->getArrayResult() in a
variable, check if the array is empty, and then either return an empty array or
throw a specific not-found exception (e.g., SubscriberNotFoundException) to
preserve the declared return type and avoid notices; update the method
(getDataById) to handle the empty result path before indexing [0].
| public function testHtmlEscapesUrlAndAppendsUid(): void | ||
| { | ||
| $raw = 'https://e.com/prefs.php?a=1&x=<tag>"\''; | ||
| $this->config->method('getValue') | ||
| ->with(ConfigOption::PreferencesUrl) | ||
| ->willReturn($raw); | ||
|
|
||
| $ctx = new PlaceholderContext(user: $this->makeUser(), format: OutputFormat::Html); | ||
|
|
||
| $resolver = new PreferencesUrlValueResolver($this->config); | ||
| $result = $resolver($ctx); | ||
|
|
||
| $this->assertSame( | ||
| sprintf('%s%suid=%s', htmlspecialchars($raw), htmlspecialchars('&'), 'UID-PREF'), | ||
| $result | ||
| ); |
There was a problem hiding this comment.
Align HTML escaping flags with production.
Line 77–78 uses htmlspecialchars() defaults, but the resolver uses ENT_QUOTES | ENT_SUBSTITUTE + UTF-8. This can break the expected string (single quotes won’t be escaped the same).
🛠️ Suggested fix
- $this->assertSame(
- sprintf('%s%suid=%s', htmlspecialchars($raw), htmlspecialchars('&'), 'UID-PREF'),
- $result
- );
+ $this->assertSame(
+ sprintf(
+ '%s%suid=%s',
+ htmlspecialchars($raw, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
+ htmlspecialchars('&', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
+ 'UID-PREF'
+ ),
+ $result
+ );🤖 Prompt for AI Agents
In
`@tests/Unit/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolverTest.php`
around lines 65 - 80, The test's expected string uses htmlspecialchars() with
default flags while PreferencesUrlValueResolver uses ENT_QUOTES | ENT_SUBSTITUTE
and 'UTF-8'; update the test in testHtmlEscapesUrlAndAppendsUid to call
htmlspecialchars($raw, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') and
htmlspecialchars('&', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') so the escaping
behavior matches PreferencesUrlValueResolver and the asserted string is produced
correctly.
| private function makeCtx(Subscriber $user = null): PlaceholderContext | ||
| { | ||
| $u = $user ?? (function () { | ||
| $s = new Subscriber(); | ||
| $s->setEmail('user@example.com'); | ||
| $s->setUniqueId('UID-X'); | ||
| // Ensure the entity has a non-null ID for repository lookup | ||
| $rp = new \ReflectionProperty(Subscriber::class, 'id'); | ||
| $rp->setAccessible(true); | ||
| $rp->setValue($s, 42); | ||
| return $s; | ||
| })(); | ||
|
|
||
| return new PlaceholderContext($u, OutputFormat::Text); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "composer.json" -o -name ".php-version" -o -name "phpversion.txt" 2>/dev/null | head -5Repository: phpList/core
Length of output: 38
🏁 Script executed:
cd /tmp && git ls-files | grep -E "(composer\.json|\.php-version)" | head -5Repository: phpList/core
Length of output: 124
🏁 Script executed:
cat /tmp/composer.json 2>/dev/null | grep -A 5 '"php"'Repository: phpList/core
Length of output: 38
🏁 Script executed:
cat /tmp/tests/Unit/Domain/Configuration/Service/Placeholder/UserDataSupportingResolverTest.php | sed -n '24,38p'Repository: phpList/core
Length of output: 179
🏁 Script executed:
pwd && ls -laRepository: phpList/core
Length of output: 1598
🏁 Script executed:
git ls-files | grep "composer.json" | head -1Repository: phpList/core
Length of output: 69
🏁 Script executed:
git ls-files | grep "UserDataSupportingResolverTest.php"Repository: phpList/core
Length of output: 142
🏁 Script executed:
cat composer.json | grep -A 3 '"php"'Repository: phpList/core
Length of output: 192
🏁 Script executed:
sed -n '20,40p' tests/Unit/Domain/Configuration/Service/Placeholder/UserDataSupportingResolverTest.phpRepository: phpList/core
Length of output: 766
🌐 Web query:
PHP 8.1 deprecated implicitly nullable parameters null default
💡 Result:
Short answer: PHP 8.1 deprecates using an implicit nullable type by writing "Type $param = null". You should change such signatures to an explicit nullable union or ?-type (e.g. Type|null $param = null or ?Type $param = null). [1][2][3]
Details and examples:
- Deprecated pattern:
function foo(SomeClass $x = null) {} // implicitly nullable — deprecated in 8.1. [1][2] - Correct replacements:
function foo(?SomeClass $x = null) {} // short nullable syntax. [1]
function foo(SomeClass|null $x = null) {} // union nullable syntax. [1] - Rationale: implicit-null defaults are confusing and conflict with other rules (and caused special-casing around optional-before-required parameters), so RFCs and the migration guide require explicit nullability. [2][1]
- Also note: PHP 8.1 added deprecations for passing null to non-nullable internal function parameters; internal functions now warn when null is passed where null wasn't explicitly allowed. [3]
If you want, I can scan your codebase for occurrences and provide an automated fix (sed/php-cs-fixer rules / Rector suggestions).
Make Subscriber explicitly nullable.
Line 24 uses a default null with a non-nullable type. PHP 8.1 deprecated this pattern—you need to explicitly mark the parameter as nullable.
Suggested fix
- private function makeCtx(Subscriber $user = null): PlaceholderContext
+ private function makeCtx(?Subscriber $user = null): PlaceholderContext📝 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.
| private function makeCtx(Subscriber $user = null): PlaceholderContext | |
| { | |
| $u = $user ?? (function () { | |
| $s = new Subscriber(); | |
| $s->setEmail('user@example.com'); | |
| $s->setUniqueId('UID-X'); | |
| // Ensure the entity has a non-null ID for repository lookup | |
| $rp = new \ReflectionProperty(Subscriber::class, 'id'); | |
| $rp->setAccessible(true); | |
| $rp->setValue($s, 42); | |
| return $s; | |
| })(); | |
| return new PlaceholderContext($u, OutputFormat::Text); | |
| } | |
| private function makeCtx(?Subscriber $user = null): PlaceholderContext | |
| { | |
| $u = $user ?? (function () { | |
| $s = new Subscriber(); | |
| $s->setEmail('user@example.com'); | |
| $s->setUniqueId('UID-X'); | |
| // Ensure the entity has a non-null ID for repository lookup | |
| $rp = new \ReflectionProperty(Subscriber::class, 'id'); | |
| $rp->setAccessible(true); | |
| $rp->setValue($s, 42); | |
| return $s; | |
| })(); | |
| return new PlaceholderContext($u, OutputFormat::Text); | |
| } |
🤖 Prompt for AI Agents
In
`@tests/Unit/Domain/Configuration/Service/Placeholder/UserDataSupportingResolverTest.php`
around lines 24 - 38, The makeCtx helper has a parameter typed as Subscriber but
defaulted to null which is deprecated; update the function signature of makeCtx
to accept a nullable Subscriber (e.g., ?Subscriber $user = null) and keep the
existing null-coalescing logic that constructs a Subscriber when $user is null;
ensure any related docblock/type hints reflect the nullable type so tools and
PHP 8.1+ no longer warn.
| ->withConsecutive( | ||
| [1, $precached, $subscriber1], | ||
| [1, $precached, $subscriber2] | ||
| ) | ||
| ->willReturnOnConsecutiveCalls($precached, $precached); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHPUnit 10 withConsecutive deprecated removed migration
💡 Result:
Short summary: PHPUnit 10 removed InvocationMocker::withConsecutive(). Replace it by (a) using argument constraints/callbacks to validate per-call arguments, (b) use willReturnOnConsecutiveCalls / willReturnCallback for returned values, or (c) adopt a framework/test-case helper that reintroduces a static withConsecutive helper (many projects provide one). See examples below.
Why: withConsecutive was deprecated in 9.6 and removed in PHPUnit 10. [1][2]
Common migration patterns
- Validate per-call arguments with a callback constraint (recommended when you asserted arguments)
- Old (PHPUnit ≤9):
$mock->expects($this->exactly(2))
->method('foo')
->withConsecutive(['a'], ['b']); - New:
$mock->expects($this->exactly(2))
->method('foo')
->with($this->callback(function ($arg) {
static $i = 0;
$expected = ['a', 'b'][$i++] ?? null;
return $arg === $expected;
}));
This pattern (use callback constraint) is documented in the PHPUnit test-doubles docs. [3]
- If you only need different return values, use willReturnOnConsecutiveCalls (no withConsecutive needed)
- $mock->method('foo')->willReturnOnConsecutiveCalls('r1', 'r2');
(Use willReturnCallback when logic is required.) [4]
- Project-level helper approach (ad hoc compatibility)
- Some frameworks/teams add a helper/trait that provides a static withConsecutive() wrapper you can call from tests (CakePHP provides an example trait/approach and docs showing a static-compatible helper). Example conversion shown in CakePHP docs: convert ->withConsecutive(...) to ->with(...self::withConsecutive([...], [...])) via a trait. [5][6]
Concrete resources to read and copy examples
- PHPUnit 9.6 test-doubles note (withConsecutive deprecated/removed). [3]
- PHPUnit 10 release/upgrade notes (removals). [1][2]
- CakePHP and Drupal migration examples showing callback/trait replacements and practical diffs. [5][6][4]
If you want, tell me which specific withConsecutive usage (code snippet) you have and I’ll provide an exact migrated replacement.
Sources
[1] PHPUnit 10 release/upgrade notes.
[2] GitHub issue / PHPUnit removal discussion (withConsecutive deprecation).
[3] PHPUnit 9.6 Test Doubles docs (callback constraint example).
[4] Drupal migration issue and examples (use of callbacks/willReturnCallback).
[5] CakePHP PHPUnit 10 upgrade notes (shows converting withConsecutive via helper/trait).
[6] CakePHP docs example conversion.
Heads up: withConsecutive was deprecated in PHPUnit 9.6 and removed in PHPUnit 10.
For PHPUnit 10 migration, the recommended pattern is using a callback constraint with a counter to validate per-call arguments, or a project-level helper/trait if you want to keep the syntax similar. willReturnOnConsecutiveCalls alone won't validate arguments—you'd need to combine it with the callback approach.
Not blocking since you're on PHPUnit 9.5.0, but worth tackling before upgrading.
🤖 Prompt for AI Agents
In
`@tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php`
around lines 371 - 375, Replace the deprecated withConsecutive usage on the mock
by switching to a willReturnCallback closure that keeps a local call counter;
inside the callback validate each invocation's arguments against the expected
values for that call (e.g., check arg0==1, arg1==$precached, arg2==$subscriber1
for call 0, then subscriber2 for call 1) and return the corresponding predefined
response (or pull from an array of return values, similar to
willReturnOnConsecutiveCalls). Concretely, change the mock setup that used
withConsecutive(...) ->willReturnOnConsecutiveCalls(...) to a single
->willReturnCallback(function(...$args) use (&$i, $expectedArgs, $returns) { /*
assert args match $expectedArgs[$i], $ret=$returns[$i]; $i++; return $ret; */
}), referencing the same expected values ($precached, $subscriber1,
$subscriber2) and returns to preserve behavior.
Summary by CodeRabbit
Release Notes
Improvements
Updates
✏️ Tip: You can customize this high-level summary in your review settings.
Thanks for contributing to phpList!