Refactor/update message sendin #374
Conversation
📝 WalkthroughWalkthroughReplaces inline message content sending with a precache DTO pipeline: MessageDataLoader normalizes message data, MessagePrecacheService builds/caches MessagePrecacheDto (templates, images, remote content, admin attrs), preparator/link-tracking consume the DTO, handler personalizes per-subscriber and sends via RateLimitedCampaignMailer/EmailBuilder and MailerInterface. Changes
Sequence Diagram(s)mermaid Handler->>Loader: load message data (Message) Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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.
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (7)
src/Domain/Messaging/Repository/TemplateRepository.php (1)
16-19: Consider using inheritedfind()method.
AbstractRepositorylikely already provides afind($id)method (inherited from Doctrine'sEntityRepository) that does exactly this.If your goal is to add clarity, this wrapper is fine. Otherwise, callers could use
$repository->find($id)directly.src/Domain/Common/TextParser.php (1)
22-26: Protocol stripping is a partial XSS mitigation.The logic strips colons from non-standard protocols but still allows
javascriptURLs through (e.g.,javascript window.open(...)). Consider using an allowlist approach or a more robust sanitization library for URL schemes.src/Domain/Common/HtmlToText.php (2)
61-66: Regex for<a>tags afterstrip_tags()is likely dead code.
strip_tags($text)at line 38 removes all HTML tags, so this regex pattern matching<a href=...>will never find matches. If this is intentional for edge cases, a comment would help clarify.
74-80: Consider limiting iterations on whitespace reduction loops.These
whileloops could theoretically run many times on pathological input. For robustness, consider a reasonable iteration cap or usingpreg_replacewith a quantifier pattern like/ {2,}/to collapse in one pass.// reduce whitespace - while (preg_match('/ /', $text)) { - $text = preg_replace('/ /', ' ', $text); - } - while (preg_match("/\n\s*\n\s*\n/", $text)) { - $text = preg_replace("/\n\s*\n\s*\n/", "\n\n", $text); - } + $text = preg_replace('/ {2,}/', ' ', $text); + $text = preg_replace("/(\n\s*){3,}/", "\n\n", $text);src/Domain/Messaging/Service/MessageDataLoader.php (1)
101-101: Consider restrictingunserialize()withallowed_classes.Deserializing stored data without class restrictions can be a security risk if the data source is ever compromised. Pass
['allowed_classes' => false]or specify expected classes.- $unserialized = unserialize(substr($storedMessageDatum->getData(), 4)); + $unserialized = unserialize(substr($storedMessageDatum->getData(), 4), ['allowed_classes' => false]);src/Domain/Common/RemotePageFetcher.php (1)
31-43:utf8_encode/utf8_decodeare deprecated + URL substitution is risky (Line 31–Line 43, Line 56–Line 66, Line 69–Line 87).
utf8_encode()/utf8_decode()are deprecated in modern PHP; replace with safe string handling (or mbstring) and avoid double transcoding.- Consider validating/allowlisting schemes/hosts before fetching remote content (admin mistakes can still become SSRF-like incidents).
Suggested direction:- $url = utf8_encode(str_ireplace("[$key]", urlencode($val), utf8_decode($url))); + $url = str_ireplace("[$key]", rawurlencode((string) $val), $url);Also applies to: 56-66, 69-87
src/Domain/Common/HtmlUrlRewriter.php (1)
18-21: Tighten API surface + avoid leaking global libxml state (Line 18–Line 21, Line 122–Line 143).
libxml_use_internal_errors(true)changes global state; please restore the previous setting after parsing.normalizePath()should declare visibility (and ideallyprivate) + return type.Also applies to: 122-143
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
composer.json(1 hunks)config/parameters.yml.dist(1 hunks)src/Domain/Common/HtmlToText.php(1 hunks)src/Domain/Common/HtmlUrlRewriter.php(1 hunks)src/Domain/Common/RemotePageFetcher.php(1 hunks)src/Domain/Common/TextParser.php(1 hunks)src/Domain/Configuration/Model/ConfigOption.php(1 hunks)src/Domain/Configuration/Repository/UrlCacheRepository.php(1 hunks)src/Domain/Identity/Model/AdminAttributeDefinition.php(4 hunks)src/Domain/Identity/Model/Administrator.php(4 hunks)src/Domain/Identity/Repository/AdminAttributeDefinitionRepository.php(2 hunks)src/Domain/Identity/Repository/AdministratorRepository.php(1 hunks)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php(4 hunks)src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php(1 hunks)src/Domain/Messaging/Repository/MessageDataRepository.php(1 hunks)src/Domain/Messaging/Repository/TemplateImageRepository.php(1 hunks)src/Domain/Messaging/Repository/TemplateRepository.php(1 hunks)src/Domain/Messaging/Service/MessageDataLoader.php(1 hunks)src/Domain/Messaging/Service/MessagePrecacheService.php(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/Repository/MessageDataRepository.phpsrc/Domain/Identity/Repository/AdministratorRepository.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Configuration/Repository/UrlCacheRepository.phpsrc/Domain/Messaging/Repository/TemplateImageRepository.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Identity/Model/AdminAttributeDefinition.phpsrc/Domain/Common/HtmlToText.phpsrc/Domain/Identity/Repository/AdminAttributeDefinitionRepository.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Identity/Model/Administrator.phpsrc/Domain/Messaging/Repository/TemplateRepository.phpsrc/Domain/Configuration/Model/ConfigOption.phpsrc/Domain/Messaging/Service/MessagePrecacheService.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Common/HtmlUrlRewriter.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 (3)
src/Domain/Common/TextParser.php (1)
src/Domain/Common/HtmlToText.php (1)
__invoke(18-84)
src/Domain/Common/HtmlToText.php (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)
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)
🔇 Additional comments (15)
src/Domain/Identity/Model/AdminAttributeDefinition.php (1)
42-47: LGTM! Relationship setup is clean.The OneToMany relationship is properly configured with orphanRemoval, correctly initialized in the constructor, and provides appropriate read access via the getter.
Also applies to: 63-63, 136-140
src/Domain/Identity/Model/Administrator.php (1)
71-72: LGTM! Relationship properly configured.The OneToMany relationship to SubscriberList is correctly set up as the inverse side (mappedBy: 'owner'), properly initialized, and provides clean read access.
Also applies to: 79-79, 222-228
src/Domain/Messaging/Repository/TemplateImageRepository.php (1)
16-24: LGTM! Query methods are well-scoped.Both methods correctly filter by
ti.template = 0to isolate non-template images, which aligns with the stated purpose of finding images by filename or ID that aren't associated with specific templates.Also applies to: 26-34
src/Domain/Configuration/Repository/UrlCacheRepository.php (1)
16-25: LGTM! Cache query methods look solid.The
findByUrlAndLastModifiedmethod correctly uses a greater-than comparison to locate cached entries newer than the provided timestamp, andgetByUrlprovides straightforward retrieval by URL.Also applies to: 27-31
src/Domain/Identity/Repository/AdministratorRepository.php (1)
59-71: LGTM! Query logic looks sound.The join path Administrator → ownedLists → listMessages → message is clean and correctly mapped. The DISTINCT clause appropriately handles potential duplicates.
src/Domain/Messaging/Repository/MessageDataRepository.php (1)
22-25: The original review comment is incorrect. ThegetForMessage()method query is actually correct.The
MessageDataentity uses a composite primary key of(id, name), where theidfield holds the message ID. The methodgetForMessage(int $messageId)correctly queriesfindBy(['id' => $messageId])to retrieve allMessageDataentries for a given message. Sinceidis part of the composite key, querying on it alone returns all rows with that message ID, regardless of theirnamevalue.The suggestion to change the filter to
['message' => $messageId]would not work—no such property exists on the entity.Likely an incorrect or invalid review comment.
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
60-60: LGTM on dependency injection of UserPersonalizer.Clean addition of the
UserPersonalizerdependency to support per-subscriber personalization at the handler level.
79-83: Good defensive early exit pattern.Suspending the campaign and returning early when
getOrCacheBaseMessageContentreturns null is a solid safeguard against processing campaigns without valid content.src/Domain/Common/TextParser.php (2)
47-50: Potential regex injection via$jplaceholder.The
$jvariable is an integer, so it's safe here, but the pattern/\%\%$j\%\%/embeds a variable directly. This is fine for integers but worth noting for future maintainers if this pattern is copied elsewhere.
56-58: LGTM on placeholder handling.The bracket and dollar sign placeholder pattern is a reasonable workaround for regex limitations with special characters.
src/Domain/Identity/Repository/AdminAttributeDefinitionRepository.php (1)
22-36: LGTM ongetForAdminimplementation.Clean use of
COALESCEfor defaulting null values and proper left join filtering by administrator.src/Domain/Common/HtmlToText.php (1)
18-38: LGTM on HTML transformation pipeline.The placeholder-based approach for preserving links during
strip_tagsis a solid pattern for HTML-to-text conversion.src/Domain/Messaging/Service/MessageDataLoader.php (2)
28-86: LGTM on default message data initialization.Comprehensive defaults with config-driven values. The structure is clear and maintainable.
141-178: LGTM on fromfield parsing logic.Thorough handling of various email/name formats with good edge case coverage for sanitization.
src/Domain/Common/HtmlUrlRewriter.php (1)
13-73: Solid approach: DOM + CSS rewriting covers the important cases (Line 13–Line 73).
The wrapping/unwrapping pattern is a practical way to handle fragments, and rewritingsrcset+ CSSurl(...)/@import is a nice completeness touch.
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
5fe45ab to
492e1d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Domain/Analytics/Service/LinkTrackService.php (1)
41-54: Update tests to passMessagePrecacheDtoinstead ofMessageContent.The method signature change is breaking: all six test cases in
LinkTrackServiceTest.php(lines 47, 74, 103, 131, 158, 224) still create and passMessageContentobjects, butextractAndSaveLinksnow expectsMessagePrecacheDto. Tests will fail with type mismatch. Update test setup to pass the correct DTO type.Additionally, the
$this->linkTrackRepository->persist()call at line 72–74 violates domain layer purity. Move persistence logic to the application layer (e.g., in a message handler) to keep the domain service free of side effects.src/Domain/Messaging/Service/Manager/TemplateImageManager.php (1)
47-48: Domain layer contains persistence side effects.This class uses
$this->entityManager->persist()(lines 47, 175) and$this->entityManager->remove()(line 155). Per coding guidelines, Domain layer should avoid persistence side effects for normal domain models.Consider moving the persistence logic to the Application layer (e.g., a MessageHandler or ApplicationService) and keeping this manager focused on business logic only.
Based on coding guidelines for
src/Domain/**.Also applies to: 155-155, 175-175
♻️ Duplicate comments (5)
src/Domain/Common/Html2Text.php (1)
81-83: Cast config value to int forwordwrap().
getValue()returns?string, butwordwrap()expects an integer for the width parameter. Cast to int to ensure type safety.- $ww = $this->configProvider->getValue(ConfigOption::WordWrap) ?? self::WORD_WRAP; + $ww = (int) ($this->configProvider->getValue(ConfigOption::WordWrap) ?? self::WORD_WRAP); return wordwrap($text, $ww);src/Domain/Messaging/Service/MessageDataLoader.php (2)
99-104:array_walk_recursivereturnsbool, not the modified array.The return value is assigned to
$data, butarray_walk_recursivemodifies the array in-place and returns a boolean. This results in$data = trueinstead of the intended array.if (str_starts_with($storedMessageDatum->getData(), 'SER:')) { $unserialized = unserialize(substr($storedMessageDatum->getData(), 4)); - $data = array_walk_recursive($unserialized, 'stripslashes'); + array_walk_recursive($unserialized, fn(&$val) => $val = is_string($val) ? stripslashes($val) : $val); + $data = $unserialized; } else { $data = stripslashes($storedMessageDatum->getData()); }
150-150: Use strict comparison forstrpos().
strpos()returnsint|false. A space at position 0 would evaluate as falsy, causing incorrect branching.- } elseif (strpos($messageData['fromfield'], ' ')) { + } elseif (strpos($messageData['fromfield'], ' ') !== false) {src/Domain/Configuration/Model/ConfigOption.php (1)
18-31: Verify new config keys exist in the config source.These enum values become contracts. Confirm the underlying keys match exactly what's stored in the database or default configs.
src/Domain/Messaging/Service/MessagePrecacheService.php (1)
44-52: Fix return-type mismatch and make cache key forward‑aware (plus optional TTL/versioning)Right now
precacheMessage()is declared to returnbool, but when the cache is warm you return whatever$this->cache->get()gives you. Since you store aMessagePrecacheDtoin the cache, this will violate theboolreturn type and throw at runtime. The cache key also still ignores$forwardContent, so forward/non‑forward modes can collide (as flagged in an earlier review).You can fix both in one go by keying on the forward flag and only returning
truefrom this method:- $cacheKey = sprintf('messaging.message.base.%d', $campaign->getId()); - - $cached = $this->cache->get($cacheKey); - if ($cached !== null) { - return $cached; - } + $cacheKey = sprintf( + 'messaging.message.base.%d.%d', + $campaign->getId(), + (int) $forwardContent + ); + + $cachedDto = $this->cache->get($cacheKey); + if ($cachedDto instanceof MessagePrecacheDto) { + // DTO already prepared and cached for this (campaign, forward) combo + return true; + } @@ - $this->cache->set($cacheKey, $messagePrecacheDto); + // Optionally add a TTL or version suffix to the key if you want to avoid + // very long‑lived/stale cached DTOs across deployments. + $this->cache->set($cacheKey, $messagePrecacheDto /*, $ttlInSeconds */);This keeps the public API
boolreturn type consistent while still caching the DTO for downstream consumers, and avoids cross‑contamination between forward/non‑forward content.Also applies to: 188-190
🧹 Nitpick comments (12)
src/Domain/Subscription/Model/Subscriber.php (1)
154-158: TODO comment noted — looks good.The comment is a helpful reminder to revisit this method's usage in relation to the new grace-time-aware blacklist check. The method itself remains unchanged and correctly returns the entity's blacklisted state.
Want me to help track down usages of
isBlacklisted()across the codebase to assess where the grace-time variant might be more appropriate?src/Domain/Subscription/Repository/UserBlacklistRepository.php (1)
45-58: Consider simplifying the grace period calculation.The nested ternary with inline assignment on line 48 is a bit of a brain-teaser. Breaking it out would make the intent clearer for future maintainers.
Also, the
?boolnullable type isn't needed here since you have a default value and don't handlenulldifferently fromtrue.- public function isEmailBlacklisted(string $email, ?bool $immediate = true): bool + public function isEmailBlacklisted(string $email, bool $immediate = true): bool { - // allow 5 minutes to send the last message acknowledging unsubscription - $grace = $immediate ? 0 : ((($gt = $this->blacklistGraceTime) >= 1 && $gt <= 15) ? $gt : 5); + // allow grace period to send the last message acknowledging unsubscription + $grace = 0; + if (!$immediate) { + $configuredGrace = $this->blacklistGraceTime; + $grace = ($configuredGrace >= 1 && $configuredGrace <= 15) ? $configuredGrace : 5; + } $cutoff = (new DateTimeImmutable())->modify('-' . $grace .' minutes');src/Domain/Analytics/Service/LinkTrackService.php (1)
59-74: Consider moving persistence side effects out of the domain layer.Just a heads-up: the
persist()call here is a side effect in the domain layer. Per the coding guidelines, domain services should ideally express intent without direct persistence operations.This isn't blocking for this PR (it's pre-existing), but you might want to consider moving this persistence logic to the application layer or using a unit-of-work pattern in a future refactoring. The domain service could return the
LinkTrackentities, and let the caller (e.g., a message handler) handle the actual persistence.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
82-100: Remove or address commented-out code.This large block of commented-out code adds noise. If it's planned future work, consider adding a TODO comment with context or tracking it in an issue. Otherwise, just remove it.
138-139: Add null check for cache retrieval.If the cache entry is missing (eviction, failure, etc.),
$messagePrecacheDtowould be null, buthandleEmailSendingexpects aMessagePrecacheDto. Might be worth a defensive check here.$messagePrecacheDto = $this->cache->get($cacheKey); + if ($messagePrecacheDto === null) { + $this->logger->error('Cache miss for precached message', ['campaign_id' => $campaign->getId()]); + continue; + } $this->handleEmailSending($campaign, $subscriber, $userMessage, $messagePrecacheDto);src/Domain/Messaging/Service/SystemMailConstructor.php (2)
24-24: Add type hint for$messageparameter.The
$messageparameter lacks a type hint. Based on usage, it appears to be a string.- public function __invoke($message, $subject = ''): array + public function __invoke(string $message, string $subject = ''): array
45-48: Minor: Movecount()outside the loop.Calling
count($matches[0])on each iteration is inefficient since the array size doesn't change.- for ($i = 0; $i < count($matches[0]); ++$i) { + $matchCount = count($matches[0]); + for ($i = 0; $i < $matchCount; ++$i) { $listsHTML .= '<li>'.$matches[1][$i].'</li>'; }Or even cleaner with
foreach:foreach ($matches[1] as $item) { $listsHTML .= '<li>' . $item . '</li>'; }src/Domain/Common/Html2Text.php (1)
75-80: Consider replacing while loops with singlepreg_replacecalls.These while loops repeatedly call
preg_matchthenpreg_replace. You can simplify by usingpreg_replacewith an unlimited pattern or a single call with+quantifier.- while (preg_match('/ /', $text)) { - $text = preg_replace('/ /', ' ', $text); - } - while (preg_match("/\n\s*\n\s*\n/", $text)) { - $text = preg_replace("/\n\s*\n\s*\n/", "\n\n", $text); - } + $text = preg_replace('/ {2,}/', ' ', $text); + $text = preg_replace("/(\n\s*){3,}/", "\n\n", $text);src/Domain/Messaging/Service/MessageDataLoader.php (1)
170-172: Simplify double-space removal.The while loop can be replaced with a single regex call.
- while (strpos($messageData['fromname'], ' ')) { - $messageData['fromname'] = str_replace(' ', ' ', $messageData['fromname']); - } + $messageData['fromname'] = preg_replace('/ {2,}/', ' ', $messageData['fromname']);src/Domain/Messaging/Service/MessagePrecacheService.php (3)
23-37: Constructor has a lot of injected collaborators; consider splitting responsibilitiesThe constructor now takes a pretty large set of dependencies, which is a hint this service might be doing too many things. If this continues to grow, it may be worth extracting some functionality into smaller collaborators (e.g. a “remote content + template prep” helper, or an “admin attributes resolver”) to keep this class focused and easier to test.
111-150: Castpreg_match()result to bool foruserSpecificUrl
$messagePrecacheDto->userSpecificUrlis set directly frompreg_match(), which returns0|1|false. If the DTO property is typed asbool, assigning0/1can be surprising (and in stricter contexts, problematic).A small tweak keeps the intent clear:
- $messagePrecacheDto->userSpecificUrl = preg_match('/\[.+\]/', $loadedMessageData['sendurl']); + $messagePrecacheDto->userSpecificUrl = (bool) preg_match('/\[.+\]/', $loadedMessageData['sendurl']);The rest of the logic (
if (!$messagePrecacheDto->userSpecificUrl)) stays unchanged.
193-223: Remove unused helper methodsbuildBasicReplacements()andapplyReplacements()Both methods are dead code—only referenced in the commented-out lines above and nowhere else in the codebase. Drop them to keep the service clean.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
config/parameters.yml.dist(2 hunks)src/Domain/Analytics/Service/LinkTrackService.php(3 hunks)src/Domain/Common/Html2Text.php(1 hunks)src/Domain/Configuration/Model/ConfigOption.php(1 hunks)src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php(1 hunks)src/Domain/Identity/Repository/AdministratorRepository.php(1 hunks)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php(6 hunks)src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php(1 hunks)src/Domain/Messaging/Repository/MessageRepository.php(2 hunks)src/Domain/Messaging/Service/Manager/TemplateImageManager.php(3 hunks)src/Domain/Messaging/Service/MessageDataLoader.php(1 hunks)src/Domain/Messaging/Service/MessagePrecacheService.php(3 hunks)src/Domain/Messaging/Service/MessageProcessingPreparator.php(2 hunks)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php(2 hunks)src/Domain/Messaging/Service/SystemMailConstructor.php(1 hunks)src/Domain/Subscription/Model/Subscriber.php(1 hunks)src/Domain/Subscription/Repository/UserBlacklistRepository.php(2 hunks)src/Domain/Subscription/Service/SubscriberCsvImporter.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Domain/Subscription/Service/SubscriberCsvImporter.php
🚧 Files skipped from review as they are similar to previous changes (3)
- config/parameters.yml.dist
- src/Domain/Identity/Repository/AdministratorRepository.php
- 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/Subscription/Model/Subscriber.phpsrc/Domain/Subscription/Repository/UserBlacklistRepository.phpsrc/Domain/Messaging/Service/SystemMailConstructor.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Configuration/Service/Provider/DefaultConfigProvider.phpsrc/Domain/Common/Html2Text.phpsrc/Domain/Messaging/Service/MessageProcessingPreparator.phpsrc/Domain/Messaging/Repository/MessageRepository.phpsrc/Domain/Analytics/Service/LinkTrackService.phpsrc/Domain/Messaging/Service/MessagePrecacheService.phpsrc/Domain/Configuration/Model/ConfigOption.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/MessageDataLoader.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 (4)
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/Messaging/Service/Manager/TemplateImageManager.php (3)
TemplateImageManager(15-177)__construct(29-34)parseLogoPlaceholders(101-113)
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/Html2Text.php (3)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Service/MessagePrecacheService.php (1)
__construct(23-38)src/Domain/Messaging/Service/SystemMailConstructor.php (1)
__construct(15-22)
src/Domain/Messaging/Service/MessageProcessingPreparator.php (2)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(7-30)src/Domain/Analytics/Service/LinkTrackService.php (2)
isExtractAndSaveLinksApplicable(30-33)extractAndSaveLinks(41-77)
src/Domain/Analytics/Service/LinkTrackService.php (1)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(7-30)
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(7-30)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (3)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(7-30)src/Domain/Messaging/Service/MessageDataLoader.php (1)
MessageDataLoader(13-200)src/Domain/Messaging/Service/MessagePrecacheService.php (1)
precacheMessage(44-191)
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-118)getNonEmptyFields(102-117)
🔇 Additional comments (4)
src/Domain/Subscription/Repository/UserBlacklistRepository.php (1)
16-22: Constructor injection looks solid.Clean dependency injection pattern. The
blacklistGraceTimeis properly injected as a readonly property, which keeps configuration externalized.src/Domain/Messaging/Repository/MessageRepository.php (1)
102-117: Looks good!Clean implementation for extracting non-empty fields. Using
HYDRATE_ARRAYis a nice touch for performance when you just need the raw data without entity overhead.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
194-195: LGTM on personalization calls.The
MessagePrecacheDtofields are typed as non-nullable strings, and the precache service ensures they're populated. The past review concern about nullable values fromgetText()/getFooter()methods doesn't apply here since you're now working with the DTO's public fields directly.src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
61-85: LGTM on the DTO refactor.The transition from
MessageContenttoMessagePrecacheDtois clean. The method properly handles early returns and mutates the DTO's content fields as expected.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
23-23: Remove unnecessary parentheses.The extra parentheses around
new Email()are not needed and can be simplified.- $email = (new Email()); + $email = new Email();src/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.php (2)
28-32: Consider injecting runtime context for testability.The hardcoded
PHP_SAPIcheck works but makes unit testing harder. You could inject a context service or use Symfony's request stack to determine if running under HTTP.Example refactor using constructor injection:
public function __construct(private readonly ?RequestStack $requestStack = null) { } public function onMessage(MessageEvent $event): void { // ... if ($this->requestStack === null || !$this->requestStack->getCurrentRequest()) { return; } // ... }
38-41: Consider validating header value components.While the header injection is syntactically correct, the value components (
$ip,$host) aren't validated. If these contain newlines or unexpected characters, they could lead to header injection vulnerabilities.Add basic sanitization:
+ // Sanitize values to prevent header injection + $ip = str_replace(["\r", "\n"], '', $ip); + $host = str_replace(["\r", "\n"], '', $host); + $msg->getHeaders()->addTextHeader( 'X-phpList-Injected-By', "from [$ip] by $host with HTTP; $time" );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/parameters.yml.dist(2 hunks)src/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.php(1 hunks)src/Domain/Messaging/Service/Builder/EmailBuilder.php(1 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/EventSubscriber/InjectedByHeaderSubscriber.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.php
🔇 Additional comments (2)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
9-16: LGTM - Clean constructor with readonly properties.The dependency injection pattern with readonly properties is solid, and the configuration parameters are well-defined for the builder's needs.
25-37: No action needed—Symfony'saddTextHeader()already handles header sanitization. Text headers encode characters that aren't permitted in message headers (like newlines or non-ASCII), and no headers in this design are vulnerable to header-injection attacks. The header logic here is sound.
src/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.php
Outdated
Show resolved
Hide resolved
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.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/Domain/Configuration/Model/ConfigOption.php (1)
32-33: Consider standardizing config key naming convention.The
PoweredByImageandPoweredByTextkeys use PascalCase, while most other new keys use snake_case (e.g.,message_from_address,admin_address). This inconsistency can make keys harder to remember and may lead to lookup errors.🔎 Suggested standardization to snake_case:
- case PoweredByImage = 'PoweredByImage'; - case PoweredByText = 'PoweredByText'; + case PoweredByImage = 'powered_by_image'; + case PoweredByText = 'powered_by_text';Note: If these keys already exist in the config storage with PascalCase names, you'd need to update the storage or add a migration to rename them.
src/Domain/Messaging/Service/TemplateImageEmbedder.php (4)
31-31: Public mutable array is a code smell.Exposing
$attachmentas a public array breaks encapsulation. External code can modify it directly, making the class state unpredictable.🔎 Consider making it private with a getter:
- public array $attachment = []; + private array $attachment = []; + + public function getAttachments(): array + { + return $this->attachment; + }
229-238: Magic numeric indices make this array hard to maintain.Using indices 0-7 for different fields is brittle. If someone adds or removes a field, it's easy to mess up. Consider using an associative array or a small DTO/value object.
🔎 Associative array alternative:
//Append to $attachment array $this->attachment[] = [ - 0 => $string, - 1 => $name, - 2 => $name, - 3 => $encoding, - 4 => $type, - 5 => true, //isStringAttachment - 6 => $disposition, - 7 => $cid, + 'content' => $string, + 'name' => $name, + 'filename' => $name, + 'encoding' => $encoding, + 'type' => $type, + 'isStringAttachment' => true, + 'disposition' => $disposition, + 'cid' => $cid, ];
193-196: Relative paths../are fragile.These paths depend on the current working directory, which can vary between CLI, web server, and test contexts. Consider making these configurable or using absolute paths derived from a known root.
66-66: Minor:count()in loop condition is re-evaluated each iteration.Not a big deal here since the array size is fixed, but caching the count is a common idiom:
🔎 Example:
- for ($index = 0; $index < count($matchedImages[1]); ++$index) { + $count = count($matchedImages[1]); + for ($index = 0; $index < $count; ++$index) {Or just use
foreachwhich is more idiomatic in PHP.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
composer.json(1 hunks)config/parameters.yml.dist(2 hunks)src/Domain/Common/Model/ContentTransferEncoding.php(1 hunks)src/Domain/Configuration/Model/ConfigOption.php(1 hunks)src/Domain/Messaging/Repository/TemplateImageRepository.php(1 hunks)src/Domain/Messaging/Service/TemplateImageEmbedder.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- composer.json
- 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/Repository/TemplateImageRepository.phpsrc/Domain/Common/Model/ContentTransferEncoding.phpsrc/Domain/Messaging/Service/TemplateImageEmbedder.phpsrc/Domain/Configuration/Model/ConfigOption.php
⏰ 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). (1)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (3)
src/Domain/Messaging/Repository/TemplateImageRepository.php (1)
16-24: Looks good - clean query helper for orphan images.The method correctly fetches template images with
template = 0. One small note: line 21 usesandWhereafterwhere, which is fine, but switching to chainedwhere(...)->andWhere(...)is the conventional pattern you've got here.src/Domain/Common/Model/ContentTransferEncoding.php (1)
7-14: Nice and clean enum definition. 👍Solid use of a string-backed enum for content transfer encodings. Covers all the standard RFC 2045 types. This will help catch invalid encodings at compile time rather than runtime.
src/Domain/Messaging/Service/TemplateImageEmbedder.php (1)
46-145: Overall structure looks reasonable for the embedding workflow.The method orchestrates external image caching, template image embedding, and filesystem image embedding in a logical sequence. The deduplication via
array_unique()is a nice touch to prevent duplicate attachments.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/Domain/Messaging/Service/TemplateImageEmbedder.php (2)
156-181: Domain layer violation: persistence side effects remain.The
configManager->create()calls on lines 156-161, 165-170, and 174-179 persist configuration changes, which violates domain layer guidelines. This issue was previously flagged but remains unresolved.Per coding guidelines, domain services shouldn't perform persistence. Consider:
- Return the computed config key/value as a DTO for the caller to persist
- Move this logic to a MessageHandler or Application service
- If persistence here is intentional, document why it's acceptable
Based on coding guidelines for domain layer.
219-222:mime_content_type()will fail with bare filename.Line 221 calls
mime_content_type($name)where$nameis typically just a filename like"image.png". This function requires a filesystem path to read file magic bytes and will fail for bare filenames.Use the existing
$mimeMaplookup based on file extension instead:🔎 Suggested fix
//If a MIME type is not specified, try to work it out from the name if ('' === $type && !empty($name)) { - $type = mime_content_type($name); + $ext = strtolower(pathinfo($name, PATHINFO_EXTENSION)); + $type = $this->mimeMap[$ext] ?? 'application/octet-stream'; }
🧹 Nitpick comments (5)
src/Domain/Messaging/Repository/MessageRepository.php (1)
103-118: Consider clarifying the method name and filtering behavior.The method name
getNonEmptyFieldssuggests it filters out all "empty" values, but it only removesnulland empty strings (''). Values like0,false, or empty arrays ([]) would still be included. This could lead to confusion.Additionally, returning raw arrays via
HYDRATE_ARRAYfrom a domain repository blurs the boundary between domain and application concerns. This type of data transformation is typically better suited for a DTO mapper or application-layer service.💡 Consider these improvements
Option 1: Clarify the method name to match its behavior
public function getFieldsExcludingNullAndEmpty(int $id): array { // ... existing implementation }Option 2: Move this logic to an application-layer service or DTO mapper
This would keep the repository focused on returning domain entities, and handle data transformation separately where it's more appropriate.
config/parameters.yml.dist (3)
30-31: Placeholder email should be documented.The default
dev@dev.comwon't work in any environment. Consider adding a comment above this parameter reminding users to set a valid email address, or use a more obviously placeholder value likedeveloper@example.com.
39-39: Clarify the inline comment.The comment "set local_domain on transport" is a bit cryptic. Consider expanding it to explain where the local_domain should be set (e.g., in services configuration or transport options) and why it's needed.
112-117: Document units for image-related parameters.The parameters
external_image_max_age,external_image_timeout, andexternal_image_max_sizedon't specify their units. Consider adding inline comments clarifying whether these are seconds, milliseconds, bytes, kilobytes, etc. This helps prevent misconfiguration.src/Domain/Messaging/Service/TemplateImageEmbedder.php (1)
48-48: Add space after=operator.Minor style nit:
$templateId= (int)should be$templateId = (int).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
config/parameters.yml.dist(2 hunks)resources/translations/messages.en.xlf(1 hunks)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php(8 hunks)src/Domain/Messaging/Repository/MessageRepository.php(4 hunks)src/Domain/Messaging/Service/Builder/EmailBuilder.php(1 hunks)src/Domain/Messaging/Service/TemplateImageEmbedder.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/Repository/MessageRepository.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Service/TemplateImageEmbedder.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/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (6)
src/Domain/Messaging/Model/Message/MessageMetadata.php (2)
getSendStart(122-125)setSendStart(127-131)src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(7-30)src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
EmailBuilder(18-148)buildPhplistEmail(36-147)src/Domain/Messaging/Service/MessageDataLoader.php (1)
MessageDataLoader(13-200)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (3)
RateLimitedCampaignMailer(14-55)send(49-54)composeEmail(24-44)src/Domain/Messaging/Service/MessagePrecacheService.php (1)
precacheMessage(44-191)
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 (1)
SystemMailConstructor(13-93)src/Domain/Messaging/Service/TemplateImageEmbedder.php (2)
TemplateImageEmbedder(16-273)__construct(33-44)src/Domain/Subscription/Repository/SubscriberRepository.php (1)
SubscriberRepository(21-208)src/Domain/Subscription/Repository/UserBlacklistRepository.php (1)
UserBlacklistRepository(14-59)src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php (2)
SubscriberHistoryManager(18-143)addHistory(45-56)src/Domain/Subscription/Model/Subscriber.php (1)
setBlacklisted(160-165)
src/Domain/Messaging/Service/TemplateImageEmbedder.php (6)
src/Domain/Common/ExternalImageService.php (3)
ExternalImageService(10-177)cache(40-92)getFromCache(24-38)src/Domain/Configuration/Service/Manager/ConfigManager.php (1)
ConfigManager(11-65)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/TemplateImageRepository.php (2)
TemplateImageRepository(12-46)findByTemplateIdAndFilename(36-45)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
__construct(54-78)__invoke(80-212)src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
__construct(20-34)
🔇 Additional comments (6)
src/Domain/Messaging/Repository/MessageRepository.php (2)
8-8: LGTM!The import is necessary for the
HYDRATE_ARRAYconstant used in the newgetNonEmptyFieldsmethod.
92-101: LGTM!Standard repository query pattern with proper status filtering. No domain purity concerns.
config/parameters.yml.dist (1)
102-103: This parameter appears to be unused in the codebase.The
messaging.blacklist_grace_timeparameter is defined with an empty default value but has no corresponding usage in any PHP files. Either document this as intentionally disabled, provide a sensible default (if the feature should be active), or clean up the orphaned configuration.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (3)
119-151: Start notification flow looks good.The persistence operations here (lines 145-150) are acceptable since this is a MessageHandler, which serves as an orchestration boundary per coding guidelines. The
UniqueConstraintViolationExceptionhandling provides idempotency for the start notification marker.
224-226: sendStart tracking implemented correctly.The check for
nullbefore settingsendStartensures the timestamp is only set once, even if the message is requeued. Good defensive pattern.
261-262: No issue. BothMessagePrecacheDto::$textContentandMessagePrecacheDto::$footerare declared as non-nullablepublic stringproperties. Thepersonalize()method expects non-nullable strings. PHP's type system guarantees these values are never null, so null coalescing is unnecessary.Likely an incorrect or invalid review comment.
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
Outdated
Show resolved
Hide resolved
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
8e75515 to
25ef84a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (13)
config/parameters.yml.dist (1)
123-124: Previous security concern remains unaddressed.The default schema is still set to
'http'instead of'https'. As flagged in the previous review, HTTPS should be the default for production deployments in 2025. Users can override to HTTP for local development via environment variables.src/Domain/Messaging/Service/MessagePrecacheService.php (3)
67-67: Use strict comparison forstrpos()to handle position-0 correctly.Line 67 uses
strpos(..., ' ')in a loose truthiness check. If a space appears at position 0, this evaluates tofalseeven though the substring exists. Use strict comparison:- } elseif (strpos($loadedMessageData['replyto'], ' ')) { + } elseif (strpos($loadedMessageData['replyto'], ' ') !== false) {
188-188: Consider adding TTL to cache entries.Line 188 stores the DTO in cache without an expiration time, so it persists indefinitely. Add a TTL to prevent stale data across deployments:
- $this->cache->set($cacheKey, $messagePrecacheDto); + $this->cache->set($cacheKey, $messagePrecacheDto, 3600); // 1 hour TTLChoose an appropriate expiration based on your deployment and staleness requirements.
179-181: Add null-safety guards forparseLogoPlaceholders()calls.Lines 179-181 call
parseLogoPlaceholders()on DTO properties that can benull(e.g.,templateis set to null at line 125 when remote content is used). IfparseLogoPlaceholders()doesn't handle null input, this will cause warnings or errors.Either guard the calls:
- $messagePrecacheDto->content = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->content); - $messagePrecacheDto->template = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->template); - $messagePrecacheDto->htmlFooter = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->htmlFooter); + $messagePrecacheDto->content = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->content ?? ''); + if ($messagePrecacheDto->template !== null) { + $messagePrecacheDto->template = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->template); + } + $messagePrecacheDto->htmlFooter = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->htmlFooter ?? '');Or update
parseLogoPlaceholders()to accept and return?stringwith null handling.src/Domain/Common/RemotePageFetcher.php (2)
19-28: Domain layer should not depend on EntityManagerInterface for non-DynamicListAttr entities.The constructor injects
EntityManagerInterfaceat line 26, and later uses it forpersist()andremove()calls (lines 75, 78) on UrlCache entities. Per coding guidelines, Domain services should not perform persistence side effects on regular entities. Move UrlCache persistence to an Infrastructure repository or Application service, and inject an abstraction (e.g.,UrlCacheRepositoryInterface) instead.As per coding guidelines, Domain layer must remain free of Doctrine persistence APIs for regular entities.
103-117: Fix URL parameter handling and strpos comparison.Two issues flagged in past review remain unaddressed:
Line 107:
preg_replace('/\W/', '', $url_append)strips essential URL characters (&,=,?, etc.), breaking query strings like&utm_source=x. Usetrim()instead.Line 109:
if (strpos($url, '?'))fails when?is at position 0. Use strict comparison:if (strpos($url, '?') !== false).🔎 Proposed fix
- $url_append = preg_replace('/\W/', '', $url_append); + $url_append = trim($url_append); if ($url_append) { - if (strpos($url, '?')) { + if (strpos($url, '?') !== false) { $url = $url.$url_append; } else { $url = $url.'?'.$url_append;src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
202-203: Add null check for cache miss before passing to handleEmailSending.Line 202 retrieves
$messagePrecacheDtofrom cache, which can returnnullif the entry is missing or expired. Line 203 passes this potentially null value tohandleEmailSending(), which expects a non-nullableMessagePrecacheDto(line 258). This will cause a type error.🔎 Proposed fix
$messagePrecacheDto = $this->cache->get($cacheKey); + if (!$messagePrecacheDto instanceof MessagePrecacheDto) { + $this->logger->error('Precache DTO missing from cache', ['campaign_id' => $campaign->getId()]); + continue; + } $this->handleEmailSending($campaign, $subscriber, $userMessage, $messagePrecacheDto);src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
99-111: Add type declarations forparseLogoPlaceholders().The method lacks parameter and return type declarations. Add strict typing:
- public function parseLogoPlaceholders($content) + public function parseLogoPlaceholders(string $content): string
172-180: Fix fragile output buffer handling.Lines 173-179 capture output in the wrong order and echo previous buffer contents. Use a local buffer instead:
🔎 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; + $imageContent = ob_get_clean();src/Domain/Common/Html2Text.php (1)
61-66: Remove unreachable anchor tag replacement.Lines 61-66 attempt to convert
<a href="...">tags, butstrip_tags()at line 38 already removed all HTML. This regex will never match anything. Remove this block:$text = str_replace($fullMatch, $linkReplace, $text); } - $text = preg_replace( - "/<a href=[\"\'](.*?)[\"\'][^>]*>(.*?)<\/a>/is", - '[URLTEXT]\\2[ENDURLTEXT][LINK]\\1[ENDLINK]', - $text, - 500 - ); $text = html_entity_decode($text, ENT_QUOTES | ENT_HTML5, 'UTF-8');src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
64-76: Add null check before callingsetBlacklisted().Line 66 calls
findOneByEmail()which can returnnull, but line 67 immediately callssetBlacklisted(true)without checking. This will cause a fatal error if no subscriber exists.🔎 Proposed fix
if (!$skipBlacklistCheck && $this->blacklistRepository->isEmailBlacklisted($to)) { $this->eventLogManager->log('', sprintf('Error, %s is blacklisted, not sending', $to)); $subscriber = $this->subscriberRepository->findOneByEmail($to); - $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', - ); + if ($subscriber) { + $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 null; }
132-140: Embedded images are not attached; capture return value and add attachments to email.Line 135 calls
templateImageEmbedderbut discards the returned HTML containing CID references. The embedder's$attachmentarray is also never accessed. Recipients will see broken images.🔎 Proposed fix
if (!empty($htmlMessage)) { - $email->html($htmlMessage); - $email->text($textMessage); - ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); + $htmlMessage = ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); + $email->html($htmlMessage); + // TODO: Attach images from $this->templateImageEmbedder->attachment array //# 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);Also consolidate the redundant
text()calls (lines 134, 138, 140).src/Domain/Messaging/Service/TemplateImageEmbedder.php (1)
219-222:mime_content_type()will fail on bare filenames.Line 221 calls
mime_content_type($name)where$nameis typically just a filename like"image.png", not a full path. This function needs an actual file path to read magic bytes and will returnfalsefor non-existent paths.🔎 Proposed fix
//If a MIME type is not specified, try to work it out from the name if ('' === $type && !empty($name)) { - $type = mime_content_type($name); + if (is_file($name) && is_readable($name)) { + $type = mime_content_type($name); + } else { + $ext = strtolower(pathinfo($name, PATHINFO_EXTENSION)); + $type = $this->mimeMap[$ext] ?? 'application/octet-stream'; + } }
🧹 Nitpick comments (2)
src/Domain/Common/RemotePageFetcher.php (2)
42-42: Fatal error: method name case mismatch.Line 42 calls
$this->expandUrl($url)(lowercase 'u'), but the method is declared asexpandURL(uppercase 'URL') at line 103. PHP method names are case-insensitive for calls, so this will work, but it's inconsistent. For clarity, match the casing:- $url = $this->expandUrl($url); + $url = $this->expandURL($url);
89-101: Log exceptions instead of silently returning empty string.Lines 98-100 catch and discard all
Throwableexceptions without logging. This makes debugging fetch failures difficult. Add a log entry before returning:} catch (Throwable $e) { + $this->eventLogManager->log('unknown page', 'Failed to fetch URL: ' . $url . ' - ' . $e->getMessage()); return ''; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
config/parameters.yml.dist(2 hunks)resources/translations/messages.en.xlf(1 hunks)src/Domain/Common/Html2Text.php(1 hunks)src/Domain/Common/RemotePageFetcher.php(1 hunks)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php(8 hunks)src/Domain/Messaging/Repository/MessageRepository.php(4 hunks)src/Domain/Messaging/Service/Builder/EmailBuilder.php(1 hunks)src/Domain/Messaging/Service/Manager/TemplateImageManager.php(4 hunks)src/Domain/Messaging/Service/MessagePrecacheService.php(3 hunks)src/Domain/Messaging/Service/TemplateImageEmbedder.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Domain/Messaging/Repository/MessageRepository.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/Manager/TemplateImageManager.phpsrc/Domain/Common/Html2Text.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/MessagePrecacheService.phpsrc/Domain/Messaging/Service/TemplateImageEmbedder.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/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/Common/Html2Text.php (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)
src/Domain/Common/RemotePageFetcher.php (3)
src/Domain/Configuration/Repository/UrlCacheRepository.php (3)
UrlCacheRepository(12-32)findByUrlAndLastModified(16-25)getByUrl(28-31)src/Domain/Common/HtmlUrlRewriter.php (2)
HtmlUrlRewriter(11-190)addAbsoluteResources(13-73)src/Domain/Configuration/Model/UrlCache.php (2)
getLastModified(48-51)setLastModified(69-73)
🔇 Additional comments (3)
config/parameters.yml.dist (2)
39-39: Helpful documentation added.The inline comment clarifies the transport configuration requirement. Nice touch!
119-122: Image directory defaults look reasonable.The relative paths for upload and editor images are straightforward. Just ensure these are properly documented as relative to the web root or application root in your deployment documentation.
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
260-262: The code at lines 260-262 is type-safe. BothMessagePrecacheDto::$textContentandMessagePrecacheDto::$footerare declared as non-nullablestringproperties (not?string). TheUserPersonalizer::personalize()method correctly expects a non-nullablestringparameter and will always receive valid strings. No null coalescing is needed.Likely an incorrect or invalid review comment.
TatevikGr
changed the title
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 (1)
tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php (1)
87-118: Verify footer field assignment in test.Line 98 sets
$messagePrecacheDto->footerbut the service (per relevant snippets) checks$content->htmlFooterfor HTML link extraction. This inconsistency means the test may not properly exercise footer link extraction. Consider usinghtmlFooterinstead if testing HTML footer links.🔎 Suggested fix
$messagePrecacheDto = new MessagePrecacheDto(); $messagePrecacheDto->subject = 'Test Subject'; $messagePrecacheDto->content = $htmlContent; - $messagePrecacheDto->footer = $footerContent; + $messagePrecacheDto->htmlFooter = $footerContent;
♻️ Duplicate comments (10)
src/Domain/Common/TextParser.php (1)
17-31: Undefined variable issue resolved.Line 18 now initializes
$link = []before the while loop, fixing the potential undefined variable access noted in the previous review.src/Domain/Messaging/Service/Manager/TemplateImageManager.php (1)
172-180: Output buffer handling remains fragile.The buffer capture sequence still captures content before starting a new buffer (line 173), then echoes it after (line 179). This can corrupt output state. The previous review suggested a cleaner approach.
🔎 Recommended 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/Service/RateLimitedCampaignMailer.php (1)
38-44: HTML body may be incomplete.Line 43 sets the HTML body to
$messagePrecacheDto->contentonly, but the TODO comment suggests this needs review. IfhtmlFootershould be included in the email body, you'll want to concatenate them. The DTO structure indicatescontentis the main HTML andhtmlFooteris separate, so the full HTML email might need both.💡 Consider concatenating content and footer
return $email ->to($subscriber->getEmail()) ->subject($messagePrecacheDto->subject) ->text($messagePrecacheDto->textContent) - // todo: check htmlFooterit should be html of textContent - ->html($messagePrecacheDto->content); + ->html($messagePrecacheDto->content . $messagePrecacheDto->htmlFooter);If the footer should be included separately or handled differently, clarify the intent and remove the TODO.
src/Domain/Messaging/Service/SystemMailConstructor.php (1)
73-74: Use strict comparison withstrpos()to avoid false negatives.
strpos()returns0when the needle is at position 0, which is falsy. Use!== falsefor correctness.- } elseif (strpos($htmlContent, '</body>')) { + } elseif (strpos($htmlContent, '</body>') !== false) {src/Domain/Messaging/Service/MessageDataLoader.php (1)
145-147: Use strict comparison withstrpos()in the while loop.If double-space appears at position 0,
strpos()returns0(falsy), causing the loop to exit prematurely. Use!== false.- while (strpos($messageData['fromname'], ' ')) { + while (strpos($messageData['fromname'], ' ') !== false) { $messageData['fromname'] = str_replace(' ', ' ', $messageData['fromname']); }src/Domain/Messaging/Service/MessagePrecacheService.php (1)
188-188: Consider adding TTL to cache storage.Line 188 stores the DTO in cache without specifying a TTL, so it persists indefinitely. Stale cache entries could survive deployments or code changes. Consider adding an explicit expiration time.
🔎 Example
- $this->cache->set($cacheKey, $messagePrecacheDto); + $this->cache->set($cacheKey, $messagePrecacheDto, 3600); // 1 hour TTLAdditionally, you might want to version the cache key (e.g., append a version constant) to invalidate cache across deployments.
src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
139-147: Critical: Embedded images are still not attached to the email.Line 142 invokes
templateImageEmbedderwhich populates its$attachmentarray with embedded images and returns HTML with CID references. However:
- The return value is discarded (not assigned)
- The
$attachmentarray is never read or added to theThis means recipients will see broken image references, as the original
$htmlMessagewithout CID updates is used (line 140) and no attachments are added.🔎 Required fix
if (!empty($htmlMessage)) { - $email->html($htmlMessage); - $email->text($textMessage); - ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); + $htmlMessage = ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); + $email->html($htmlMessage); + + // Add embedded image attachments + foreach ($this->templateImageEmbedder->attachment as $attachment) { + // Add attachment to email using appropriate Symfony Mime API + // (implementation depends on your email library's attachment API) + } + //# 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);Note: You'll need to verify the correct Symfony Mime Email API for adding embedded attachments with CIDs.
src/Domain/Messaging/Service/TemplateImageEmbedder.php (2)
147-209: Domain service performs configuration persistence side effects.Lines 159-161, 168-173, and 179-184 call
$this->configManager->create(), which persists configuration changes. Per coding guidelines, Domain services insrc/Domain/**should not have persistence side effects—those belong in the Infrastructure or Application layer.Consider refactoring to return a computed
UploadImageRootvalue alongside the base64 image data, then let the Application layer (e.g., a message handler) perform the actualconfigManager->create()persistence.Based on coding guidelines, persistence operations should be in Infrastructure/Application layers.
230-233:mime_content_type()expects a file path, not a bare filename.Line 232 calls
mime_content_type($name)where$nameis typically just a filename like"image.png". This function requires an actual filesystem path to read file magic bytes and will fail or return false for bare filenames.🔎 Suggested fix
//If a MIME type is not specified, try to work it out from the name if ('' === $type && !empty($name)) { - $type = mime_content_type($name); + $ext = strtolower(pathinfo($name, PATHINFO_EXTENSION)); + $type = $this->mimeMap[$ext] ?? 'application/octet-stream'; }This uses the existing
$mimeMapto derive the MIME type from the file extension, which is safer and doesn't require filesystem access.src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
7-30: Risk: Several typed properties lack default values.Multiple non-nullable properties (
replyToName,fromName,fromEmail,to,subject,content,footer,textFooter,htmlFormatted,sendFormat,userSpecificUrl,googleTrack) have no defaults. Reading any of these before assignment will cause a fatal error.If this DTO requires all fields to be set explicitly, consider documenting that contract. Otherwise, provide sensible defaults (empty strings, false, etc.) or make properties nullable.
🔎 Example fix
- public string $replyToName; + public string $replyToName = ''; - public string $fromName; + public string $fromName = ''; - public string $fromEmail; + public string $fromEmail = ''; // ... and so on for other propertiesOr make them nullable:
- public string $replyToName; + public ?string $replyToName = null;
🧹 Nitpick comments (6)
src/Domain/Common/TextParser.php (1)
9-84: Complex text transformation logic.The multi-pass regex approach handles emails, URLs, and special characters through placeholders. While complex, this handles edge cases like trailing punctuation and scheme normalization. For very large text blocks, the multiple regex passes could be a performance consideration, but it's likely acceptable for typical message content.
src/Domain/Messaging/Service/SystemMailConstructor.php (1)
24-24: Add type hint for$messageparameter.The
$messageparameter lacks a type declaration. Addingstring $messagewould improve clarity and catch type errors early.- public function __invoke($message, $subject = ''): array + public function __invoke(string $message, string $subject = ''): arraysrc/Domain/Messaging/Service/MessageDataLoader.php (1)
161-171: Consider using strict comparison for string equality.The loose comparisons (
==) work here but strict comparisons (===) are more explicit and safer for string checks.- if (empty($messageData['campaigntitle'])) { - if ($messageData['subject'] != '(no subject)') { + 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)') { + if ($messageData['campaigntitle'] === '(no title)' && $messageData['subject'] !== '(no subject)') { $messageData['campaigntitle'] = $messageData['subject']; }src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
94-111: Remove or track commented-out code.This block of commented-out code (and the one at lines 159-177) adds noise. If this functionality is planned, consider creating an issue to track it instead of leaving dead code in the file.
src/Domain/Common/RemotePageFetcher.php (1)
50-50: Inconsistent method name casing.Line 50 calls
expandUrl()but the method is defined asexpandURL()at line 111. While PHP is case-insensitive for method names, this inconsistency can confuse readers and violates PSR naming conventions.🔎 Fix for consistency
- $url = $this->expandUrl($url); + $url = $this->expandURL($url);src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
141-147: Redundant text() calls.Lines 141, 145, and 147 all call
$email->text($textMessage). The third call (line 147) outside the conditional will overwrite any previous calls. Consider consolidating to a single call.🔎 Simplified approach
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);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
config/parameters.yml.distsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Configuration/Service/Provider/DefaultConfigProvider.phpsrc/Domain/Identity/Repository/AdminAttributeDefinitionRepository.phpsrc/Domain/Messaging/Exception/MessageCacheMissingException.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/MessagePrecacheService.phpsrc/Domain/Messaging/Service/MessageProcessingPreparator.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/Service/SystemMailConstructor.phpsrc/Domain/Messaging/Service/TemplateImageEmbedder.phptests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.phptests/Unit/Domain/Common/RemotePageFetcherTest.phptests/Unit/Domain/Common/TextParserTest.phptests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.phptests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- config/parameters.yml.dist
- src/Domain/Identity/Repository/AdminAttributeDefinitionRepository.php
- src/Domain/Configuration/Service/Provider/DefaultConfigProvider.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/Exception/MessageCacheMissingException.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Messaging/Service/MessageProcessingPreparator.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/Service/MessagePrecacheService.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/SystemMailConstructor.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Messaging/Service/TemplateImageEmbedder.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.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)
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (2)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
EmailBuilder(19-155)src/Domain/Messaging/Service/Manager/MessageDataManager.php (1)
MessageDataManager(13-96)
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (2)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(7-30)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/Messaging/Service/MessageProcessingPreparator.php (2)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(7-30)src/Domain/Analytics/Service/LinkTrackService.php (2)
isExtractAndSaveLinksApplicable(30-33)extractAndSaveLinks(41-77)
src/Domain/Common/RemotePageFetcher.php (6)
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/Html2Text.php (1)
__construct(14-16)src/Domain/Common/HtmlUrlRewriter.php (2)
HtmlUrlRewriter(11-190)addAbsoluteResources(13-73)src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php (1)
get(584-589)src/Domain/Configuration/Model/UrlCache.php (2)
getLastModified(48-51)setLastModified(69-73)
src/Domain/Messaging/Service/SystemMailConstructor.php (4)
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/Messaging/Service/Manager/TemplateImageManager.php (3)
TemplateImageManager(14-192)__construct(28-32)parseLogoPlaceholders(99-111)
tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php (2)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(7-30)src/Domain/Analytics/Service/LinkTrackService.php (1)
extractAndSaveLinks(41-77)
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(13-186)
🔇 Additional comments (10)
src/Domain/Messaging/Exception/MessageCacheMissingException.php (1)
9-15: LGTM!Clean exception implementation following standard patterns.
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
28-32: Good refactor to use repository pattern.Moving from direct EntityManager usage to TemplateImageRepository properly separates domain concerns and addresses the previous domain-layer persistence violation.
99-111: LGTM!Proper return type declaration added, addressing previous review feedback.
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
51-99: Test setup properly updated for new dependencies.The addition of UserPersonalizer, EmailBuilder, and MessageDataLoader mocks correctly reflects the handler's updated constructor. The personalization callback returning unchanged text is a good approach for keeping assertions valid.
tests/Unit/Domain/Common/TextParserTest.php (1)
10-69: Solid test coverage for text parsing.Tests cover the key scenarios: email linking, URL autolinking (both http:// and www.), newline handling, and special character preservation. Clear assertions make the expected behavior easy to understand.
tests/Unit/Domain/Common/RemotePageFetcherTest.php (1)
21-203: Comprehensive test suite for remote fetching.Tests cover the full caching hierarchy (PSR cache → DB cache → remote fetch), error handling, and URL expansion with placeholders. Good use of mock expectations to verify the caching behavior and that remote requests only happen when needed.
tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php (1)
31-130: Solid test coverage for the new MessageDataLoader.This test effectively validates the core behaviors: default merging, subject normalization, schedule field array conversion, target list extraction, sendurl inference, and from-field parsing. The time drift allowance (±120s) handles test execution variability well.
One minor observation: the anonymous class at lines 73-75 works but consider using
$this->createMock()with a properListMessageinterface/class for better type safety and consistency with the rest of the test setup.src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
61-85: Clean DTO-based refactor for link processing.The switch to
MessagePrecacheDtois well done. Links are now correctly extracted from and replaced in bothcontentandhtmlFooterfields, addressing the previous review feedback. The TODO at line 75 is a good reminder for future text message link handling.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
203-207: Good fix for the cache miss handling.The null check and exception throw properly address the previous review concern about
$this->cache->get()potentially returning null.
265-266: DTO field initialization is guaranteed by MessagePrecacheService.The
MessagePrecacheDtopropertiesfooterandtextFooterare declared without defaults in the DTO, butMessagePrecacheService::precacheMessage()always initializes them before caching. The personalization calls on lines 265-266 are safe because$processedcomes fromMessageProcessingPreparator::processMessageLinks(), which returns the initialized DTO. For clarity and to prevent future issues, consider adding default values to the DTO properties that currently lack them:public string $footer = '';andpublic string $textFooter = '';.
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
Outdated
Show resolved
Hide resolved
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
Outdated
Show resolved
Hide resolved
| if (str_starts_with($storedMessageDatum->getData(), 'SER:')) { | ||
| $unserialized = unserialize(substr($storedMessageDatum->getData(), 4), ['allowed_classes' => false]); | ||
| array_walk_recursive($unserialized, function (&$val) { | ||
| $val = stripslashes($val); | ||
| }); | ||
|
|
||
| $data = $unserialized; |
There was a problem hiding this comment.
Add error handling for unserialize() failure.
unserialize() returns false on failure. Passing false to array_walk_recursive() would cause unexpected behavior or errors. Consider validating the result.
🔎 Proposed fix
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);
});
$data = $unserialized;🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/MessageDataLoader.php around lines 78 to 84, the
code calls unserialize() and immediately passes its result to
array_walk_recursive(), but unserialize() can return false on failure; validate
the result before using it. Update the block to capture the result of
unserialize into a variable, check that it is an array or object (not false),
and only call array_walk_recursive when valid; if unserialize fails, handle it
explicitly (e.g., log the error with context and message id, throw a descriptive
exception, or set $data to a safe default like an empty array) so subsequent
code won’t receive false and cause errors.
| $textContent = str_replace('[CONTENT]', $textMessage, $textTemplate); | ||
| $textContent = str_replace('[SUBJECT]', $subject, $textContent); | ||
| $textContent = str_replace('[FOOTER]', '', $textContent); | ||
| $phpListPowered = trim(($this->html2Text)($this->configProvider->getValue(ConfigOption::PoweredByText))); |
There was a problem hiding this comment.
Handle potential null from getValue().
ConfigProvider::getValue() can return null, but Html2Text::__invoke() expects a string. If PoweredByText is not configured, this could throw a TypeError.
- $phpListPowered = trim(($this->html2Text)($this->configProvider->getValue(ConfigOption::PoweredByText)));
+ $phpListPowered = trim(($this->html2Text)($this->configProvider->getValue(ConfigOption::PoweredByText) ?? ''));📝 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.
| $phpListPowered = trim(($this->html2Text)($this->configProvider->getValue(ConfigOption::PoweredByText))); | |
| $phpListPowered = trim(($this->html2Text)($this->configProvider->getValue(ConfigOption::PoweredByText) ?? '')); |
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/SystemMailConstructor.php around line 82, the
call passes the result of ConfigProvider::getValue() directly into
Html2Text::__invoke(), but getValue() can return null while Html2Text expects a
string; update the code to coalesce a safe default string (e.g. empty string) or
cast the value to string before invoking html2Text so null is never passed, then
apply trim to the result.
b1c80cd to
1643a29
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
src/Domain/Messaging/Service/SystemMailConstructor.php (2)
72-78: Use strict comparison withstrpos()(duplicate issue).This was already flagged in a previous review:
strpos()returns0if the needle is at the start of the string, which is falsy. Use!== falsefor the check on line 74.🔎 Proposed fix
if (str_contains($htmlContent, '[SIGNATURE]')) { $htmlContent = str_replace('[SIGNATURE]', $phpListPowered, $htmlContent); - } elseif (strpos($htmlContent, '</body>')) { + } elseif (strpos($htmlContent, '</body>') !== false) { $htmlContent = str_replace('</body>', $phpListPowered.'</body>', $htmlContent); } else { $htmlContent .= $phpListPowered; }
80-88: Handle potential null fromgetValue()(duplicate issue).This was already flagged in a previous review: On line 83,
ConfigProvider::getValue()can returnnull, butHtml2Text::__invoke()expects astring. Add null coalescing to prevent aTypeError.🔎 Proposed fix
$textContent = str_replace('[CONTENT]', $textMessage, $textTemplate); $textContent = str_replace('[SUBJECT]', $subject, $textContent); $textContent = str_replace('[FOOTER]', '', $textContent); - $phpListPowered = trim(($this->html2Text)($this->configProvider->getValue(ConfigOption::PoweredByText))); + $phpListPowered = trim(($this->html2Text)($this->configProvider->getValue(ConfigOption::PoweredByText) ?? '')); if (str_contains($textContent, '[SIGNATURE]')) { $textContent = str_replace('[SIGNATURE]', $phpListPowered, $textContent); } else { $textContent .= "\n\n" . $phpListPowered; }src/Domain/Messaging/Service/MessagePrecacheService.php (1)
47-52: Consider adding TTL to prevent stale cached DTOs.The cache stores the DTO indefinitely with no TTL. After deployments or data changes, stale precached content could persist. Adding an expiration or including a version suffix in the cache key would help.
🔎 Suggested approach
- $this->cache->set($cacheKey, $messagePrecacheDto); + $this->cache->set($cacheKey, $messagePrecacheDto, 3600); // 1 hour TTLAlso applies to: 189-191
src/Domain/Messaging/Service/TemplateImageEmbedder.php (2)
160-189: Domain layer persistence violation persists.These
$this->configManager->create(...)calls at lines 160, 169, and 180 are side effects that write configuration changes. Per coding guidelines, Domain services shouldn't perform persistence. Consider:
- Having this method return the discovered path alongside the image data
- Letting the Application layer caller persist the config value
231-234:mime_content_type()expects a filesystem path, not a bare filename.When
$nameis just"image.png"without a path,mime_content_type()can't read magic bytes and will return false or emit a warning. Fall back to the extension-based$mimeMaplookup instead.🔎 Suggested fix
//If a MIME type is not specified, try to work it out from the name if ('' === $type && !empty($name)) { - $type = mime_content_type($name); + $ext = strtolower(pathinfo($name, PATHINFO_EXTENSION)); + $type = $this->mimeMap[$ext] ?? 'application/octet-stream'; }src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
9-29: Several properties still lack default values.Properties like
replyToName,fromName,fromEmail,to,subject,content,footer,textFooter,htmlFormatted,sendFormat,userSpecificUrl, andgoogleTrackhave no defaults. Accessing them before assignment will throw aTypeError.While
MessagePrecacheServicemay always populate them, defensive defaults prevent accidents:🔎 Suggested defaults
public string $replyToEmail = ''; - public string $replyToName; - public string $fromName; - public string $fromEmail; - public string $to; - public string $subject; - public string $content; + public string $replyToName = ''; + public string $fromName = ''; + public string $fromEmail = ''; + public string $to = ''; + public string $subject = ''; + public string $content = ''; public string $textContent = ''; - public string $footer; - public string $textFooter; + public string $footer = ''; + public string $textFooter = ''; public string $htmlFooter = ''; - public bool $htmlFormatted; - public string $sendFormat; + public bool $htmlFormatted = false; + public string $sendFormat = ''; public ?string $template = null; public ?string $templateText = null; public ?int $templateId = null; public string $htmlCharset= 'UTF-8'; public string $textCharset= 'UTF-8'; - public bool $userSpecificUrl; - public string $googleTrack; + public bool $userSpecificUrl = false; + public string $googleTrack = ''; public array $adminAttributes = [];
🧹 Nitpick comments (8)
src/Domain/Messaging/Service/SystemMailConstructor.php (4)
25-26: Add type hints for better type safety.The method signature lacks parameter and return type hints, which reduces type safety and IDE support.
🔎 Suggested type hints
- public function __invoke($message, $subject = ''): array + public function __invoke(string $message, string $subject = ''): array
29-40: Remove commented code.Line 36 contains commented-out code that should be removed to keep the codebase clean.
Also,
stripslashes()on line 30 is typically used to handle magic quotes (deprecated since PHP 5.4). If magic quotes aren't a concern in your environment, this call can be removed.🔎 Cleanup suggestions
if ($hasHTML) { - $message = stripslashes($message); $textMessage = ($this->html2Text)($message); $htmlMessage = $message; } else { $textMessage = $message; $htmlMessage = $message; - // $htmlMessage = str_replace("\n\n","\n",$htmlMessage); $htmlMessage = nl2br($htmlMessage);
42-50: Consider using foreach for readability.The for loop with
count()can be simplified to a foreach loop for better readability.🔎 Refactor suggestion
if (preg_match('/<ul>\s+(\*.*)<\/ul>/imsxU', $htmlMessage, $listsmatch)) { $lists = $listsmatch[1]; $listsHTML = ''; preg_match_all('/\*([^\*]+)/', $lists, $matches); - for ($i = 0; $i < count($matches[0]); ++$i) { - $listsHTML .= '<li>'.$matches[1][$i].'</li>'; + foreach ($matches[1] as $item) { + $listsHTML .= '<li>' . $item . '</li>'; } $htmlMessage = str_replace($listsmatch[0], '<ul>'.$listsHTML.'</ul>', $htmlMessage); }
54-57: Validate template ID before casting.The cast to
(int)on line 56 is unsafe. IfgetValue()returns a non-numeric string (e.g.,"abc"), it will cast to0, potentially loading an unintended template or causing unexpected behavior.🔎 Safer template ID handling
$templateId = $this->configProvider->getValue(ConfigOption::SystemMessageTemplate); - if ($templateId) { - $template = $this->templateRepository->findOneById((int)$templateId); + if ($templateId && is_numeric($templateId)) { + $template = $this->templateRepository->findOneById((int)$templateId); if ($template) {src/Domain/Messaging/Service/TemplateImageEmbedder.php (1)
65-73: Consider usingcount()outside loop condition for minor perf gain.The
count($matchedImages[1])andcount($externalImages)are evaluated on each iteration. Stashing these in a variable beforehand is a minor optimization but keeps things tidy.Also applies to: 78-89
tests/Unit/Domain/Messaging/Service/RateLimitedCampaignMailerTest.php (1)
106-135: Consider removing unusedMessageContentconstruction from helper.The
buildMessagehelper still constructs a fullMessageContentobject (lines 113-118), but the test now usesMessagePrecacheDtofor content. Ifmessage.getContent()isn't accessed in production anymore, you could simplify this to pass minimal/null content.src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
24-28: Consider: Trailing comma in method signature.The method signature includes a trailing comma after the last parameter. While valid in PHP 8.0+, it's worth confirming this aligns with your project's style guidelines.
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
366-372: Consider replacingwithConsecutive()withwillReturnCallback()for PHPUnit 10+ compatibility.The
withConsecutive()method was deprecated in PHPUnit 9 and removed in PHPUnit 10. Since the project currently uses PHPUnit ^9.5, this isn't blocking today, but refactoring now avoids friction when upgrading. There is no 1:1 replacement, but the suggestedwillReturnCallback()pattern with a counter works cleanly as a modern alternative.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpsrc/Domain/Messaging/Service/MessagePrecacheService.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/Service/SystemMailConstructor.phpsrc/Domain/Messaging/Service/TemplateImageEmbedder.phpsrc/Domain/Subscription/Repository/UserBlacklistRepository.phptests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.phptests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.phptests/Unit/Domain/Messaging/Service/Manager/TemplateImageManagerTest.phptests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.phptests/Unit/Domain/Messaging/Service/RateLimitedCampaignMailerTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Domain/Subscription/Repository/UserBlacklistRepository.php
- tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php
🧰 Additional context used
📓 Path-based instructions (1)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/Service/SystemMailConstructor.phpsrc/Domain/Messaging/Service/TemplateImageEmbedder.phpsrc/Domain/Messaging/Service/MessagePrecacheService.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/Model/Dto/MessagePrecacheDto.php
🧬 Code graph analysis (6)
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (3)
src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
EmailBuilder(19-155)src/Domain/Messaging/Service/MessageDataLoader.php (1)
MessageDataLoader(13-186)src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(7-30)
tests/Unit/Domain/Messaging/Service/RateLimitedCampaignMailerTest.php (2)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(7-30)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
composeEmail(24-44)
src/Domain/Messaging/Service/TemplateImageEmbedder.php (4)
src/Domain/Common/ExternalImageService.php (3)
ExternalImageService(10-177)cache(40-92)getFromCache(24-38)src/Domain/Configuration/Service/Manager/ConfigManager.php (1)
ConfigManager(11-65)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/TemplateImageRepository.php (2)
TemplateImageRepository(12-46)findByTemplateIdAndFilename(36-45)
tests/Unit/Domain/Messaging/Service/Manager/TemplateImageManagerTest.php (2)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/TemplateImageRepository.php (1)
TemplateImageRepository(12-46)
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (2)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(7-30)src/Domain/Messaging/Message/AsyncEmailMessage.php (1)
getReplyTo(49-52)
tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php (2)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(7-30)src/Domain/Messaging/Service/MessageProcessingPreparator.php (2)
processMessageLinks(61-85)MessageProcessingPreparator(15-97)
🔇 Additional comments (16)
src/Domain/Messaging/Service/SystemMailConstructor.php (2)
15-23: Constructor looks solid.Clean dependency injection with readonly properties. The boolean flag is fine given the suppression annotation.
92-92: Return statement is correct.Returns a tuple of HTML and text content as expected. Would benefit from the return type hint suggested earlier.
tests/Unit/Domain/Messaging/Service/Manager/TemplateImageManagerTest.php (2)
17-29: LGTM! Clean dependency update.The test setup correctly reflects the production change from
EntityManagerInterfacetoConfigProviderand shifts persistence expectations to the repository. Mocks are properly wired.
32-46: Test properly validates repository-based persistence.The expectation on
templateImageRepository->persistwithexactly(2)calls aligns with the two image paths being processed. Good coverage.src/Domain/Messaging/Service/MessagePrecacheService.php (2)
79-90: DTO population looks solid.The reply-to parsing, from/to fields, and forward content handling are cleanly structured. Good use of conditional forwarding logic.
142-151: Nice placeholder replacement loop.Clean iteration over the common placeholders with conditional skipping for user-specific URLs. The null-coalescing on
textContent(initialized to''in DTO) keeps things safe.tests/Unit/Domain/Messaging/Service/RateLimitedCampaignMailerTest.php (2)
50-55: Clean test update for DTO-based email composition.The precached DTO is properly populated with subject, textContent, and content fields matching the assertion values. Good alignment with the production API change.
79-84: Second test case also properly updated.Consistent pattern with the first test - DTO populated and passed to
composeEmail. Assertions verify the email headers and body correctly.tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php (3)
119-136: LGTM! DTO-based flow correctly tested.The test properly verifies the early return path when link tracking is not applicable, ensuring the same DTO instance is returned unmodified.
138-158: LGTM! Empty links path correctly tested.The test verifies that when no links are extracted, the original DTO is returned unchanged. Good coverage of the edge case.
160-192: LGTM! Link replacement logic thoroughly verified.The test correctly validates that:
- Links in both
contentandhtmlFooterare replaced with tracking endpoints- The same DTO object is returned (verified with
assertSame)- Tracking IDs are properly embedded in the URLs
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (5)
53-103: LGTM! Test setup correctly reflects DTO-based architecture.The test setup properly initializes all new dependencies:
UserPersonalizermock returns original text (preserving link replacements for assertions)CacheInterfaceandMessageDataLoadermocks addedMailerInterfaceseparated fromRateLimitedCampaignMailer- All dependencies correctly wired into the handler
123-154: LGTM! Precaching integration verified.The test correctly verifies that:
precacheMessageis called before subscriber processing- No emails are sent when there are no subscribers
- Status updates and flushes occur as expected
156-194: LGTM! Invalid email handling verified.The test confirms that subscribers with invalid emails are skipped without processing or sending. Good edge case coverage.
196-265: LGTM! Complete DTO-based send flow verified.The test validates the full flow:
- Message is precached to DTO
- DTO is retrieved from cache
- Links are processed per-subscriber
- Email is composed from DTO
- Email is sent via Symfony Mailer
267-324: LGTM! Exception handling properly tested.The test verifies that mailer exceptions are logged with appropriate context and don't halt the handler execution.
| $messagePrecacheDto->content = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->content); | ||
| $messagePrecacheDto->template = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->template); | ||
| $messagePrecacheDto->htmlFooter = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->htmlFooter); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find TemplateImageManager class
fd -type f -name "*.php" | xargs grep -l "class TemplateImageManager"Repository: phpList/core
Length of output: 228
🏁 Script executed:
#!/bin/bash
# Search for parseLogoPlaceholders method
rg "parseLogoPlaceholders" -A 10 -B 2 --type phpRepository: phpList/core
Length of output: 4522
🏁 Script executed:
#!/bin/bash
# Find the MessagePrecacheService file and examine lines around 126 and 180-182
fd "MessagePrecacheService.php" --type f | head -1 | xargs cat -n | grep -A 5 -B 5 "126\|180"Repository: phpList/core
Length of output: 1559
🏁 Script executed:
#!/bin/bash
# Also find TemplateImageManager file
fd "TemplateImageManager.php" --type f | head -1 | xargs cat -nRepository: phpList/core
Length of output: 8049
Add null-safety to parseLogoPlaceholders to handle nullable template fields.
The method signature at TemplateImageManager::parseLogoPlaceholders requires a string parameter, but $messagePrecacheDto->template can be null (set explicitly at line 126). With declare(strict_types=1) enabled, passing null to this method will throw a TypeError. Either add an early return for null input in parseLogoPlaceholders, or add null checks before each call at lines 180–182.
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/MessagePrecacheService.php around lines 180–182,
calls pass potentially null fields into
TemplateImageManager::parseLogoPlaceholders which expects a string; this will
TypeError under strict types. Fix by making parseLogoPlaceholders null-safe
(accept ?string and return null when input is null) or add null-guards here
(only call parseLogoPlaceholders when the DTO property is !== null and otherwise
leave it null); update types/signature and callers consistently if you choose
the former.
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.
| if ($this->externalImageMaxSize && (strlen($cacheFileContent) > $this->externalImageMaxSize)) { | ||
| $cacheFileContent = 'MAX_SIZE'; | ||
| } |
There was a problem hiding this comment.
Avoid writing error markers to cache files.
When content exceeds externalImageMaxSize, the code writes the string 'MAX_SIZE' to the cache file. This error marker will later pass validation (it's > 64 bytes) and be returned as base64-encoded "content" in getFromCache, causing downstream failures. Either skip caching entirely for oversized images or return early without writing a file.
🔎 Proposed 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);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Domain/Common/ExternalImageService.php around lines 68-70, the code
currently writes the literal 'MAX_SIZE' into the cache when an image exceeds
externalImageMaxSize; instead, do not write any error marker to cache and return
early (or skip caching) for oversized images. Change the flow so that when
strlen($cacheFileContent) > $this->externalImageMaxSize you exit the caching
path (e.g., return null/false or continue without creating/writing the cache
file) and ensure downstream getFromCache cannot return a fabricated 'MAX_SIZE'
payload.
| 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.
| 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'; | ||
| } | ||
|
|
||
| return $cacheFileContent; | ||
| } |
There was a problem hiding this comment.
Return null instead of 'FGC_ERROR' string.
Similar to the cURL method, writing 'FGC_ERROR' into the cache file will corrupt the cache. Return null or throw an exception on failure.
🔎 Proposed fix
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;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private function 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'; | |
| } | |
| return $cacheFileContent; | |
| } | |
| 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) { | |
| $this->logger->error('file_get_contents error downloading image', ['url' => $filename]); | |
| return null; | |
| } | |
| return $cacheFileContent; | |
| } |
🤖 Prompt for AI Agents
In src/Domain/Common/ExternalImageService.php around lines 143-159, the
downloadUsingFileGetContent method currently returns the literal 'FGC_ERROR' on
failure which corrupts the cache; change the method to return null on failure
instead by updating its return type to ?string (or adjust docblock) and replace
the 'FGC_ERROR' assignment with return null; then update any callers to handle a
null result (skip writing to cache / treat as download failure) or
propagate/throw an appropriate exception.
| private function writeCacheFile(string $cacheFileName, $content): void | ||
| { | ||
| try { | ||
| $handle = fopen($cacheFileName, 'wb'); | ||
|
|
||
| if ($handle === false) { | ||
| $this->logger->error('Cannot open cache file', [ | ||
| 'file' => $cacheFileName, | ||
| ]); | ||
| return; | ||
| } | ||
|
|
||
| if (flock($handle, LOCK_EX)) { | ||
| fwrite($handle, $content); | ||
| fflush($handle); | ||
| flock($handle, LOCK_UN); | ||
| } | ||
|
|
||
| fclose($handle); | ||
| } catch (Throwable $e) { | ||
| $this->logger->error('Cache file write failed', [ | ||
| 'file' => $cacheFileName, | ||
| 'error' => $e->getMessage(), | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Check fwrite return value for partial writes.
Line 210 calls fwrite but doesn't verify all bytes were written. Partial writes (e.g., disk full) will silently corrupt the cache. Check the return value against strlen($content) and log failures.
🔎 Proposed fix
if (flock($handle, LOCK_EX)) {
- fwrite($handle, $content);
+ $written = fwrite($handle, $content);
+ if ($written === false || $written !== strlen($content)) {
+ $this->logger->error('Incomplete cache write', [
+ 'file' => $cacheFileName,
+ 'expected' => strlen($content),
+ 'written' => $written,
+ ]);
+ }
fflush($handle);
flock($handle, LOCK_UN);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private function writeCacheFile(string $cacheFileName, $content): void | |
| { | |
| try { | |
| $handle = fopen($cacheFileName, 'wb'); | |
| if ($handle === false) { | |
| $this->logger->error('Cannot open cache file', [ | |
| 'file' => $cacheFileName, | |
| ]); | |
| return; | |
| } | |
| if (flock($handle, LOCK_EX)) { | |
| fwrite($handle, $content); | |
| fflush($handle); | |
| flock($handle, LOCK_UN); | |
| } | |
| fclose($handle); | |
| } catch (Throwable $e) { | |
| $this->logger->error('Cache file write failed', [ | |
| 'file' => $cacheFileName, | |
| 'error' => $e->getMessage(), | |
| ]); | |
| } | |
| } | |
| private function writeCacheFile(string $cacheFileName, $content): void | |
| { | |
| try { | |
| $handle = fopen($cacheFileName, 'wb'); | |
| if ($handle === false) { | |
| $this->logger->error('Cannot open cache file', [ | |
| 'file' => $cacheFileName, | |
| ]); | |
| return; | |
| } | |
| if (flock($handle, LOCK_EX)) { | |
| $written = fwrite($handle, $content); | |
| if ($written === false || $written !== strlen($content)) { | |
| $this->logger->error('Incomplete cache write', [ | |
| 'file' => $cacheFileName, | |
| 'expected' => strlen($content), | |
| 'written' => $written, | |
| ]); | |
| } | |
| fflush($handle); | |
| flock($handle, LOCK_UN); | |
| } | |
| fclose($handle); | |
| } catch (Throwable $e) { | |
| $this->logger->error('Cache file write failed', [ | |
| 'file' => $cacheFileName, | |
| 'error' => $e->getMessage(), | |
| ]); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Domain/Common/ExternalImageService.php around lines 197 to 222, the call
to fwrite does not verify that all bytes were written, so partial writes (e.g.
disk full) can corrupt the cache; change the write logic to check fwrite's
return value against strlen($content) and handle partial writes by looping until
all bytes are written (or an error/zero-bytes is returned), and if a full write
cannot be completed log an error with the file name, expected byte count and
actual bytes written, ensure fflush and fclose still run, and consider deleting
the incomplete cache file on failure.
| public string $replyToName; | ||
| public string $fromName; | ||
| public string $fromEmail; | ||
| public string $to; | ||
| public string $subject; | ||
| public string $content; | ||
| public string $textContent = ''; | ||
| public string $footer; | ||
| public string $textFooter; | ||
| public string $htmlFooter = ''; | ||
| public bool $htmlFormatted; | ||
| public string $sendFormat; |
There was a problem hiding this comment.
Initialize all non-nullable typed properties to prevent fatal errors.
Several non-nullable properties (replyToName, fromName, fromEmail, to, subject, content, footer, textFooter, htmlFormatted, sendFormat, userSpecificUrl, googleTrack) lack default values. Accessing an uninitialized typed property in PHP 7.4+ throws a fatal error. Since this DTO flows through precache and processing pipelines, any missed assignment will crash at runtime.
🔎 Suggested fix: initialize all properties
- public string $replyToName;
+ public string $replyToName = '';
- public string $fromName;
+ public string $fromName = '';
- public string $fromEmail;
+ public string $fromEmail = '';
- public string $to;
+ public string $to = '';
- public string $subject;
+ public string $subject = '';
- public string $content;
+ public string $content = '';
- public string $footer;
+ public string $footer = '';
- public string $textFooter;
+ public string $textFooter = '';
- public bool $htmlFormatted;
+ public bool $htmlFormatted = false;
- public string $sendFormat;
+ public string $sendFormat = '';
- public bool $userSpecificUrl;
+ public bool $userSpecificUrl = false;
- public string $googleTrack;
+ public string $googleTrack = '';Also applies to: 28-29
🤖 Prompt for AI Agents
In src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php around lines 11 to 22
(and likewise apply the same for lines 28-29), several non-nullable typed
properties are not initialized which will cause fatal errors if accessed before
assignment; initialize each non-nullable property with a safe default (e.g.,
empty string for string properties, false for booleans) so the DTO can be
constructed and passed through pipelines without throwing uninitialized property
errors.
| private function validateRecipientAndSubject(?string $to, ?string $subject): bool | ||
| { | ||
| if (preg_match("/\n/", $to)) { | ||
| $this->eventLogManager->log('', 'Error: invalid recipient, containing newlines, email blocked'); | ||
|
|
||
| return false; | ||
| } | ||
| if (preg_match("/\n/", $subject)) { | ||
| $this->eventLogManager->log('', 'Error: invalid subject, containing newlines, email blocked'); | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Add null safety for preg_match on potentially null strings.
$to and $subject can be null, but preg_match() expects a string. This will cause a TypeError in PHP 8+.
🔎 Proposed fix
private function validateRecipientAndSubject(?string $to, ?string $subject): bool
{
- if (preg_match("/\n/", $to)) {
+ if ($to !== null && preg_match("/\n/", $to)) {
$this->eventLogManager->log('', 'Error: invalid recipient, containing newlines, email blocked');
return false;
}
- if (preg_match("/\n/", $subject)) {
+ if ($subject !== null && preg_match("/\n/", $subject)) {
$this->eventLogManager->log('', 'Error: invalid subject, containing newlines, email blocked');
return false;
}🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Builder/EmailBuilder.php around lines 78–89, the
code calls preg_match() on $to and $subject which may be null under PHP 8+,
causing a TypeError; update the checks to ensure null-safety by testing for
non-null (or is_string) before calling preg_match (e.g. only call preg_match
when $to !== null and when $subject !== null), leaving the existing
logging/return behavior unchanged.
| private function decodeLogoImageData(?string $logoData): ?string | ||
| { | ||
| $imageContent = base64_decode($logoData ?? '', true); | ||
|
|
||
| if (!empty($imageContent)) { | ||
| return $imageContent; | ||
| } | ||
|
|
||
| $fallback = base64_decode('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAABGdBTUEAALGPC/xhBQAAAAZQTFRF////AAAAVcLTfgAAAAF0Uk5TAEDm2GYAAAABYktHRACIBR1IAAAACXBIWXMAAAsSAAALEgHS3X78AAAAB3RJTUUH0gQCEx05cqKA8gAAAApJREFUeJxjYAAAAAIAAUivpHEAAAAASUVORK5CYII='); | ||
|
|
||
| $fallbackContent = base64_decode($fallback, true); | ||
|
|
||
| return $fallbackContent !== false ? $fallbackContent : null; | ||
| } |
There was a problem hiding this comment.
Double base64 decode on the fallback — unnecessary operation.
Line 170 decodes a base64 string, then line 172 decodes it again. The fallback is already a valid base64 string — you only need to decode once.
🔎 Proposed fix
- $fallback = base64_decode('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAABGdBTUEAALGPC/xhBQAAAAZQTFRF////AAAAVcLTfgAAAAF0Uk5TAEDm2GYAAAABYktHRACIBR1IAAAACXBIWXMAAAsSAAALEgHS3X78AAAAB3RJTUUH0gQCEx05cqKA8gAAAApJREFUeJxjYAAAAAIAAUivpHEAAAAASUVORK5CYII=');
-
- $fallbackContent = base64_decode($fallback, true);
+ $fallbackContent = base64_decode('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAABGdBTUEAALGPC/xhBQAAAAZQTFRF////AAAAVcLTfgAAAAF0Uk5TAEDm2GYAAAABYktHRACIBR1IAAAACXBIWXMAAAsSAAALEgHS3X78AAAAB3RJTUUH0gQCEx05cqKA8gAAAApJREFUeJxjYAAAAAIAAUivpHEAAAAASUVORK5CYII=', true);🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Manager/TemplateImageManager.php around lines
162 to 175, the code double-decodes the fallback base64 string (base64_decode
called twice); replace that by decoding the literal fallback base64 string only
once and use that result (e.g., remove the intermediate $fallback variable and
call base64_decode on the literal with strict=true), then return the decoded
result if it is not false or null otherwise.
| private function getDateArray(?int $timestamp = null): array | ||
| { | ||
| return [ | ||
| 'year' => date('Y', $timestamp), | ||
| 'month' => date('m', $timestamp), | ||
| 'day' => date('d', $timestamp), | ||
| 'hour' => date('H', $timestamp), | ||
| 'minute' => date('i', $timestamp), | ||
| ]; |
There was a problem hiding this comment.
date() with null timestamp uses current time but issues a deprecation warning in PHP 8.1+.
When $timestamp is null, date() expects an int. In PHP 8.1+, passing null triggers a deprecation warning. Use $timestamp ?? time().
🔎 Proposed fix
private function getDateArray(?int $timestamp = null): array
{
+ $ts = $timestamp ?? time();
return [
- 'year' => date('Y', $timestamp),
- 'month' => date('m', $timestamp),
- 'day' => date('d', $timestamp),
- 'hour' => date('H', $timestamp),
- 'minute' => date('i', $timestamp),
+ 'year' => date('Y', $ts),
+ 'month' => date('m', $ts),
+ 'day' => date('d', $ts),
+ 'hour' => date('H', $ts),
+ 'minute' => date('i', $ts),
];
}🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/MessageDataLoader.php around lines 45 to 53, the
getDateArray method calls date() with a nullable $timestamp which triggers a
deprecation warning in PHP 8.1+ when null; ensure you default the timestamp to
the current time before calling date (e.g. $ts = $timestamp ?? time()) and use
$ts for all date() calls so date() always receives an int, updating all five
date() invocations accordingly.
| while (strpos($messageData['fromname'], ' ')) { | ||
| $messageData['fromname'] = str_replace(' ', ' ', $messageData['fromname']); | ||
| } |
There was a problem hiding this comment.
Use strict comparison with strpos() in while loop.
Using strpos() in a boolean context fails when the substring is at position 0. Use str_contains() or strict comparison.
🔎 Proposed fix
- while (strpos($messageData['fromname'], ' ')) {
+ while (str_contains($messageData['fromname'], ' ')) {
$messageData['fromname'] = str_replace(' ', ' ', $messageData['fromname']);
}📝 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.
| while (strpos($messageData['fromname'], ' ')) { | |
| $messageData['fromname'] = str_replace(' ', ' ', $messageData['fromname']); | |
| } | |
| while (str_contains($messageData['fromname'], ' ')) { | |
| $messageData['fromname'] = str_replace(' ', ' ', $messageData['fromname']); | |
| } |
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/MessageDataLoader.php around lines 192 to 194,
the while loop uses strpos($messageData['fromname'], ' ') in a boolean context
which breaks when the match is at position 0; change the condition to a strict
comparison (strpos(...) !== false) or use str_contains($messageData['fromname'],
' ') if available, so the loop reliably detects double-spaces at any position
and continues replacing them until none remain.
f2db3f5 to
4a9e895
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (22)
src/Domain/Common/HtmlUrlRewriter.php (1)
63-72: Potential null pointer if wrapper element is not found.If
getElementById('__wrap__')returnsnull(e.g., malformed HTML causes DOM issues), the subsequent$wrap->childNodesaccess will throw. Consider adding a null check with a fallback.🔎 Proposed fix
// Extract the original fragment back out $wrap = $dom->getElementById('__wrap__'); + if ($wrap === null) { + libxml_clear_errors(); + return $html; // fallback to original + } $out = ''; foreach ($wrap->childNodes as $child) { $out .= $dom->saveHTML($child); }src/Domain/Common/ExternalImageService.php (5)
68-70: Avoid writing error markers to cache files.When content exceeds
externalImageMaxSize, writing'MAX_SIZE'to the cache file will later pass validation (it's > 64 bytes) and be returned as base64-encoded "content", causing downstream failures. Return early without writing.
108-115: Strengthen filename sanitization against path traversal.The regex only removes consecutive dots, but
../patterns could still sneak through. Usebasename()or parse the URL path first to ensure filenames don't escape the cache directory.
138-143: Return null instead of error-marker strings.Writing
'CURL_ERROR_*'or'HTTP_CODE_*'into the cache file will corrupt the cache. Returnnullon error instead.
159-162: Return null instead of 'FGC_ERROR' string.Similar to the cURL method, writing
'FGC_ERROR'into the cache file will corrupt the cache. Returnnullon failure.
215-218: Check fwrite return value for partial writes.
fwritedoesn't guarantee all bytes are written. Check the return value againststrlen($content)to detect disk-full or other partial write scenarios.src/Domain/Messaging/Service/Manager/TemplateImageManager.php (3)
170-177: Double base64 decode on the fallback — unnecessary operation.Line 170 decodes a base64 string, then line 174 decodes it again. The fallback is already a valid base64 string — you only need to decode once.
🔎 Proposed fix
private function decodeLogoImageData(?string $logoData): ?string { $imageContent = base64_decode($logoData ?? '', true); if (!empty($imageContent)) { return $imageContent; } - $fallback = base64_decode('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAABGdBTUEAALGPC/' - . 'xhBQAAAAZQTFRF////AAAAVcLTfgAAAAF0Uk5TAEDm2GYAAAABYktHRACIBR1IAAAACXBIWXMAAAsSAAALEgHS3X78' - . 'AAAAB3RJTUUH0gQCEx05cqKA8gAAAApJREFUeJxjYAAAAAIAAUivpHEAAAAASUVORK5CYII='); - - $fallbackContent = base64_decode($fallback, true); + $fallbackContent = base64_decode( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAABGdBTUEAALGPC/' + . 'xhBQAAAAZQTFRF////AAAAVcLTfgAAAAF0Uk5TAEDm2GYAAAABYktHRACIBR1IAAAACXBIWXMAAAsSAAALEgHS3X78' + . 'AAAAB3RJTUUH0gQCEx05cqKA8gAAAApJREFUeJxjYAAAAAIAAUivpHEAAAAASUVORK5CYII=', + true + ); return $fallbackContent !== false ? $fallbackContent : null; }
231-239: Output buffer handling is fragile and leaky.The buffer capture sequence 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.🔎 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();
159-160: Domain layer performs persistence operations.Lines 159 and 228 call
templateImageRepository->persist()and->remove(). Per coding guidelines, domain services should not invoke persistence APIs on regular entities. Consider moving these side effects to an Application layer service.Also applies to: 227-229
src/Domain/Messaging/Service/MessageDataLoader.php (3)
46-55:date()with null timestamp issues deprecation warning in PHP 8.1+.When
$timestampis null,date()expects an int. Use$timestamp ?? time()to avoid the warning.🔎 Proposed fix
private function getDateArray(?int $timestamp = null): array { + $ts = $timestamp ?? time(); return [ - 'year' => date('Y', $timestamp), - 'month' => date('m', $timestamp), - 'day' => date('d', $timestamp), - 'hour' => date('H', $timestamp), - 'minute' => date('i', $timestamp), + 'year' => date('Y', $ts), + 'month' => date('m', $ts), + 'day' => date('d', $ts), + 'hour' => date('H', $ts), + 'minute' => date('i', $ts), ]; }
114-120: Add error handling forunserialize()failure.
unserialize()returnsfalseon failure. Passingfalsetoarray_walk_recursive()would cause unexpected behavior. Consider validating the result.🔎 Proposed fix
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); });
192-195: Use strict comparison withstrpos()in while loop.Using
strpos()in a boolean context fails when the substring is at position 0. Usestr_contains()for consistency with line 174.🔎 Proposed fix
- while (strpos($messageData['fromname'], ' ')) { + while (str_contains($messageData['fromname'], ' ')) { $messageData['fromname'] = str_replace(' ', ' ', $messageData['fromname']); }src/Domain/Messaging/Service/MessagePrecacheService.php (2)
93-97: Add null-safety toparseLogoPlaceholderscalls.
$messagePrecacheDto->templatecan benull(set explicitly at line 192 when remote content is used). With strict types, passingnulltoparseLogoPlaceholders(which expectsstring) will throw a TypeError.🔎 Proposed fix
- $messagePrecacheDto->content = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->content); - $messagePrecacheDto->template = $this->templateImageManager - ->parseLogoPlaceholders($messagePrecacheDto->template); - $messagePrecacheDto->htmlFooter = $this->templateImageManager - ->parseLogoPlaceholders($messagePrecacheDto->htmlFooter); + $messagePrecacheDto->content = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->content); + if ($messagePrecacheDto->template !== null) { + $messagePrecacheDto->template = $this->templateImageManager + ->parseLogoPlaceholders($messagePrecacheDto->template); + } + $messagePrecacheDto->htmlFooter = $this->templateImageManager + ->parseLogoPlaceholders($messagePrecacheDto->htmlFooter);
51-54: Consider adding TTL and cache key versioning.The cache stores the DTO object directly with no TTL, so it persists indefinitely. Adding an expiration time to
cache->set()and/or including a version suffix in the cache key would prevent stale data after code changes.Also applies to: 99-99
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (3)
113-113: Verify cache key consistency with precache service.Ensure this cache key format matches what
MessagePrecacheService::precacheMessage()uses. The past review suggested the precache service includes aforwardContentparameter in the key.#!/bin/bash # Check the cache key format used by MessagePrecacheService ast-grep --pattern $'function precacheMessage($$$) { $$$ sprintf($$$) $$$ }' # Also search for cache key patterns rg -nP "messaging\.message\.base\.\%d" --type php -C 3
209-213: Fix type mismatch: nullable properties passed to non-nullable parameter.The
textContentandfooterproperties are nullable, butUserPersonalizer::personalize()expects a non-nullablestring. Use the null-coalescing operator to provide a safe fallback.🔎 Proposed fix
- $processed->textContent = $this->userPersonalizer->personalize( - $processed->textContent, + $processed->textContent = $this->userPersonalizer->personalize( + $processed->textContent ?? '', $subscriber->getEmail(), ); - $processed->footer = $this->userPersonalizer->personalize($processed->footer, $subscriber->getEmail()); + $processed->footer = $this->userPersonalizer->personalize($processed->footer ?? '', $subscriber->getEmail());
300-302: Fix Symfony Translator placeholder syntax.Symfony Translator uses
%placeholder%syntax with an associative array, not printf-style%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']] ),src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
78-89: Critical: Add null checks beforepreg_match()to prevent TypeError.Lines 80 and 85 call
preg_match()on$toand$subjectwhich are nullable, but PHP 8+ requires non-null strings. The null checks at lines 90-99 happen too late—move them before thepreg_match()calls.🔎 Proposed fix
private function validateRecipientAndSubject(?string $to, ?string $subject): bool { + if (!$to) { + $this->eventLogManager->log('', sprintf('Error: empty To: in message with subject %s to send', $subject)); + return false; + } + if (!$subject) { + $this->eventLogManager->log('', sprintf('Error: empty Subject: in message to send to %s', $to)); + return false; + } + if (preg_match("/\n/", $to)) { $this->eventLogManager->log('', 'Error: invalid recipient, containing newlines, email blocked'); - return false; } if (preg_match("/\n/", $subject)) { $this->eventLogManager->log('', 'Error: invalid subject, containing newlines, email blocked'); - return false; } - if (!$to) { - $this->eventLogManager->log('', sprintf('Error: empty To: in message with subject %s to send', $subject)); - - return false; - } - if (!$subject) { - $this->eventLogManager->log('', sprintf('Error: empty Subject: in message to send to %s', $to)); - - return false; - } return true; }
188-206: Critical: Capture embedder return value and eliminate redundant text assignments.Line 199 discards the return value from
templateImageEmbedder, which contains HTML with CID references for embedded images. The original HTML (without CIDs) is used instead, so embedded images won't display. Also, text is set three times (lines 198, 203, 205)—the last two are redundant.🔎 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); + $htmlMessage = ($this->templateImageEmbedder)(html: $htmlMessage, messageId: $messageId); + $email->html($htmlMessage); //# 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); }Note: You'll also need to attach the embedder's images to the email. Check the embedder's
$attachmentproperty and add those to the Email object.src/Domain/Messaging/Service/TemplateImageEmbedder.php (3)
132-145: Critical: Remove persistence side effect from Domain service.Lines 135-140 call
$this->configManager->create(), which persists configuration. Per coding guidelines, Domain services must not have persistence side effects. Compute the config value here but return it to the caller (Application/MessageHandler layer) for persistence.As per coding guidelines, Domain layer should not perform persistence operations.
🔎 Recommended approach
Return a tuple/DTO containing both the image data and the computed config value:
// In Domain service return ['imageData' => $data, 'suggestedUploadRoot' => $candidate['config']]; // In Application layer (MessageHandler/Controller) if ($result['suggestedUploadRoot'] !== null) { $this->configManager->create( ConfigOption::UploadImageRoot->value, $result['suggestedUploadRoot'], false, 'string' ); }
169-172: Fix MIME type detection:mime_content_type()requires a file path.
mime_content_type($name)at line 171 expects a filesystem path, not a bare filename like"image.png". Use the extension from$namewith your$mimeMapinstead.🔎 Proposed fix
//If a MIME type is not specified, try to work it out from the name if ('' === $type && !empty($name)) { - $type = mime_content_type($name); + $ext = strtolower(pathinfo($name, PATHINFO_EXTENSION)); + $type = $this->mimeMap[$ext] ?? 'application/octet-stream'; }
193-194: Fix copy-paste bug:$pageRootreads wrong config key.Line 194 reads
ConfigOption::UploadImageRootfor$pageRoot, but it should readConfigOption::PageRoot. Both variables are currently set to the same value.🔎 Proposed fix
private function filesystemImageExists($filename): bool { //# find the image referenced and see if it's on the server $imageRoot = $this->configProvider->getValue(ConfigOption::UploadImageRoot); - $pageRoot = $this->configProvider->getValue(ConfigOption::UploadImageRoot); + $pageRoot = $this->configProvider->getValue(ConfigOption::PageRoot);
🧹 Nitpick comments (3)
src/Domain/Identity/Repository/AdminAttributeDefinitionRepository.php (2)
22-36: Looks solid!The query is clean, parameterized, and read-only—no persistence side effects, so it's compliant with domain guidelines. The LEFT JOIN with COALESCE ensures you get consistent results even when an admin has no attribute values set.
If you know the actual type of
aav.valuein your schema (e.g.,string), you could tighten the return-type annotation frommixedto that specific type, but it's fine as-is.
38-44: Add a more specific return-type annotation.The method is functionally correct and free of persistence side effects. However, it returns the same shape as
getForAdmin(array{value: mixed, name: string}) but uses a barearraytype. Consider matching the specificity ofgetForAdminfor consistency and better type safety.🔎 Suggested return-type annotation
+ /** @return array<int, array{value: string, name: string}> */ public function getAllWithEmptyValues(): arraySince this method always sets
valueto an empty string, you can even usestringinstead ofmixed.src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
91-100: Consider case-insensitive URL matching in replaceLinks.The
str_replaceon line 96 is case-sensitive. If the original URL in the HTML differs in case from what was extracted, the replacement won't match. Consider usingstr_ireplaceor normalizing URLs during extraction.🔎 Optional enhancement
private function replaceLinks(array $savedLinks, string $htmlText): string { foreach ($savedLinks as $linkTrack) { $originalUrl = $linkTrack->getUrl(); $trackUrl = self::LINK_TRACK_ENDPOINT . '?id=' . $linkTrack->getId(); - $htmlText = str_replace('href="' . $originalUrl . '"', 'href="' . $trackUrl . '"', $htmlText); + $htmlText = preg_replace( + '/href=["\']' . preg_quote($originalUrl, '/') . '["\']/i', + 'href="' . $trackUrl . '"', + $htmlText + ); } return $htmlText; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
config/PhpCodeSniffer/ruleset.xmlsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Common/Html2Text.phpsrc/Domain/Common/HtmlUrlRewriter.phpsrc/Domain/Common/TextParser.phpsrc/Domain/Identity/Repository/AdminAttributeDefinitionRepository.phpsrc/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.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/MessagePrecacheService.phpsrc/Domain/Messaging/Service/MessageProcessingPreparator.phpsrc/Domain/Messaging/Service/SystemMailConstructor.phpsrc/Domain/Messaging/Service/TemplateImageEmbedder.phpsrc/Domain/Subscription/Repository/UserBlacklistRepository.phptests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.phptests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Domain/Common/TextParser.php
- src/Domain/Common/Html2Text.php
- tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php
- src/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.php
- src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php
- src/Domain/Messaging/Service/SystemMailConstructor.php
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Subscription/Repository/UserBlacklistRepository.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Service/MessageProcessingPreparator.phpsrc/Domain/Common/ExternalImageService.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Messaging/Service/MessagePrecacheService.phpsrc/Domain/Identity/Repository/AdminAttributeDefinitionRepository.phpsrc/Domain/Common/HtmlUrlRewriter.phpsrc/Domain/Messaging/Service/TemplateImageEmbedder.phpsrc/Domain/Messaging/Service/MessageDataLoader.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (7)
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-103)src/Domain/Messaging/Service/TemplateImageEmbedder.php (1)
TemplateImageEmbedder(16-307)src/Domain/Subscription/Repository/SubscriberRepository.php (1)
SubscriberRepository(21-208)src/Domain/Subscription/Repository/UserBlacklistRepository.php (3)
UserBlacklistRepository(14-60)isEmailBlacklisted(45-59)findOneByEmail(38-43)src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php (2)
SubscriberHistoryManager(18-143)addHistory(45-56)
src/Domain/Messaging/Service/MessageProcessingPreparator.php (2)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Analytics/Service/LinkTrackService.php (2)
isExtractAndSaveLinksApplicable(30-33)extractAndSaveLinks(41-77)
src/Domain/Common/ExternalImageService.php (2)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Service/TemplateImageEmbedder.php (1)
__construct(33-44)
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-89)MessageProcessingPreparator(15-101)
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/TemplateImageEmbedder.php (4)
src/Domain/Common/ExternalImageService.php (4)
ExternalImageService(12-229)__construct(16-25)cache(43-76)getFromCache(27-41)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/TemplateImageRepository.php (2)
TemplateImageRepository(12-46)findByTemplateIdAndFilename(36-45)src/Domain/Messaging/Service/Manager/TemplateImageManager.php (1)
__construct(28-32)
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)
🔇 Additional comments (9)
src/Domain/Identity/Repository/AdminAttributeDefinitionRepository.php (1)
11-11: LGTM!The import is appropriate for the new
getForAdminmethod.src/Domain/Subscription/Repository/UserBlacklistRepository.php (1)
45-59: Looks good, clean implementation of blacklist checking.The grace period logic and query are well structured. One small observation: the method returns
trueif a blacklist entry exists that was added before the cutoff time. This seems intentional to allow a grace period for recently blacklisted emails to still receive acknowledgment messages.src/Domain/Common/HtmlUrlRewriter.php (1)
84-107: Nice job on the URL absolutization logic.The handling of protocol-relative URLs, anchors, placeholders, and special schemes (mailto, data, javascript) is thorough. The path normalization with
..handling is correctly implemented.tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php (1)
160-201: Tests look solid for the DTO-based refactor.Good coverage of the link extraction and replacement flow. The assertions correctly verify that links are replaced in both
contentandhtmlFooterproperties of the DTO.src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
61-89: Clean refactor to DTO-based processing.The switch from
MessageContenttoMessagePrecacheDtois well implemented. The link replacement now correctly targetscontentandhtmlFooterproperties.src/Domain/Messaging/Service/MessageDataLoader.php (1)
28-44: Clean design for the message data loading workflow.The
__invokepattern provides a nice callable interface, and the step-by-step normalization pipeline is easy to follow. Good separation of concerns with the private helper methods.src/Domain/Messaging/Service/MessagePrecacheService.php (3)
26-41: Solid constructor setup for the precache workflow.The dependency injection is well organized with all the collaborators needed for the DTO-based precaching pipeline. The configuration flags (
useManualTextPart,uploadImageDir,publicSchema) are appropriately constructor-injected.
109-132: Good handling of reply-to field parsing.The logic correctly handles email extraction, space-based name detection, and domain fallbacks. Line 122 properly uses
str_contains()for the space check.
182-206: Clean remote content fetching with proper error handling.The flow correctly fetches remote content, substitutes it in the DTO, clears template context on success, and logs failures via EventLogManager. Returning
falseon failure allows the caller to handle the error appropriately.
| <rule ref="Squiz.WhiteSpace.OperatorSpacing"> | ||
| <properties> | ||
| <property name="ignoreNewlines" value="true"/> | ||
| </properties> | ||
| </rule> |
There was a problem hiding this comment.
Duplicate rule: consolidate the operator spacing configuration.
The Squiz.WhiteSpace.OperatorSpacing rule is declared twice—once here with the ignoreNewlines property and again at line 111 without properties. PHP_CodeSniffer may only apply the last declaration, which would ignore your ignoreNewlines setting.
Consider removing the duplicate at line 111 or moving this property configuration there instead (since line 111 sits in the Whitespace section where it semantically belongs).
🔎 Suggested fix: Remove line 111 to keep this configuration
Remove the duplicate at line 111 in the Whitespace section:
<!-- Whitespace -->
<rule ref="PEAR.WhiteSpace.ObjectOperatorIndent"/>
<rule ref="PEAR.WhiteSpace.ScopeClosingBrace"/>
<rule ref="Squiz.WhiteSpace.CastSpacing"/>
<rule ref="Squiz.WhiteSpace.LogicalOperatorSpacing"/>
-<rule ref="Squiz.WhiteSpace.OperatorSpacing"/>
<rule ref="Squiz.WhiteSpace.SemicolonSpacing"/>Alternatively, move the property configuration to line 111 and remove lines 57-61.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In config/PhpCodeSniffer/ruleset.xml around lines 57-61 there is a duplicate
declaration of Squiz.WhiteSpace.OperatorSpacing (this block sets
ignoreNewlines=true) and another declaration at line 111 without properties;
remove the duplicate at line 111 in the Whitespace section so the operator
spacing rule keeps the ignoreNewlines property defined here, or alternatively
move the property block from lines 57-61 to line 111 and delete the current
declaration to avoid duplicate rule definitions.
| $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 %s', | ||
| $loadedMessageData['subject'] | ||
| ), | ||
| inBlast: false, | ||
| ); | ||
|
|
||
| // todo: check if from name should be from config | ||
| $envelope = new Envelope( | ||
| sender: new Address($this->messageEnvelope, 'PHPList'), | ||
| recipients: [new Address($email->getTo()[0]->getAddress())], | ||
| ); | ||
| $this->mailer->send(message: $email, envelope: $envelope); |
There was a problem hiding this comment.
Critical: Check if buildPhplistEmail returns null before accessing email properties.
Line 296 calls buildPhplistEmail(), which can return null if the recipient is blacklisted, has newlines, or is empty. Line 310 accesses $email->getTo()[0] without checking, causing a fatal error if $email is null.
🔎 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 %s',
$loadedMessageData['subject']
),
inBlast: false,
);
+ if (!$email || empty($email->getTo())) {
+ $this->logger->warning('Failed to build admin notification email', ['to' => $notification]);
+ continue;
+ }
+
// todo: check if from name should be from config
$envelope = new Envelope(
sender: new Address($this->messageEnvelope, 'PHPList'),
recipients: [new Address($email->getTo()[0]->getAddress())],
);
$this->mailer->send(message: $email, envelope: $envelope);🤖 Prompt for AI Agents
In src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
around lines 296 to 312, buildPhplistEmail() may return null for
blacklisted/invalid recipients but the code immediately dereferences
$email->getTo()[0], causing a fatal error; update the code to check if $email
=== null (or not an Email instance) after the call, and if null, skip
constructing Envelope/sending (optionally log a warning with recipient/campaign
id) and return or continue, otherwise proceed to create the Envelope and call
$this->mailer->send with the non-null $email.
| if ($this->devEmail && $destinationEmail !== $this->devEmail) { | ||
| $email->getHeaders()->addTextHeader('X-Originally to', $destinationEmail); | ||
| } |
There was a problem hiding this comment.
Fix unreachable "X-Originally to" header logic.
Line 177 checks $this->devEmail && $destinationEmail !== $this->devEmail, but line 136 already sets $destinationEmail = $this->devEmail when both devVersion and devEmail are set. This makes the condition at line 177 always false, so the header is never added. Should this compare against the original $to parameter instead?
🔎 Possible 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);
}🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Builder/EmailBuilder.php around lines 177 to
179, the condition that adds the "X-Originally to" header is unreachable because
$destinationEmail is overwritten earlier with $this->devEmail; preserve the
original recipient (e.g., store $originalTo = $to before any dev override) and
change the header check to compare against that original value (if
$this->devEmail is set and $originalTo !== $this->devEmail then add the header
with $originalTo). Ensure you use the preserved original variable when creating
the header so the intended original recipient is recorded.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Thanks for contributing to phpList!