Refactor/update message sendin #374
Conversation
📝 WalkthroughWalkthroughReplaces inline message content handling with a precache pipeline: MessageDataLoader normalizes message data; MessagePrecacheService builds/caches MessagePrecacheDto (templates, html/text/footer, remote content, admin attrs, images); processing, link extraction and mail composition now operate on the DTO; per-subscriber personalization and sending use new builders, embedder, and mailer integrations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Handler as CampaignProcessorMessageHandler
participant Loader as MessageDataLoader
participant Precache as MessagePrecacheService
participant Cache as PSR-16 Cache
participant Remote as RemotePageFetcher
participant Rewriter as HtmlUrlRewriter
participant Preparator as MessageProcessingPreparator
participant Personalizer as UserPersonalizer
participant Composer as RateLimitedCampaignMailer
participant Mailer as MailerInterface
Handler->>Loader: load(message)
Loader-->>Handler: messageData (array)
Handler->>Precache: precacheMessage(message, messageData)
Precache->>Cache: get(campaignKey)
alt cache miss
Precache->>Remote: fetch remote content (if URL present)
Remote->>Rewriter: rewrite HTML resources
Rewriter-->>Remote: rewritten HTML
Remote-->>Precache: content
Precache->>Cache: set(MessagePrecacheDto)
else cache hit
Cache-->>Precache: MessagePrecacheDto
end
Precache-->>Handler: MessagePrecacheDto
loop per subscriber
Handler->>Preparator: processMessageLinks(campaignId, DTO, subscriber)
Preparator-->>Handler: DTO with links replaced
Handler->>Personalizer: personalize DTO for subscriber
Personalizer-->>Handler: personalized DTO
Handler->>Composer: composeEmail(message, subscriber, DTO)
Composer->>Mailer: send(Email)
Mailer-->>Handler: success / exception
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
157-166: Guard personalization against null text/footer to avoid runtime errorsRight now you call
personalize()ongetText()andgetFooter()unconditionally. GivenMessageContent::getText()/getFooter()are treated as nullable elsewhere, this can blow up with aTypeErrorif either isnull(and tests hide this by forcing non-null mocks).A small null-check keeps this safe:
private function handleEmailSending( Message $campaign, Subscriber $subscriber, UserMessage $userMessage, Message\MessageContent $precachedContent, ): void { - $processed = $this->messagePreparator->processMessageLinks($campaign->getId(), $precachedContent, $subscriber); - $processed->setText($this->userPersonalizer->personalize($processed->getText(), $subscriber->getEmail())); - $processed->setFooter($this->userPersonalizer->personalize($processed->getFooter(), $subscriber->getEmail())); + $processed = $this->messagePreparator->processMessageLinks( + $campaign->getId(), + $precachedContent, + $subscriber + ); + + $htmlText = $processed->getText(); + if ($htmlText !== null) { + $processed->setText( + $this->userPersonalizer->personalize($htmlText, $subscriber->getEmail()) + ); + } + + $footer = $processed->getFooter(); + if ($footer !== null) { + $processed->setFooter( + $this->userPersonalizer->personalize($footer, $subscriber->getEmail()) + ); + }This matches how
MessageProcessingPreparatoralready treats these fields and avoids surprising failures when campaigns have no HTML or footer.
🧹 Nitpick comments (1)
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
175-205: Content mocks align with new personalization flow; consider adding a null‑footer caseStubbing
getText()/getFooter()in these tests to return concrete HTML/footer strings is a good way to keep the new personalization path exercised without changing expectations.Once you add null-guards in the handler, it’d be worth adding a small test where
getFooter()(and/orgetText()) returnsnullto lock in that behavior and prevent regressions.Also applies to: 236-269, 291-325
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php(3 hunks)src/Domain/Messaging/Service/MessageProcessingPreparator.php(1 hunks)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php(1 hunks)tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php(6 hunks)tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php(0 hunks)
💤 Files with no reviewable changes (1)
- tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/MessageProcessingPreparator.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (1)
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
src/Domain/Messaging/Message/AsyncEmailMessage.php (1)
getReplyTo(49-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (4)
src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
76-87: No behavioral change here; TODO still relevantThis is just a comment tweak; existing logic around
getText()/getFooter()and link replacement remains consistent and safe. All good.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
7-9: UserPersonalizer wiring looks goodImporting and injecting
UserPersonalizerinto the handler keeps responsibilities in the message handler (where persistence and orchestration already live) and lines up with the new personalization flow. No issues here.Also applies to: 44-62
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
10-11: Test wiring for UserPersonalizer is solidInjecting a
UserPersonalizermock and stubbingpersonalize()to be identity keeps existing assertions unchanged while exercising the new dependency. The constructor args line up with the handler changes, so this looks good.Also applies to: 48-92
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
23-42: composeEmail refactor improves clarity and matches new pipelineUsing
$messagefor options and$processedContentfor subject/text/HTML matches the new “preprocess then personalize” flow and keeps the method signature self-explanatory. The handler call site aligns with this, so the change looks clean.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
5fe45ab to
492e1d0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (4)
101-113: Missing return type declaration (still unresolved).This method still lacks a return type and parameter type hint. Add
string $contentand: stringfor type safety.- public function parseLogoPlaceholders($content) + public function parseLogoPlaceholders(string $content): string
122-125: Null pointer dereference risk (still unresolved).
findByFilename()can returnnull, but line 123 calls->getData()without a null check. This will throw a fatal error.$orgLogoImage = $this->templateImageRepository->findByFilename("ORGANISATIONLOGO$size.png"); - if (!empty($orgLogoImage->getData())) { + if ($orgLogoImage !== null && !empty($orgLogoImage->getData())) { return; }
127-128: Null pointer dereference risk (still unresolved).
findById()can returnnull, but line 128 calls->getData()without checking. Add a null guard before decoding.$logoImage = $this->templateImageRepository->findById((int) $logoImageId); + if ($logoImage === null) { + return; + } $imageContent = base64_decode($logoImage->getData());
154-176: Output buffer handling is fragile (still unresolved).The buffer capture (lines 169-175) grabs content before calling
ob_start(), then echoes it back after capturing the image. This can corrupt output. Use a local buffer instead:- //# rather convoluted way to get the image contents - $buffer = ob_get_contents(); - ob_end_clean(); - ob_start(); - imagepng($resized); - $imageContent = ob_get_contents(); - ob_end_clean(); - echo $buffer; + ob_start(); + imagepng($resized); + $imageContent = ob_get_clean();composer.json (1)
82-85: Past review comment still applies.The addition of these three PHP extensions is correct for the new functionality, but as noted in the previous review, please update the PR description to document why these extensions are required.
🧹 Nitpick comments (5)
src/Domain/Common/ExternalImageService.php (5)
50-55: Avoid error suppression; handle mkdir failures explicitly.Using
@mkdirhides potential errors. Consider checking if the directory creation succeeds and logging or returning an error if it fails.Apply this diff:
- if (!file_exists($this->externalCacheDir)) { - @mkdir($this->externalCacheDir); - } - - if (!file_exists($this->externalCacheDir) || !is_writable($this->externalCacheDir)) { - return false; - } + if (!file_exists($this->externalCacheDir)) { + if (!mkdir($this->externalCacheDir) && !is_dir($this->externalCacheDir)) { + return false; + } + } + + if (!is_writable($this->externalCacheDir)) { + return false; + }
64-66: Remove redundant curl_init check.Since
ext-curlis now a required dependency in composer.json, thefunction_exists('curl_init')check is unnecessary.Apply this diff:
- if (function_exists('curl_init')) { - $cacheFileContent = $this->downloadUsingCurl($filename); - } + $cacheFileContent = $this->downloadUsingCurl($filename);
76-84: Avoid error suppression on file operations.Using
@fopensilently ignores errors. Handle the failure case explicitly instead.
94-117: Address error suppression and TODO comment.This method uses
@suppression extensively (opendir, readdir, filemtime, unlink, closedir). Consider proper error handling or at least logging failures.Also, the TODO at line 102 should be resolved or tracked in an issue.
Would you like me to open an issue to track the TODO comment?
10-177: Consider moving to Infrastructure layer.This service performs filesystem and network I/O operations, which are side effects that typically belong in the Infrastructure layer rather than the Domain layer. According to the coding guidelines for domain purity, domain code should avoid persistence and transaction side effects.
While this service doesn't use Doctrine persistence, it does interact with external systems (filesystem and HTTP) which may violate domain purity principles. Consider moving this to
src/Infrastructure/instead.Based on coding guidelines, domain layer should be pure and free of side effects.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composer.json(1 hunks)config/parameters.yml.dist(2 hunks)src/Domain/Common/ExternalImageService.php(1 hunks)src/Domain/Messaging/Service/Manager/TemplateImageManager.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/parameters.yml.dist
🧰 Additional context used
📓 Path-based instructions (1)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Common/ExternalImageService.php
🧬 Code graph analysis (2)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/TemplateImageRepository.php (3)
TemplateImageRepository(12-35)findByFilename(16-24)findById(26-34)
src/Domain/Common/ExternalImageService.php (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)
🔇 Additional comments (3)
src/Domain/Common/ExternalImageService.php (3)
14-22: Constructor looks good.Clean dependency injection and simple initialization logic.
24-38: Clean cache retrieval logic.Proper validation and error handling with null returns.
119-126: Filename sanitization logic is correct.The regex patterns work but could be documented for clarity. Consider adding a comment explaining what characters are being removed/replaced.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
8e75515 to
25ef84a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
TatevikGr
changed the title
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
b1c80cd to
1643a29
Compare
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: 14
♻️ Duplicate comments (14)
src/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.php (1)
30-31: TODO remains unresolved: filter by message type.This TODO was flagged in a previous review. The header currently gets added to all HTTP-sent emails, but should only apply to individual messages (not campaign sends).
Consider checking message metadata or event context to distinguish campaigns from individual sends before injecting the header.
src/Domain/Common/RemotePageFetcher.php (2)
65-70: Domain purity breach: persistence operations in Domain layer.Lines 67 and 70 use
$this->entityManager->remove()and$this->entityManager->persist()directly, which violates the domain layer purity guideline. This was flagged in a previous review and appears to still be present.Consider moving the cache write operations to the Infrastructure or Application layer, or encapsulate them in the
UrlCacheRepositorywith methods likeremoveByUrl()andsave().Based on coding guidelines, Domain services should not perform persistence operations.
118-126: Encoding parameters are still reversed.The outer
mb_convert_encoding()should convert the result back TO'UTF-8'FROM'ISO-8859-1', but the parameters are backwards. This causes double-conversion to ISO-8859-1 instead of returning UTF-8.🔎 Proposed fix
$url = mb_convert_encoding( str_ireplace( '[' . $key . ']', urlencode($val), mb_convert_encoding($url, 'ISO-8859-1', 'UTF-8') ), - 'ISO-8859-1', - 'UTF-8' + 'UTF-8', + 'ISO-8859-1' );Or if all data is already UTF-8, simplify to:
- $url = mb_convert_encoding( - str_ireplace( - '[' . $key . ']', - urlencode($val), - mb_convert_encoding($url, 'ISO-8859-1', 'UTF-8') - ), - 'ISO-8859-1', - 'UTF-8' - ); + $url = str_ireplace('[' . $key . ']', urlencode($val), $url);src/Domain/Common/ExternalImageService.php (1)
122-122: CRITICAL: SSL certificate verification is disabled.This line disables SSL certificate verification, exposing the application to MITM attacks. Enable verification by default and make it configurable only if needed for specific environments.
The previous review comment on lines 111-141 already covers this issue in detail.
src/Domain/Messaging/Service/MessagePrecacheService.php (2)
51-54: Consider adding TTL to cache entries.The DTO is cached indefinitely with no TTL, so it persists across deployments. Adding a reasonable expiration or cache key versioning would prevent stale data issues.
93-95: Null-safety needed forparseLogoPlaceholderscalls.
$messagePrecacheDto->templatecan benull(explicitly set at line 190), butparseLogoPlaceholders()expects astring. This will throw aTypeErrorunder strict types.🔎 Proposed fix
$messagePrecacheDto->content = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->content); - $messagePrecacheDto->template = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->template); + if ($messagePrecacheDto->template !== null) { + $messagePrecacheDto->template = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->template); + } $messagePrecacheDto->htmlFooter = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->htmlFooter);src/Domain/Messaging/Service/SystemMailConstructor.php (3)
40-48: Handle potential null fromgetValue()beforepreg_replace().
ConfigProvider::getValue()returns?string, butpreg_replace()expects a non-null subject. IfPoweredByImageisn't configured, this could cause issues.🔎 Proposed fix
- $phpListPowered = preg_replace( - '/src=".*power-phplist.png"/', - 'src="powerphplist.png"', - $this->configProvider->getValue(ConfigOption::PoweredByImage), - ); + $phpListPowered = preg_replace( + '/src=".*power-phplist.png"/', + 'src="powerphplist.png"', + $this->configProvider->getValue(ConfigOption::PoweredByImage) ?? '', + );
49-52: Use strict comparison withstrpos()for consistency.Line 49 uses
str_contains()which is great, but line 51 usesstrpos()without strict comparison. If</body>is at position 0, the condition would incorrectly evaluate to false.🔎 Proposed fix
- } elseif (strpos($htmlContent, '</body>')) { + } elseif (strpos($htmlContent, '</body>') !== false) {
60-60: Null-coalesce before passing toHtml2Text.
getValue()can return null, butHtml2Text::__invoke()expects a string.🔎 Proposed fix
- $phpListPowered = trim(($this->html2Text)($this->configProvider->getValue(ConfigOption::PoweredByText))); + $phpListPowered = trim(($this->html2Text)($this->configProvider->getValue(ConfigOption::PoweredByText) ?? ''));src/Domain/Messaging/Service/Manager/TemplateImageManager.php (1)
229-237: Output buffer handling is fragile and could corrupt output.The buffer capture sequence captures content before
ob_start()(line 230), then echoes the old buffer after capturing (line 236). This can corrupt output and cause unexpected side effects.🔎 Proposed fix
- //# 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();src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
292-295: Translator placeholder syntax incorrect.Symfony Translator uses
%placeholder%syntax, not%s. The subject won't be interpolated correctly.🔎 Proposed fix
message: $this->translator->trans( - 'phplist has started sending the campaign with subject %s', - $loadedMessageData['subject'] + 'phplist has started sending the campaign with subject %subject%', + ['%subject%' => $loadedMessageData['subject']] ),
299-304: Check ifbuildPhplistEmailreturns null before accessing the email.
buildPhplistEmailcan returnnull(e.g., if recipient is blacklisted or invalid), but line 302 accesses$email->getTo()[0]without null check, which would cause a fatal error.🔎 Proposed fix
$email = $this->emailBuilder->buildPhplistEmail( messageId: $campaign->getId(), to: $notification, subject: $this->translator->trans('Campaign started'), message: $this->translator->trans( 'phplist has started sending the campaign with subject %subject%', ['%subject%' => $loadedMessageData['subject']] ), inBlast: false, ); + if ($email === null) { + $this->logger->warning('Failed to build notification email', ['to' => $notification]); + continue; + } // todo: check if from name should be from config $envelope = new Envelope(src/Domain/Messaging/Service/MessageDataLoader.php (1)
113-127: Add error handling forunserialize()failure.
unserialize()returnsfalseon failure. Passingfalsetoarray_walk_recursive()would cause an error. The code should validate the result before proceeding.🔎 Proposed fix
foreach ($storedMessageData as $storedMessageDatum) { if (str_starts_with($storedMessageDatum->getData(), 'SER:')) { $unserialized = unserialize(substr($storedMessageDatum->getData(), 4), ['allowed_classes' => false]); + if ($unserialized === false) { + continue; // Skip corrupted serialized data + } array_walk_recursive($unserialized, function (&$val) { $val = stripslashes($val); });src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
196-205: Embedded images not attached and text set redundantly.The
templateImageEmbedderreturn value is discarded (line 199), so image CIDs won't be in the email. Also,$email->text()is called three times (lines 198, 202, 204) — only the last one matters.🔎 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); + // TODO: Attach images from $this->templateImageEmbedder->attachment to $email } $email->text($textMessage); }
🧹 Nitpick comments (18)
src/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.php (2)
39-42: Redundant CLI check.The check for CLI context is unnecessary since
getCurrentRequest()already returns null in CLI environments, triggering the early return at line 26-27.🔎 Simplify by removing the CLI check
- // Only when triggered by HTTP request context (not CLI workers) - if (PHP_SAPI === 'cli') - { - return; - } - $ip = $request->getClientIp() ?? 'unknown';
32-36: Minor style inconsistency.The opening brace placement here differs from the rest of the file (lines 26, 40 use same-line braces).
🔎 Align brace style
$msg = $event->getMessage(); - if (!$msg instanceof Email) - { + if (!$msg instanceof Email) { return; }src/Domain/Common/HtmlUrlRewriter.php (1)
109-133: Consider makingnormalizePathprivate.This method appears to be an internal helper and isn't used outside this class. Keeping it
publicexpands the API surface unnecessarily. If it's needed elsewhere, fair enough - just flagging it.src/Domain/Common/RemotePageFetcher.php (1)
90-92: Consider logging the exception before returning empty.Silently swallowing exceptions makes debugging fetch failures tricky. Even a debug-level log would help track down issues when remote fetches mysteriously return nothing.
🔎 Proposed fix
} catch (Throwable $e) { + $this->eventLogManager->log( + page: 'remote-fetch', + entry: 'Failed to fetch ' . $url . ': ' . $e->getMessage() + ); return ''; }src/Domain/Common/ExternalImageService.php (5)
27-41: Replace magic number 64 with a named constant.The value
64appears here and at line 190 as a minimum file size threshold. Extract it to a class constant likeMIN_VALID_FILE_SIZEfor clarity and maintainability.🔎 Proposed refactor
+ private const MIN_VALID_FILE_SIZE = 64; + public function getFromCache(string $filename, int $messageId): ?string { $cacheFile = $this->generateLocalFileName($filename, $messageId); - if (!is_file($cacheFile) || filesize($cacheFile) <= 64) { + if (!is_file($cacheFile) || filesize($cacheFile) <= self::MIN_VALID_FILE_SIZE) { return null; }
86-86: Clarify or remove the TODO comment.The TODO suggests uncertainty about the dotfile filtering logic. If
str_starts_with($cacheFile, '.')is correct, remove the TODO. If there's a specific concern, document it or open an issue.Would you like me to open an issue to track this verification task?
80-99: Reduce error suppression for better debugging.The
@operator suppresses errors onopendir,readdir,filemtime,unlink, andclosedir. This hides issues like permission problems or filesystem errors. Consider explicit error checks or logging failures without suppression.
173-185: Set explicit permissions and handle mkdir failures.The
@mkdirsuppresses errors, and no explicit permissions are passed (defaults to 0777 & umask). Specify secure permissions (e.g.,0755or0700) and log failures for easier troubleshooting.🔎 Proposed refactor
private function ensureCacheDirectory(): bool { if (!file_exists($this->externalCacheDir)) { - @mkdir($this->externalCacheDir); + if (!mkdir($this->externalCacheDir, 0755, true)) { + $this->logger->error('Failed to create cache directory', [ + 'dir' => $this->externalCacheDir, + ]); + return false; + } } if (!file_exists($this->externalCacheDir) || !is_writable($this->externalCacheDir)) { return false; } return true; }
12-223: Consider moving I/O-heavy service to Infrastructure layer.This service is in the Domain layer but performs network requests (cURL,
file_get_contents) and filesystem operations (mkdir, file writes). Domain models and services typically express business logic without direct I/O side effects. Consider moving this to the Infrastructure layer or wrapping the I/O operations behind an interface defined in Domain and implemented in Infrastructure.tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php (1)
68-80: Consider using a proper mock instead of anonymous class.The anonymous class works fine, but using
$this->createMock()with a configuredgetListId()would be more consistent with the rest of the test setup and avoids the potential for type mismatches ifgetListMessages()has a typed return.That said, this is totally fine for now — the test is comprehensive and covers the key behaviors well.
src/Domain/Messaging/Service/SystemMailConstructor.php (1)
24-24: Add type hint for$messageparameter.The
$messageparameter lacks a type hint. Based on usage inbuildMessageBodies, it should bestring.🔎 Proposed fix
- public function __invoke($message, string $subject = ''): array + public function __invoke(string $message, string $subject = ''): arraysrc/Domain/Messaging/Service/Manager/TemplateImageManager.php (1)
192-198: Use specific types instead ofmixedfor clarity.The parameters
$newWidth,$newHeight, and$sizeFactorare typed asmixed, but they should beint,int, andfloatrespectively based oncalculateDimensions()return.🔎 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/Messaging/Service/MessagePrecacheService.php (2)
69-69: Castpreg_matchresult to bool for semantic clarity.
preg_matchreturnsint|false, butuserSpecificUrlis typed asbool. While PHP coerces this, an explicit cast or!!would make the intent clearer.🔎 Proposed fix
- $messagePrecacheDto->userSpecificUrl = preg_match('/\[.+\]/', $loadedMessageData['sendurl']); + $messagePrecacheDto->userSpecificUrl = (bool) preg_match('/\[.+\]/', $loadedMessageData['sendurl']);
159-159: Minor: Extra space before closing parenthesis.Tiny formatting nit on line 159.
🔎 Proposed fix
- if ($this->isHtml($messagePrecacheDto->footer )) { + if ($this->isHtml($messagePrecacheDto->footer)) {src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
93-111: Remove or document commented-out code.This large block of commented code clutters the file. If it's planned future work, consider adding a TODO or moving it to an issue. If it's no longer needed, delete it.
127-145: More commented-out code to clean up.Similar to the earlier block — this commented code should be removed or tracked in an issue if it represents deferred functionality.
src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
145-152: Use specific type instead ofmixedfor$destinationEmail.The parameter is typed as
mixedbut it's always a string based onresolveDestinationEmailAndMessage()return.🔎 Proposed fix
private function createBaseEmail( int $messageId, - mixed $destinationEmail, + string $destinationEmail, ?string $fromEmail, ?string $fromName, ?string $subject, ?bool $inBlast ) : Email {src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
26-27: Add space before assignment operator for consistency.Lines 26-27 have
$htmlCharset=and$textCharset=without a space before the equals sign, while all other properties use proper spacing. Adding a space improves consistency with PSR-12 style.🔎 Suggested formatting fix
- public string $htmlCharset= 'UTF-8'; + public string $htmlCharset = 'UTF-8'; - public string $textCharset= 'UTF-8'; + public string $textCharset = 'UTF-8';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
config/PHPMD/rules.xmlsrc/Bounce/Service/LockService.phpsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Common/Html2Text.phpsrc/Domain/Common/HtmlUrlRewriter.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Identity/Model/Dto/AdminAttributeDefinitionDto.phpsrc/Domain/Identity/Model/Dto/CreateAdministratorDto.phpsrc/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.phpsrc/Domain/Messaging/Message/SubscriberConfirmationMessage.phpsrc/Domain/Messaging/Message/SubscriptionConfirmationMessage.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Model/Dto/CreateTemplateDto.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Model/Dto/UpdateTemplateDto.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Messaging/Service/MessagePrecacheService.phpsrc/Domain/Messaging/Service/SystemMailConstructor.phpsrc/Domain/Messaging/Service/TemplateImageEmbedder.phpsrc/Domain/Subscription/Model/Dto/AttributeDefinitionDto.phpsrc/Domain/Subscription/Model/Dto/CreateSubscriberListDto.phpsrc/Domain/Subscription/Model/Dto/SubscriberImportOptions.phpsrc/Domain/Subscription/Repository/UserBlacklistRepository.phpsrc/Domain/Subscription/Service/Manager/SubscribePageManager.phpsrc/Domain/Subscription/Service/SubscriberCsvImporter.phptests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.phptests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php
💤 Files with no reviewable changes (11)
- src/Domain/Messaging/Message/SubscriptionConfirmationMessage.php
- src/Domain/Messaging/Model/Dto/CreateTemplateDto.php
- src/Domain/Messaging/Model/Dto/UpdateTemplateDto.php
- src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php
- src/Domain/Identity/Model/Dto/CreateAdministratorDto.php
- src/Domain/Subscription/Model/Dto/CreateSubscriberListDto.php
- src/Domain/Identity/Model/Dto/AdminAttributeDefinitionDto.php
- src/Domain/Subscription/Model/Dto/SubscriberImportOptions.php
- src/Bounce/Service/LockService.php
- src/Domain/Subscription/Service/Manager/SubscribePageManager.php
- src/Domain/Messaging/Message/SubscriberConfirmationMessage.php
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Domain/Subscription/Service/SubscriberCsvImporter.php
- src/Domain/Common/Html2Text.php
- src/Domain/Common/TextParser.php
- src/Domain/Messaging/Service/TemplateImageEmbedder.php
- src/Domain/Subscription/Repository/UserBlacklistRepository.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/SystemMailConstructor.phpsrc/Domain/Common/HtmlUrlRewriter.phpsrc/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.phpsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/Service/MessagePrecacheService.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.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 (8)
src/Domain/Messaging/Service/SystemMailConstructor.php (3)
src/Domain/Common/Html2Text.php (3)
Html2Text(10-85)__construct(14-16)__invoke(18-84)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/TemplateRepository.php (2)
TemplateRepository(12-20)findOneById(16-19)
src/Domain/Common/ExternalImageService.php (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)
tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php (2)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Messaging/Service/MessageProcessingPreparator.php (2)
processMessageLinks(61-85)MessageProcessingPreparator(15-97)
src/Domain/Common/RemotePageFetcher.php (4)
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-206)addAbsoluteResources(13-73)src/Domain/Configuration/Model/UrlCache.php (2)
getLastModified(48-51)setLastModified(69-73)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (6)
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-206)buildPhplistEmail(39-76)src/Domain/Messaging/Service/MessageDataLoader.php (1)
MessageDataLoader(13-226)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (3)
RateLimitedCampaignMailer(14-55)composeEmail(24-44)send(49-54)src/Domain/Messaging/Service/MessagePrecacheService.php (1)
precacheMessage(48-100)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (6)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Service/SystemMailConstructor.php (1)
SystemMailConstructor(13-101)src/Domain/Messaging/Service/TemplateImageEmbedder.php (1)
TemplateImageEmbedder(16-306)src/Domain/Subscription/Repository/SubscriberRepository.php (1)
SubscriberRepository(21-208)src/Domain/Subscription/Repository/UserBlacklistRepository.php (3)
UserBlacklistRepository(14-59)isEmailBlacklisted(45-58)findOneByEmail(38-43)src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php (2)
SubscriberHistoryManager(18-143)addHistory(45-56)
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/MessagePrecacheService.php (8)
src/Domain/Common/Html2Text.php (1)
Html2Text(10-85)src/Domain/Common/RemotePageFetcher.php (1)
RemotePageFetcher(17-132)src/Domain/Common/TextParser.php (1)
TextParser(7-85)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Identity/Repository/AdminAttributeDefinitionRepository.php (3)
AdminAttributeDefinitionRepository(13-45)getForAdmin(23-36)getAllWithEmptyValues(38-44)src/Domain/Identity/Repository/AdministratorRepository.php (2)
AdministratorRepository(20-72)getMessageRelatedAdmins(60-71)src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
TemplateImageManager(14-241)parseLogoPlaceholders(99-111)
🔇 Additional comments (10)
config/PHPMD/rules.xml (2)
44-44: Good addition for email code.Adding
$toto the ShortVariable exceptions makes sense for message sending functionality—it's a standard convention for recipient addresses. The variable is used throughout the messaging service (EmailBuilder, MessagePrecacheDto) and fits naturally with existing exceptions like$idand$ip.
10-10: The BooleanArgumentFlag rule disabling is justified. The codebase extensively uses boolean parameters with clear, self-documenting names (e.g.,setActive(bool),precacheMessage(...?bool $forwardContent),passesBlacklistCheck(...?bool $skipBlacklistCheck)). These aren't arbitrary flags but intentional design patterns for setters and service options that make sense in context.src/Domain/Common/HtmlUrlRewriter.php (1)
13-73: Nice work on the URL rewriting logic.The edge case handling (anchors, placeholders, protocol-relative URLs, data URIs, etc.) is comprehensive, and using DOM parsing for HTML attributes is way more robust than regex. The srcset and CSS url() handling is a solid touch too.
src/Domain/Common/RemotePageFetcher.php (2)
95-109: TheexpandURLlogic looks solid now.Good use of
str_contains()instead of the oldstrpos()approach, and the sanitization withstrip_tags()+trim()+ltrim('?&')handles edge cases nicely. 👍
81-93: HTTP options are now correct.The
max_redirectsandtimeoutoptions are the proper Symfony HttpClient parameters. Nice fix from the earlier review feedback.tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php (1)
119-201: Tests correctly updated for DTO-based flow.The test cases properly exercise the refactored
processMessageLinksmethod withMessagePrecacheDto. The assertions on lines 189-200 correctly verify that link tracking URLs are injected into bothcontentandhtmlFooterfields.tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php (1)
31-137: Solid test coverage for the data loader.This test thoroughly validates the merging, parsing, and normalization logic. The temporal bounds assertion for
finishsendingis a nice touch for handling timing variations. LGTM overall.src/Domain/Messaging/Service/MessageDataLoader.php (1)
26-43: Clean structure for the data loading pipeline.The
__invokemethod nicely decomposes the loading/merging logic into focused private methods. Easy to follow and maintain.src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
22-37: Solid dependency injection setup.The constructor is well-organized with appropriate readonly properties. The configuration flags (
devVersion,devEmail, etc.) are sensibly placed as constructor parameters.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
204-205: The null-safety concern is not valid. Thefooterproperty is always initialized inMessagePrecacheService::populateBasicFields()before theMessagePrecacheDtoreaches the handler—it's assigned from$loadedMessageData['footer']sourced from the database. While the DTO property lacks a default value in its declaration, the service layer guarantees initialization. Thepersonalize()method acceptsstringparameters (with strict type hints), so passing an empty string or any database-sourced value is safe.
| private function generateLocalFileName(string $filename, int $messageId): string | ||
| { | ||
| return $this->externalCacheDir | ||
| . '/' | ||
| . $messageId | ||
| . '_' | ||
| . preg_replace([ '~[\.][\.]+~Ui', '~[^\w\.]~Ui',], ['', '_'], $filename); | ||
| } |
There was a problem hiding this comment.
Strengthen filename sanitization against path traversal.
The regex ~[\.][\.]+~Ui only removes consecutive dots (.. becomes empty), but ../ is split by the slash and not fully neutralized. An attacker could craft a URL that, after sanitization, still contains relative path components. Use basename() or a whitelist approach to ensure filenames don't escape the cache directory.
🔎 Proposed fix
private function generateLocalFileName(string $filename, int $messageId): string
{
- return $this->externalCacheDir
- . '/'
- . $messageId
- . '_'
- . preg_replace([ '~[\.][\.]+~Ui', '~[^\w\.]~Ui',], ['', '_'], $filename);
+ $sanitized = preg_replace('~[^\w\.-]~', '_', basename(parse_url($filename, PHP_URL_PATH)));
+ return $this->externalCacheDir . '/' . $messageId . '_' . $sanitized;
}🤖 Prompt for AI Agents
In src/Domain/Common/ExternalImageService.php around lines 102 to 109, the
current preg_replace sanitization can still allow path traversal (e.g., "../"
split by slashes) so replace it with a stronger approach: strip URL-decoding and
null bytes, then call basename() on the incoming filename (or apply a strict
whitelist of allowed characters) to remove any directory components, remove any
remaining disallowed characters, and truncate to a safe length before
concatenating with $this->externalCacheDir and $messageId; ensure no directory
separators remain and consider validating the final path is inside the cache dir
(realpath check) before returning.
f2db3f5 to
4a9e895
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
165c59c to
3bb34cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
35-49: Domain layer must not perform persistence operations.Line 45 directly calls
templateImageRepository->persist($image). Per the coding guidelines, domain services should not invoke Doctrine persistence APIs on regular entities. This method should return the array ofTemplateImageentities to be persisted by the Application layer caller.As per coding guidelines, domain layer should not use
$repository->persist(...)for non-DynamicListAttr entities.🔎 Suggested approach
The domain method already returns the entities. Remove the persist call here and let the Application layer/MessageHandler persist them after calling this method:
$image->setData(null); - $this->templateImageRepository->persist($image); $templateImages[] = $image; }
94-97: Domain layer must not perform persistence operations.The
delete()method callstemplateImageRepository->remove(). Per coding guidelines, domain services should not invoke Doctrine persistence APIs (including remove) on regular entities. Move this side effect to the Application layer.As per coding guidelines, domain layer should not use
$repository->remove(...)for non-DynamicListAttr entities.
♻️ Duplicate comments (9)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (1)
229-236: Output buffer handling remains fragile.The output buffer sequence (get contents → end clean → start → imagepng → get contents → end clean → echo) is unnecessarily convoluted and was flagged in a previous review. This pattern can corrupt output by echoing stale buffer contents.
🔎 Simplified approach
Replace the fragile buffer dance with a single local buffer:
- //# rather convoluted way to get the image contents - $buffer = ob_get_contents(); - ob_end_clean(); - ob_start(); - imagepng($resized); - $imageContent = ob_get_contents(); - ob_end_clean(); - echo $buffer; + ob_start(); + imagepng($resized); + $imageContent = ob_get_clean();This keeps buffering local and prevents corrupting global output.
src/Domain/Common/ExternalImageService.php (4)
68-72: Avoid caching error markers – oversized images write 'MAX_SIZE' to cache.When content exceeds
externalImageMaxSize,'MAX_SIZE'is written to the cache file. This sentinel string will pass the 64-byte validation check inisValidCacheFile()and be returned as base64-encoded garbage ingetFromCache(). Return early or skip caching instead.🔎 Suggested fix
if ($this->externalImageMaxSize && (strlen($cacheFileContent) > $this->externalImageMaxSize)) { - $cacheFileContent = 'MAX_SIZE'; + $this->logger->warning('External image exceeds max size', [ + 'url' => $filename, + 'size' => strlen($cacheFileContent), + 'max' => $this->externalImageMaxSize, + ]); + return false; } - - $this->writeCacheFile($cacheFileName, $cacheFileContent); }
108-115: Path traversal vulnerability in filename sanitization.The regex only removes consecutive dots but doesn't prevent
../sequences split by slashes. Usebasename()withparse_url()to safely extract just the filename portion.🔎 Suggested fix
private function generateLocalFileName(string $filename, int $messageId): string { - return $this->externalCacheDir - . '/' - . $messageId - . '_' - . preg_replace([ '~[\.][\.]+~Ui', '~[^\w\.]~Ui',], ['', '_'], $filename); + $path = parse_url($filename, PHP_URL_PATH) ?? $filename; + $sanitized = preg_replace('~[^\w\.-]~', '_', basename($path)); + return $this->externalCacheDir . '/' . $messageId . '_' . $sanitized; }
138-143: Return null on cURL errors instead of writing error markers to cache.Writing
'CURL_ERROR_*'or'HTTP_CODE_*'strings to the cache corrupts it. Returnnulland log the error instead.🔎 Suggested fix
if ($cURLErrNo != 0) { - $cacheFileContent = 'CURL_ERROR_' . $cURLErrNo; + $this->logger->error('cURL error downloading image', ['url' => $filename, 'errno' => $cURLErrNo]); + return null; } if ($cURLInfo['http_code'] >= 400) { - $cacheFileContent = 'HTTP_CODE_' . $cURLInfo['http_code']; + $this->logger->error('HTTP error downloading image', ['url' => $filename, 'code' => $cURLInfo['http_code']]); + return null; }
159-165: Return null on file_get_contents failure instead of error string.Same issue as cURL –
'FGC_ERROR'will be cached and served as corrupt image data.🔎 Suggested fix
$cacheFileContent = file_get_contents($filename, false, $remoteURLContext); if ($cacheFileContent === false) { - $cacheFileContent = 'FGC_ERROR'; + $this->logger->error('file_get_contents error', ['url' => $filename]); + return null; } - return $cacheFileContent; + return $cacheFileContent ?: null; }src/Domain/Common/RemotePageFetcher.php (2)
65-70: Domain layer persistence breach – EntityManager usage.Per coding guidelines for
src/Domain/**, persistence side effects are not allowed. Lines 67 and 70 call$this->entityManager->remove()and$this->urlCacheRepository->persist(). This violates domain purity. Move cache write logic to Infrastructure/Application layer or inject a repository interface that abstracts persistence.Based on coding guidelines, Domain services should not perform persistence operations.
48-59: Second cache lookup withtime()as lastModified is ineffective.Line 52 calls
findByUrlAndLastModified($url, $lastModified)where$lastModified = time(). The repository query finds entries wherelastModified > :lastModified, meaning it looks for entries with a future timestamp – these won't exist. This block essentially never returns cached content. Consider whether this query is needed or if the logic should be reworked.src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
177-179: X-Originally-to header logic is unreachable.Line 136 sets
$destinationEmail = $this->devEmailwhendevVersion && devEmailis set. Then line 177 checks$this->devEmail && $destinationEmail !== $this->devEmail, which is always false in that case. The header is never added. Should this compare against the original$to?🔎 Suggested fix
- if ($this->devEmail && $destinationEmail !== $this->devEmail) { - $email->getHeaders()->addTextHeader('X-Originally to', $destinationEmail); + if ($this->devVersion && $this->devEmail && $to !== $destinationEmail) { + $email->getHeaders()->addTextHeader('X-Originally to', $to); }Note: You'll need to pass
$tointocreateBaseEmailor capture it earlier.
196-206: Image embedder return value discarded – embedded images won't work.Line 199 calls
($this->templateImageEmbedder)(...)but discards the return value. The embedder returns modified HTML with CID references and populates its$attachmentarray. Without capturing the return and attaching the images to the email, recipients will see broken image links.Also,
$email->text($textMessage)is set three times (lines 198, 203, 205) – lines 203 and 205 are redundant.🔎 Suggested fix
private function applyContentAndFormatting(Email $email, $htmlMessage, $textMessage, int $messageId): void { $newWrap = $this->configProvider->getValue(ConfigOption::WordWrap); if ($newWrap) { $textMessage = wordwrap($textMessage, (int)$newWrap); } if (!empty($htmlMessage)) { - $email->html($htmlMessage); + $embeddedHtml = ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); + $email->html($embeddedHtml); + + foreach ($this->templateImageEmbedder->attachment as $cid => $data) { + $email->embedFromPath($data['path'], $cid, $data['mime']); + } + $email->text($textMessage); - ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); - //# In the above phpMailer strips all tags, which removes the links - // which are wrapped in < and > by HTML2text - //# so add it again - $email->text($textMessage); + } else { + $email->text($textMessage); } - $email->text($textMessage); }
🧹 Nitpick comments (4)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (1)
195-197: Consider more specific parameter types.The parameters use
mixedtypes, but they could be more specific:intfor$newWidthand$newHeight, andfloatfor$sizeFactor. This would make the contract clearer and catch type mismatches earlier.🔎 Proposed type refinement
private function resizeImageIfNecessary( string $imageContent, array $imgSize, - mixed $newWidth, - mixed $newHeight, - mixed $sizeFactor, + int $newWidth, + int $newHeight, + float $sizeFactor, ?TemplateImage $orgLogoImage ): string {src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
38-44: Handle nullablecontentwhen building HTML body.The
MessagePrecacheDto::contentproperty is nullable (?string), so concatenating it directly withhtmlFootercould produce unexpected results (e.g.,"null<footer>"or type errors in strict contexts). Consider using the null coalescing operator.🔎 Suggested fix
- $html = $messagePrecacheDto->content . $messagePrecacheDto->htmlFooter; + $html = ($messagePrecacheDto->content ?? '') . $messagePrecacheDto->htmlFooter;src/Domain/Messaging/Service/MessageDataLoader.php (1)
221-234: Minor: Redundant condition check.Lines 223-228 and 231-233 have overlapping logic. If
campaigntitleis empty and subject isn't(no subject), you set campaigntitle to subject. Then immediately after, if campaigntitle is(no title)and subject isn't(no subject), you set it again. The second block is only reachable if the first set(no title)which can't happen given the first condition. Consider simplifying.🔎 Suggested simplification
private function deriveCampaignTitle(array &$messageData): void { if (empty($messageData['campaigntitle'])) { - if ($messageData['subject'] != '(no subject)') { - $messageData['campaigntitle'] = $messageData['subject']; - } else { - $messageData['campaigntitle'] = '(no title)'; - } - } - //# copy subject to title - if ($messageData['campaigntitle'] === '(no title)' && $messageData['subject'] !== '(no subject)') { - $messageData['campaigntitle'] = $messageData['subject']; + $messageData['campaigntitle'] = ($messageData['subject'] !== '(no subject)') + ? $messageData['subject'] + : '(no title)'; } }src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
93-111: Commented-out code blocks.Lines 94-111 and 127-146 contain large blocks of commented-out legacy code. If this is intentional for reference during migration, consider adding a TODO with a ticket reference to clean it up, or remove it if it's no longer needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
config/PhpCodeSniffer/ruleset.xmlresources/translations/messages.en.xlfsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phptests/Unit/Domain/Common/RemotePageFetcherTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Domain/Common/TextParser.php
- src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php
- resources/translations/messages.en.xlf
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (5)
tests/Unit/Domain/Common/RemotePageFetcherTest.php (5)
src/Domain/Common/HtmlUrlRewriter.php (1)
HtmlUrlRewriter(11-208)src/Domain/Common/RemotePageFetcher.php (1)
RemotePageFetcher(17-125)src/Domain/Configuration/Repository/UrlCacheRepository.php (1)
UrlCacheRepository(12-32)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Model/UrlCache.php (1)
setLastModified(69-73)
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (2)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Messaging/Message/AsyncEmailMessage.php (1)
getReplyTo(49-52)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (7)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Service/SystemMailConstructor.php (2)
SystemMailConstructor(13-103)__construct(17-25)src/Domain/Messaging/Service/TemplateImageEmbedder.php (2)
TemplateImageEmbedder(16-307)__construct(33-44)src/Domain/Subscription/Repository/SubscriberRepository.php (1)
SubscriberRepository(21-208)src/Domain/Subscription/Repository/UserBlacklistRepository.php (4)
UserBlacklistRepository(14-60)__construct(16-22)isEmailBlacklisted(45-59)findOneByEmail(38-43)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
__construct(55-79)src/Domain/Messaging/Service/MessagePrecacheService.php (1)
__construct(26-41)
src/Domain/Messaging/Service/MessageDataLoader.php (2)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/MessageDataRepository.php (2)
MessageDataRepository(12-26)getForMessage(22-25)
src/Domain/Common/ExternalImageService.php (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)
🔇 Additional comments (10)
config/PhpCodeSniffer/ruleset.xml (1)
106-110: Nice fix! Duplicate rule issue resolved.The addition of the
ignoreNewlinesproperty looks good, and I can confirm that the duplicate rule declaration mentioned in the previous review has been addressed. The rule is now declared once in the Whitespace section where it semantically belongs, with the property configuration intact.Setting
ignoreNewlinestotruemakes the coding standard more flexible about operator spacing when they span multiple lines, which is a reasonable choice for readability.src/Domain/Messaging/Service/Manager/TemplateImageManager.php (4)
28-31: Constructor refactoring looks good.Replacing
EntityManagerInterfacewithConfigProvideris a step in the right direction for separating concerns. The constructor is clean and properly typed.
99-111: Logo placeholder parsing logic is sound.The regex pattern correctly matches
[LOGO:SIZE]placeholders, with sensible default handling for the size parameter. The replacement strategy is clean.
162-175: Image decoding with fallback is well-structured.The base64 decoding logic with a fallback to a minimal 1x1 transparent PNG is a sensible approach for handling missing or invalid logo data.
177-190: Dimension calculation preserves aspect ratio correctly.The logic properly identifies the larger dimension and scales proportionally, which is the right approach for logo resizing.
src/Domain/Messaging/Service/MessageDataLoader.php (1)
14-46: Solid implementation addressing past concerns.The class properly handles the serialization security with
['allowed_classes' => false], validates unserialize results, and usesstr_contains()for string checks. The$timestamp ?? time()pattern on line 50 also handles the PHP 8.1+ deprecation warning correctly.tests/Unit/Domain/Common/RemotePageFetcherTest.php (2)
88-100: Test expectation may not match implementation.
RemotePageFetcher::__invokecallsfindByUrlAndLastModifiedtwice (lines 42 and 52 of the fetcher). This test expects exactly one call, so it will fail unless the first call's return value causes an early return. If$recent->getLastModified()returns current time,$timeout < $this->defaultTtlis true and it returns early – that works. Just noting this is somewhat fragile if the TTL/timing changes.
170-202: Good coverage of URL expansion and placeholder substitution.Nice test verifying that
&is normalized, user placeholders are substituted (excluding password), and remote URL append config is applied. The assertion on line 201 confirms the flow works end-to-end.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
291-331: Admin notification handling looks solid.Good null check on line 307-309 for
UniqueConstraintViolationExceptionhandling (lines 326-329) gracefully handles duplicatestart_notifiedentries. Clean implementation.
360-364: Exception handling is delegated to message bus framework—no action needed.
MessageCacheMissingExceptionis thrown in a Symfony#[AsMessageHandler]and will be caught and handled by Symfony Messenger's framework. When the exception propagates from__invoke(), the message bus automatically handles it via logging, retry policies, and dead-letter queue configuration. This is the correct pattern for message handlers.
| if ($cacheFileContent === null) { | ||
| $cacheFileContent = $this->downloadUsingFileGetContent($filename); | ||
| } | ||
|
|
||
| if ($this->externalImageMaxSize && (strlen($cacheFileContent) > $this->externalImageMaxSize)) { | ||
| $cacheFileContent = 'MAX_SIZE'; | ||
| } | ||
|
|
||
| $this->writeCacheFile($cacheFileName, $cacheFileContent); |
There was a problem hiding this comment.
Null check needed before strlen on line 68.
If both downloadUsingCurl and downloadUsingFileGetContent return null (after applying the fixes above), strlen($cacheFileContent) will fail. Add a null guard.
🔎 Suggested fix
if ($cacheFileContent === null) {
$cacheFileContent = $this->downloadUsingFileGetContent($filename);
}
+ if ($cacheFileContent === null) {
+ return false;
+ }
+
if ($this->externalImageMaxSize && (strlen($cacheFileContent) > $this->externalImageMaxSize)) {🤖 Prompt for AI Agents
In src/Domain/Common/ExternalImageService.php around lines 64–72, strlen() is
called on $cacheFileContent without guarding for null; update the logic to first
check $cacheFileContent !== null before performing the size check (i.e. only run
the externalImageMaxSize/strlen branch when $cacheFileContent is not null) and
ensure the value passed to writeCacheFile is null-safe (either the downloaded
content, or a clear failure marker like 'DOWNLOAD_FAILED' / 'MAX_SIZE' depending
on the condition) so writeCacheFile never receives an unexpected null that would
break downstream logic.
| private function passesBlacklistCheck(?string $to, ?bool $skipBlacklistCheck): bool | ||
| { | ||
|
|
||
| if (!$skipBlacklistCheck && $this->blacklistRepository->isEmailBlacklisted($to)) { | ||
| $this->eventLogManager->log('', sprintf('Error, %s is blacklisted, not sending', $to)); | ||
| $subscriber = $this->subscriberRepository->findOneByEmail($to); | ||
| if (!$subscriber) { | ||
| $this->logger->error('Error: subscriber not found', ['email' => $to]); | ||
|
|
||
| return false; | ||
| } | ||
| $subscriber->setBlacklisted(true); | ||
|
|
||
| $this->subscriberHistoryManager->addHistory( | ||
| subscriber: $subscriber, | ||
| message: 'Marked Blacklisted', | ||
| details: 'Found user in blacklist while trying to send an email, marked black listed', | ||
| ); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Subscriber marked blacklisted but not persisted.
Line 115 calls $subscriber->setBlacklisted(true) but there's no flush() or persist() call. The change won't be saved to the database. Consider whether this should be persisted (probably needs to be handled in the calling layer per domain purity guidelines).
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Builder/EmailBuilder.php around lines 104-127,
the code marks the Subscriber as blacklisted via
$subscriber->setBlacklisted(true) but never persists or flushes that change so
it will not be saved to the DB; either persist/flush the change here or
(preferably per domain-purity) do not persist in this method and instead return
an explicit signal so the caller can persist. Fix by one of two approaches: A)
inject the appropriate repository/entity manager and call persist/flush after
setBlacklisted(true) and update logs/histories accordingly; or B) remove
responsibility for persistence from this method, return false plus a flag or
throw a domain event that the caller will handle and persist the subscriber
change there; implement the chosen approach consistently and add a comment
documenting that persistence is handled by the caller if you choose option B.
| public function createCachedLogoImage(int $size): void | ||
| { | ||
| $logoImageId = $this->configProvider->getValue(ConfigOption::OrganisationLogo); | ||
| if (empty($logoImageId)) { | ||
| return; | ||
| } | ||
|
|
||
| $orgLogoImage = $this->templateImageRepository->findByFilename(sprintf('ORGANISATIONLOGO%s.png', $size)); | ||
| if ($orgLogoImage !== null && !empty($orgLogoImage->getData())) { | ||
| return; | ||
| } | ||
|
|
||
| $logoImage = $this->templateImageRepository->findById((int) $logoImageId); | ||
| if ($logoImage === null) { | ||
| return; | ||
| } | ||
|
|
||
| $imageContent = $this->decodeLogoImageData($logoImage->getData()); | ||
| if ($imageContent === null) { | ||
| return; | ||
| } | ||
|
|
||
| $imgSize = getimagesizefromstring($imageContent); | ||
| if ($imgSize === false) { | ||
| return; | ||
| } | ||
|
|
||
| [$newWidth, $newHeight, $sizeFactor] = $this->calculateDimensions($imgSize, $size); | ||
|
|
||
| $imageContent = $this->resizeImageIfNecessary( | ||
| $imageContent, | ||
| $imgSize, | ||
| $newWidth, | ||
| $newHeight, | ||
| $sizeFactor, | ||
| $orgLogoImage | ||
| ); | ||
|
|
||
| // else copy original | ||
| $templateImage = (new TemplateImage()) | ||
| ->setFilename('ORGANISATIONLOGO' . $size . '.png') | ||
| ->setMimetype($imgSize['mime']) | ||
| ->setData(base64_encode($imageContent)) | ||
| ->setWidth($newWidth) | ||
| ->setHeight($newHeight); | ||
|
|
||
| $this->templateImageRepository->persist($templateImage); | ||
| } |
There was a problem hiding this comment.
Domain layer must not perform persistence operations.
Line 159 calls templateImageRepository->persist($templateImage). This is a persistence operation in the domain layer on a regular entity. Per coding guidelines, the domain method should return the TemplateImage entity to be persisted by the Application layer caller, or make this method void and have the caller handle both creation and persistence.
As per coding guidelines, domain layer should not use $repository->persist(...) for non-DynamicListAttr entities.
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Manager/TemplateImageManager.php around lines
113-160, the domain method createCachedLogoImage performs a persistence call
($this->templateImageRepository->persist(...)) which violates the rule that the
domain layer must not persist regular entities; instead modify the method to
return the created TemplateImage (or null) without calling persist so the
Application layer can handle persistence, i.e., remove the persist call and
change the signature/return to return the TemplateImage instance (or adjust the
caller to receive the image data/object and persist it).
| private function resizeImageIfNecessary( | ||
| string $imageContent, | ||
| array $imgSize, | ||
| mixed $newWidth, | ||
| mixed $newHeight, | ||
| mixed $sizeFactor, | ||
| ?TemplateImage $orgLogoImage | ||
| ): string { | ||
| if ($sizeFactor >= 1) { | ||
| return $imageContent; | ||
| } | ||
|
|
||
| $original = imagecreatefromstring($imageContent); | ||
| //# creates a black image (why would you want that....) | ||
| $resized = imagecreatetruecolor($newWidth, $newHeight); | ||
| imagesavealpha($resized, true); | ||
| //# white. All the methods to make it transparent didn't work for me @@TODO really make transparent | ||
| $transparent = imagecolorallocatealpha($resized, 255, 255, 255, 127); | ||
| imagefill($resized, 0, 0, $transparent); | ||
| [$sizeW, $sizeH] = [$imgSize[0], $imgSize[1]]; | ||
|
|
||
| if (imagecopyresized( | ||
| $resized, | ||
| $original, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| $newWidth, | ||
| $newHeight, | ||
| $sizeW, | ||
| $sizeH, | ||
| )) { | ||
| if ($orgLogoImage !== null) { | ||
| $this->templateImageRepository->remove($orgLogoImage); | ||
| } | ||
|
|
||
| //# rather convoluted way to get the image contents | ||
| $buffer = ob_get_contents(); | ||
| ob_end_clean(); | ||
| ob_start(); | ||
| imagepng($resized); | ||
| $imageContent = ob_get_contents(); | ||
| ob_end_clean(); | ||
| echo $buffer; | ||
| } | ||
|
|
||
| return $imageContent; | ||
| } |
There was a problem hiding this comment.
Domain layer must not perform persistence operations.
Line 226 calls templateImageRepository->remove($orgLogoImage). This is a persistence operation in the domain layer. Per coding guidelines, remove this side effect and handle removal in the Application layer.
As per coding guidelines, domain layer should not use $repository->remove(...) for non-DynamicListAttr entities.
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
3bb34cd to
b6e2ee2
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
34-50: Domain layer must not perform persistence operations.Line 45 calls
$this->templateImageRepository->persist($image)on a regular entity. Per coding guidelines, domain services should not invoke persistence APIs on non-DynamicListAttr entities. Instead, return the array ofTemplateImageentities and let the Application layer caller handle persistence.As per coding guidelines, domain layer should not use
$repository->persist(...)for non-DynamicListAttr entities.🔎 Suggested approach
Remove the
persist()call and let the caller handle persistence:$templateImages = []; foreach ($imagePaths as $path) { $image = new TemplateImage(); $image->setTemplate($template); $image->setFilename($path); $image->setMimeType($this->guessMimeType($path)); $image->setData(null); - $this->templateImageRepository->persist($image); $templateImages[] = $image; } return $templateImages;Then have the Application layer caller persist each image after calling this method.
94-97: Domain layer must not perform persistence operations.Line 96 calls
$this->templateImageRepository->remove($templateImage). Per coding guidelines, the domain layer should not invokeremove()on regular entities. Make this methodvoidor return a boolean indicating intent, and let the Application layer handle the actual removal.As per coding guidelines, domain layer should not use
$repository->remove(...)for non-DynamicListAttr entities.
♻️ Duplicate comments (16)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (3)
113-160: Domain layer must not perform persistence operations.Line 159 calls
$this->templateImageRepository->persist($templateImage). This same issue was flagged in previous reviews. Per coding guidelines, the domain method should return the createdTemplateImageentity (or make the method return the entity) so the Application layer can handle persistence.As per coding guidelines, domain layer should not use
$repository->persist(...)for non-DynamicListAttr entities.
225-227: Domain layer must not perform persistence operations.Line 226 calls
$this->templateImageRepository->remove($orgLogoImage). This same issue was flagged in previous reviews. Per coding guidelines, remove this side effect and handle removal in the Application layer.As per coding guidelines, domain layer should not use
$repository->remove(...)for non-DynamicListAttr entities.
229-236: Output buffer handling is fragile and leaky.Lines 230-236 implement the same convoluted buffer capture that was previously flagged. The code captures content before
ob_start()is called, then echoes the old buffer after capturing image data. This can corrupt output and cause unexpected side effects.🔎 Recommended fix
Replace the fragile buffer sequence with a clean, local buffer:
if (imagecopyresized( $resized, $original, 0, 0, 0, 0, $newWidth, $newHeight, $sizeW, $sizeH, )) { if ($orgLogoImage !== null) { $this->templateImageRepository->remove($orgLogoImage); } - //# rather convoluted way to get the image contents - $buffer = ob_get_contents(); - ob_end_clean(); ob_start(); imagepng($resized); - $imageContent = ob_get_contents(); - ob_end_clean(); - echo $buffer; + $imageContent = ob_get_clean(); } return $imageContent;src/Domain/Common/RemotePageFetcher.php (3)
48-59: Cache lookup logic seems contradictory.Line 52 calls
findByUrlAndLastModified($url, $lastModified)where$lastModified = time(). Based on the repository snippet, this likely queries for entries wherelastModified > time()- i.e., future-dated entries. This effectively bypasses the DB cache on this path. If the intention is to always fetch fresh content here, the commented rationale and the code don't quite align.#!/bin/bash # Check the repository method to understand the query logic cat -n src/Domain/Configuration/Repository/UrlCacheRepository.php
65-68: Domain layer persistence violation with EntityManager::remove().Line 67 calls
$this->entityManager->remove($cache)directly. Per the coding guidelines,src/Domain/**should not have persistence side effects. Thepersist()call on line 70 via repository is acceptable, but the directEntityManager::remove()should be moved to the repository.🔎 Proposed fix
Add a
removeAll(array $caches)ordeleteByUrl(string $url)method toUrlCacheRepository:// In UrlCacheRepository public function removeByUrl(string $url): void { $caches = $this->getByUrl($url); foreach ($caches as $cache) { $this->entityManager->remove($cache); } }Then in RemotePageFetcher:
- $caches = $this->urlCacheRepository->getByUrl($url); - foreach ($caches as $cache) { - $this->entityManager->remove($cache); - } + $this->urlCacheRepository->removeByUrl($url);
116-121: Encoding conversion corrupts URL on each iteration.Line 119 converts
$urlfrom UTF-8 to ISO-8859-1 on every loop iteration. After multiple substitutions, non-ASCII characters will be progressively lost. Either remove the encoding entirely (if UTF-8 is the standard) or convert once after the loop.🔎 Proposed fix - remove unnecessary conversion
foreach ($userData as $key => $val) { if ($key !== 'password') { $url = str_replace('[' . $key . ']', rawurlencode($val), $url); - $url = mb_convert_encoding($url, 'ISO-8859-1', 'UTF-8'); } }If ISO-8859-1 output is truly required, convert once at the end:
foreach ($userData as $key => $val) { if ($key !== 'password') { $url = str_replace('[' . $key . ']', rawurlencode($val), $url); } } + $url = mb_convert_encoding($url, 'ISO-8859-1', 'UTF-8'); return $this->expandUrl($url);src/Domain/Common/ExternalImageService.php (4)
64-72: Null guard and error marker issues remain unaddressed.Two issues here that were flagged previously:
- If both download methods return
null, line 68 callsstrlen($cacheFileContent)onnull, causing aTypeErrorin PHP 8+.- Writing
'MAX_SIZE'as content corrupts the cache - this marker will pass the> 64 bytesvalidation check and be served as base64-encoded "image data."🔎 Proposed fix
if ($cacheFileContent === null) { $cacheFileContent = $this->downloadUsingFileGetContent($filename); } + if ($cacheFileContent === null) { + $this->logger->warning('Failed to download external image', ['url' => $filename]); + return false; + } + if ($this->externalImageMaxSize && (strlen($cacheFileContent) > $this->externalImageMaxSize)) { - $cacheFileContent = 'MAX_SIZE'; + $this->logger->warning('External image exceeds max size', [ + 'url' => $filename, + 'size' => strlen($cacheFileContent), + 'max' => $this->externalImageMaxSize, + ]); + return false; } $this->writeCacheFile($cacheFileName, $cacheFileContent);
108-115: Path traversal sanitization remains weak.The regex approach doesn't fully neutralize path traversal. A crafted URL could still escape the cache directory. Using
basename()on the URL path would be more robust.🔎 Proposed fix
private function generateLocalFileName(string $filename, int $messageId): string { - return $this->externalCacheDir - . '/' - . $messageId - . '_' - . preg_replace([ '~[\.][\.]+~Ui', '~[^\w\.]~Ui',], ['', '_'], $filename); + $path = parse_url($filename, PHP_URL_PATH) ?: $filename; + $sanitized = preg_replace('~[^\w.\-]~', '_', basename($path)); + return $this->externalCacheDir . '/' . $messageId . '_' . $sanitized; }
138-143: Error marker strings still written to cache content.Lines 139 and 142 assign error strings like
'CURL_ERROR_7'or'HTTP_CODE_404'to$cacheFileContent. These will be written to disk and potentially served as corrupted image data. Returnnullon error instead.🔎 Proposed fix
if ($cURLErrNo != 0) { - $cacheFileContent = 'CURL_ERROR_' . $cURLErrNo; + $this->logger->error('cURL error downloading image', [ + 'url' => $filename, + 'errno' => $cURLErrNo, + ]); + return null; } if ($cURLInfo['http_code'] >= 400) { - $cacheFileContent = 'HTTP_CODE_' . $cURLInfo['http_code']; + $this->logger->error('HTTP error downloading image', [ + 'url' => $filename, + 'http_code' => $cURLInfo['http_code'], + ]); + return null; }
149-165: Return type mismatch and error marker issue.The method returns
'FGC_ERROR'on failure, which will corrupt the cache. The return type should be?stringand returnnullon failure.🔎 Proposed fix
- private function downloadUsingFileGetContent(string $filename): string + private function downloadUsingFileGetContent(string $filename): ?string { $remoteURLContext = stream_context_create([ 'http' => [ 'method' => 'GET', 'timeout' => $this->externalImageMaxSize, 'max_redirects' => '10', ] ]); $cacheFileContent = file_get_contents($filename, false, $remoteURLContext); if ($cacheFileContent === false) { - $cacheFileContent = 'FGC_ERROR'; + $this->logger->error('file_get_contents error downloading image', ['url' => $filename]); + return null; } return $cacheFileContent; }src/Domain/Messaging/Service/Builder/EmailBuilder.php (4)
104-127: Subscriber blacklist change not persisted.Line 115 calls
$subscriber->setBlacklisted(true)but there's nopersist()orflush()call, so this change won't be saved to the database. ThesubscriberHistoryManager->addHistory()does persist the history entry, but the subscriber entity change itself is orphaned.Per domain purity guidelines, this service shouldn't call
flush(). However, the caller (buildPhplistEmail) returnsnullafter this, so there's no opportunity for the calling code to flush.Consider one of these approaches:
- Return a result object that signals the caller to persist the subscriber
- Move the blacklist marking to the caller layer (MessageHandler) where persistence is allowed
129-143: Dev mode with no devEmail leaves destinationEmail empty.If
devVersionis true butdevEmailis null/empty,$destinationEmailstays as''. This empty string gets passed to the email, potentially causing send failures or unexpected behavior.Suggested fix
private function resolveDestinationEmailAndMessage(?string $to, ?string $message): array { - $destinationEmail = ''; - if ($this->devVersion) { $message = 'To: ' . $to . PHP_EOL . $message; - if ($this->devEmail) { - $destinationEmail = $this->devEmail; - } + $destinationEmail = $this->devEmail ?? $to; } else { $destinationEmail = $to; } return [$destinationEmail, $message]; }
177-179: Unreachable "X-Originally to" header logic.When
devVersionis true anddevEmailis set,resolveDestinationEmailAndMessagesets$destinationEmail = $this->devEmail. The condition at line 177 ($destinationEmail !== $this->devEmail) will always be false, so the header is never added.To preserve the original recipient for debugging, you'd need to compare against the original
$toparameter:- if ($this->devEmail && $destinationEmail !== $this->devEmail) { - $email->getHeaders()->addTextHeader('X-Originally to', $destinationEmail); + if ($this->devVersion && $this->devEmail && $to !== $destinationEmail) { + $email->getHeaders()->addTextHeader('X-Originally to', $to); }This requires passing
$totocreateBaseEmailor storing it before resolution.
196-206: Embedded images still not attached; return value discarded.Line 199 calls
templateImageEmbedderbut:
- The return value (HTML with embedded CID references) is discarded
- The embedder's
$attachmentarray is never accessed$email->html($htmlMessage)is set on line 197 before the embedder runsThe email will have the original HTML without embedded image CIDs, and no attachments.
Also, lines 198, 203, and 205 set
$email->text($textMessage)three times - the last call makes the previous two redundant.Suggested fix
private function applyContentAndFormatting(Email $email, $htmlMessage, $textMessage, int $messageId): void { $newWrap = $this->configProvider->getValue(ConfigOption::WordWrap); if ($newWrap) { $textMessage = wordwrap($textMessage, (int)$newWrap); } if (!empty($htmlMessage)) { - $email->html($htmlMessage); - $email->text($textMessage); - ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); - //# In the above phpMailer strips all tags, which removes the links - // which are wrapped in < and > by HTML2text - //# so add it again - $email->text($textMessage); + $processedHtml = ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); + $email->html($processedHtml); + // TODO: Attach embedded images from $this->templateImageEmbedder->attachment } $email->text($textMessage); }src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
40-44: Null subject will cause TypeError.
MessagePrecacheDto::subjectis nullable (?string), but Symfony'sEmail::subject()requires a non-null string. Ifsubjectis null, this will throw a TypeError at runtime.return $email ->to($subscriber->getEmail()) - ->subject($messagePrecacheDto->subject) + ->subject($messagePrecacheDto->subject ?? '') ->text($messagePrecacheDto->textContent) ->html($html);src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
312-316: Potential empty array access on getTo().Line 314 accesses
$email->getTo()[0]but doesn't verify thatgetTo()returns a non-empty array. While unlikely givenbuildPhplistEmailsets a recipient, a defensive check would be safer:if (!$email) { continue; } + if (empty($email->getTo())) { + $this->logger->warning('Email has no recipients', ['notification' => $notification]); + continue; + } // todo: check if from name should be from config $envelope = new Envelope(
🧹 Nitpick comments (10)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (1)
192-199: Prefer specific type hints overmixed.Lines 195-197 use
mixedfor parameters that are clearlyint(newWidth, newHeight) andfloat(sizeFactor) based on how they're calculated incalculateDimensions. Using specific types improves type safety and IDE support.🔎 Proposed type hint improvements
private function resizeImageIfNecessary( string $imageContent, array $imgSize, - mixed $newWidth, - mixed $newHeight, - mixed $sizeFactor, + int $newWidth, + int $newHeight, + float $sizeFactor, ?TemplateImage $orgLogoImage ): string {src/Domain/Common/TextParser.php (1)
58-61: Consider optimizing placeholder restoration.The loop does
$indexseparatepreg_replacecalls. Since the placeholders are fixed strings (%%0%%,%%1%%, etc.), you could usestr_replacefor a minor performance gain, or build a single replacement map.💡 Alternative with str_replace
for ($j = 0; $j < $index; ++$j) { $replacement = $link[$j]; - $text = preg_replace('/\%\%' . $j . '\%\%/', $replacement, $text); + $text = str_replace('%%' . $j . '%%', $replacement, $text); }src/Domain/Common/ExternalImageService.php (1)
93-94: TODO comment needs clarification or resolution.The comment
// todo: make sure that this is what we needsuggests uncertainty. The current logic skips hidden files (starting with.), which seems reasonable for a cache directory. Either confirm this is correct and remove the TODO, or clarify what needs verification.Would you like me to help clarify the intended behavior here?
tests/Unit/Domain/Common/RemotePageFetcherTest.php (1)
170-202: Good URL expansion test, could be stronger on password exclusion.The test covers placeholder substitution and URL expansion. Consider adding an explicit assertion (via argument capture on
request()) that the password placeholder wasn't substituted, since that's security-critical behavior.src/Domain/Common/RemotePageFetcher.php (1)
81-93: HTTP options look correct now.The
max_redirectsandtimeoutoptions are valid Symfony HttpClient parameters. Consider adding debug-level logging in the catch block to aid troubleshooting.src/Domain/Messaging/Service/MessageDataLoader.php (1)
221-234: Minor: Consider strict comparison for consistency.Line 224 uses
!=while line 231 uses!==. For consistency and to avoid type coercion surprises, consider using strict comparison throughout.- if ($messageData['subject'] != '(no subject)') { + if ($messageData['subject'] !== '(no subject)') {src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
145-152: Parameter type is overly permissive.Line 147 declares
$destinationEmailasmixed, but it should be?stringto match what's actually passed fromresolveDestinationEmailAndMessage.private function createBaseEmail( int $messageId, - mixed $destinationEmail, + ?string $destinationEmail, ?string $fromEmail,
55-59: Clean up or address commented code.Lines 57-58 have commented-out replyTo logic. Either implement this functionality or remove the dead code to keep the codebase clean.
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
94-111: Remove or document commented legacy code.Lines 94-111 contain a large block of commented-out legacy logic. If this is intentional dead code pending removal, consider deleting it. If it's a reference for future implementation, add a clear TODO with a tracking issue.
127-146: Additional commented code block.Same as above - lines 127-146 contain another large commented block. Consider cleaning this up to improve maintainability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
config/PhpCodeSniffer/ruleset.xmlconfig/parameters.yml.distresources/translations/messages.en.xlfsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phptests/Unit/Domain/Common/RemotePageFetcherTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- resources/translations/messages.en.xlf
- config/parameters.yml.dist
- src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (6)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/TemplateImageRepository.php (3)
TemplateImageRepository(12-46)findByFilename(16-24)findById(26-34)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (5)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Service/SystemMailConstructor.php (2)
SystemMailConstructor(13-103)__construct(17-25)src/Domain/Messaging/Service/TemplateImageEmbedder.php (2)
TemplateImageEmbedder(16-307)__construct(33-44)src/Domain/Subscription/Repository/UserBlacklistRepository.php (4)
UserBlacklistRepository(14-60)__construct(16-22)isEmailBlacklisted(45-59)findOneByEmail(38-43)src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php (2)
SubscriberHistoryManager(18-143)addHistory(45-56)
tests/Unit/Domain/Common/RemotePageFetcherTest.php (5)
src/Domain/Common/HtmlUrlRewriter.php (1)
HtmlUrlRewriter(11-208)src/Domain/Common/RemotePageFetcher.php (1)
RemotePageFetcher(17-125)src/Domain/Configuration/Repository/UrlCacheRepository.php (1)
UrlCacheRepository(12-32)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Model/UrlCache.php (1)
setLastModified(69-73)
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (2)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Messaging/Message/AsyncEmailMessage.php (1)
getReplyTo(49-52)
src/Domain/Messaging/Service/MessageDataLoader.php (3)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/MessageDataRepository.php (2)
MessageDataRepository(12-26)getForMessage(22-25)src/Domain/Messaging/Repository/MessageRepository.php (2)
MessageRepository(16-119)getNonEmptyFields(103-118)
src/Domain/Common/ExternalImageService.php (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)
🔇 Additional comments (20)
config/PhpCodeSniffer/ruleset.xml (1)
106-110: Nice fix! Duplicate rule resolved.The duplicate
Squiz.WhiteSpace.OperatorSpacingdeclaration has been successfully removed. You kept the version with theignoreNewlinesproperty in the Whitespace section where it belongs, which is exactly what was needed. The XML is clean and the configuration looks good.src/Domain/Messaging/Service/Manager/TemplateImageManager.php (4)
28-32: Constructor refactoring looks solid.Switching from
EntityManagertoTemplateImageRepositoryandConfigProvideris a step in the right direction for domain layer separation.
99-111: Clean placeholder parsing logic.The regex-based logo placeholder replacement is straightforward and handles optional size parameters correctly.
162-175: Fallback handling looks good.The single base64 decode with fallback to a minimal placeholder PNG is clean and handles the empty case gracefully.
177-190: Dimension calculation logic is solid.The aspect-ratio-preserving resize math correctly handles both portrait and landscape orientations.
src/Domain/Common/TextParser.php (2)
9-11: Clean start.The method signature and leading whitespace trim are straightforward and safe.
67-83: Placeholder logic is functional.The substitution of special characters and newlines with placeholders works as intended. No issues here.
src/Domain/Common/ExternalImageService.php (2)
16-25: LGTM on constructor setup.Clean dependency injection with sensible defaults. The cache directory derivation from
$tempDiris straightforward.
203-220: Nice job on the write validation.The partial write detection and logging is solid. Good use of
LOCK_EXfor atomicity.tests/Unit/Domain/Common/RemotePageFetcherTest.php (5)
21-40: Clean test setup with proper mocking.Good use of intersection types for mock declarations. The
setUp()method properly initializes all required mocks.
56-74: Good coverage of the PSR cache path.Properly verifies that when in-memory cache is fresh, neither DB nor HTTP are touched.
76-100: Solid DB cache fallback test.Correctly validates the DB cache path when PSR cache misses but DB has fresh content.
102-148: Comprehensive happy-path coverage.This test validates the full fetch-rewrite-cache flow nicely. The
atLeast(2)expectation correctly reflects the implementation's cache-checking pattern.
150-168: Good failure path coverage.Properly verifies that HTTP failures don't poison the cache and return empty string.
src/Domain/Common/RemotePageFetcher.php (2)
19-29: Constructor looks clean, but EntityManager dependency is concerning.The
EntityManagerInterfacedependency hints at persistence operations in the domain layer. Based on the coding guidelines forsrc/Domain/**, this should be flagged.
95-109: URL expansion logic is solid.Good use of
str_contains()for the query string check, and proper handling of the delimiter. Thestrip_tags()andtrim()sanitization is appropriate.src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
24-45: LGTM on the DTO-based refactor.The transition from MessageContent to MessagePrecacheDto looks clean. The HTML body now correctly combines
contentwithhtmlFooter(line 38), addressing the prior review feedback.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (3)
166-173: Good addition of SendStart tracking.Setting
SendStartwhen transitioning toInProcessstatus (if not already set) is a nice improvement for campaign analytics.
291-331: Well-structured admin notification handling.Good implementation:
- Null check for
UniqueConstraintViolationExceptionhandling for idempotent marker persistence is solid- Persistence in MessageHandler is appropriate per coding guidelines
360-364: Cache miss handling improved.Good fix - throwing
MessageCacheMissingExceptionwhen the precached DTO is missing prevents the type error that would occur when passing null tohandleEmailSending. This is a clear, explicit failure mode.
| curl_setopt($cURLHandle, CURLOPT_TIMEOUT, $this->externalImageTimeout); | ||
| curl_setopt($cURLHandle, CURLOPT_FOLLOWLOCATION, true); | ||
| curl_setopt($cURLHandle, CURLOPT_MAXREDIRS, 10); | ||
| curl_setopt($cURLHandle, CURLOPT_SSL_VERIFYPEER, false); |
There was a problem hiding this comment.
Disabling SSL peer verification is a security risk.
CURLOPT_SSL_VERIFYPEER => false disables certificate validation, making the connection vulnerable to MITM attacks. Consider making this configurable or removing it.
🤖 Prompt for AI Agents
In src/Domain/Common/ExternalImageService.php at line 128,
CURLOPT_SSL_VERIFYPEER is being set to false which disables certificate
validation and is a security risk; change this so SSL peer verification is
enabled by default (set to true or remove the explicit false), and if you need
to support environments with self-signed certs make it configurable via a
constructor/config option (e.g., allow a boolean flag or environment value) that
defaults to true, document the option, and ensure any exception or SSL error is
handled/logged rather than silently bypassing verification.
| if (!preg_match('/^(http:)|(mailto:)|(ftp:)|(https:)/i', $url)) { | ||
| // avoid this | ||
| //<a href="javascript:window.open('http://hacker.com?cookie='+document.cookie)"> | ||
| $url = preg_replace('/:/', '', $url); | ||
| } |
There was a problem hiding this comment.
Removing all colons breaks legitimate URLs.
Line 26 strips every colon from the URL, which corrupts:
- Port numbers:
example.com:8080becomesexample.com8080 - Query parameters:
?time=12:30becomes?time1230 - IPv6 addresses:
[::1]becomes[]
Instead of removing colons, consider rejecting the URL entirely or prepending a safe default scheme like http://.
🔎 Safer approach: reject or default to http://
if (!preg_match('/^(http:)|(mailto:)|(ftp:)|(https:)/i', $url)) {
- // avoid this
- //<a href="javascript:window.open('http://hacker.com?cookie='+document.cookie)">
- $url = preg_replace('/:/', '', $url);
+ // Default to http:// for relative or protocol-less URLs
+ $url = 'http://' . $url;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!preg_match('/^(http:)|(mailto:)|(ftp:)|(https:)/i', $url)) { | |
| // avoid this | |
| //<a href="javascript:window.open('http://hacker.com?cookie='+document.cookie)"> | |
| $url = preg_replace('/:/', '', $url); | |
| } | |
| if (!preg_match('/^(http:)|(mailto:)|(ftp:)|(https:)/i', $url)) { | |
| // Default to http:// for relative or protocol-less URLs | |
| $url = 'http://' . $url; | |
| } |
🤖 Prompt for AI Agents
In src/Domain/Common/TextParser.php around lines 23 to 27, the current code
strips every colon from URLs which corrupts valid URLs (ports, times, IPv6);
instead of removing colons, change the logic to either reject the URL when it
doesn't start with an allowed scheme or prepend a safe default scheme (e.g.,
"http://") before further processing; do not use preg_replace to remove ':' —
validate the scheme with the regex, and if missing, either return/flag the URL
as invalid or safely prepend "http://" and proceed.
| //<a href="javascript:window.open('http://hacker.com?cookie='+document.cookie)"> | ||
| $url = preg_replace('/:/', '', $url); | ||
| } | ||
| $link[$index] = '<a href="' . $url . '" ' . $rest . '>' . $matches[4] . '</a>'; |
There was a problem hiding this comment.
Unsafe URL interpolation enables attribute injection.
Directly concatenating $url, $rest, and $matches[4] without HTML-escaping allows an attacker to inject attributes or break out of the href. For example, a crafted anchor with href="\" onclick=\"alert(1)\"" would execute script.
Escape or validate these values before building the tag.
🔎 Escape dynamic content
- $link[$index] = '<a href="' . $url . '" ' . $rest . '>' . $matches[4] . '</a>';
+ $link[$index] = '<a href="' . htmlspecialchars($url, ENT_QUOTES, 'UTF-8') . '" ' . $rest . '>' . htmlspecialchars($matches[4], ENT_QUOTES, 'UTF-8') . '</a>';Note: $rest (the attributes between href and >) is harder to sanitize safely. Consider parsing it properly or restricting which attributes are allowed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $link[$index] = '<a href="' . $url . '" ' . $rest . '>' . $matches[4] . '</a>'; | |
| $link[$index] = '<a href="' . htmlspecialchars($url, ENT_QUOTES, 'UTF-8') . '" ' . $rest . '>' . htmlspecialchars($matches[4], ENT_QUOTES, 'UTF-8') . '</a>'; |
🤖 Prompt for AI Agents
In src/Domain/Common/TextParser.php around line 28, the code concatenates $url,
$rest, and $matches[4] directly into an <a> tag which allows attribute
injection; escape and validate each dynamic piece instead of raw concatenation:
HTML-escape the link text ($matches[4]) and attribute values with
htmlspecialchars(…, ENT_QUOTES, 'UTF-8'); validate/sanitize $url by parsing it
(parse_url) and allowing only safe schemes (http, https, mailto) or
prefix/normalize data URIs as required; do not include raw $rest — instead parse
$rest into attributes and whitelist allowed attributes (e.g., rel, target) and
escape their values before reconstruction, or drop $rest entirely if unknown;
then build the anchor by safely-escaped href and validated, escaped attributes.
| $text = preg_replace( | ||
| '/(https?:\/\/)(?!www)([a-zA-Z0-9\.\/#~:?+=&%@!_\\-]+)/i', | ||
| '<a href="\\1\\2" class="url" target="_blank">\\2</a>', | ||
| $text | ||
| ); | ||
| //eg -- http://www.google.com -> <a href"http://www.google.com" target="_blank">www.google.com</a> | ||
| $text = preg_replace( | ||
| '/(https?:\/\/)(www\.)([a-zA-Z0-9\.\/#~:?+=&%@!\\-_]+)/i', | ||
| '<a href="\\1\\2\\3" class="url" target="_blank">\\2\\3</a>', | ||
| $text | ||
| ); |
There was a problem hiding this comment.
URL auto-linking lacks HTML escaping.
Lines 40-42 and 46-48 insert matched URL components directly into href and link text without escaping. If the URL contains special characters like " or <, it could break out of the attribute or inject markup.
Apply htmlspecialchars() to matched groups before concatenating them into the anchor tags.
🤖 Prompt for AI Agents
In src/Domain/Common/TextParser.php around lines 39 to 49, the regex
replacements inject raw matched URL groups into href and link text which can
allow special characters to break attributes or inject markup; update the
replacement flow to HTML-escape the matched components before building the <a>
tag (apply htmlspecialchars or equivalent to the protocol, host/prefix and path
groups) and then use the escaped values when constructing both the href
attribute and the anchor text so quotes and angle brackets are safe.
| $processed->textContent = $this->userPersonalizer->personalize( | ||
| $processed->textContent, | ||
| $subscriber->getEmail(), | ||
| ); | ||
| $processed->footer = $this->userPersonalizer->personalize($processed->footer, $subscriber->getEmail()); |
There was a problem hiding this comment.
Null footer may cause TypeError in personalize.
MessagePrecacheDto::footer is ?string, but per the past review context, UserPersonalizer::personalize() expects a non-nullable string $value parameter. If footer is null, this will throw a TypeError.
- $processed->footer = $this->userPersonalizer->personalize($processed->footer, $subscriber->getEmail());
+ $processed->footer = $this->userPersonalizer->personalize($processed->footer ?? '', $subscriber->getEmail());🤖 Prompt for AI Agents
In src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
around lines 209 to 213, MessagePrecacheDto::footer is nullable but
UserPersonalizer::personalize() requires a non-null string; avoid calling
personalize with null by checking if $processed->footer is not null before
calling personalize (or pass an explicit non-null default like '' if business
logic allows), e.g. conditionally call personalize and leave footer as null
otherwise so no TypeError is thrown.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (20)
src/Domain/Common/TextParser.php (4)
12-16: Email regex still permits invalid formats.The pattern allows addresses starting with dots, underscores, or hyphens (e.g.,
.user@domain), which isn't RFC-compliant. Tighten the regex to require alphanumeric starts and consider escaping matched content before HTML insertion.
23-27: Colon-stripping still corrupts valid URLs.Removing all colons breaks port numbers (
example.com:8080), query times, and IPv6 addresses. Consider defaulting tohttp://or rejecting non-standard schemes instead.
28-28: Unsafe concatenation still enables attribute injection.Direct interpolation of
$url,$rest, and$matches[4]without escaping allows attackers to break out ofhrefor inject script-bearing attributes.
39-49: URL auto-linking still lacks HTML escaping.Matched URL components are inserted directly into
hrefand link text. Special characters like"or<could break attributes or inject markup.src/Domain/Messaging/Service/Manager/TemplateImageManager.php (3)
159-159: Domain layer must not perform persistence operations.Line 159 calls
templateImageRepository->persist($templateImage). This is a persistence operation in the domain layer. Per coding guidelines, the domain method should return the created TemplateImage (or remain void) and let the Application layer handle persistence.As per coding guidelines, domain layer should not use
$repository->persist(...)for non-DynamicListAttr entities.
226-226: Domain layer must not perform persistence operations.Line 226 calls
templateImageRepository->remove($orgLogoImage). This is a persistence operation in the domain layer. Per coding guidelines, remove this side effect and handle removal in the Application layer.As per coding guidelines, domain layer should not use
$repository->remove(...)for non-DynamicListAttr entities.
229-236: Output buffer handling remains fragile and leaky.The buffer capture sequence (line 230) captures content before
ob_start()is called (line 232), then echoes the old buffer after capturing image data (line 236). This can corrupt output and cause unexpected side effects.🔎 Recommended fix using ob_get_clean()
if ($orgLogoImage !== null) { $this->templateImageRepository->remove($orgLogoImage); } - //# rather convoluted way to get the image contents - $buffer = ob_get_contents(); - ob_end_clean(); ob_start(); imagepng($resized); - $imageContent = ob_get_contents(); - ob_end_clean(); - echo $buffer; + $imageContent = ob_get_clean(); } return $imageContent;src/Domain/Common/ExternalImageService.php (5)
64-70: Missing null check and error marker issues.Two issues already flagged in previous reviews remain unaddressed:
- Line 68:
strlen()is called on$cacheFileContentwithout checking if it's null first (after both download methods could return null).- Line 69: Writing the literal string
'MAX_SIZE'into the cache file creates a corrupted cache entry that will be served as valid image data.
108-115: Path traversal vulnerability in filename sanitization.The regex-based sanitization is insufficient to prevent path traversal attacks. An attacker could craft URLs that, after sanitization, still contain relative path components. This issue was previously flagged.
128-128: SSL peer verification disabled.Setting
CURLOPT_SSL_VERIFYPEERtofalsedisables certificate validation, making downloads vulnerable to MITM attacks. This security risk was flagged in a previous review.
138-143: Error markers still written to cache.Lines 139 and 142 write error strings (
'CURL_ERROR_X'and'HTTP_CODE_X') into$cacheFileContent, which then gets written to the cache file and could be served as image data. This was flagged previously (marked as addressed in commit 4a9e895, but the code still shows the issue).
149-165: Error marker written to cache on failure.Line 161 writes the literal string
'FGC_ERROR'instead of returning null or throwing an exception. This corrupts the cache file. This issue was flagged in a previous review.src/Domain/Common/RemotePageFetcher.php (3)
48-59: Caching logic has a potential issue with redundant query.The second call to
findByUrlAndLastModified($url, $lastModified)at line 52 with$lastModified = time()is querying for cache entries with that exact timestamp - which you just set. This will almost never return a result since cache entries use previous timestamps when created. The logic flow seems confusing - the first check (lines 42-46) handles fresh cache, but this second check appears redundant.Consider if this second DB lookup is necessary, or if the fresh-cache check at lines 42-46 is sufficient.
65-70: Domain layer should not call EntityManager::remove() directly.Line 67 calls
$this->entityManager->remove($cache)which violates domain purity per coding guidelines. Thesrc/Domain/**path should not perform persistence side effects. Consider moving cache purge logic toUrlCacheRepositoryor the Infrastructure layer.🔎 Suggested refactor
- $caches = $this->urlCacheRepository->getByUrl($url); - foreach ($caches as $cache) { - $this->entityManager->remove($cache); - } + $this->urlCacheRepository->deleteByUrl($url); $urlCache = (new UrlCache())->setUrl($url)->setContent($content)->setLastModified($lastModified); $this->urlCacheRepository->persist($urlCache);Based on coding guidelines, Domain services should not perform persistence operations.
116-121: Encoding conversion applied on each iteration corrupts URL.Line 119 converts
$urlfrom UTF-8 to ISO-8859-1 on every loop iteration without converting back. After multiple substitutions, the URL will be progressively corrupted.🔎 Suggested fix – remove unnecessary conversion
foreach ($userData as $key => $val) { if ($key !== 'password') { $url = str_replace('[' . $key . ']', rawurlencode($val), $url); - $url = mb_convert_encoding($url, 'ISO-8859-1', 'UTF-8'); } }src/Domain/Messaging/Service/Builder/EmailBuilder.php (4)
129-143: Dev mode with no devEmail leaves destinationEmail empty.If
devVersionis true butdevEmailis null, line 131 initializes$destinationEmail = ''and line 136 is skipped. The email will be sent to an empty address, which will likely fail or behave unexpectedly.🔎 Suggested fix
private function resolveDestinationEmailAndMessage(?string $to, ?string $message): array { - $destinationEmail = ''; - if ($this->devVersion) { $message = 'To: ' . $to . PHP_EOL . $message; - if ($this->devEmail) { - $destinationEmail = $this->devEmail; - } + $destinationEmail = $this->devEmail ?? $to; } else { $destinationEmail = $to; } return [$destinationEmail, $message]; }
196-206: Embedded images not attached to email; redundant text assignments.Line 199 invokes
templateImageEmbedderbut discards the return value (which contains CID-rewritten HTML). The email still uses the original$htmlMessagewithout embedded image references. Also,$email->text($textMessage)is called three times (lines 198, 203, 205) - only the last call has any effect.🔎 Proposed fix
private function applyContentAndFormatting(Email $email, $htmlMessage, $textMessage, int $messageId): void { $newWrap = $this->configProvider->getValue(ConfigOption::WordWrap); if ($newWrap) { $textMessage = wordwrap($textMessage, (int)$newWrap); } if (!empty($htmlMessage)) { - $email->html($htmlMessage); - $email->text($textMessage); - ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); - //# In the above phpMailer strips all tags, which removes the links - // which are wrapped in < and > by HTML2text - //# so add it again - $email->text($textMessage); + $embeddedHtml = ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); + $email->html($embeddedHtml); } - $email->text($textMessage); + $email->text($textMessage); }
104-127: Subscriber marked blacklisted but change not persisted.Line 115 calls
$subscriber->setBlacklisted(true)but there's noflush()orpersist(). The change won't be saved to the database. Per domain purity guidelines, persistence should be handled by the calling layer - but this needs to be documented or signaled to callers.
177-179: Unreachable "X-Originally to" header logic.Line 177 checks
$this->devEmail && $destinationEmail !== $this->devEmail, butcreateBaseEmailreceives$destinationEmailwhich was already set to$this->devEmailinresolveDestinationEmailAndMessagewhen dev mode is active. This condition is always false when it matters, so the header is never added.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
209-213: Null footer may cause TypeError in personalize.
MessagePrecacheDto::footeris?string, butUserPersonalizer::personalize()expects a non-nullablestringparameter. Iffooteris null, this will throw a TypeError.🔎 Suggested fix
- $processed->footer = $this->userPersonalizer->personalize($processed->footer, $subscriber->getEmail()); + $processed->footer = $this->userPersonalizer->personalize($processed->footer ?? '', $subscriber->getEmail());
🧹 Nitpick comments (8)
src/Domain/Common/TextParser.php (2)
58-61: Consider escaping$replacementin the regex replacement.Line 60 uses
$replacementdirectly inpreg_replace. If$replacementcontains$or\, it could be interpreted as backreferences. Usepreg_quoteor simpler string replacement logic.🔎 Safer approach using str_replace
- for ($j = 0; $j < $index; ++$j) { - $replacement = $link[$j]; - $text = preg_replace('/\%\%' . $j . '\%\%/', $replacement, $text); - } + for ($j = 0; $j < $index; ++$j) { + $text = str_replace('%%' . $j . '%%', $link[$j], $text); + }
67-83: Placeholder approach for special characters is functional but fragile.Using HTML comment placeholders like
<!--LB-->works but could collide if input already contains those strings. Consider a more robust escaping strategy or use a unique prefix less likely to appear in user content.src/Domain/Messaging/Service/Manager/TemplateImageManager.php (1)
195-197: Consider stronger type hints for helper parameters.Parameters
$newWidth,$newHeight, and$sizeFactorusemixedtype hints, weakening type safety. Based on the usage incalculateDimensions(lines 186-187), these should beintfor width/height andfloatfor sizeFactor.🔎 Proposed fix
private function resizeImageIfNecessary( string $imageContent, array $imgSize, - mixed $newWidth, - mixed $newHeight, - mixed $sizeFactor, + int $newWidth, + int $newHeight, + float $sizeFactor, ?TemplateImage $orgLogoImage ): string {src/Domain/Common/ExternalImageService.php (4)
43-43: Add type hints to parameters.The
$filenameand$messageIdparameters lack type declarations, which reduces type safety and IDE support.🔎 Proposed fix
- public function cache($filename, $messageId): bool + public function cache(string $filename, int $messageId): bool
167-167: Add type hint to parameter.The
$filenameparameter lacks a type declaration.🔎 Proposed fix
- private function isCacheableUrl($filename): bool + private function isCacheableUrl(string $filename): bool
203-203: Add type hint to parameter.The
$contentparameter lacks a type declaration.🔎 Proposed fix
- private function writeCacheFile(string $cacheFileName, $content): void + private function writeCacheFile(string $cacheFileName, string $content): void
93-93: TODO comment: Verify hidden file handling.The comment indicates uncertainty about skipping files starting with
.. This logic typically excludes hidden files and directory entries (.and..), which is correct for cache cleanup.Do you want me to confirm this logic is appropriate, or would you like me to open an issue to track this verification?
tests/Unit/Domain/Common/RemotePageFetcherTest.php (1)
109-113: Test expectation may be fragile due to implementation coupling.The test expects
findByUrlAndLastModifiedto be calledatLeast(2)times with alogicalOrmatcher. This couples the test tightly to the current implementation's redundant cache lookup behavior (which was flagged as potentially problematic in the main file). If the implementation is simplified to remove the redundant query, this test will break.Consider testing the observable behavior (content is fetched and cached) rather than the exact number of repository calls.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
config/PhpCodeSniffer/ruleset.xmlconfig/parameters.yml.distresources/translations/messages.en.xlfsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phptests/Unit/Domain/Common/RemotePageFetcherTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Domain/Messaging/Service/RateLimitedCampaignMailer.php
- config/parameters.yml.dist
- resources/translations/messages.en.xlf
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Common/TextParser.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (3)
src/Domain/Common/ExternalImageService.php (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (7)
src/Domain/Messaging/Exception/MessageCacheMissingException.php (1)
MessageCacheMissingException(9-15)src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
EmailBuilder(20-207)buildPhplistEmail(39-76)src/Domain/Messaging/Service/MessageDataLoader.php (1)
MessageDataLoader(14-235)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (3)
RateLimitedCampaignMailer(14-56)composeEmail(24-45)send(50-55)src/Domain/Messaging/Service/MessagePrecacheService.php (1)
precacheMessage(48-102)src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
processMessageLinks(61-89)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/TemplateImageRepository.php (3)
TemplateImageRepository(12-46)findByFilename(16-24)findById(26-34)
🪛 GitHub Actions: phpList Core Build
src/Domain/Messaging/Service/MessageDataLoader.php
[error] 20-20: PhpList\Core\Domain\Messaging\Service\MessageDataLoader::__construct(): Argument #4 ($logger) not passed
🔇 Additional comments (15)
src/Domain/Common/TextParser.php (1)
18-18: Good fix –$linkis now initialized.This resolves the undefined variable issue when no anchors are present.
config/PhpCodeSniffer/ruleset.xml (1)
106-110: The configuration is correct. TheignoreNewlinesproperty is a valid, documented option forSquiz.WhiteSpace.OperatorSpacingthat allows newlines before and after operators when set totrue.src/Domain/Common/RemotePageFetcher.php (1)
81-93: LGTM on HTTP client configuration.The HTTP request options (
max_redirectsandtimeout) now use correct Symfony HttpClient syntax. Past issues with invalid options have been addressed.tests/Unit/Domain/Common/RemotePageFetcherTest.php (2)
56-74: LGTM - PSR cache hit test.Clean test that verifies the in-memory cache path works correctly and short-circuits repository/HTTP calls.
150-168: LGTM - HTTP failure handling test.Good coverage ensuring that on HTTP failure, no cache is set and empty string is returned.
src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
78-102: LGTM - Input validation.Good validation of recipient and subject, including null checks and newline detection for header injection prevention.
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
7-31: LGTM - DTO structure is clean.All properties now have sensible defaults, preventing uninitialized property errors. The mix of nullable and non-nullable types with defaults is appropriate for a DTO that gets progressively populated.
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (4)
360-364: LGTM - Cache miss handling.Good null check for the cached DTO with proper exception throwing when cache is missing.
291-331: LGTM - Admin notification handling.Clean implementation with proper null check for
buildPhplistEmailresult, correct Symfony translator placeholder syntax, and graceful handling of duplicate constraint violations when persisting the notification flag.
166-173: LGTM - Status update with SendStart timestamp.Good addition of SendStart timestamp when transitioning to InProcess status. Persistence in MessageHandler is allowed per coding guidelines.
334-368: LGTM - Subscriber processing loop.Well-structured loop with time limiting, existing message check, email validation, and proper cache retrieval with null handling.
src/Domain/Messaging/Service/MessageDataLoader.php (4)
114-136: LGTM - Secure deserialization with proper error handling.Good implementation with
allowed_classes: falseto prevent object injection,is_array()validation, and graceful handling of corrupted data with logging.
48-59: LGTM - Date array helper.Clean implementation with null-coalescing to avoid PHP 8.1+ deprecation warnings when passing null to
date().
170-209: LGTM - From field normalization.Comprehensive parsing of the from field with proper fallbacks, sanitization of email/name components, and handling of edge cases like empty names and double spaces.
61-100: LGTM - Default message data structure.Well-organized default values with config provider integration for template, footer, and notification settings.
| $image->setData(null); | ||
|
|
||
| $this->entityManager->persist($image); | ||
| $this->templateImageRepository->persist($image); |
There was a problem hiding this comment.
Domain layer must not perform persistence operations.
Line 45 calls templateImageRepository->persist($image). Per the coding guidelines, domain services should not invoke persistence APIs on regular entities. Move this side effect to an Application layer service or MessageHandler. The domain method can return the array of TemplateImage entities to be persisted by the caller.
As per coding guidelines, domain layer should not use $repository->persist(...) for non-DynamicListAttr entities.
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Manager/TemplateImageManager.php around line 45,
remove the direct call to $this->templateImageRepository->persist($image)
because the domain layer must not perform persistence; instead change the domain
method to collect and return the TemplateImage entity (or an array of entities)
to the caller and delete the side-effecting persist call; then update the
Application layer service or MessageHandler that invokes this domain method to
receive those TemplateImage entities and perform persistence there via the
repository (keeping persist only for DynamicListAttr where allowed).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
src/Domain/Common/ExternalImageService.php (5)
64-70: Null check needed before strlen, and avoid writing error markers to cache.Line 68 calls
strlen($cacheFileContent)without checking if download returned null, which will cause a TypeError. Additionally, writing'MAX_SIZE'to the cache (line 69) creates a corrupt cache file that will pass validation and be returned as "image content" later.🔎 Proposed fix
if ($cacheFileContent === null) { $cacheFileContent = $this->downloadUsingFileGetContent($filename); } + if ($cacheFileContent === null) { + $this->logger->warning('Failed to download external image', ['url' => $filename]); + return false; + } + if ($this->externalImageMaxSize && (strlen($cacheFileContent) > $this->externalImageMaxSize)) { - $cacheFileContent = 'MAX_SIZE'; + $this->logger->warning('External image exceeds max size', [ + 'url' => $filename, + 'size' => strlen($cacheFileContent), + 'max' => $this->externalImageMaxSize, + ]); + return false; } $this->writeCacheFile($cacheFileName, $cacheFileContent);
108-115: Strengthen filename sanitization against path traversal.The current regex pattern doesn't fully prevent path traversal attacks. For example,
../components separated by slashes might not be fully neutralized. Usingbasename()on the URL path would be more robust.🔎 Proposed fix
private function generateLocalFileName(string $filename, int $messageId): string { - return $this->externalCacheDir - . '/' - . $messageId - . '_' - . preg_replace([ '~[\.][\.]+~Ui', '~[^\w\.]~Ui',], ['', '_'], $filename); + $path = parse_url($filename, PHP_URL_PATH) ?: $filename; + $sanitized = preg_replace('~[^\w\.-]~', '_', basename($path)); + return $this->externalCacheDir . '/' . $messageId . '_' . $sanitized; }
128-128: SSL peer verification is disabled, creating a security risk.
CURLOPT_SSL_VERIFYPEER => falsedisables certificate validation, making connections vulnerable to MITM attacks. Consider enabling verification by default or making it configurable with a security warning.
138-143: Error marker strings are still written to cache.Lines 139 and 142 assign error strings like
'CURL_ERROR_7'to$cacheFileContent, which will be written to the cache file and potentially served as image data. Past review indicated this was addressed, but the code still contains these assignments. Returnnullinstead and skip caching on error.🔎 Proposed fix
if ($cURLErrNo != 0) { - $cacheFileContent = 'CURL_ERROR_' . $cURLErrNo; + $this->logger->error('cURL error downloading image', [ + 'url' => $filename, + 'errno' => $cURLErrNo, + ]); + return null; } if ($cURLInfo['http_code'] >= 400) { - $cacheFileContent = 'HTTP_CODE_' . $cURLInfo['http_code']; + $this->logger->error('HTTP error downloading image', [ + 'url' => $filename, + 'http_code' => $cURLInfo['http_code'], + ]); + return null; }
149-165: Return null instead of error marker string, and fix return type.Line 161 assigns
'FGC_ERROR'which will corrupt the cache. Additionally, the return type isstring(line 149) but should be?stringto matchdownloadUsingCurland allow returning null on failure.🔎 Proposed fix
- private function downloadUsingFileGetContent(string $filename): string + private function downloadUsingFileGetContent(string $filename): ?string { $remoteURLContext = stream_context_create([ 'http' => [ 'method' => 'GET', 'timeout' => $this->externalImageTimeout, 'max_redirects' => '10', ] ]); $cacheFileContent = file_get_contents($filename, false, $remoteURLContext); if ($cacheFileContent === false) { - $cacheFileContent = 'FGC_ERROR'; + $this->logger->error('file_get_contents error downloading image', ['url' => $filename]); + return null; } return $cacheFileContent; }src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
177-179: X-Originally to header logic is unreachable.The condition
$destinationEmail !== $this->devEmailis always false in dev mode because line 139 sets$destinationEmail = $this->devEmail. The original$toparameter should be captured before the dev override and used in the header check instead.🔎 Proposed fix
In
buildPhplistEmail, capture the original recipient before callingresolveDestinationEmailAndMessage:+ $originalTo = $to; [$destinationEmail, $message] = $this->resolveDestinationEmailAndMessage($to, $message);Then in
createBaseEmail, update the header check:- if ($this->devEmail && $destinationEmail !== $this->devEmail) { - $email->getHeaders()->addTextHeader('X-Originally to', $destinationEmail); + if ($this->devVersion && $this->devEmail && $originalTo !== $destinationEmail) { + $email->getHeaders()->addTextHeader('X-Originally to', $originalTo); }
🧹 Nitpick comments (4)
src/Domain/Common/RemotePageFetcher.php (1)
81-93: Consider logging exceptions before returning empty string.The catch block silently swallows all exceptions (line 90-91), which makes debugging fetch failures difficult. Consider logging the exception at debug level before returning the empty string.
🔎 Suggested enhancement
private function fetchUrlDirect(string $url): string { try { $response = $this->httpClient->request('GET', $url, [ 'max_redirects' => 5, 'timeout' => 10, ]); return $response->getContent(false); } catch (Throwable $e) { + // Log for debugging: $this->logger->debug('Failed to fetch URL: ' . $url, ['exception' => $e]); return ''; } }tests/Unit/Domain/Common/TextParserTest.php (1)
10-69: Consider adding edge case tests.The current tests cover the happy paths well, but consider adding tests for:
- Existing HTML anchors in input - The implementation mentions handling existing anchors with placeholders. Testing this would prevent double-wrapping bugs.
- Mixed content - Text containing both emails and URLs together would test real-world usage.
- Invalid URL schemes - The implementation removes the first colon for non-http/https/ftp/mailto schemes; worth verifying this behavior.
- Multiple URLs/emails - Test that multiple URLs or emails in the same string are all properly converted.
These would strengthen confidence in the parser's robustness.
tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php (1)
74-80: Consider using a proper mock instead of an anonymous class.The anonymous class works but is a bit unconventional for unit tests. A mock object would be more idiomatic and provide better type safety and IDE support.
🔎 Alternative approach
$listMessage = $this->createMock(\PhpList\Core\Domain\Messaging\Model\ListMessage::class); $listMessage->method('getListId')->willReturn(42); $message->method('getListMessages')->willReturn( new ArrayCollection([$listMessage]) );src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
196-206: Text is set redundantly three times.Lines 198, 203, and 205 all set the text body. The comment on lines 200-202 explains line 203, but line 205 appears unnecessary. Consider consolidating to avoid confusion.
🔎 Suggested simplification
if (!empty($htmlMessage)) { $email->html($htmlMessage); - $email->text($textMessage); ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); //# In the above phpMailer strips all tags, which removes the links // which are wrapped in < and > by HTML2text //# so add it again $email->text($textMessage); } - $email->text($textMessage); + if (empty($htmlMessage)) { + $email->text($textMessage); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/Exception/DevEmailNotConfiguredException.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phptests/Unit/Domain/Common/TextParserTest.phptests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Domain/Common/TextParser.php
- src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php
🧰 Additional context used
📓 Path-based instructions (1)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Common/RemotePageFetcher.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Exception/DevEmailNotConfiguredException.phpsrc/Domain/Common/ExternalImageService.php
🧬 Code graph analysis (3)
tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php (4)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/MessageDataRepository.php (1)
MessageDataRepository(12-26)src/Domain/Messaging/Repository/MessageRepository.php (1)
MessageRepository(16-119)src/Domain/Messaging/Service/MessageDataLoader.php (1)
MessageDataLoader(14-235)
src/Domain/Common/RemotePageFetcher.php (5)
src/Domain/Configuration/Repository/UrlCacheRepository.php (3)
UrlCacheRepository(12-32)findByUrlAndLastModified(16-25)getByUrl(28-31)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Common/HtmlUrlRewriter.php (2)
HtmlUrlRewriter(11-208)addAbsoluteResources(13-73)src/Domain/Common/TextParser.php (1)
__invoke(9-94)src/Domain/Configuration/Model/UrlCache.php (2)
getLastModified(48-51)setLastModified(69-73)
tests/Unit/Domain/Common/TextParserTest.php (1)
src/Domain/Common/TextParser.php (1)
TextParser(7-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (8)
src/Domain/Common/RemotePageFetcher.php (1)
121-121: Question: Is ISO-8859-1 encoding necessary here?Line 121 converts the prepared URL to ISO-8859-1 after all substitutions. Modern HTTP and URL standards typically use UTF-8. This conversion could cause issues with URLs containing characters outside the ISO-8859-1 range.
Is this required for legacy compatibility? If not, consider removing the conversion to keep URLs in UTF-8.
tests/Unit/Domain/Common/TextParserTest.php (6)
14-17: Clean setup.The setUp method correctly initializes the parser with no unnecessary complexity.
19-28: Solid email linking test.The test correctly verifies that email addresses are converted to mailto links with proper URL encoding in the href and escaped HTML in the display text. Good use of a complex email address to test the edge cases.
30-40: Nice period handling verification.The test properly verifies that trailing periods are moved outside the anchor tag and that non-www URLs omit the scheme in the display text. The inline comment adds helpful clarity.
42-51: www prefix handling looks good.The test correctly verifies that www-prefixed URLs get http:// added to the href while preserving www in the display text.
62-68: Special character preservation verified.Simple and effective test that dollar signs and parentheses pass through unchanged.
53-60: No changes needed—the expected output is correct.The
<br />and literal newline are preserved together by design. Looking at the TextParser implementation (line 94), it explicitly replaces newlines with<br />\n, keeping both the HTML break tag and the character. The test correctly reflects this intentional behavior.src/Domain/Messaging/Exception/DevEmailNotConfiguredException.php (1)
9-15: LGTM!Clean exception implementation with a clear, actionable error message for the dev-mode email routing scenario.
| $lastModified = time(); | ||
| $cacheUrl = $this->urlCacheRepository->findByUrlAndLastModified($url, $lastModified); | ||
| $content = $cacheUrl?->getContent(); | ||
| if ($cacheUrl) { | ||
| // todo: check what the page should be for this log | ||
| $this->eventLogManager->log(page: 'unknown page', entry: $url . ' was cached in database'); |
There was a problem hiding this comment.
Caching logic flaw: querying for future-dated entries defeats the cache.
Lines 51-52 set $lastModified = time() then call findByUrlAndLastModified($url, $lastModified). Looking at the repository implementation (where clause u.lastModified > :lastModified), this queries for cache entries with lastModified > time() — essentially future-dated entries that will never exist, so the cache lookup will always fail.
Rethink this logic: either remove the second query (lines 51-56) if the first check (lines 42-46) already handles freshness, or fix the repository query to properly compare cache staleness.
| $caches = $this->urlCacheRepository->getByUrl($url); | ||
| foreach ($caches as $cache) { | ||
| $this->entityManager->remove($cache); | ||
| } | ||
| $urlCache = (new UrlCache())->setUrl($url)->setContent($content)->setLastModified($lastModified); | ||
| $this->urlCacheRepository->persist($urlCache); |
There was a problem hiding this comment.
Domain purity breach: direct EntityManager usage in Domain layer.
Line 67 calls $this->entityManager->remove($cache) directly, which introduces persistence side effects in src/Domain/**. Per the coding guidelines, domain services should not perform Doctrine persistence operations (this isn't DynamicListAttr-related code).
Move this cache purging logic to the Infrastructure or Application layer. Consider returning a value object or emitting a domain event that a MessageHandler can consume to perform the actual persistence operations.
Based on coding guidelines, Domain services should not perform persistence operations.
| $subscriber->setBlacklisted(true); | ||
|
|
||
| $this->subscriberHistoryManager->addHistory( | ||
| subscriber: $subscriber, | ||
| message: 'Marked Blacklisted', | ||
| details: 'Found user in blacklist while trying to send an email, marked black listed', | ||
| ); |
There was a problem hiding this comment.
Subscriber state change may not be persisted.
Line 116 marks the subscriber as blacklisted via setBlacklisted(true), but there's no persistence call. If this change is intended to be saved, the calling layer (e.g., message handler) should handle the flush. Consider returning a signal or domain event so the caller knows to persist, or document that persistence is the caller's responsibility.
Based on coding guidelines for domain purity.
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Builder/EmailBuilder.php around lines 116 to
122, the code sets $subscriber->setBlacklisted(true) but does not persist the
change or signal to the caller that persistence is required; update the method
to either (a) persist the change here via the repository/manager (call the
appropriate save/flush on the subscriber repository or entity manager), or (b)
emit/return a domain event or boolean flag indicating the subscriber was
modified so the caller (message handler) can flush; choose one approach
consistent with domain purity (prefer returning a domain event or flag if
persistence belongs to the caller) and document the chosen contract in the
method PHPDoc.
7ebc63c to
ca6dc94
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
src/Domain/Common/TextParser.php (3)
33-37: Still breaking legitimate URLs with colon removal.Line 36 strips every colon from URLs, which corrupts ports (
example.com:8080), query params with times (?time=12:30), and IPv6 addresses. The comment acknowledges this but the 🤷 doesn't make it safe.Instead of removing colons, either reject non-whitelisted schemes or default to
http://prefix.🔎 Safer approach
if (!preg_match('/^(http:)|(mailto:)|(ftp:)|(https:)/i', $url)) { - // Removing all colons breaks legitimate URLs but avoids this 🤷 - //<a href="javascript:window.open('http://hacker.com?cookie='+document.cookie)"> - $url = preg_replace('/:/', '', $url); + // Default to http:// for protocol-less URLs + $url = 'http://' . $url; }
38-38: Attribute injection vulnerability via unsafe concatenation.Directly concatenating
$url,$rest, and$matches[4]without escaping allows attackers to inject attributes or break out of the href. A crafted anchor likehref="\" onclick=\"alert(1)\""would execute JavaScript.Escape each piece before building the tag:
htmlspecialchars($url, ENT_QUOTES, 'UTF-8')for the href, and the same for link text. The$restattributes need to be parsed and whitelisted (or dropped entirely) since raw attribute strings are unsafe.
49-59: URL auto-linking needs HTML escaping.Lines 51 and 57 inject raw matched URL groups directly into
hrefattributes and link text. If a URL contains special characters like"or<, it could break out of the attribute or inject markup.Consider switching from
preg_replacetopreg_replace_callback(like you did for email) so you can applyhtmlspecialchars()to each matched component before building the anchor tag.src/Domain/Common/RemotePageFetcher.php (3)
51-56: Caching logic still queries for future-dated entries.Line 51 sets
$lastModified = time()then passes it tofindByUrlAndLastModified, which (per the repository'sWHERE u.lastModified > :lastModifiedclause) looks for cache entries newer than "now." Since cache entries are created with current/past timestamps, this will never match, causing unnecessary cache misses.
67-67: Domain purity breach: direct EntityManager usage.Line 67 calls
$this->entityManager->remove($cache)directly, introducing persistence side effects in the Domain layer. Per coding guidelines, Doctrine persistence APIs should not be used here (this isn't DynamicListAttr-related).Move cache purging to Infrastructure/Application layer or use the repository abstraction to handle removals.
Based on coding guidelines, Domain services should not perform persistence operations.
121-121: Remove the unnecessary ISO-8859-1 encoding conversion.Line 121 converts the URL to ISO-8859-1 after it's been properly encoded with
rawurlencode, which contradicts modern HTTP standards (UTF-8 expected). There's no documentation or system requirement for this encoding in the codebase, and it appears to be a leftover that could corrupt international URLs. Strip it out.foreach ($userData as $key => $val) { if ($key !== 'password') { $url = str_replace('[' . $key . ']', rawurlencode($val), $url); } } - $url = mb_convert_encoding($url, 'ISO-8859-1', 'UTF-8'); return $this->expandUrl($url);src/Domain/Messaging/Service/Builder/EmailBuilder.php (3)
116-122: Subscriber marked blacklisted but not persisted.This concern was previously raised. However, per domain purity guidelines, expressing intent via
setBlacklisted(true)without handling persistence here is acceptable—the calling layer (message handler) should flush. If you need explicit signaling, consider returning a domain event or flag so the caller knows to persist.
177-179: Unreachable "X-Originally to" header logic.This issue was previously raised. The condition is always false because
resolveDestinationEmailAndMessage(line 139) sets$destinationEmail = $this->devEmail, so$destinationEmail !== $this->devEmailnever evaluates to true. Preserve the original$toand compare against that instead.
188-206: Image embedder return value discarded; email will have broken embedded images.Despite a past comment indicating this was addressed, line 199 still discards the embedder's return value. The embedder modifies HTML to use CID references for embedded images, but that modified HTML is never captured or used. Line 197 sets the original
$htmlMessage(without CIDs), so recipients will see broken image links.Additionally, text is redundantly set three times (lines 198, 203, 205)—only the final call at line 205 is needed after word-wrapping.
🔎 Proposed fix
private function applyContentAndFormatting(Email $email, $htmlMessage, $textMessage, int $messageId): void { $newWrap = $this->configProvider->getValue(ConfigOption::WordWrap); if ($newWrap) { $textMessage = wordwrap($textMessage, (int)$newWrap); } if (!empty($htmlMessage)) { + $htmlMessage = ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); $email->html($htmlMessage); - $email->text($textMessage); - ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); - //# In the above phpMailer strips all tags, which removes the links - // which are wrapped in < and > by HTML2text - //# so add it again - $email->text($textMessage); } $email->text($textMessage); }
🧹 Nitpick comments (4)
tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php (1)
69-81: Consider using a proper mock instead of an anonymous class.The anonymous class works fine for returning a listId, but using
$this->createMock()with a stub class or interface would be more maintainable and provide better IDE support. That said, this approach is functional for a simple test case.Alternative approach using a mock
$listMessage = $this->createMock(ListMessage::class); $listMessage->method('getListId')->willReturn(42); $message->method('getListMessages')->willReturn( new ArrayCollection([$listMessage]) );src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
26-27: Consider removing commented-out charset properties.If these charset properties aren't needed, removing the commented lines would keep the code cleaner. If they're planned for future use, a TODO comment would clarify the intent.
src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
58-59: Remove or implement commented code.Dead code adds clutter. If reply-to functionality is planned, create a TODO or issue tracker item; otherwise remove these lines.
147-147: Consider typing$destinationEmailasstringinstead ofmixed.All usages (lines 156, 173, 181) treat it as a string. Explicit typing improves clarity and catches errors earlier.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Domain/Common/ExternalImageService.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/Exception/DevEmailNotConfiguredException.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phptests/Unit/Domain/Common/TextParserTest.phptests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Domain/Common/ExternalImageService.php
- tests/Unit/Domain/Common/TextParserTest.php
🧰 Additional context used
📓 Path-based instructions (1)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/Exception/DevEmailNotConfiguredException.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Common/RemotePageFetcher.php
🧬 Code graph analysis (3)
src/Domain/Messaging/Exception/DevEmailNotConfiguredException.php (1)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
__construct(23-38)
src/Domain/Common/TextParser.php (1)
src/Domain/Common/Html2Text.php (1)
__invoke(18-84)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (6)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Exception/DevEmailNotConfiguredException.php (2)
DevEmailNotConfiguredException(9-15)__construct(11-14)src/Domain/Messaging/Service/SystemMailConstructor.php (2)
SystemMailConstructor(13-103)__construct(17-25)src/Domain/Messaging/Service/TemplateImageEmbedder.php (2)
TemplateImageEmbedder(16-307)__construct(33-44)src/Domain/Subscription/Repository/SubscriberRepository.php (1)
SubscriberRepository(21-208)src/Domain/Subscription/Repository/UserBlacklistRepository.php (4)
UserBlacklistRepository(14-60)__construct(16-22)isEmailBlacklisted(45-59)findOneByEmail(38-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (7)
src/Domain/Common/TextParser.php (1)
11-26: Email conversion looks solid.The regex properly validates email structure and escaping is correctly applied:
htmlspecialcharsfor the display text andrawurlencodefor the mailto URL. Nice work addressing the previous email regex concerns.tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php (2)
1-30: Solid test setup.The mock declarations and setUp method follow standard PHPUnit best practices. The use of intersection types for mocks is clean.
91-139: Nice comprehensive test with proper time drift handling.The assertions thoroughly cover the expected transformations and the time-based validation with a ±120 second tolerance is well-designed to handle test execution variance.
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (3)
1-9: Clean DTO structure.The namespace placement is appropriate and suppressing the TooManyFields warning is reasonable for a DTO that aggregates email precache data.
10-25: Property initialization looks solid.All typed properties now have appropriate defaults. The previous issues with uninitialized properties have been resolved—non-nullable properties have concrete defaults and nullable properties default to null.
28-30: Final properties initialized correctly.The boolean, nullable string, and array properties all have appropriate defaults.
src/Domain/Messaging/Exception/DevEmailNotConfiguredException.php (1)
1-15: LGTM!Clean exception class following standard patterns. The message clearly indicates the specific configuration problem (dev mode requires dev email), making it easy to diagnose and fix.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.
Thanks for contributing to phpList!