Refactor CampaignProcessorMessageHandler #374
Conversation
📝 WalkthroughWalkthroughIntroduces a DTO-based precache pipeline (MessagePrecacheDto), moves personalization to the handler, adds remote page fetching, HTML/CSS URL rewriting, HTML→text and text parsing utilities, new repositories and template/logo handling, and updates messaging services and tests to use the DTO. 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 (1 warning)
✅ Passed checks (2 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.
| $ww = $this->configProvider->getValue(ConfigOption::WordWrap) ?? self::WORD_WRAP; | ||
|
|
||
| return wordwrap($text, $ww); |
There was a problem hiding this comment.
Type mismatch: wordwrap expects int, but config returns string.
getValue() returns ?string, so $ww could be "75" (string). Cast to int for 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);📝 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.
| $ww = $this->configProvider->getValue(ConfigOption::WordWrap) ?? self::WORD_WRAP; | |
| return wordwrap($text, $ww); | |
| $ww = (int) ($this->configProvider->getValue(ConfigOption::WordWrap) ?? self::WORD_WRAP); | |
| return wordwrap($text, $ww); |
🤖 Prompt for AI Agents
In src/Domain/Common/HtmlToText.php around lines 81 to 83, the value retrieved
from configProvider is ?string but wordwrap requires an int; cast the config
value to int (e.g. $ww =
(int)($this->configProvider->getValue(ConfigOption::WordWrap) ??
self::WORD_WRAP);) or use intval with the same fallback so wordwrap always
receives an integer.
| public function __construct( | ||
| private readonly HttpClientInterface $httpClient, | ||
| private readonly CacheInterface $cache, | ||
| private readonly ConfigProvider $configProvider, | ||
| private readonly UrlCacheRepository $urlCacheRepository, | ||
| private readonly EventLogManager $eventLogManager, | ||
| private readonly HtmlUrlRewriter $htmlUrlRewriter, | ||
| private readonly EntityManagerInterface $entityManager, | ||
| private readonly int $defaultTtl = 300, | ||
| ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd RemotePageFetcher.phpRepository: phpList/core
Length of output: 95
🏁 Script executed:
cat -n src/Domain/Common/RemotePageFetcher.phpRepository: phpList/core
Length of output: 5109
Domain purity breach: persistence side effects in src/Domain/**
Lines 75 and 78 call $this->entityManager->remove() and $this->entityManager->persist() directly. This violates domain layer purity; move UrlCache write logic to Infrastructure or Application (e.g., a dedicated service or handler) and inject a repository interface that abstracts away the persistence concern.
🤖 Prompt for AI Agents
In src/Domain/Common/RemotePageFetcher.php around lines 19 and 28 (and
specifically lines ~75 and ~78) the domain class directly calls
$this->entityManager->remove() and $this->entityManager->persist(), creating
persistence side effects inside the Domain layer; replace direct EntityManager
usage by depending on an abstract repository interface (e.g.,
UrlCacheRepositoryInterface) injected into the constructor, move the concrete
EntityManager-based implementation into the Infrastructure layer, and refactor
RemotePageFetcher to call only repository methods (e.g.,
deleteUrlCache()/saveUrlCache()) so all persistence logic lives in
Infrastructure/Application and the Domain remains pure.
| private function fetchUrlDirect(string $url): string | ||
| { | ||
| try { | ||
| $response = $this->httpClient->request('GET', $url, [ | ||
| // 'timeout' => 10, | ||
| 'timeout' => 600, | ||
| 'allowRedirects' => 1, | ||
| 'method' => 'HEAD', | ||
| ]); | ||
|
|
||
| return $response->getContent(false); | ||
| } catch (Throwable $e) { | ||
| return ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Symfony HttpClient request method options allowRedirects max_redirects
💡 Result:
Short answer: Symfony HttpClient uses the request option max_redirects (not allowRedirects). It controls how many redirects the client will follow (default 20). Set max_redirects: 0 to disable following redirects; set to an integer N to allow up to N redirects. You can set it per request or as a global default_options in framework.http_client. [1][2]
Examples:
- Per request:
$response = $client->request('GET', 'https://...', ['max_redirects' => 0]); [1] - Global (config/packages/framework.yaml):
framework:
http_client:
default_options:
max_redirects: 7 [1]
Sources: Symfony HttpClient docs and HttpClient interface defaults. [1][2]
🌐 Web query:
Symfony HttpClient API reference options parameters
💡 Result:
Here are the request option keys supported by Symfony's HttpClient (from HttpClientInterface::OPTIONS_DEFAULTS) with short notes on their expected values:
- auth_basic — array|string (username[:password]) for HTTP Basic auth. [1]
- auth_bearer — string bearer token. [1]
- query — array of query params to merge with the URL. [1]
- headers — iterable|string[]|string[][] request headers. [1]
- body — array|string|resource|\Traversable|\Closure (form payload, raw body, or streaming callback). [1]
- json — mixed (will set body to JSON and Content-Type if not provided). [1]
- user_data — mixed (opaque value accessible via $response->getInfo('user_data')). [1]
- max_redirects — int (max redirects; <=0 = no redirects). [1]
- http_version — string (e.g. "1.1", "2.0"). [1]
- base_uri — string (base URI for resolving relative URLs). [1]
- buffer — bool|resource|\Closure (whether/how to buffer response body). [1]
- on_progress — callable(int $dlNow, int $dlSize, array $info) called during progress. [1]
- resolve — array map of host => IP to override DNS. [1]
- proxy — string proxy URL (environment vars may be honored by implementations). [1]
- no_proxy — string (comma-separated hosts to bypass proxy). [1]
- timeout — float (idle timeout). [1]
- max_duration — float (maximum total time for request+response; <=0 = unlimited). [1]
- bindto — string (interface or local socket to bind to). [1]
- verify_peer, verify_host, cafile, capath, local_cert, local_pk, passphrase, ciphers, peer_fingerprint, capture_peer_cert_chain — SSL/peer verification and cert options. [1]
- extra — array (additional custom options that may be ignored by some implementations). [1]
For usage examples (headers, query, auth, redirects, proxies, withOptions()/default_options, HttpOptions helper), see the Symfony HttpClient docs. [2]
Sources: Symfony HttpClientInterface options listing and the HttpClient docs. [1][2]
HTTP request options are incorrect; code likely won't work as intended (Lines 89–103).
You call request('GET', ...) but pass invalid options: 'allowRedirects' doesn't exist in Symfony HttpClient (should be max_redirects), and 'method' => 'HEAD' isn't a valid option—it's silently ignored. The 600s timeout also masks the commented-out 10s, and swallowing all exceptions silently makes debugging hard.
Fix to:
- $response = $this->httpClient->request('GET', $url, [
- 'timeout' => 600,
- 'allowRedirects' => 1,
- 'method' => 'HEAD',
- ]);
+ $response = $this->httpClient->request('GET', $url, [
+ 'timeout' => 10,
+ 'max_redirects' => 5,
+ ]);Also log the exception (at least at debug level) instead of silently returning ''.
🤖 Prompt for AI Agents
In src/Domain/Common/RemotePageFetcher.php around lines 89-103, the HTTP options
and method are wrong and exceptions are swallowed; replace the options with
Symfony HttpClient-compatible ones (use 'max_redirects' instead of
'allowRedirects'), remove the invalid 'method' option and call
$this->httpClient->request('HEAD', $url, [...]) if you intend a HEAD request (or
keep 'GET' and call request('GET', ...)), reduce the timeout back to a
reasonable value (e.g. 10), and in the catch block log the exception at debug
level (e.g. $this->logger->debug or the class logger) before returning an empty
string so errors are not silently discarded.
| public function getOrCacheBaseMessageContent(Message $campaign, ?bool $forwardContent = false): ?MessagePrecacheDto | ||
| { | ||
| $cacheKey = sprintf('messaging.message.base.%d', $campaign->getId()); | ||
|
|
||
| $cached = $this->getFromCache($cacheKey); | ||
| $cached = $this->cache->get($cacheKey); | ||
| if ($cached !== null) { | ||
| return $cached; | ||
| } |
There was a problem hiding this comment.
Blocker: cache key ignores $forwardContent and can return the wrong content (Line 48–Line 55, Line 85–Line 95).
$subject/$content/$footer depend on $forwardContent, but the cache key doesn’t—so whichever call runs first “poisons” the cache for the other mode.
Fix:
- $cacheKey = sprintf('messaging.message.base.%d', $campaign->getId());
+ $cacheKey = sprintf('messaging.message.base.%d.%d', $campaign->getId(), (int) $forwardContent);Also applies to: 85-95
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/MessagePrecacheService.php around lines 48–55
and again around lines 85–95 the cache key currently ignores the $forwardContent
flag so cached base message content for one mode can be returned for the other;
include $forwardContent (e.g. cast to int or '1'/'0') in the sprintf cache key
so keys differ by forward mode, and apply the same change to the second block so
both cache reads/writes use the forward-aware key.
| $cached = $this->cache->get($cacheKey); | ||
| if ($cached !== null) { | ||
| return $cached; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -e php MessagePrecacheServiceRepository: phpList/core
Length of output: 111
🏁 Script executed:
cat -n src/Domain/Messaging/Service/MessagePrecacheService.phpRepository: phpList/core
Length of output: 16937
🏁 Script executed:
fd -e yaml -e xml -e php config | grep -i cache | head -20Repository: phpList/core
Length of output: 38
🏁 Script executed:
fd -name "*.yaml" -o -name "*.yml" -o -name "services.php" | head -20Repository: phpList/core
Length of output: 286
🏁 Script executed:
fd '\.(yaml|yml)$'Repository: phpList/core
Length of output: 644
🏁 Script executed:
grep -r "cache" config/ --include="*.yml" --include="*.yaml" -i | grep -E "(service|backend|psr|adapter)" | head -20Repository: phpList/core
Length of output: 500
🏁 Script executed:
cat config/services/services.yml | head -100Repository: phpList/core
Length of output: 3022
🏁 Script executed:
rg "cache\.app" config/Repository: phpList/core
Length of output: 248
🏁 Script executed:
grep -A 10 "cache:" config/config*.ymlRepository: phpList/core
Length of output: 38
🏁 Script executed:
fd MessagePrecacheDtoRepository: phpList/core
Length of output: 109
🏁 Script executed:
cat -n src/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpRepository: phpList/core
Length of output: 1066
🏁 Script executed:
grep -r "framework:" config/ | head -5Repository: phpList/core
Length of output: 193
🏁 Script executed:
cat config/config.yml | grep -A 20 "^framework:"Repository: phpList/core
Length of output: 627
Consider adding TTL and cache key versioning (Lines 52–55, 193–195).
The cache stores the DTO object directly with no TTL, so it persists indefinitely across deployments. Add an expiration time to cache.set() and/or include a version/timestamp suffix in the cache key to prevent stale data after code changes.
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/MessagePrecacheService.php around lines 52–55
and 193–195, the code reads and writes the DTO to cache without an expiration
and uses a plain cache key, so cached DTOs can persist across deployments and
become stale; fix by appending a cache version or deploy timestamp suffix to the
cache key (e.g. a class constant CACHE_VERSION or an injected deployVersion) and
pass an explicit TTL when calling cache->set() (choose an appropriate expiry
like hours/days depending on expected staleness), ensuring the read uses the
same versioned key.
| private function createCachedLogoImage(int $size): void | ||
| { | ||
| $logoImageId = $this->configProvider->getValue(ConfigOption::OrganisationLogo); | ||
| if (empty($logoImageId)) { | ||
| return; | ||
| } | ||
|
|
||
| $orgLogoImage = $this->templateImageRepository->findByFilename("ORGANISATIONLOGO$size.png"); | ||
| if (!empty($orgLogoImage->getData())) { | ||
| return; | ||
| } | ||
|
|
||
| $logoImage = $this->templateImageRepository->findById((int) $logoImageId); | ||
| $imageContent = base64_decode($logoImage->getData()); | ||
| if (empty($imageContent)) { | ||
| //# fall back to a single pixel, so that there are no broken images | ||
| $imageContent = base64_decode('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAABGdBTUEAALGPC/xhBQAAAAZQTFRF////AAAAVcLTfgAAAAF0Uk5TAEDm2GYAAAABYktHRACIBR1IAAAACXBIWXMAAAsSAAALEgHS3X78AAAAB3RJTUUH0gQCEx05cqKA8gAAAApJREFUeJxjYAAAAAIAAUivpHEAAAAASUVORK5CYII='); | ||
| } | ||
|
|
||
| return null; | ||
| $imgSize = getimagesizefromstring($imageContent); | ||
| $sizeW = $imgSize[0]; | ||
| $sizeH = $imgSize[1]; | ||
| if ($sizeH > $sizeW) { | ||
| $sizeFactor = (float) ($size / $sizeH); | ||
| } else { | ||
| $sizeFactor = (float) ($size / $sizeW); | ||
| } | ||
| $newWidth = (int) ($sizeW * $sizeFactor); | ||
| $newHeight = (int) ($sizeH * $sizeFactor); | ||
|
|
||
| if ($sizeFactor < 1) { | ||
| $original = imagecreatefromstring($imageContent); | ||
| //# creates a black image (why would you want that....) | ||
| $resized = imagecreatetruecolor($newWidth, $newHeight); | ||
| imagesavealpha($resized, true); | ||
| //# white. All the methods to make it transparent didn't work for me @@TODO really make transparent | ||
| $transparent = imagecolorallocatealpha($resized, 255, 255, 255, 127); | ||
| imagefill($resized, 0, 0, $transparent); | ||
|
|
||
| if (imagecopyresized($resized, $original, 0, 0, 0, 0, $newWidth, $newHeight, $sizeW, $sizeH)) { | ||
| $this->entityManager->remove($orgLogoImage); | ||
|
|
||
| //# rather convoluted way to get the image contents | ||
| $buffer = ob_get_contents(); | ||
| ob_end_clean(); | ||
| ob_start(); | ||
| imagepng($resized); | ||
| $imageContent = ob_get_contents(); | ||
| ob_end_clean(); | ||
| echo $buffer; | ||
| } | ||
| } | ||
| // else copy original | ||
| $templateImage = (new TemplateImage()) | ||
| ->setFilename("ORGANISATIONLOGO$size.png") | ||
| ->setMimetype($imgSize['mime']) | ||
| ->setData(base64_encode($imageContent)) | ||
| ->setWidth($newWidth) | ||
| ->setHeight($newHeight); | ||
|
|
||
| $this->entityManager->persist($templateImage); | ||
|
|
||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*Repository*.php" | grep -i template | head -20Repository: phpList/core
Length of output: 249
🏁 Script executed:
find . -type f -path "*/Domain/Messaging/*" -name "*.php" | head -30Repository: phpList/core
Length of output: 1859
🏁 Script executed:
fd "MessagePrecacheService" --type fRepository: phpList/core
Length of output: 111
🏁 Script executed:
cat -n src/Domain/Messaging/Repository/TemplateImageRepository.phpRepository: phpList/core
Length of output: 1407
🏁 Script executed:
fd "TemplateImage.php" --type fRepository: phpList/core
Length of output: 100
🏁 Script executed:
head -100 src/Domain/Messaging/Service/MessagePrecacheService.phpRepository: phpList/core
Length of output: 5493
🏁 Script executed:
sed -n '240,310p' src/Domain/Messaging/Service/MessagePrecacheService.php | cat -nRepository: phpList/core
Length of output: 3250
🏁 Script executed:
cat -n src/Domain/Messaging/Model/TemplateImage.phpRepository: phpList/core
Length of output: 3753
🏁 Script executed:
# Check if there are any null checks for $logoImage before line 255
sed -n '242,260p' src/Domain/Messaging/Service/MessagePrecacheService.phpRepository: phpList/core
Length of output: 998
🏁 Script executed:
# Verify ob_* safety by checking PHP documentation context
# Check if there's any ob_start() before this method is called
rg "ob_start|ob_get_level" src/Domain/Messaging/Service/MessagePrecacheService.phpRepository: phpList/core
Length of output: 83
Fix null dereference hazards and move image generation + persistence to Infrastructure/Application layer (Lines 242–304).
- Line 250:
findByFilename()returns nullable?TemplateImage, but$orgLogoImage->getData()is called without null check → fatal error if method returns null. - Line 255:
findById()returns nullable?TemplateImage, but$logoImage->getData()is called immediately without null check → fatal error if method returns null. - Line 262:
getimagesizefromstring()can returnfalse; direct array access to$imgSize[0],$imgSize[1], and$imgSize['mime']at lines 263–264 and 298 will fatal if the function fails. - Lines 286–296: Output buffering without checking
ob_get_level()beforeob_get_contents()/ob_end_clean()can emit notices or corrupt output if no buffer is active. - Lines 283, 303:
$this->entityManager->remove()and$this->entityManager->persist()belong in Infrastructure/Application layer (e.g., MessageHandler), not in Domain service. Image generation + GD work + DB writes should be moved to a dedicated application/infrastructure service and called from here via a port.
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/MessagePrecacheService.php around lines 242–304,
there are multiple null-dereference and runtime hazards and a layering
violation: add null checks after templateImageRepository->findByFilename(...)
and findById(...) and handle the case when they return null (return or throw a
domain-specific exception), check the result of getimagesizefromstring() and
bail out cleanly if it returns false, avoid direct array access on $imgSize
until validated, guard any ob_get_contents()/ob_end_clean() calls by verifying
ob_get_level() > 0 (or use a local output buffer via ob_start() and always
ob_get_clean()), and remove all EntityManager persistence/removal and GD
image-generation side effects from this domain service — instead extract image
generation, resizing and repository persistence into an
Infrastructure/Application service behind an injected port/interface and call
that port from this domain service (pass raw image data or identifiers); keep
the domain service free of DB/GD side effects and only delegate to the new
application/infrastructure collaborator.
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.
| $text = preg_replace( | ||
| "/<a href=[\"\'](.*?)[\"\'][^>]*>(.*?)<\/a>/is", | ||
| '[URLTEXT]\\2[ENDURLTEXT][LINK]\\1[ENDLINK]', | ||
| $text, | ||
| 500 | ||
| ); |
There was a problem hiding this comment.
Unreachable code: anchor tags are already stripped.
This regex replacement looks for <a href=...> tags, but strip_tags() at line 38 already removed all HTML tags. This block will never match anything.
If this is intentional for edge cases, consider adding a comment explaining when it would apply, otherwise remove it.
🤖 Prompt for AI Agents
In src/Domain/Common/Html2Text.php around lines 61 to 66, the preg_replace that
converts <a href="..."> tags into placeholder text is unreachable because
strip_tags() earlier (line ~38) already removed all HTML; either remove this
unreachable block, or move the regex so it runs before strip_tags() if the
intent is to preserve link text/URLs, or if it was left for a specific edge case
add a clarifying comment explaining that edge case and why strip_tags() does not
remove those anchors; pick one of these fixes and apply it consistently.
| 'PoweredByImage' => [ | ||
| 'value' => '<p class="poweredby" style="text-align:center"><a href="https://www.phplist.com/poweredby?utm_source=pl&utm_medium=poweredlocalimg&utm_campaign=phpList" title="visit the phpList website"><img src="images/power-phplist.png" title="powered by phpList version , © phpList ltd" alt="powered by phpList , © phpList ltd" border="0"/></a></p>', | ||
| 'description' => 'logo/image ndicates that emails are sent by phpList', | ||
| 'type' => 'string', | ||
| 'allowempty' => false, | ||
| 'category' => 'general', | ||
| ], | ||
| 'PoweredByText' => [ | ||
| 'value' => '<div style="clear: both; font-family: arial, verdana, sans-serif; font-size: 8px; font-variant: small-caps; font-weight: normal; padding: 2px; padding-left:10px;padding-top:20px;">powered by <a href="https://www.phplist.com/poweredby?utm_source=download&utm_medium=poweredtxt&utm_campaign=phpList" target="_blank" title="powered by phpList version, © phpList ltd">phpList</a></div>', | ||
| 'description' => 'text indicates that emails are sent by phpList', | ||
| 'type' => 'string', | ||
| 'allowempty' => false, | ||
| 'category' => 'general', | ||
| ], |
There was a problem hiding this comment.
Typo in description and inconsistent type usage.
Line 562 has a typo: "ndicates" should be "indicates". Also, the type is set to 'string' for both items, while similar text-based configs in this file use 'text' or 'textarea'. Consider aligning for consistency.
'PoweredByImage' => [
'value' => '<p class="poweredby" style="text-align:center"><a href="https://www.phplist.com/poweredby?utm_source=pl&utm_medium=poweredlocalimg&utm_campaign=phpList" title="visit the phpList website"><img src="images/power-phplist.png" title="powered by phpList version , © phpList ltd" alt="powered by phpList , © phpList ltd" border="0"/></a></p>',
- 'description' => 'logo/image ndicates that emails are sent by phpList',
- 'type' => 'string',
+ 'description' => 'logo/image indicates that emails are sent by phpList',
+ 'type' => 'textarea',
'allowempty' => false,
'category' => 'general',
],
'PoweredByText' => [
'value' => '<div style="clear: both; font-family: arial, verdana, sans-serif; font-size: 8px; font-variant: small-caps; font-weight: normal; padding: 2px; padding-left:10px;padding-top:20px;">powered by <a href="https://www.phplist.com/poweredby?utm_source=download&utm_medium=poweredtxt&utm_campaign=phpList" target="_blank" title="powered by phpList version, © phpList ltd">phpList</a></div>',
'description' => 'text indicates that emails are sent by phpList',
- 'type' => 'string',
+ 'type' => 'textarea',
'allowempty' => false,
'category' => 'general',
],📝 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.
| 'PoweredByImage' => [ | |
| 'value' => '<p class="poweredby" style="text-align:center"><a href="https://www.phplist.com/poweredby?utm_source=pl&utm_medium=poweredlocalimg&utm_campaign=phpList" title="visit the phpList website"><img src="images/power-phplist.png" title="powered by phpList version , © phpList ltd" alt="powered by phpList , © phpList ltd" border="0"/></a></p>', | |
| 'description' => 'logo/image ndicates that emails are sent by phpList', | |
| 'type' => 'string', | |
| 'allowempty' => false, | |
| 'category' => 'general', | |
| ], | |
| 'PoweredByText' => [ | |
| 'value' => '<div style="clear: both; font-family: arial, verdana, sans-serif; font-size: 8px; font-variant: small-caps; font-weight: normal; padding: 2px; padding-left:10px;padding-top:20px;">powered by <a href="https://www.phplist.com/poweredby?utm_source=download&utm_medium=poweredtxt&utm_campaign=phpList" target="_blank" title="powered by phpList version, © phpList ltd">phpList</a></div>', | |
| 'description' => 'text indicates that emails are sent by phpList', | |
| 'type' => 'string', | |
| 'allowempty' => false, | |
| 'category' => 'general', | |
| ], | |
| 'PoweredByImage' => [ | |
| 'value' => '<p class="poweredby" style="text-align:center"><a href="https://www.phplist.com/poweredby?utm_source=pl&utm_medium=poweredlocalimg&utm_campaign=phpList" title="visit the phpList website"><img src="images/power-phplist.png" title="powered by phpList version , © phpList ltd" alt="powered by phpList , © phpList ltd" border="0"/></a></p>', | |
| 'description' => 'logo/image indicates that emails are sent by phpList', | |
| 'type' => 'textarea', | |
| 'allowempty' => false, | |
| 'category' => 'general', | |
| ], | |
| 'PoweredByText' => [ | |
| 'value' => '<div style="clear: both; font-family: arial, verdana, sans-serif; font-size: 8px; font-variant: small-caps; font-weight: normal; padding: 2px; padding-left:10px;padding-top:20px;">powered by <a href="https://www.phplist.com/poweredby?utm_source=download&utm_medium=poweredtxt&utm_campaign=phpList" target="_blank" title="powered by phpList version, © phpList ltd">phpList</a></div>', | |
| 'description' => 'text indicates that emails are sent by phpList', | |
| 'type' => 'textarea', | |
| 'allowempty' => false, | |
| 'category' => 'general', | |
| ], |
🤖 Prompt for AI Agents
In src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php around
lines 560 to 573, fix the typo in the PoweredByImage description ("ndicates" →
"indicates") and make the type usage consistent with other text-based configs by
changing the 'type' for both PoweredByImage and PoweredByText from 'string' to
'text' (leave other keys like value, description, allowempty and category
unchanged).
| public function parseLogoPlaceholders($content) | ||
| { | ||
| //# replace Logo placeholders | ||
| preg_match_all('/\[LOGO\:?(\d+)?\]/', $content, $logoInstances); | ||
| foreach ($logoInstances[0] as $index => $logoInstance) { | ||
| $size = sprintf('%d', $logoInstances[1][$index]); | ||
| $logoSize = !empty($size) ? $size : '500'; | ||
| $this->createCachedLogoImage((int)$logoSize); | ||
| $content = str_replace($logoInstance, 'ORGANISATIONLOGO'.$logoSize.'.png', $content); | ||
| } | ||
|
|
||
| return $content; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing return type declaration.
The method parseLogoPlaceholders lacks a return type. Add : string for consistency with strict typing.
- public function parseLogoPlaceholders($content)
+ public function parseLogoPlaceholders(string $content): string🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Manager/TemplateImageManager.php around lines
101 to 113, the method parseLogoPlaceholders is missing a return type; change
its signature to declare a string return type (and also type-hint the $content
parameter as string) so it becomes strongly typed and returns a string, then
ensure the method still returns $content (a string) and update any callers if
they pass non-string values.
| $orgLogoImage = $this->templateImageRepository->findByFilename("ORGANISATIONLOGO$size.png"); | ||
| if (!empty($orgLogoImage->getData())) { | ||
| return; | ||
| } | ||
|
|
||
| $logoImage = $this->templateImageRepository->findById((int) $logoImageId); | ||
| $imageContent = base64_decode($logoImage->getData()); |
There was a problem hiding this comment.
Null pointer dereference risk.
Both findByFilename() and findById() can return null. Calling ->getData() on a null object will throw a fatal error.
$orgLogoImage = $this->templateImageRepository->findByFilename("ORGANISATIONLOGO$size.png");
- if (!empty($orgLogoImage->getData())) {
+ if ($orgLogoImage !== null && !empty($orgLogoImage->getData())) {
return;
}
$logoImage = $this->templateImageRepository->findById((int) $logoImageId);
+ if ($logoImage === null) {
+ return;
+ }
$imageContent = base64_decode($logoImage->getData());🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Manager/TemplateImageManager.php around lines
122-128, the code assumes findByFilename() and findById() always return an
object and calls ->getData() directly, risking a null pointer dereference;
update the code to check each repository call for null before accessing
getData() (e.g., if $orgLogoImage === null or $orgLogoImage->getData() is empty,
handle gracefully and return or log/throw as appropriate), and likewise verify
$logoImage !== null before decoding its data (only call base64_decode when
$logoImage exists and getData() is non-empty) so you avoid fatal errors.
| if (imagecopyresized($resized, $original, 0, 0, 0, 0, $newWidth, $newHeight, $sizeW, $sizeH)) { | ||
| $this->entityManager->remove($orgLogoImage); | ||
|
|
||
| //# rather convoluted way to get the image contents | ||
| $buffer = ob_get_contents(); | ||
| ob_end_clean(); | ||
| ob_start(); | ||
| imagepng($resized); | ||
| $imageContent = ob_get_contents(); | ||
| ob_end_clean(); | ||
| echo $buffer; | ||
| } |
There was a problem hiding this comment.
Output buffer handling is fragile and leaky.
The buffer capture sequence captures content before ob_start() is called (line 158), then echoes the old buffer after capturing image data (line 164). This can corrupt output and cause unexpected side effects.
Consider using a memory stream 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();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (imagecopyresized($resized, $original, 0, 0, 0, 0, $newWidth, $newHeight, $sizeW, $sizeH)) { | |
| $this->entityManager->remove($orgLogoImage); | |
| //# rather convoluted way to get the image contents | |
| $buffer = ob_get_contents(); | |
| ob_end_clean(); | |
| ob_start(); | |
| imagepng($resized); | |
| $imageContent = ob_get_contents(); | |
| ob_end_clean(); | |
| echo $buffer; | |
| } | |
| if (imagecopyresized($resized, $original, 0, 0, 0, 0, $newWidth, $newHeight, $sizeW, $sizeH)) { | |
| $this->entityManager->remove($orgLogoImage); | |
| ob_start(); | |
| imagepng($resized); | |
| $imageContent = ob_get_clean(); | |
| } |
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/Manager/TemplateImageManager.php around lines
154–165, the current output-buffer dance captures and restores buffers in the
wrong order and leaks/echoes prior buffer contents; replace the fragile sequence
with a single, local buffer: call ob_start() immediately before
imagepng($resized), then capture the image into $imageContent with $imageContent
= ob_get_clean(), remove the earlier ob_get_contents()/ob_end_clean() calls and
the echo $buffer, and ensure you only remove the entity once the image data is
safely captured; this keeps buffering local and prevents corrupting global
output.
| $storedMessageData = $this->messageDataRepository->getForMessage($message->getId()); | ||
| foreach ($storedMessageData as $storedMessageDatum) { | ||
| if (str_starts_with($storedMessageDatum->getData(), 'SER:')) { | ||
| $unserialized = unserialize(substr($storedMessageDatum->getData(), 4)); |
There was a problem hiding this comment.
Security: unserialize() on stored data can lead to object injection.
Using unserialize() on data from the database is risky if that data can be influenced by users. Consider using json_decode() or at minimum specify allowed_classes: false.
- $unserialized = unserialize(substr($storedMessageDatum->getData(), 4));
+ $unserialized = unserialize(substr($storedMessageDatum->getData(), 4), ['allowed_classes' => false]);🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/MessageDataLoader.php around line 100, the code
calls unserialize() on stored DB data which risks PHP object injection; replace
this by either decoding a safe serialized format (preferably json_decode() and
update storage to JSON) or, if keeping PHP serialization, call unserialize with
the options parameter to disallow object instantiation (unserialize($data,
['allowed_classes' => false])) and validate the resulting value; also preserve
the existing substr(...) trimming if it’s required for your storage format and
add error handling for decode failures.
| // parse the reply-to field into its components - email and name | ||
| if (preg_match('/([^ ]+@[^ ]+)/', $loadedMessageData['replyto'], $regs)) { | ||
| // if there is an email in the from, rewrite it as "name <email>" | ||
| $loadedMessageData['replyto'] = str_replace($regs[0], '', $loadedMessageData['replyto']); | ||
| $replyToEmail = $regs[0]; | ||
| // if the email has < and > take them out here | ||
| $replyToEmail = str_replace('<', '', $replyToEmail); | ||
| $replyToEmail = str_replace('>', '', $replyToEmail); | ||
| $messagePrecacheDto->replyToEmail = $replyToEmail; | ||
| // make sure there are no quotes around the name | ||
| $messagePrecacheDto->replyToName = str_replace('"', '', ltrim(rtrim($loadedMessageData['replyto']))); | ||
| } elseif (strpos($loadedMessageData['replyto'], ' ')) { | ||
| // if there is a space, we need to add the email | ||
| $messagePrecacheDto->replyToName = $loadedMessageData['replyto']; | ||
| $messagePrecacheDto->replyToEmail = "listmaster@$domain"; | ||
| } elseif (!empty($loadedMessageData['replyto'])) { | ||
| $messagePrecacheDto->replyToEmail = $loadedMessageData['replyto']."@$domain"; | ||
| //# makes more sense not to add the domain to the word, but the help says it does | ||
| //# so let's keep it for now | ||
| $messagePrecacheDto->replyToName = $loadedMessageData['replyto']."@$domain"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd MessagePrecacheService.phpRepository: phpList/core
Length of output: 111
🏁 Script executed:
cat -n src/Domain/Messaging/Service/MessagePrecacheService.php | sed -n '50,85p'Repository: phpList/core
Length of output: 2621
🏁 Script executed:
# Check if there are tests for this method
fd -e php | xargs grep -l "MessagePrecacheService\|replyto" | grep -i testRepository: phpList/core
Length of output: 280
🏁 Script executed:
# Look at the method signature and where $loadedMessageData comes from
sed -n '40,56p' src/Domain/Messaging/Service/MessagePrecacheService.phpRepository: phpList/core
Length of output: 773
🏁 Script executed:
# Check the test fixtures to understand what replyto values are expected
cat tests/Integration/Domain/Messaging/Fixtures/MessageFixture.php | grep -A 5 -B 5 "replyto" | head -40Repository: phpList/core
Length of output: 373
🏁 Script executed:
# Look at the MessagePrecacheDto to understand what this field represents
fd MessagePrecacheDto.php -x cat {}Repository: phpList/core
Length of output: 856
🏁 Script executed:
# Look for tests of the precacheMessage method
fd -e php | xargs grep -l "precacheMessage" | grep -i testRepository: phpList/core
Length of output: 38
🏁 Script executed:
# Check if there are any unit tests for MessagePrecacheService
fd MessagePrecacheServiceTest.phpRepository: phpList/core
Length of output: 38
Use strict comparison for strpos() to avoid position-0 falsy trap
Line 67: The strpos() check needs strict comparison. If a space appears at position 0, the current condition evaluates to false even though the substring exists.
- } elseif (strpos($loadedMessageData['replyto'], ' ')) {
+ } elseif (strpos($loadedMessageData['replyto'], ' ') !== false) {This makes the intent explicit and prevents edge cases from silently skipping the branch.
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/MessagePrecacheService.php around lines 56 to
76, the strpos() check on line 67 uses a loose truthy test which will fail when
a space is at position 0; change the condition to use strict comparison
(strpos(...) !== false) so the branch runs whenever a space exists anywhere in
the string, ensuring correct behavior for position 0 matches.
| // todo: check if getTextMessage should replace links as well | ||
| if ($cachedMessageDto->content) { | ||
| $cachedMessageDto->content = $this->replaceLinks($savedLinks, $cachedMessageDto->content); | ||
| } | ||
|
|
||
| if ($footer !== null) { | ||
| $footer = $this->replaceLinks($savedLinks, $footer); | ||
| $footer = $this->userPersonalizer->personalize($footer, $subscriber->getEmail()); | ||
| $content->setFooter($footer); | ||
| if ($cachedMessageDto->footer) { | ||
| $cachedMessageDto->footer = $this->replaceLinks($savedLinks, $cachedMessageDto->footer); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find MessagePrecacheDto definition
fd MessagePrecacheDtoRepository: phpList/core
Length of output: 109
🏁 Script executed:
# Find LinkTrackService and its extractAndSaveLinks method
fd LinkTrackServiceRepository: phpList/core
Length of output: 166
🏁 Script executed:
# Search for where footer and htmlFooter are assigned/populated in MessagePrecacheDto context
rg "htmlFooter|footer" --type php -B 2 -A 2 | head -100Repository: phpList/core
Length of output: 11877
🏁 Script executed:
cat -n src/Domain/Messaging/Model/Dto/MessagePrecacheDto.phpRepository: phpList/core
Length of output: 1066
🏁 Script executed:
cat -n src/Domain/Analytics/Service/LinkTrackService.phpRepository: phpList/core
Length of output: 3550
🏁 Script executed:
cat -n src/Domain/Messaging/Service/MessageProcessingPreparator.php | head -120Repository: phpList/core
Length of output: 4292
Replace links in htmlFooter, not footer.
Links extracted from $content->htmlFooter need to be replaced there as well. Line 81 currently replaces in $cachedMessageDto->footer, but should replace in $cachedMessageDto->htmlFooter to match what was extracted.
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/MessageProcessingPreparator.php around lines 75
to 82, the code replaces links in $cachedMessageDto->footer but the extracted
links came from $content->htmlFooter; change the replacement target to
$cachedMessageDto->htmlFooter (and only if it exists) so links extracted from
htmlFooter are written back to htmlFooter rather than plain footer; keep the
existing call to $this->replaceLinks($savedLinks, ...) and mirror the same
null/empty check currently used for footer.
| ->subject($processedContent->subject) | ||
| ->text($processedContent->textContent) | ||
| // todo: check htmlFooterit should be html of textContent | ||
| ->html($processedContent->htmlFooter); | ||
| } |
There was a problem hiding this comment.
Likely bug: HTML body is set to footer only, not the full content.
The html() method is being set to $processedContent->htmlFooter, which is just the footer portion. Looking at MessagePrecacheDto, the main HTML content should come from the content field. The TODO comment at line 42 also suggests this needs review.
The email body should probably include the actual content, not just the footer.
return $email
->to($subscriber->getEmail())
->subject($processedContent->subject)
->text($processedContent->textContent)
- // todo: check htmlFooterit should be html of textContent
- ->html($processedContent->htmlFooter);
+ ->html($processedContent->content);📝 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.
| ->subject($processedContent->subject) | |
| ->text($processedContent->textContent) | |
| // todo: check htmlFooterit should be html of textContent | |
| ->html($processedContent->htmlFooter); | |
| } | |
| ->subject($processedContent->subject) | |
| ->text($processedContent->textContent) | |
| ->html($processedContent->content); | |
| } |
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/RateLimitedCampaignMailer.php around lines 40 to
44, the html body is incorrectly set to only $processedContent->htmlFooter;
change it to use the main HTML content from $processedContent->content and
append the footer (e.g. $processedContent->content .
$processedContent->htmlFooter) so the full message is included; ensure you call
->html(...) with that combined value and remove the TODO comment.
| if (strpos($htmlContent, '[SIGNATURE]')) { | ||
| $htmlContent = str_replace('[SIGNATURE]', $phpListPowered, $htmlContent); | ||
| } elseif (strpos($htmlContent, '</body>')) { | ||
| $htmlContent = str_replace('</body>', $phpListPowered.'</body>', $htmlContent); | ||
| } else { | ||
| $htmlContent .= $phpListPowered; | ||
| } |
There was a problem hiding this comment.
Use strict comparison with strpos() to avoid false negatives.
strpos() returns 0 if the needle is at the start of the string, which is falsy. If [SIGNATURE] happens to be at position 0, this condition would incorrectly skip the replacement. Use !== false instead.
Same issue applies at line 83.
- if (strpos($htmlContent, '[SIGNATURE]')) {
+ if (strpos($htmlContent, '[SIGNATURE]') !== false) {
$htmlContent = str_replace('[SIGNATURE]', $phpListPowered, $htmlContent);
} elseif (strpos($htmlContent, '</body>')) {And at line 83:
- if (strpos($textContent, '[SIGNATURE]')) {
+ if (strpos($textContent, '[SIGNATURE]') !== false) {
$textContent = str_replace('[SIGNATURE]', $phpListPowered, $textContent);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/SystemMailConstructor.php around lines 71-77
(and similarly at line 83), the code uses strpos() in a truthy check which fails
when the needle is at position 0; change those checks to use strict comparison
!== false (e.g., if (strpos($htmlContent, '[SIGNATURE]') !== false) and likewise
for '</body>') so the presence tests work correctly and replacements execute
when the substring is at the start of the string.
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.
|
|
||
| public function onMessage(MessageEvent $event): void | ||
| { | ||
| // todo: add custom header only to messages sent individually not from campaigns |
There was a problem hiding this comment.
Address the TODO: filter by message type.
The comment indicates this header should only be added to individual messages, not campaign emails. Currently, all emails sent via HTTP get this header.
Consider checking the event context or message metadata to distinguish campaign sends from individual messages. Would you like me to open an issue to track this, or do you plan to implement it in this PR?
🤖 Prompt for AI Agents
In src/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.php around
line 20, the TODO notes the custom header must be added only for individual
messages (not campaign sends); update the subscriber to inspect the
event/message context/metadata (e.g., check for a 'campaign_id' or a
sendType/campaign flag on the message/envelope or event) and only inject the
header when that indicator is absent or when sendType == 'individual'; add a
simple guard/if before header injection to skip campaign messages and add/adjust
unit tests to cover both individual and campaign cases.
| $ip = $_SERVER['REMOTE_ADDR'] ?? 'unknown'; | ||
| $host = gethostname() ?: 'unknown-host'; | ||
| $time = date('r', $_SERVER['REQUEST_TIME'] ?? time()); |
There was a problem hiding this comment.
Direct $_SERVER access raises security and architectural concerns.
Three issues here:
- Security:
REMOTE_ADDRdoesn't account for proxies/load balancers—useX-Forwarded-Foror Symfony'sRequest::getClientIp(). - Testability: Direct superglobal access makes unit testing difficult.
- Architecture: Domain layer is coupled to HTTP infrastructure; this logic typically belongs in the Infrastructure or Application layer.
Consider injecting Symfony's RequestStack and using it to safely retrieve client information:
+use Symfony\Component\HttpFoundation\RequestStack;
+
class InjectedByHeaderSubscriber implements EventSubscriberInterface
{
+ public function __construct(private readonly RequestStack $requestStack)
+ {
+ }
+
public function onMessage(MessageEvent $event): void
{
// ...
- if (PHP_SAPI === 'cli')
+ $request = $this->requestStack->getCurrentRequest();
+ if ($request === null)
{
return;
}
- $ip = $_SERVER['REMOTE_ADDR'] ?? 'unknown';
+ $ip = $request->getClientIp() ?? 'unknown';
$host = gethostname() ?: 'unknown-host';
- $time = date('r', $_SERVER['REQUEST_TIME'] ?? time());
+ $time = date('r', $request->server->getInt('REQUEST_TIME', time()));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.php around
lines 34 to 36, direct access to $_SERVER for REMOTE_ADDR, hostname and
REQUEST_TIME should be removed: inject
Symfony\Component\HttpFoundation\RequestStack (or better, accept an IP/metadata
provider via constructor) from the Infrastructure/Application layer, use
RequestStack->getCurrentRequest() (or the provider) to obtain client IP via
Request::getClientIp() and headers like X-Forwarded-For, fall back to a sensible
default when null, and obtain timestamp from a DateTimeImmutable passed in or
Request rather than $_SERVER; refactor the subscriber to receive these
dependencies through constructor injection so the domain layer has no direct
superglobal/HTTP coupling and becomes testable.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.
Thanks for contributing to phpList!