Feat/user personalizer #375
Conversation
📝 WalkthroughWalkthroughThe PR introduces output format-aware placeholder resolution with extensible resolver implementations, refactors email/message construction through new MailConstructor services, adds configuration parameters for preference page and forwarding settings, adopts constructor property promotion across multiple message handlers, and extends repositories with subscriber/list data access methods. Changes
Sequence DiagramsequenceDiagram
participant Handler as CampaignProcessorMessageHandler
participant MailConstructor
participant UserPersonalizer
participant PlaceholderResolvers
participant Mailer as RateLimitedCampaignMailer
Handler->>MailConstructor: build(message, subscriber, precacheDto)
MailConstructor->>MailConstructor: loadDefaultHtmlStyle()
MailConstructor->>MailConstructor: replaceUserSpecificRemoteContent()
MailConstructor->>UserPersonalizer: personalize(content, email, OutputFormat::Html)
UserPersonalizer->>PlaceholderResolvers: iterate resolvers
PlaceholderResolvers->>PlaceholderResolvers: resolve placeholders with PlaceholderContext
PlaceholderResolvers-->>UserPersonalizer: resolved value (format-aware)
UserPersonalizer-->>MailConstructor: personalized html content
MailConstructor->>UserPersonalizer: personalize(content, email, OutputFormat::Text)
UserPersonalizer->>PlaceholderResolvers: iterate resolvers (text mode)
PlaceholderResolvers-->>UserPersonalizer: resolved value (plain text)
UserPersonalizer-->>MailConstructor: personalized text content
MailConstructor->>MailConstructor: apply templates, set footers
MailConstructor-->>Handler: Email object
Handler->>Mailer: send(email)
Mailer-->>Handler: success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
198-212: Named parameters improve clarity; minor formatting nit.Good adoption of named parameters and
OutputFormat::Textfor text content personalization. Small inconsistency on line 206: missing space after colon (format:OutputFormat::Textvsformat: OutputFormat::Texton line 211).🔎 Suggested fix
$processed->textContent = $this->userPersonalizer->personalize( value: $processed->textContent, email: $subscriber->getEmail(), - format:OutputFormat::Text, + format: OutputFormat::Text, );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
config/parameters.yml.distconfig/services/services.ymlsrc/Domain/Configuration/Model/OutputFormat.phpsrc/Domain/Configuration/Service/UserPersonalizer.phpsrc/Domain/Messaging/MessageHandler/AsyncEmailMessageHandler.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/MessageHandler/PasswordResetMessageHandler.phpsrc/Domain/Messaging/MessageHandler/SubscriberConfirmationMessageHandler.phpsrc/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandler.phpsrc/Domain/Messaging/Service/MessagePrecacheService.phpsrc/Domain/Subscription/Repository/SubscriberListRepository.phpsrc/Domain/Subscription/Service/SubscriberCsvImporter.php
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow, flag ANY DB write / finalization:
$entityManager->flush(...),$this->entityManager->flush(...)$em->beginTransaction(),$em->commit(),$em->rollback(),$em->transactional(...)$em->getConnection()->executeStatement(...)for DML/DDL (INSERT/UPDATE/DELETE/ALTER/...)✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes. Accessing Doctrine persistence APIs
persist, remove, etc.) is acceptable, allow scheduling changes in the UnitOfWork (no DB writes)✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Configuration/Model/OutputFormat.phpsrc/Domain/Messaging/Service/MessagePrecacheService.phpsrc/Domain/Messaging/MessageHandler/AsyncEmailMessageHandler.phpsrc/Domain/Subscription/Repository/SubscriberListRepository.phpsrc/Domain/Configuration/Service/UserPersonalizer.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Subscription/Service/SubscriberCsvImporter.phpsrc/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandler.phpsrc/Domain/Messaging/MessageHandler/PasswordResetMessageHandler.phpsrc/Domain/Messaging/MessageHandler/SubscriberConfirmationMessageHandler.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/AsyncEmailMessageHandler.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandler.phpsrc/Domain/Messaging/MessageHandler/PasswordResetMessageHandler.phpsrc/Domain/Messaging/MessageHandler/SubscriberConfirmationMessageHandler.php
🧬 Code graph analysis (7)
src/Domain/Messaging/MessageHandler/AsyncEmailMessageHandler.php (6)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
__construct(52-73)src/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandler.php (1)
__construct(24-31)src/Domain/Messaging/MessageHandler/PasswordResetMessageHandler.php (1)
__construct(16-21)src/Domain/Messaging/MessageHandler/SubscriberConfirmationMessageHandler.php (1)
__construct(19-24)src/Domain/Subscription/Service/SubscriberCsvImporter.php (1)
__construct(37-48)src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
__construct(20-26)
src/Domain/Subscription/Repository/SubscriberListRepository.php (1)
src/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandler.php (1)
getListNames(59-62)
src/Domain/Configuration/Service/UserPersonalizer.php (1)
src/Domain/Subscription/Repository/SubscriberListRepository.php (2)
SubscriberListRepository(23-94)getActiveListNamesForSubscriber(78-93)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (3)
src/Domain/Configuration/Service/UserPersonalizer.php (1)
personalize(32-128)src/Domain/Messaging/Message/SubscriberConfirmationMessage.php (1)
getEmail(26-29)src/Domain/Messaging/Message/SubscriptionConfirmationMessage.php (1)
getEmail(29-32)
src/Domain/Subscription/Service/SubscriberCsvImporter.php (5)
src/Domain/Subscription/Service/Manager/SubscriberManager.php (1)
SubscriberManager(19-143)src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php (1)
SubscriberAttributeManager(17-88)src/Domain/Subscription/Repository/SubscriberRepository.php (1)
SubscriberRepository(21-208)src/Domain/Subscription/Service/CsvToDtoImporter.php (1)
CsvToDtoImporter(14-62)src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php (1)
SubscriberHistoryManager(18-143)
src/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandler.php (4)
src/Domain/Messaging/Service/EmailService.php (1)
EmailService(14-134)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Service/UserPersonalizer.php (2)
UserPersonalizer(16-129)personalize(32-128)src/Domain/Subscription/Repository/SubscriberListRepository.php (2)
SubscriberListRepository(23-94)getListNames(58-72)
src/Domain/Messaging/MessageHandler/SubscriberConfirmationMessageHandler.php (3)
src/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandler.php (1)
__construct(24-31)src/Domain/Messaging/MessageHandler/AsyncEmailMessageHandler.php (1)
__construct(17-19)src/Domain/Messaging/MessageHandler/PasswordResetMessageHandler.php (1)
__construct(16-21)
🪛 GitHub Actions: phpList Core Build
src/Domain/Configuration/Service/UserPersonalizer.php
[error] 20-20: Too few arguments to function PhpList\Core\Domain\Configuration\Service\UserPersonalizer::__construct(), 5 passed in /home/runner/work/core/core/tests/Unit/Domain/Configuration/Service/UserPersonalizerTest.php on line 42 and at least 7 expected
src/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandler.php
[error] 46-46: Expectation failed for method name is "personalize" when invoked 1 time(s). Parameter 0 for invocation UserPersonalizer::personalize('Hi {{name}}, you subscribed to: ', 'user-123', OutputFormat): string does not match expected value. Expected 'Hi {{name}}, you subscribed to: [LISTS]' but got 'Hi {{name}}, you subscribed to: '
🔇 Additional comments (14)
src/Domain/Messaging/Service/MessagePrecacheService.php (1)
198-198: Formatting looks good.The string concatenation adjustment is fine and has no behavioral impact.
Optional note: you might want to verify that
$loadedMessageData['sendurl']is the intended value to log here, since the actual fetch attempt uses$regs[1]from line 185. If they differ, logging$regs[1]might provide clearer debugging context. (Pre-existing pattern, so not blocking.)src/Domain/Messaging/MessageHandler/AsyncEmailMessageHandler.php (1)
17-19: Nice refactor to constructor property promotion.Clean modernization using PHP 8's constructor property promotion. Consistent with the pattern adopted across other message handlers in this PR.
src/Domain/Subscription/Service/SubscriberCsvImporter.php (1)
37-48: Constructor property promotion looks good.All 9 dependencies are now using PHP 8's constructor property promotion with
private readonly. The class docblock appropriately notes this is an application-layer service exception for complex import orchestration.src/Domain/Configuration/Model/OutputFormat.php (1)
1-11: Clean enum definition.Simple and focused backed enum for output formatting. Good use of PHP 8.1 features. The lowercase string values follow common conventions.
config/parameters.yml.dist (1)
34-35: Parameter addition follows existing conventions.The new parameter uses the same pattern as other boolean-like environment variables in this file (string '0'/'1' defaults). Environment variable naming is consistent.
src/Domain/Messaging/MessageHandler/PasswordResetMessageHandler.php (1)
16-21: Constructor property promotion looks solid.Clean refactor consistent with the other message handlers in this PR. All dependencies properly marked as
private readonly.src/Domain/Subscription/Repository/SubscriberListRepository.php (2)
58-72: Clean implementation with proper empty-check guard.Good practice returning early for empty input. The query is straightforward and uses proper parameter binding.
74-93: The semantic mapping is correct; no bug found.The database column
activeis intentionally mapped to the entity property$publicwith getterisPublic()(lines 155–157 in SubscriberList.php). The filterl.active = truewhen$showPrivate = falsecorrectly returns public (active) lists. The docblock accurately describes this: "only active/public lists are included." No changes needed.src/Domain/Messaging/MessageHandler/SubscriberConfirmationMessageHandler.php (1)
19-24: LGTM! Clean constructor property promotion.The refactor to use constructor property promotion is clean and consistent with the pattern used in other message handlers.
src/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandler.php (1)
24-30: Constructor property promotion looks good.Clean refactor to use promoted properties, consistent with the pattern applied across other handlers.
src/Domain/Configuration/Service/UserPersonalizer.php (4)
32-32: personalize() signature enhanced with OutputFormat.The addition of the
OutputFormat $formatparameter enables format-aware rendering. This is a breaking change, but the enhanced functionality justifies it and I can see it's being adopted in the handlers (e.g., SubscriptionConfirmationMessageHandler).
42-85: URL rendering with format-aware HTML link generation looks solid.The implementation properly handles both Html and Text formats:
- HTML format generates anchor tags with translated labels
- Proper escaping applied (
ENT_QUOTES | ENT_SUBSTITUTE)- Text format returns plain URLs
- Consistent pattern across all three URL placeholders
94-115: LISTS placeholder implementation is well-designed.The logic handles all the important cases:
- Fetches list names via repository with privacy flag support
- Graceful empty-state handling with a translated message
- Format-aware rendering (HTML
<br/>vs text newline)- Proper HTML escaping when needed
- Clean use of named parameters for clarity
20-30: Test constructor call is incomplete - needs the three new dependencies.The
UserPersonalizerTest.phpinstantiation at line 37 passes only 5 arguments, but the constructor now requires 8 (with 1 optional default). Add mocks forSubscriberListRepositoryandTranslatorInterfaceto the test setup and pass them to the constructor.
| PhpList\Core\Domain\Configuration\Service\UserPersonalizer: | ||
| autowire: true | ||
| autoconfigure: true | ||
| arguments: | ||
| $preferencePageShowPrivateLists: '%app.preference_page_show_private_lists%' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the UserPersonalizer class file
find . -name "*.php" -type f | xargs grep -l "class UserPersonalizer" | head -5Repository: phpList/core
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Check the UserPersonalizer constructor and parameter usage
ast-grep --pattern 'class UserPersonalizer {
$$$
public function __construct($$$) {
$$$
}
$$$
}'Repository: phpList/core
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Also search for parameters.yml.dist
find . -name "parameters.yml*" -type fRepository: phpList/core
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Read the UserPersonalizer class to check constructor and parameter handling
cat -n ./src/Domain/Configuration/Service/UserPersonalizer.phpRepository: phpList/core
Length of output: 6367
🏁 Script executed:
#!/bin/bash
# Read parameters.yml.dist to check the parameter definition
cat -n ./config/parameters.yml.dist | grep -A 5 -B 5 "preference_page_show_private_lists"Repository: phpList/core
Length of output: 595
String-to-boolean conversion will cause incorrect behavior.
The parameter $preferencePageShowPrivateLists is type-hinted as bool (line 28 of UserPersonalizer.php), but the services configuration passes a string value from parameters.yml.dist. In PHP, (bool)'0' evaluates to true (non-empty string), not false. This means the feature will behave opposite to the intended default—always showing private lists when it should hide them.
Use an integer instead of a string in parameters.yml.dist:
env(PREFERENCEPAGE_SHOW_PRIVATE_LISTS): 0Or apply explicit casting in services.yml using !php/const or a cast operator.
🤖 Prompt for AI Agents
In config/services/services.yml around lines 123 to 127, the
$preferencePageShowPrivateLists argument is receiving a string parameter which
will convert to true in PHP even for "0"; change the wiring so a boolean is
passed: either update parameters.yml.dist to supply an integer/env var (e.g. use
env(...) with 0/1) or explicitly cast/resolve the parameter to a boolean in
services.yml (use a PHP cast or a proper YAML PHP constant resolution) so the
UserPersonalizer receives a real bool not a non-empty string.
| $replacedTextContent = str_replace('[LISTS]', $listOfLists, $textContent); | ||
|
|
||
| $personalizedTextContent = $this->userPersonalizer->personalize( | ||
| value: $replacedTextContent, | ||
| email: $message->getUniqueId(), | ||
| format: OutputFormat::Text, | ||
| ); |
There was a problem hiding this comment.
Critical: [LISTS] replacement conflicts with UserPersonalizer.
The flow now replaces [LISTS] before calling personalize(), but UserPersonalizer expects to handle the [LISTS] placeholder itself (see lines 94-115 in UserPersonalizer.php). This creates duplication and breaks the expected behavior.
The pipeline failure confirms this:
Expected 'Hi {{name}}, you subscribed to: [LISTS]' but got 'Hi {{name}}, you subscribed to: '
The test expects [LISTS] to be present when personalize() is called so that UserPersonalizer can handle the replacement with proper formatting.
🔎 Proposed fix
Remove the manual [LISTS] replacement and let UserPersonalizer handle it:
$subject = $this->configProvider->getValue(ConfigOption::SubscribeEmailSubject);
$textContent = $this->configProvider->getValue(ConfigOption::SubscribeMessage);
- $listOfLists = $this->getListNames($message->getListIds());
- $replacedTextContent = str_replace('[LISTS]', $listOfLists, $textContent);
-
- $personalizedTextContent = $this->userPersonalizer->personalize(
- value: $replacedTextContent,
+
+ $personalizedTextContent = $this->userPersonalizer->personalize(
+ value: $textContent,
email: $message->getUniqueId(),
format: OutputFormat::Text,
);If you need to ensure the list IDs are available to the personalizer, that should be handled through the subscriber's subscriptions rather than passing them separately.
Also applies to: 52-52, 61-61
🧰 Tools
🪛 GitHub Actions: phpList Core Build
[error] 46-46: Expectation failed for method name is "personalize" when invoked 1 time(s). Parameter 0 for invocation UserPersonalizer::personalize('Hi {{name}}, you subscribed to: ', 'user-123', OutputFormat): string does not match expected value. Expected 'Hi {{name}}, you subscribed to: [LISTS]' but got 'Hi {{name}}, you subscribed to: '
🤖 Prompt for AI Agents
In
src/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandler.php
around lines 41-47 (and similarly at lines 52 and 61), the code is pre-replacing
the [LISTS] placeholder before calling UserPersonalizer which causes duplication
and breaks personalization; remove the manual str_replace calls that replace
[LISTS] so you call $this->userPersonalizer->personalize(...) with the original
text containing [LISTS], and ensure any needed list/subscription data is
available through the subscriber model passed into the personalizer rather than
passing or injecting the replaced string.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
198-212: Looks good overall, minor spacing nit.The switch to named parameters makes the code much more readable. Using
OutputFormat::Textfor bothtextContentandfooteris the right call for plain text parts.Small thing: Line 206 is missing a space after
format:while line 211 has it. Consistency would be nice, but no biggie.- format:OutputFormat::Text, + format: OutputFormat::Text,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/MessagePrecacheService.php
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow, flag ANY DB write / finalization:
$entityManager->flush(...),$this->entityManager->flush(...)$em->beginTransaction(),$em->commit(),$em->rollback(),$em->transactional(...)$em->getConnection()->executeStatement(...)for DML/DDL (INSERT/UPDATE/DELETE/ALTER/...)✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes. Accessing Doctrine persistence APIs
persist, remove, etc.) is acceptable, allow scheduling changes in the UnitOfWork (no DB writes)✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/MessagePrecacheService.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/MessageHandler/CampaignProcessorMessageHandler.php (3)
src/Domain/Configuration/Service/UserPersonalizer.php (1)
personalize(32-128)src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
buildPhplistEmail(40-77)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
send(50-55)
⏰ 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/MessageHandler/CampaignProcessorMessageHandler.php (1)
11-11: LGTM on the import.Clean addition of the
OutputFormatenum to support format-aware personalization. Nice and tidy.src/Domain/Messaging/Service/MessagePrecacheService.php (2)
71-73: Clean refactor to unconditional call.Making
applyRemoteContentIfPresentalways run and return a bool is a nice simplification. The early-exit pattern inside the method handles the user-specific URL case, so the caller doesn't need to worry about it. Solid.
199-206: Template reset logic is well-documented.Appreciate the comment referencing issue #17086 - makes it clear why templates are discarded when remote content is used. The null assignments are explicit and intentional.
| $email = $this->emailBuilder->buildPhplistEmail( | ||
| $campaign->getId(), | ||
| $subscriber->getEmail(), | ||
| $processed->subject, | ||
| $processed->textContent, | ||
| ); | ||
| $this->rateLimitedCampaignMailer->send($email); |
There was a problem hiding this comment.
Missing null check on buildPhplistEmail return value.
Hey, buildPhplistEmail can return null (e.g., if recipient validation fails or blacklist check triggers). Passing null to rateLimitedCampaignMailer->send() will throw a TypeError.
You've got the right pattern in handleAdminNotifications (lines 258-260) where you check for null and continue. Consider something similar here:
🔎 Proposed fix
$email = $this->emailBuilder->buildPhplistEmail(
$campaign->getId(),
$subscriber->getEmail(),
$processed->subject,
$processed->textContent,
);
+ if ($email === null) {
+ $this->updateUserMessageStatus($userMessage, UserMessageStatus::NotSent);
+ $this->logger->warning($this->translator->trans('Email could not be built for: %email%', [
+ '%email%' => $subscriber->getEmail(),
+ ]));
+ return;
+ }
$this->rateLimitedCampaignMailer->send($email);🤖 Prompt for AI Agents
In src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
around lines 215 to 221, the result of
$this->emailBuilder->buildPhplistEmail(...) can be null and is passed directly
to $this->rateLimitedCampaignMailer->send(...), causing a TypeError; add a null
check after building the email (mirror the pattern used in
handleAdminNotifications at lines ~258-260): if the built email is null, skip
sending by continuing the loop (or otherwise return/skip this recipient); only
call send() when the email is non-null.
| if ( | ||
| $messagePrecacheDto->userSpecificUrl | ||
| || !preg_match('/\[URL:([^\s]+)\]/i', $messagePrecacheDto->content, $regs) | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| $remoteContent = ($this->remotePageFetcher)($regs[1], []); | ||
| if (!$remoteContent) { | ||
| $this->eventLogManager->log( | ||
| page: 'unknown page', | ||
| entry: 'Error fetching URL: ' . $loadedMessageData['sendurl'] . ' cannot proceed', | ||
| ); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| $messagePrecacheDto->content = str_replace($regs[0], $remoteContent, $messagePrecacheDto->content); | ||
| $messagePrecacheDto->htmlFormatted = $this->isHtml($remoteContent); | ||
|
|
||
| //# 17086 - disregard any template settings when we have a valid remote URL | ||
| $messagePrecacheDto->template = null; | ||
| $messagePrecacheDto->templateText = null; | ||
| $messagePrecacheDto->templateId = null; | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Logic looks solid, but there's a logging mismatch.
The restructured flow is clean and easy to follow. Early returns for the no-op cases, then fetch → fail-fast → replace → reset template. 👍
One thing though: on line 189 you fetch from $regs[1] (the URL extracted from content), but on line 193 you log $loadedMessageData['sendurl']. These might be different values, which could make debugging a head-scratcher.
🔎 Suggested fix
$remoteContent = ($this->remotePageFetcher)($regs[1], []);
if (!$remoteContent) {
$this->eventLogManager->log(
page: 'unknown page',
- entry: 'Error fetching URL: ' . $loadedMessageData['sendurl'] . ' cannot proceed',
+ entry: 'Error fetching URL: ' . $regs[1] . ' cannot proceed',
);
return false;
}🤖 Prompt for AI Agents
In src/Domain/Messaging/Service/MessagePrecacheService.php around lines 182 to
207, the error log uses $loadedMessageData['sendurl'] while the fetch actually
uses $regs[1], which can mislead debugging; update the EventLogManager call to
log the actual URL used for the fetch (use $regs[1]) and optionally include
$loadedMessageData['sendurl'] as additional context if it exists, ensuring you
reference only defined variables to avoid notices.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
config/services/services.yml (1)
156-160: String-to-boolean conversion issue remains unaddressed.As noted in the previous review, the
$preferencePageShowPrivateListsparameter is type-hinted asboolbut receives a string value from the configuration. This causes(bool)'0'to evaluate totrueinstead offalse, resulting in incorrect behavior where private lists are always shown when they should be hidden.The previous review suggested using an integer in parameters.yml.dist or applying explicit casting in the service configuration. Please address this before merging.
🧹 Nitpick comments (1)
src/Domain/Messaging/Exception/RemotePageFetchException.php (1)
11-14: Consider accepting a URL parameter for better debugging context.The fixed error message lacks information about which URL failed to fetch. While this might be logged elsewhere, including the URL in the exception message would make debugging easier when multiple remote pages are being fetched.
🔎 Proposed enhancement
- public function __construct() + public function __construct(?string $url = null) { - parent::__construct('Failed to fetch URL for subscriber'); + $message = 'Failed to fetch URL for subscriber'; + if ($url !== null) { + $message .= ': ' . $url; + } + parent::__construct($message); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
config/parameters.yml.distconfig/services/builders.ymlconfig/services/messenger.ymlconfig/services/parameters.ymlconfig/services/repositories.ymlconfig/services/services.ymlsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Messaging/Exception/RemotePageFetchException.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.phpsrc/Domain/Messaging/Service/Constructor/MailConstructorInterface.phpsrc/Domain/Messaging/Service/Constructor/SystemMailConstructor.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Subscription/Repository/SubscriberRepository.phptests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.phptests/Unit/Domain/Messaging/Service/Builder/EmailBuilderTest.phptests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- config/parameters.yml.dist
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow, flag ANY DB write / finalization:
$entityManager->flush(...),$this->entityManager->flush(...)$em->beginTransaction(),$em->commit(),$em->rollback(),$em->transactional(...)$em->getConnection()->executeStatement(...)for DML/DDL (INSERT/UPDATE/DELETE/ALTER/...)✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes. Accessing Doctrine persistence APIs
persist, remove, etc.) is acceptable, allow scheduling changes in the UnitOfWork (no DB writes)✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Subscription/Repository/SubscriberRepository.phpsrc/Domain/Messaging/Exception/RemotePageFetchException.phpsrc/Domain/Common/RemotePageFetcher.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Constructor/SystemMailConstructor.phpsrc/Domain/Messaging/Service/Constructor/MailConstructorInterface.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/Service/Builder/EmailBuilder.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/Exception/RemotePageFetchException.php (2)
src/Domain/Common/RemotePageFetcher.php (1)
__construct(19-29)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
__construct(20-27)
tests/Unit/Domain/Messaging/Service/Builder/EmailBuilderTest.php (1)
src/Domain/Messaging/Service/Constructor/SystemMailConstructor.php (1)
SystemMailConstructor(13-103)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
src/Domain/Configuration/Service/UserPersonalizer.php (1)
personalize(32-128)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
composeEmail(29-55)
tests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php (1)
src/Domain/Messaging/Service/Constructor/SystemMailConstructor.php (1)
SystemMailConstructor(13-103)
src/Domain/Messaging/Service/Constructor/SystemMailConstructor.php (1)
src/Domain/Messaging/Service/Constructor/MailConstructorInterface.php (1)
__invoke(17-17)
src/Domain/Messaging/Service/Constructor/MailConstructorInterface.php (1)
src/Domain/Messaging/Service/Constructor/SystemMailConstructor.php (1)
__invoke(27-73)
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (4)
src/Domain/Common/RemotePageFetcher.php (1)
RemotePageFetcher(17-125)src/Domain/Messaging/Exception/RemotePageFetchException.php (1)
RemotePageFetchException(9-15)src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Subscription/Repository/SubscriberRepository.php (2)
SubscriberRepository(21-218)getDataById(209-217)
🔇 Additional comments (23)
config/services/repositories.yml (1)
25-28: Configuration is solid—the classes are properly wired.The new
UrlCacheRepositoryservice definition follows the established pattern and both referenced classes exist in the codebase. Standard DI setup with the parent repository and model argument all checks out.src/Domain/Messaging/Service/Constructor/MailConstructorInterface.php (1)
1-18: Clean interface abstraction!This interface provides a clear contract for mail construction and enables flexible dependency injection. The documentation is precise and the return type tuple is well-defined.
tests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php (1)
12-12: Namespace import correctly updated.The import path properly reflects the new location of
SystemMailConstructorin theConstructornamespace.tests/Unit/Domain/Messaging/Service/Builder/EmailBuilderTest.php (2)
11-11: Import path correctly updated.Properly reflects the new namespace for
SystemMailConstructor.
72-72: Parameter rename aligns with interface-based dependency.Changing from
systemMailConstructortomailConstructorcorrectly reflects thatEmailBuildernow depends onMailConstructorInterfacerather than a specific implementation.config/services/messenger.yml (1)
34-37: DI configuration is properly structured.The service configuration follows Symfony conventions with appropriate autowiring and argument injection. Both referenced dependencies check out:
Core.EmailBuilder.systemis defined inbuilders.ymlandapp.config.message_from_addressis defined inparameters.yml.config/services/builders.yml (1)
27-50: LGTM! Clean separation of email builder variants.The service definitions properly wire two EmailBuilder instances with different mail constructors while sharing common configuration parameters. This design allows for clean separation between system and campaign email construction logic.
config/services/services.yml (3)
46-77: LGTM! Well-documented service definitions.The service definitions for content processing components (Html2Text, HtmlUrlRewriter, ExternalImageService, TemplateImageEmbedder) are properly configured with clear comments explaining their purpose and sensible default values.
178-192: LGTM! Clean service definitions for message processing.The service definitions for MessageDataLoader, TextParser, and RemotePageFetcher are properly configured with appropriate parameter bindings and clear documentation.
194-201: LGTM! MessagePrecacheService properly configured.The service definition includes all necessary parameters for pre-caching campaign message content with appropriate parameter bindings.
config/services/parameters.yml (1)
1-16: LGTM! Pragmatic dual-format parameter configuration.The approach of providing both flattened parameters (for direct DI) and grouped arrays (for legacy/config-provider usage) is well-documented and pragmatic, addressing Symfony's limitation with dot-notation access into parameter arrays.
src/Domain/Common/RemotePageFetcher.php (2)
81-92: LGTM! Better error semantics with null return.Returning
nullon fetch failure (instead of an empty string) provides clearer semantics—an empty string could be valid content, whereasnullexplicitly signals failure. This aligns well with the nullable return type and makes error handling more explicit.
31-31: Null handling already in place.The return type change to
?stringis justified by the implementation. Both callers (RateLimitedCampaignMailerandMessagePrecacheService) properly handle null returns—one with an explicit=== nullcheck that throws an exception, the other with a falsy check that returns early. No action needed.src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
11-11: Nice refactor to interface-based dependency.Switching from
SystemMailConstructortoMailConstructorInterfaceis a solid move for flexibility and testability. The callable invocation pattern on line 63 is clean.Also applies to: 29-29, 63-63
65-72: Named arguments improve readability here.Using named arguments for
createBaseEmailmakes the call site self-documenting. Good stuff.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
198-212: Named arguments and OutputFormat usage look good.The explicit
OutputFormat::Textparameter makes the personalization intent clear. Named arguments throughout improve readability.
214-221: Email composition flow updated correctly.The new flow using
composeEmail+sendonrateLimitedCampaignMaileris clean. The previous concern about null checks frombuildPhplistEmailno longer applies here sincecomposeEmailreturns a non-nullablesrc/Domain/Messaging/Service/Constructor/SystemMailConstructor.php (2)
5-5: Clean interface implementation.The namespace move to
Constructorsubdirectory andMailConstructorInterfaceimplementation are well done. The explicitstring $contenttype improves type safety.Also applies to: 13-13, 27-29
75-75: Good type annotation on private method.Adding explicit
stringtype to$messageparameter aligns with the interface contract.src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (4)
20-27: Constructor promotion is clean.The new dependencies are well organized with readonly property promotion. Good use of dependency injection.
35-38: User-specific content handling added.Good check for the
userSpecificUrlflag before doing the additional work. The flow is clear: fetch user data, then replace remote content.
67-103: Remote content replacement implementation looks solid.The regex pattern for
[URL:...]tokens, URL scheme normalization, and error handling with proper exception throwing are all well implemented.One small consideration: the method mutates
$messagePrecacheDtodirectly rather than returning a new instance. This is fine for a service-layer operation, just worth noting for anyone reading the code later.
89-96: Good error handling for remote fetch failures.Logging the failure before throwing
RemotePageFetchExceptionprovides good observability. However, this will abort the entire email send for this subscriber.Is this the intended behavior? If one
[URL:...]token fails to fetch, the whole email fails. Consider whether you'd want to:
- Skip that specific token and continue (graceful degradation)
- Fail the entire message (current behavior - strict mode)
If strict is intended, this is fine as-is.
| public function getDataById(int $subscriberId): array | ||
| { | ||
| return $this->createQueryBuilder('s') | ||
| ->select('s') | ||
| ->where('s.id = :subscriberId') | ||
| ->setParameter('subscriberId', $subscriberId) | ||
| ->getQuery() | ||
| ->getArrayResult(); | ||
| } |
There was a problem hiding this comment.
getArrayResult() returns an array of rows, not a single row.
When querying by a unique ID, getArrayResult() will return [['id' => 1, 'email' => '...', ...]] if found or [] if not found. This is probably not what callers expect for a "get by ID" method.
Consider using getOneOrNullResult() or accessing the first element explicitly.
🔎 Proposed fix
public function getDataById(int $subscriberId): array
{
- return $this->createQueryBuilder('s')
+ $result = $this->createQueryBuilder('s')
->select('s')
->where('s.id = :subscriberId')
->setParameter('subscriberId', $subscriberId)
->getQuery()
- ->getArrayResult();
+ ->getOneOrNullResult(\Doctrine\ORM\Query::HYDRATE_ARRAY);
+
+ return $result ?? [];
}🤖 Prompt for AI Agents
In src/Domain/Subscription/Repository/SubscriberRepository.php around lines
209-217, the method uses getArrayResult() which returns an array of rows rather
than a single row; change the query to fetch a single associative row by
replacing getArrayResult() with getOneOrNullResult(AbstractQuery::HYDRATE_ARRAY)
(or setMaxResults(1) and take the first element), and ensure the method returns
an associative array for a found subscriber or an empty array (or null if
preferred by callers) when none is found.
| // campaign emails are built via campaignEmailBuilder and sent via RateLimitedCampaignMailer | ||
| $campaignEmailBuilder = (new ReflectionClass($this->handler)) | ||
| ->getProperty("campaignEmailBuilder"); | ||
| /** @var EmailBuilder|MockObject $campaignBuilderMock */ | ||
| $campaignBuilderMock = $campaignEmailBuilder->getValue($this->handler); | ||
|
|
||
| return (new Email()) | ||
| $campaignBuilderMock->expects($this->once()) | ||
| ->method('buildPhplistEmail') | ||
| ->willReturn( | ||
| (new Email()) | ||
| ->from('news@example.com') | ||
| ->to('test@example.com') | ||
| ->subject('Test Subject') | ||
| ->text('Test text message') | ||
| ->html('<p>Test HTML message</p>'); | ||
| }); | ||
| ->html('<p>Test HTML message</p>') | ||
| ); |
There was a problem hiding this comment.
Property name mismatch will cause test failure.
The test tries to access a property named campaignEmailBuilder, but looking at the handler constructor (line 69), the property is actually named emailBuilder. This will throw a ReflectionException at runtime.
🔎 Proposed fix
// campaign emails are built via campaignEmailBuilder and sent via RateLimitedCampaignMailer
$campaignEmailBuilder = (new ReflectionClass($this->handler))
- ->getProperty("campaignEmailBuilder");
+ ->getProperty("emailBuilder");
/** @var EmailBuilder|MockObject $campaignBuilderMock */
$campaignBuilderMock = $campaignEmailBuilder->getValue($this->handler);🤖 Prompt for AI Agents
In
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php
around lines 233 to 248, the test uses Reflection to access a non-existent
property campaignEmailBuilder which will throw a ReflectionException because the
handler property is named emailBuilder (see constructor at line 69); update the
ReflectionClass->getProperty call to use "emailBuilder" and adjust the local
variable name if desired so the mock is retrieved from the correct property,
then keep the existing expectation/method stubbing against that mock.
| $campaignBuilderMock->expects($this->once()) | ||
| ->method('buildPhplistEmail') | ||
| ->willReturn( | ||
| (new Email()) | ||
| ->from('news@example.com') | ||
| ->to('test@example.com') | ||
| ->subject('Test Subject') | ||
| ->text('Test text message') | ||
| ->html('<p>Test HTML message</p>'); | ||
| }); | ||
| ->html('<p>Test HTML message</p>') | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php | sed -n '210,225p'Repository: phpList/core
Length of output: 912
🏁 Script executed:
cat -n tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php | sed -n '235,250p'Repository: phpList/core
Length of output: 834
🏁 Script executed:
rg -n "composeEmail|buildPhplistEmail" src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpRepository: phpList/core
Length of output: 197
🏁 Script executed:
cat -n src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php | sed -n '240,260p'Repository: phpList/core
Length of output: 1169
🏁 Script executed:
cat -n src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php | sed -n '1,50p'Repository: phpList/core
Length of output: 2747
🏁 Script executed:
cat -n tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php | sed -n '225,260p'Repository: phpList/core
Length of output: 1648
🏁 Script executed:
cat -n tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php | sed -n '180,240p'Repository: phpList/core
Length of output: 2960
🏁 Script executed:
cat -n tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php | sed -n '300,310p'Repository: phpList/core
Length of output: 747
🏁 Script executed:
cat -n tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php | sed -n '378,395p'Repository: phpList/core
Length of output: 1027
🏁 Script executed:
cat -n src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php | sed -n '50,120p'Repository: phpList/core
Length of output: 3855
🏁 Script executed:
rg -n "campaignEmailBuilder|rateLimitedCampaignMailer" src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php | head -20Repository: phpList/core
Length of output: 274
🏁 Script executed:
cat -n src/Domain/Messaging/Service/RateLimitedCampaignMailer.php | head -50Repository: phpList/core
Length of output: 2252
🏁 Script executed:
rg -n "composeEmail" src/Domain/Messaging/Service/RateLimitedCampaignMailer.php -A 10 | head -30Repository: phpList/core
Length of output: 522
🏁 Script executed:
cat -n tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php | sed -n '1,50p'Repository: phpList/core
Length of output: 2885
🏁 Script executed:
cat -n tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php | sed -n '50,100p'Repository: phpList/core
Length of output: 3141
🏁 Script executed:
rg -n "campaignEmailBuilder|mailer" tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php | head -20Repository: phpList/core
Length of output: 1268
🏁 Script executed:
rg -n "campaignEmailBuilder" src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php -B 2 -A 2Repository: phpList/core
Length of output: 38
🏁 Script executed:
cat -n src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php | sed -n '50,75p'Repository: phpList/core
Length of output: 1592
🏁 Script executed:
rg -n "campaignEmailBuilder" src/ -t phpRepository: phpList/core
Length of output: 38
🏁 Script executed:
cat -n tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php | sed -n '233,240p'Repository: phpList/core
Length of output: 574
🏁 Script executed:
python3 << 'EOF'
# Check if property "campaignEmailBuilder" exists in the handler
import subprocess
result = subprocess.run(
['grep', '-n', 'campaignEmailBuilder', 'src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php'],
capture_output=True,
text=True
)
print("Exit code:", result.returncode)
print("Output:", result.stdout)
print("Stderr:", result.stderr)
EOFRepository: phpList/core
Length of output: 86
🏁 Script executed:
git ls-files src/Domain/Messaging/MessageHandler/ | head -20Repository: phpList/core
Length of output: 416
🏁 Script executed:
cat -n tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php | sed -n '260,280p'Repository: phpList/core
Length of output: 1137
🏁 Script executed:
cat -n src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php | grep -n "private\|public" | head -30Repository: phpList/core
Length of output: 2288
🏁 Script executed:
python3 << 'EOF'
# Let's trace through the handler constructor to understand the properties
import re
with open('src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php', 'r') as f:
content = f.read()
# Find all constructor parameters
matches = re.findall(r'private readonly (\w+) \$(\w+)', content)
print("Constructor parameters:")
for type_hint, param_name in matches:
print(f" ${param_name}: {type_hint}")
# Check if campaignEmailBuilder exists anywhere
if 'campaignEmailBuilder' in content:
print("\nFound 'campaignEmailBuilder' in handler file")
else:
print("\n'campaignEmailBuilder' NOT found in handler file")
if 'emailBuilder' in content:
print("Found 'emailBuilder' in handler file")
else:
print("'emailBuilder' NOT found in handler file")
EOFRepository: phpList/core
Length of output: 897
🏁 Script executed:
cat -n tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php | sed -n '195,260p'Repository: phpList/core
Length of output: 3182
🏁 Script executed:
cat -n src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php | sed -n '192,240p'Repository: phpList/core
Length of output: 2445
🏁 Script executed:
cat -n tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php | sed -n '80,105p'Repository: phpList/core
Length of output: 1409
🏁 Script executed:
python3 << 'EOF'
# Check if tests actually run and what happens when accessing nonexistent property
import subprocess
# Check if the test would fail
result = subprocess.run(
['grep', '-A', '5', 'getProperty', 'tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php'],
capture_output=True,
text=True
)
print("Test property access patterns:")
print(result.stdout)
EOFRepository: phpList/core
Length of output: 1067
🏁 Script executed:
python3 << 'EOF'
# Let's verify if the test would actually run by checking if the property exists
import subprocess
import sys
# Run a simple PHP/reflection check simulation
test_code = '''<?php
class TestHandler {
private readonly string $emailBuilder;
public function __construct() {}
}
$reflection = new ReflectionClass(TestHandler::class);
try {
$prop = $reflection->getProperty("campaignEmailBuilder");
echo "Property found\\n";
} catch (ReflectionException $e) {
echo "Property NOT found: " . $e->getMessage() . "\\n";
}
try {
$prop = $reflection->getProperty("emailBuilder");
echo "emailBuilder property found\\n";
} catch (ReflectionException $e) {
echo "emailBuilder NOT found\\n";
}
?>'''
with open('/tmp/test_reflection.php', 'w') as f:
f.write(test_code)
result = subprocess.run(['php', '/tmp/test_reflection.php'], capture_output=True, text=True)
print(result.stdout)
print(result.stderr)
EOFRepository: phpList/core
Length of output: 164
Test mocks nonexistent property and wrong method on RateLimitedCampaignMailer.
The tests access a property campaignEmailBuilder that doesn't exist in the handler (which only has emailBuilder). Even if it did exist, the test mocks buildPhplistEmail, but the handler never calls that method for campaign emails. Instead, at line 215, the handler calls $this->rateLimitedCampaignMailer->composeEmail() followed by send().
The test setup should mock the actual RateLimitedCampaignMailer instance ($this->mailer in the test) with expectations on composeEmail() and send(), not attempt to access a nonexistent property to mock a different method.
Affects lines 239-248, 305-307, 383-388.
🤖 Prompt for AI Agents
In
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php
around lines 239-248 (also affecting 305-307 and 383-388), the test is trying to
stub a nonexistent property campaignEmailBuilder and mock buildPhplistEmail, but
the handler actually uses $this->rateLimitedCampaignMailer (the test's
$this->mailer) and calls composeEmail() then send(); update the test to remove
any references to campaignEmailBuilder, instead set expectations on
$this->mailer (or the RateLimitedCampaignMailer mock) to receive
composeEmail(...) returning the Email object used in assertions and then expect
send(...) to be called (with appropriate args), and adjust the assertions at the
other affected line ranges to reflect these mocked calls.
| // Build email and throw on rate-limited sender | ||
| $campaignEmailBuilder = (new ReflectionClass($this->handler)) | ||
| ->getProperty("campaignEmailBuilder"); | ||
| $campaignEmailBuilder->setAccessible(true); | ||
| /** @var EmailBuilder|MockObject $campaignBuilderMock */ | ||
| $campaignBuilderMock = $campaignEmailBuilder->getValue($this->handler); | ||
| $campaignBuilderMock->expects($this->once()) | ||
| ->method('buildPhplistEmail') | ||
| ->willReturn((new Email())->to('test@example.com')->subject('Test Subject')->text('x')); |
There was a problem hiding this comment.
Same property name issue here.
Same fix needed - use emailBuilder instead of campaignEmailBuilder.
🔎 Proposed fix
// Build email and throw on rate-limited sender
$campaignEmailBuilder = (new ReflectionClass($this->handler))
- ->getProperty("campaignEmailBuilder");
+ ->getProperty("emailBuilder");
$campaignEmailBuilder->setAccessible(true);📝 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.
| // Build email and throw on rate-limited sender | |
| $campaignEmailBuilder = (new ReflectionClass($this->handler)) | |
| ->getProperty("campaignEmailBuilder"); | |
| $campaignEmailBuilder->setAccessible(true); | |
| /** @var EmailBuilder|MockObject $campaignBuilderMock */ | |
| $campaignBuilderMock = $campaignEmailBuilder->getValue($this->handler); | |
| $campaignBuilderMock->expects($this->once()) | |
| ->method('buildPhplistEmail') | |
| ->willReturn((new Email())->to('test@example.com')->subject('Test Subject')->text('x')); | |
| // Build email and throw on rate-limited sender | |
| $campaignEmailBuilder = (new ReflectionClass($this->handler)) | |
| ->getProperty("emailBuilder"); | |
| $campaignEmailBuilder->setAccessible(true); | |
| /** @var EmailBuilder|MockObject $campaignBuilderMock */ | |
| $campaignBuilderMock = $campaignEmailBuilder->getValue($this->handler); | |
| $campaignBuilderMock->expects($this->once()) | |
| ->method('buildPhplistEmail') | |
| ->willReturn((new Email())->to('test@example.com')->subject('Test Subject')->text('x')); |
🤖 Prompt for AI Agents
In
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php
around lines 299 to 307, the test accesses the wrong private property name
"campaignEmailBuilder"; change it to "emailBuilder" everywhere in this block
(the ReflectionClass->getProperty call and any related comments/annotations),
keep setAccessible(true) and getValue($this->handler) as before, and ensure the
docblock/typehint reflects EmailBuilder|MockObject if present.
| // Configure builder to return emails for first two subscribers | ||
| $campaignEmailBuilder = (new ReflectionClass($this->handler)) | ||
| ->getProperty("campaignEmailBuilder"); | ||
| /** @var EmailBuilder|MockObject $campaignBuilderMock */ | ||
| $campaignBuilderMock = $campaignEmailBuilder->getValue($this->handler); | ||
| $campaignBuilderMock->expects($this->exactly(2)) | ||
| ->method('buildPhplistEmail') | ||
| ->willReturnOnConsecutiveCalls( | ||
| (new Email())->to('test1@example.com')->subject('Test Subject')->text('x'), | ||
| (new Email())->to('test2@example.com')->subject('Test Subject')->text('x') | ||
| ); |
There was a problem hiding this comment.
Same property name mismatch.
Consistent fix needed here too.
🔎 Proposed fix
// Configure builder to return emails for first two subscribers
$campaignEmailBuilder = (new ReflectionClass($this->handler))
- ->getProperty("campaignEmailBuilder");
+ ->getProperty("emailBuilder");📝 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.
| // Configure builder to return emails for first two subscribers | |
| $campaignEmailBuilder = (new ReflectionClass($this->handler)) | |
| ->getProperty("campaignEmailBuilder"); | |
| /** @var EmailBuilder|MockObject $campaignBuilderMock */ | |
| $campaignBuilderMock = $campaignEmailBuilder->getValue($this->handler); | |
| $campaignBuilderMock->expects($this->exactly(2)) | |
| ->method('buildPhplistEmail') | |
| ->willReturnOnConsecutiveCalls( | |
| (new Email())->to('test1@example.com')->subject('Test Subject')->text('x'), | |
| (new Email())->to('test2@example.com')->subject('Test Subject')->text('x') | |
| ); | |
| // Configure builder to return emails for first two subscribers | |
| $campaignEmailBuilder = (new ReflectionClass($this->handler)) | |
| ->getProperty("emailBuilder"); | |
| /** @var EmailBuilder|MockObject $campaignBuilderMock */ | |
| $campaignBuilderMock = $campaignEmailBuilder->getValue($this->handler); | |
| $campaignBuilderMock->expects($this->exactly(2)) | |
| ->method('buildPhplistEmail') | |
| ->willReturnOnConsecutiveCalls( | |
| (new Email())->to('test1@example.com')->subject('Test Subject')->text('x'), | |
| (new Email())->to('test2@example.com')->subject('Test Subject')->text('x') | |
| ); |
🤖 Prompt for AI Agents
In
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php
around lines 378 to 388, the ReflectionClass is trying to access a nonexistent
property "campaignEmailBuilder"; change the property name to the correct one
used in the class (e.g., "campaignBuilder") in the getProperty call, and adjust
the local variable name/comment if needed so the reflected property and mock
refer to the same, correct property.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/Domain/Configuration/Service/Placeholder/BlacklistUrlValueResolver.php:
- Around line 24-34: In BlacklistUrlValueResolver::__invoke replace the
incorrect ConfigOption::UnsubscribeUrl usage with ConfigOption::BlacklistUrl so
the resolver reads the blacklist base URL from the config; i.e. change the
argument passed to $this->config->getValue(...) and keep the subsequent call to
$this->urlBuilder->withEmail($base, $ctx->getUser()->getEmail()) and the HTML
escaping logic unchanged.
In @src/Domain/Messaging/Service/Constructor/PersonalizedContentConstructor.php:
- Line 63: The code in PersonalizedContentConstructor.php currently defaults
schemeless URLs to 'http://' which downgrades security; change the fallback in
the URL normalization logic (the expression assigning $url from $rawUrl, e.g.,
the ternary using preg_match('/^https?:\/\//i', $rawUrl)) to prepend 'https://'
instead of 'http://' so schemeless inputs become HTTPS by default; ensure you
only alter the fallback string and keep the existing regex check and variable
names ($rawUrl and $url) intact.
🧹 Nitpick comments (14)
src/Domain/Configuration/Service/Placeholder/JumpoffValueResolver.php (1)
29-33: Clarify the intent: HTML context returns empty string while text appends&jo=1.The asymmetric behavior is a bit unusual—HTML renders nothing while text gets the full URL with the jump-off parameter. If this is intentional (e.g., HTML uses a different placeholder), a brief comment would help future maintainers understand the design choice.
Also, consider using
http_build_queryor theLegacyUrlBuilderpattern instead of raw string concatenation for&jo=1to stay consistent with the URL-building approach elsewhere.src/Domain/Configuration/Service/LegacyUrlBuilder.php (1)
19-38: Consider defensive handling for malformed or empty base URLs.If
$baseUrlis empty or lacks a host, the reconstructed URL could become malformed (e.g.,https://orhttps://:8080/path). The current fallbacks ($host ?? '') won't prevent this.A quick guard at the top or returning
$baseUrlunchanged when it's empty would prevent downstream surprises.🛡️ Optional guard
private function withQueryParam(string $baseUrl, string $paramName, string $paramValue): string { + if ($baseUrl === '') { + return ''; + } + $parts = parse_url($baseUrl) ?: []; + if (empty($parts['host'])) { + return $baseUrl; + } + $query = [];src/Domain/Configuration/Service/Placeholder/UnsubscribeValueResolver.php (1)
11-11: Use the contracts interface instead of the deprecated one.
Symfony\Component\Translation\TranslatorInterfaceis deprecated. The modern alternative isSymfony\Contracts\Translation\TranslatorInterface.📦 Fix the import
-use Symfony\Component\Translation\TranslatorInterface; +use Symfony\Contracts\Translation\TranslatorInterface;src/Domain/Configuration/Service/Placeholder/BlacklistValueResolver.php (2)
11-11: Use the contracts interface instead of the deprecated one.Same as
UnsubscribeValueResolver—switch toSymfony\Contracts\Translation\TranslatorInterface.📦 Fix the import
-use Symfony\Component\Translation\TranslatorInterface; +use Symfony\Contracts\Translation\TranslatorInterface;
31-36: Consider a more appropriate label for the blacklist action.The translation key
'Unsubscribe'is reused here, but blacklisting is semantically different from unsubscribing—blacklisting typically means "never email this address again" across all lists. Using a distinct key like'Blacklist'or'Stop all emails'would be clearer for users.config/services/services.yml (1)
61-65: Hardcoded defaults might be better as parameters.These literal defaults (
externalImageMaxAge: 0,externalImageMaxSize: 204800,externalImageTimeout: 30) work, but extracting them toparameters.ymlwould make them easier to configure per environment without touching service definitions.src/Domain/Messaging/Service/Constructor/PersonalizedContentConstructor.php (2)
16-16: Address the TODO or remove it before merging.The
// todo: check this classcomment suggests this might be a work-in-progress. If it's ready for review, consider removing the TODO; if not, perhaps the review should wait.Want me to open an issue to track any remaining work on this class?
76-76: Potential HTML comment injection if URL contains--.If
$urlcontains--, the resulting<!--$url-->becomes malformed HTML. Unlikely in practice, but something to be aware of if URLs come from user input.src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (3)
29-56: Method marked for removal but still contains business logic.The
// todo: remove this methodcomment is noted, but this method still has active callers (presumably). If it's truly deprecated, consider adding a@deprecatedannotation so static analysis tools can flag usage.Also worth noting: the method now calls
replaceUserSpecificRemoteContentwhich creates side effects by mutating$messagePrecacheDto- might be worth documenting that the DTO is modified in-place.
90-97: Exception thrown after logging - good pattern.Nice that you log the error before throwing
RemotePageFetchException. However, the exception is thrown with no message or context. Consider passing meaningful info to the exception:💡 Suggestion
- throw new RemotePageFetchException(); + throw new RemotePageFetchException( + sprintf('Failed to fetch remote URL: %s for subscriber: %s', $rawUrl, $subscriber->getEmail()) + );
68-104: Duplication is intentional—composeEmailis marked for removal.The method
replaceUserSpecificRemoteContentis indeed duplicated inPersonalizedContentConstructor, but sincecomposeEmailhas a// todo: remove this methodcomment (line 29), this duplication is temporary and expected as part of the migration. No action needed here.Minor edge case: the regex pattern
[^\s]+will break if a URL contains a]character (e.g.,[URL:https://example.com/path]query]), since it stops at the first non-whitespace]. Unlikely but possible in URLs with special query params.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
62-73: Consider consistent alignment for constructor parameters.The alignment here is a bit inconsistent - some parameters have extra spacing (lines 62-73) while earlier ones don't. Not a big deal, just a formatting nit.
205-214: Format-aware personalization is a nice addition.Using
OutputFormat::Textfor bothtextContentandfooteris correct for the text email part. The named parameters make the intent clear.Small formatting nit on line 208: missing space after
format:.🧹 Minor formatting fix
- format:OutputFormat::Text, + format: OutputFormat::Text,src/Domain/Configuration/Service/UserPersonalizer.php (1)
44-48: Inconsistent closure signatures between resolvers.Line 44 uses
fn(PlaceholderContext $ctx) => ...for EMAIL, but the closures for CONFIRMATIONURL (line 50), PREFERENCESURL (line 65), and LISTS (line 88) don't acceptPlaceholderContextand instead use$formatfrom the outer scope viause.This inconsistency could lead to confusion. The injected
$placeholderResolvers(line 46-48) likely implement__invoke(PlaceholderContext $ctx), so they'll work correctly. But the inline closures are mixed.Consider making all closures accept
PlaceholderContext $ctxfor consistency, or document why some use closure capture instead.♻️ Example of consistent approach
- $resolver->register('CONFIRMATIONURL', function () use ($user, $format) { + $resolver->register('CONFIRMATIONURL', function (PlaceholderContext $ctx) use ($user) { $base = $this->config->getValue(ConfigOption::ConfirmationUrl) ?? ''; $url = $this->urlBuilder->withUid($base, $user->getUniqueId()); - if ($format === OutputFormat::Html) { + if ($ctx->isHtml()) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
config/services/resolvers.ymlconfig/services/services.ymlsrc/Domain/Configuration/Model/ConfigOption.phpsrc/Domain/Configuration/Model/Dto/PlaceholderContext.phpsrc/Domain/Configuration/Service/LegacyUrlBuilder.phpsrc/Domain/Configuration/Service/Placeholder/BlacklistUrlValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/BlacklistValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/JumpoffUrlValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/JumpoffValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/PlaceholderValueResolverInterface.phpsrc/Domain/Configuration/Service/Placeholder/UnsubscribeUrlValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/UnsubscribeValueResolver.phpsrc/Domain/Configuration/Service/PlaceholderResolver.phpsrc/Domain/Configuration/Service/UserPersonalizer.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Constructor/PersonalizedContentConstructor.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.php
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow, flag ANY DB write / finalization:
$entityManager->flush(...),$this->entityManager->flush(...)$em->beginTransaction(),$em->commit(),$em->rollback(),$em->transactional(...)$em->getConnection()->executeStatement(...)for DML/DDL (INSERT/UPDATE/DELETE/ALTER/...)✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes. Accessing Doctrine persistence APIs
persist, remove, etc.) is acceptable, allow scheduling changes in the UnitOfWork (no DB writes)✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Configuration/Model/ConfigOption.phpsrc/Domain/Configuration/Service/Placeholder/JumpoffUrlValueResolver.phpsrc/Domain/Configuration/Model/Dto/PlaceholderContext.phpsrc/Domain/Configuration/Service/Placeholder/PlaceholderValueResolverInterface.phpsrc/Domain/Messaging/Service/Constructor/PersonalizedContentConstructor.phpsrc/Domain/Configuration/Service/Placeholder/BlacklistUrlValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/BlacklistValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/UnsubscribeValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/UnsubscribeUrlValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/JumpoffValueResolver.phpsrc/Domain/Configuration/Service/UserPersonalizer.phpsrc/Domain/Configuration/Service/LegacyUrlBuilder.phpsrc/Domain/Configuration/Service/PlaceholderResolver.phpsrc/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (8)
src/Domain/Configuration/Service/Placeholder/JumpoffUrlValueResolver.php (4)
src/Domain/Configuration/Service/Placeholder/JumpoffValueResolver.php (2)
JumpoffValueResolver(12-35)name(19-22)src/Domain/Configuration/Service/Placeholder/BlacklistUrlValueResolver.php (1)
name(19-22)src/Domain/Configuration/Service/Placeholder/PlaceholderValueResolverInterface.php (1)
name(11-11)src/Domain/Configuration/Service/Placeholder/UnsubscribeUrlValueResolver.php (1)
name(19-22)
src/Domain/Configuration/Service/Placeholder/PlaceholderValueResolverInterface.php (7)
src/Domain/Configuration/Model/Dto/PlaceholderContext.php (1)
PlaceholderContext(10-38)src/Domain/Configuration/Service/Placeholder/BlacklistUrlValueResolver.php (2)
name(19-22)__invoke(24-34)src/Domain/Configuration/Service/Placeholder/BlacklistValueResolver.php (2)
name(21-24)__invoke(26-40)src/Domain/Configuration/Service/Placeholder/JumpoffUrlValueResolver.php (1)
name(9-12)src/Domain/Configuration/Service/Placeholder/JumpoffValueResolver.php (2)
name(19-22)__invoke(24-34)src/Domain/Configuration/Service/Placeholder/UnsubscribeUrlValueResolver.php (2)
name(19-22)__invoke(24-34)src/Domain/Configuration/Service/Placeholder/UnsubscribeValueResolver.php (2)
name(21-24)__invoke(26-40)
src/Domain/Messaging/Service/Constructor/PersonalizedContentConstructor.php (8)
src/Domain/Common/RemotePageFetcher.php (1)
RemotePageFetcher(17-125)src/Domain/Configuration/Service/UserPersonalizer.php (2)
UserPersonalizer(18-121)__construct(22-34)src/Domain/Messaging/Exception/RemotePageFetchException.php (1)
RemotePageFetchException(9-15)src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Subscription/Repository/SubscriberRepository.php (2)
SubscriberRepository(21-218)getDataById(209-217)src/Domain/Configuration/Service/Placeholder/BlacklistUrlValueResolver.php (1)
__construct(14-17)src/Domain/Configuration/Service/Placeholder/JumpoffValueResolver.php (1)
__construct(14-17)src/Domain/Configuration/Service/Placeholder/UnsubscribeUrlValueResolver.php (1)
__construct(14-17)
src/Domain/Configuration/Service/Placeholder/BlacklistValueResolver.php (7)
src/Domain/Configuration/Service/LegacyUrlBuilder.php (2)
LegacyUrlBuilder(7-39)withEmail(14-17)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Model/Dto/PlaceholderContext.php (3)
PlaceholderContext(10-38)getUser(34-37)isHtml(19-22)src/Domain/Configuration/Service/Placeholder/BlacklistUrlValueResolver.php (2)
name(19-22)__invoke(24-34)src/Domain/Configuration/Service/Placeholder/JumpoffValueResolver.php (2)
name(19-22)__invoke(24-34)src/Domain/Configuration/Service/Placeholder/UnsubscribeValueResolver.php (2)
name(21-24)__invoke(26-40)src/Domain/Configuration/Service/Placeholder/PlaceholderValueResolverInterface.php (2)
name(11-11)__invoke(12-12)
src/Domain/Configuration/Service/Placeholder/UnsubscribeValueResolver.php (6)
src/Domain/Configuration/Service/LegacyUrlBuilder.php (2)
LegacyUrlBuilder(7-39)withUid(9-12)src/Domain/Configuration/Model/Dto/PlaceholderContext.php (4)
PlaceholderContext(10-38)__construct(12-17)getUser(34-37)isHtml(19-22)src/Domain/Configuration/Service/Placeholder/BlacklistValueResolver.php (3)
__construct(15-19)name(21-24)__invoke(26-40)src/Domain/Configuration/Service/Placeholder/JumpoffValueResolver.php (3)
__construct(14-17)name(19-22)__invoke(24-34)src/Domain/Configuration/Service/Placeholder/UnsubscribeUrlValueResolver.php (3)
__construct(14-17)name(19-22)__invoke(24-34)src/Domain/Configuration/Service/Placeholder/PlaceholderValueResolverInterface.php (2)
name(11-11)__invoke(12-12)
src/Domain/Configuration/Service/Placeholder/UnsubscribeUrlValueResolver.php (5)
src/Domain/Configuration/Service/LegacyUrlBuilder.php (2)
LegacyUrlBuilder(7-39)withUid(9-12)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Service/Placeholder/BlacklistValueResolver.php (3)
__construct(15-19)name(21-24)__invoke(26-40)src/Domain/Configuration/Service/Placeholder/JumpoffValueResolver.php (3)
__construct(14-17)name(19-22)__invoke(24-34)src/Domain/Configuration/Service/Placeholder/PlaceholderValueResolverInterface.php (2)
name(11-11)__invoke(12-12)
src/Domain/Configuration/Service/UserPersonalizer.php (3)
src/Domain/Configuration/Model/Dto/PlaceholderContext.php (1)
PlaceholderContext(10-38)src/Domain/Subscription/Repository/SubscriberListRepository.php (2)
SubscriberListRepository(23-94)getActiveListNamesForSubscriber(78-93)src/Domain/Configuration/Service/PlaceholderResolver.php (3)
PlaceholderResolver(9-39)register(14-17)resolve(19-38)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
src/Domain/Messaging/Service/Constructor/PersonalizedContentConstructor.php (2)
PersonalizedContentConstructor(17-82)build(27-43)src/Domain/Configuration/Service/UserPersonalizer.php (2)
UserPersonalizer(18-121)personalize(36-120)
🔇 Additional comments (17)
config/services/resolvers.yml (1)
16-19: Nice setup for auto-tagging placeholder resolvers.The
_instanceofconfiguration cleanly wires up allPlaceholderValueResolverInterfaceimplementations with thephplist.placeholder_resolvertag. This keeps the service definitions DRY as new resolvers are added.src/Domain/Configuration/Model/Dto/PlaceholderContext.php (1)
10-37: Clean DTO design with format-aware helpers.The
PlaceholderContextclass is well-structured with readonly properties and useful predicates (isHtml(),isText()). ThegetUser()alias provides a nice semantic accessor. Solid work.src/Domain/Configuration/Model/ConfigOption.php (1)
13-13: Consistent enum case addition.The new
BlacklistUrlcase follows the existing naming and value conventions. 👍src/Domain/Configuration/Service/Placeholder/JumpoffUrlValueResolver.php (1)
7-12: Potential issue: inherited__invoke()returns empty string for HTML.This class inherits
__invoke()fromJumpoffValueResolver, which returns''when$ctx->isHtml()is true. Other*UrlValueResolverclasses (likeUnsubscribeUrlValueResolver,BlacklistUrlValueResolver) return the actual URL (HTML-escaped) in HTML context.Is returning an empty string for
JUMPOFFURLin HTML intentional, or should this class override__invoke()to return the URL like its sibling resolvers?Also, the
implements PlaceholderValueResolverInterfaceis redundant since the parent already implements it.♻️ If URL should be returned for HTML context
class JumpoffUrlValueResolver extends JumpoffValueResolver implements PlaceholderValueResolverInterface { public function name(): string { return 'JUMPOFFURL'; } + + public function __invoke(PlaceholderContext $ctx): string + { + $base = $this->config->getValue(ConfigOption::UnsubscribeUrl) ?? ''; + $url = $this->urlBuilder->withUid($base, $ctx->getUser()->getUniqueId()) . '&jo=1'; + + if ($ctx->isHtml()) { + return htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); + } + + return $url; + } }Note: This would require constructor injection of
ConfigProviderandLegacyUrlBuilder, or making parent propertiesprotected.src/Domain/Configuration/Service/Placeholder/PlaceholderValueResolverInterface.php (1)
9-13: Clean interface design.The callable pattern via
__invoke()is a nice touch - keeps resolvers simple and easy to use. Thename()method provides clear placeholder identification.src/Domain/Configuration/Service/Placeholder/UnsubscribeUrlValueResolver.php (1)
12-34: Solid implementation with proper HTML escaping.The resolver correctly builds the unsubscribe URL and applies appropriate HTML escaping for the HTML context. Clean and consistent with the resolver pattern.
src/Domain/Configuration/Service/Placeholder/UnsubscribeValueResolver.php (1)
26-40: LGTM—clean HTML escaping and consistent pattern.Proper use of
htmlspecialcharsfor both label and URL, and the resolver follows the same structure as the relatedBlacklistValueResolver. Nice and tidy.config/services/services.yml (1)
196-202: LGTM—service wiring for message precaching looks solid.The dependencies and parameters are properly configured. Minor note: consider adding a trailing newline at EOF for consistency with POSIX conventions.
src/Domain/Configuration/Service/PlaceholderResolver.php (1)
25-37: Clean implementation with a note on case sensitivity.The regex
[A-Z0-9_]+only matches uppercase placeholders. If lowercase or mixed-case tokens like[email]need to work, you'd want to add theiflag or includea-zin the character class. If uppercase-only is intentional, that's fine—just wanted to flag it.The
str_containsbail-out and the explicit string cast are nice touches.src/Domain/Messaging/Service/Constructor/PersonalizedContentConstructor.php (1)
80-80:strip_tagscomparison is a pragmatic HTML detector—works for this use case.Just noting that this heuristic (
strip_tags($content) !== $content) will mark content as HTML-formatted if it contains any tags, including benign ones like<br>. That's probably fine for email content, but worth keeping in mind.src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
20-27: Constructor looks solid with the new DI approach.Clean property promotion with all the new dependencies needed for remote content fetching. No domain purity violations here.
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
11-26: Clean dependency swap fromEmailBuilder(for compose) toPersonalizedContentConstructor.The transition looks good. Note that you still keep
EmailBuilderfor admin notifications (line 70), which makes sense since that's a different flow.Also applies to: 70-73
216-222: No null-return concern here; the type signature protects against it.The
build()method returnsRemotePageFetchExceptionfrom remote content replacement) is caught by theThrowablehandler at line 231, which keeps the error handling safe.src/Domain/Configuration/Service/UserPersonalizer.php (4)
22-34: Constructor expanded with format-aware dependencies.The new dependencies make sense for supporting format-aware rendering. The
$preferencePageShowPrivateListsflag with a default is a nice touch for backwards compatibility.
50-63: CONFIRMATIONURL resolver with proper HTML escaping - looks good.The
htmlspecialcharswithENT_QUOTES | ENT_SUBSTITUTEand explicit UTF-8 encoding is the correct approach for XSS prevention. The trailing space viaself::PHP_SPACEis preserved from previous behavior.
87-109: LISTS resolver is nicely format-aware.Good handling of:
- Empty list case with translated message
- HTML escaping of list names when in HTML format
- Different separators (
<br/>vs\n)One thought: the empty-list message is quite long. If this appears in a layout-sensitive context, it might be worth having a shorter fallback. But that's a UX decision, not a code issue.
119-119: PlaceholderContext instantiation looks correct.Passing
$userand$formatto the context aligns with the expected constructor signature from the relevant snippets. The context is then passed toresolve()which dispatches to all registered resolvers.
| public function __invoke(PlaceholderContext $ctx): string | ||
| { | ||
| $base = $this->config->getValue(ConfigOption::UnsubscribeUrl) ?? ''; | ||
| $url = $this->urlBuilder->withEmail($base, $ctx->getUser()->getEmail()); | ||
|
|
||
| if ($ctx->isHtml()) { | ||
| return htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); | ||
| } | ||
|
|
||
| return $url; | ||
| } |
There was a problem hiding this comment.
Likely bug: should use ConfigOption::BlacklistUrl instead of UnsubscribeUrl.
Line 26 uses ConfigOption::UnsubscribeUrl as the base, but this is BlacklistUrlValueResolver. The new ConfigOption::BlacklistUrl case was added in this PR - should this resolver be using that instead?
For comparison, BlacklistValueResolver (from snippets) uses ConfigOption::BlacklistUrl.
🐛 Proposed fix
public function __invoke(PlaceholderContext $ctx): string
{
- $base = $this->config->getValue(ConfigOption::UnsubscribeUrl) ?? '';
+ $base = $this->config->getValue(ConfigOption::BlacklistUrl) ?? '';
$url = $this->urlBuilder->withEmail($base, $ctx->getUser()->getEmail());
if ($ctx->isHtml()) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function __invoke(PlaceholderContext $ctx): string | |
| { | |
| $base = $this->config->getValue(ConfigOption::UnsubscribeUrl) ?? ''; | |
| $url = $this->urlBuilder->withEmail($base, $ctx->getUser()->getEmail()); | |
| if ($ctx->isHtml()) { | |
| return htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); | |
| } | |
| return $url; | |
| } | |
| public function __invoke(PlaceholderContext $ctx): string | |
| { | |
| $base = $this->config->getValue(ConfigOption::BlacklistUrl) ?? ''; | |
| $url = $this->urlBuilder->withEmail($base, $ctx->getUser()->getEmail()); | |
| if ($ctx->isHtml()) { | |
| return htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); | |
| } | |
| return $url; | |
| } |
🤖 Prompt for AI Agents
In @src/Domain/Configuration/Service/Placeholder/BlacklistUrlValueResolver.php
around lines 24 - 34, In BlacklistUrlValueResolver::__invoke replace the
incorrect ConfigOption::UnsubscribeUrl usage with ConfigOption::BlacklistUrl so
the resolver reads the blacklist base URL from the config; i.e. change the
argument passed to $this->config->getValue(...) and keep the subsequent call to
$this->urlBuilder->withEmail($base, $ctx->getUser()->getEmail()) and the HTML
escaping logic unchanged.
| continue; | ||
| } | ||
|
|
||
| $url = preg_match('/^https?:\/\//i', $rawUrl) ? $rawUrl : 'http://' . $rawUrl; |
There was a problem hiding this comment.
Default to https:// rather than http://.
Falling back to http:// for schemeless URLs is a security downgrade in 2025. Most sites expect HTTPS these days, and using HTTP could trigger mixed-content warnings or fail entirely on HTTPS-only hosts.
🔒 Suggested fix
-$url = preg_match('/^https?:\/\//i', $rawUrl) ? $rawUrl : 'http://' . $rawUrl;
+$url = preg_match('/^https?:\/\//i', $rawUrl) ? $rawUrl : 'https://' . $rawUrl;📝 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.
| $url = preg_match('/^https?:\/\//i', $rawUrl) ? $rawUrl : 'http://' . $rawUrl; | |
| $url = preg_match('/^https?:\/\//i', $rawUrl) ? $rawUrl : 'https://' . $rawUrl; |
🤖 Prompt for AI Agents
In @src/Domain/Messaging/Service/Constructor/PersonalizedContentConstructor.php
at line 63, The code in PersonalizedContentConstructor.php currently defaults
schemeless URLs to 'http://' which downgrades security; change the fallback in
the URL normalization logic (the expression assigning $url from $rawUrl, e.g.,
the ternary using preg_match('/^https?:\/\//i', $rawUrl)) to prepend 'https://'
instead of 'http://' so schemeless inputs become HTTPS by default; ensure you
only alter the fallback string and keep the existing regex check and variable
names ($rawUrl and $url) intact.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In @resources/translations/messages.en.xlf:
- Around line 753-772: The target strings for the translation units PpLvt2Z,
njfOwly, 7r3SSnf, YibbDEd, and BkWF7jw incorrectly include a "__" prefix; update
each <target> value to exactly match its <source> (remove the "__" prefix) so
"Unsubscribe", "This link", "Confirm", "Update preferences", and "Sorry, you are
not subscribed to any of our newsletters with this email address." are used as
the target values.
In
@src/Domain/Configuration/Service/Placeholder/ConfirmationUrlValueResolver.php:
- Around line 22-32: The uid is inserted into the query string without
URL-encoding in the __invoke method (using $ctx->getUser()->getUniqueId()),
which can break URLs; compute an encoded uid (e.g. $encodedUid =
rawurlencode($ctx->getUser()->getUniqueId())) and use that in both return paths,
and in the HTML branch also HTML-escape the encoded uid (e.g.
htmlspecialchars($encodedUid)) so the query value is both URL-safe and safe for
HTML output.
In @src/Domain/Configuration/Service/Placeholder/ForwardUrlValueResolver.php:
- Around line 27-35: The HTML branch in ForwardUrlValueResolver.php (inside the
conditional using $ctx->isHtml() and the sprintf call) omits the trailing space
present in the non-HTML branch; edit the sprintf format string in that branch to
append a single trailing space (e.g., add a space after the final substitution
in the format) so the HTML output formatting matches the non-HTML branch while
preserving the existing htmlspecialchars calls and argument order.
In @src/Domain/Configuration/Service/Placeholder/ForwardValueResolver.php:
- Around line 34-41: The sprintf call in ForwardValueResolver returning the
anchor should URL-encode the uid from $ctx->getUser()->getUniqueId() and handle
nullable messageId from $ctx->messageId(); change the format to accept a string
for message id (or coalesce to a safe default) and pass
urlencode($ctx->getUser()->getUniqueId()) and a null-safe message id (e.g.,
$ctx->messageId() ?? 0 or (string)($ctx->messageId() ?? '')) so the uid won’t
break the query string and the message id won’t cause a %d type error.
In
@src/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolver.php:
- Around line 27-29: When rendering in HTML context inside
PreferencesUrlValueResolver (the branch checking $ctx->isHtml()), the $url is
not escaped; update the HTML branch to run the configured PreferencesUrl through
htmlspecialchars (same flags/encoding/boolean for double-encode as used in
ForwardUrlValueResolver) before formatting the final string so both the URL and
$sep are escaped and the output avoids rendering/XSS issues.
In @src/Domain/Configuration/Service/Placeholder/PreferencesValueResolver.php:
- Around line 34-40: The uid parameter is not URL-encoded when building the link
in PreferencesValueResolver (the sprintf using $ctx->getUser()->getUniqueId()),
which can break links with special characters; update the code to URL-encode the
uid (use rawurlencode(...) as done in ForwardValueResolver) when interpolating
into the query string and apply the same change to the plain-text path
occurrence on the subsequent line so both HTML and text outputs use the encoded
UID.
In @src/Domain/Configuration/Service/Placeholder/SignatureValueResolver.php:
- Around line 25-34: The code calls
$this->config->getValue(ConfigOption::PoweredByText) and
$this->config->getValue(ConfigOption::PoweredByImage) which return ?string but
the method expects a non-nullable string and preg_replace on null triggers PHP
8.1+ deprecation warnings; fix by coalescing those calls to a safe string (e.g.
use null coalescing to '' or a sensible default) before returning or passing
into preg_replace, e.g. replace direct calls in the $ctx->isHtml() branch with
$text = $this->config->getValue(ConfigOption::PoweredByText) ?? '' and $img =
$this->config->getValue(ConfigOption::PoweredByImage) ?? '' and use $text /
preg_replace(..., $img) so the method never receives null.
In @src/Domain/Configuration/Service/UserPersonalizer.php:
- Around line 58-63: The CONTACTURL resolver currently unconditionally applies
htmlspecialchars; change it to only escape when the placeholder context requests
HTML by checking PlaceholderContext::$ctx->isHtml(): fetch the URL via
$this->config->getValue(ConfigOption::VCardUrl) ?? '', and return
htmlspecialchars(...) only if $ctx->isHtml() is true, otherwise return the raw
URL string so text-format output isn't HTML-escaped.
In @src/Domain/Messaging/Service/Constructor/MailConstructor.php:
- Around line 94-97: The build() method in MailConstructor.php currently returns
a bare new Email() discarding the computed $htmlmessage, $textmessage,
recipients and footer data; update the method to apply those computed values to
the Email instance (use the Email returned from new Email() and call its setters
to set subject, from, to/cc/bcc, setHtml/setText or equivalent, add attachments
and any headers/footers) and then return that configured Email object instead of
an empty one so the generated content and recipients are actually sent.
🧹 Nitpick comments (5)
src/Domain/Configuration/Service/Placeholder/SubscribeValueResolver.php (1)
28-34: Consider sprintf for consistency.String concatenation works fine here, but other resolvers (like ForwardValueResolver) use
sprintffor building HTML. Using a consistent approach across resolvers makes the codebase easier to maintain.Optional refactor
- return '<a href="' . $safeUrl . '">' . $safeLabel . '</a>'; + return sprintf('<a href="%s">%s</a>', $safeUrl, $safeLabel);src/Domain/Configuration/Service/Placeholder/SignatureValueResolver.php (1)
30-34: Consider potential preg_replace failure.
preg_replace()can returnnullon error, but the method signature requires a non-nullablestring. While errors are rare, defensive handling would improve robustness.💡 Optional: Add fallback for preg_replace errors
- return preg_replace( + $result = preg_replace( '/src=".*power-phplist.png"/', 'src="powerphplist.png"', $this->config->getValue(ConfigOption::PoweredByImage) ?? '' ); + + return $result ?? '';src/Domain/Configuration/Service/Placeholder/ListsValueResolver.php (1)
31-34: Potential XSS: translated message not escaped in HTML context.When the list is empty, the translated string is returned raw. If output in HTML context, it could be a vector for XSS (unlikely with translations, but good hygiene).
♻️ Consider escaping the fallback message in HTML context
if ($names === []) { - return $this->translator - ->trans('Sorry, you are not subscribed to any of our newsletters with this email address.'); + $msg = $this->translator + ->trans('Sorry, you are not subscribed to any of our newsletters with this email address.'); + return $ctx->isHtml() + ? htmlspecialchars($msg, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') + : $msg; }src/Domain/Messaging/Service/Constructor/MailConstructor.php (2)
41-42: Unused variables:$defaultstyleand$adddefaultstyle.These are assigned but never read. If they're intended for future use, consider adding a TODO; otherwise, remove them to reduce noise.
117-117: Consider defaulting to HTTPS instead of HTTP.Falling back to
http://for URLs without a protocol could cause mixed content issues or security downgrades on HTTPS sites.♻️ Prefer HTTPS as default protocol
- $url = preg_match('/^https?:\/\//i', $rawUrl) ? $rawUrl : 'http://' . $rawUrl; + $url = preg_match('/^https?:\/\//i', $rawUrl) ? $rawUrl : 'https://' . $rawUrl;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
config/parameters.yml.distconfig/services/builders.ymlresources/translations/messages.en.xlfsrc/Domain/Configuration/Model/ConfigOption.phpsrc/Domain/Configuration/Model/Dto/PlaceholderContext.phpsrc/Domain/Configuration/Service/Placeholder/BlacklistValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/ConfirmationUrlValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/ForwardUrlValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/ForwardValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/ListsValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/PreferencesValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/SignatureValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/SubscribeUrlValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/SubscribeValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/UnsubscribeValueResolver.phpsrc/Domain/Configuration/Service/UserPersonalizer.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/Constructor/MailConstructor.php
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Domain/Configuration/Model/ConfigOption.php
- src/Domain/Configuration/Service/Placeholder/BlacklistValueResolver.php
- config/parameters.yml.dist
- config/services/builders.yml
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow, flag ANY DB write / finalization:
$entityManager->flush(...),$this->entityManager->flush(...)$em->beginTransaction(),$em->commit(),$em->rollback(),$em->transactional(...)$em->getConnection()->executeStatement(...)for DML/DDL (INSERT/UPDATE/DELETE/ALTER/...)✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes. Accessing Doctrine persistence APIs
persist, remove, etc.) is acceptable, allow scheduling changes in the UnitOfWork (no DB writes)✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Configuration/Service/Placeholder/PreferencesValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/SubscribeUrlValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/SignatureValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolver.phpsrc/Domain/Configuration/Service/UserPersonalizer.phpsrc/Domain/Configuration/Service/Placeholder/SubscribeValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/ListsValueResolver.phpsrc/Domain/Configuration/Service/Placeholder/ForwardValueResolver.phpsrc/Domain/Messaging/Service/Constructor/MailConstructor.phpsrc/Domain/Configuration/Service/Placeholder/ConfirmationUrlValueResolver.phpsrc/Domain/Configuration/Model/Dto/PlaceholderContext.phpsrc/Domain/Configuration/Service/Placeholder/ForwardUrlValueResolver.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Configuration/Service/Placeholder/UnsubscribeValueResolver.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 (13)
src/Domain/Configuration/Service/Placeholder/PreferencesValueResolver.php (4)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Service/Placeholder/ConfirmationUrlValueResolver.php (3)
__construct(13-15)name(17-20)__invoke(22-32)src/Domain/Configuration/Service/Placeholder/ListsValueResolver.php (3)
__construct(13-17)name(19-22)__invoke(24-46)src/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolver.php (3)
__construct(13-15)name(17-20)__invoke(22-32)
src/Domain/Configuration/Service/Placeholder/SubscribeUrlValueResolver.php (4)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Model/Dto/PlaceholderContext.php (3)
PlaceholderContext(10-44)__construct(12-18)isHtml(20-23)src/Domain/Configuration/Service/Placeholder/SubscribeValueResolver.php (3)
__construct(14-17)name(19-22)__invoke(24-37)src/Domain/Configuration/Service/Placeholder/UnsubscribeValueResolver.php (3)
__construct(15-19)name(21-24)__invoke(26-44)
src/Domain/Configuration/Service/Placeholder/SignatureValueResolver.php (3)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Model/Dto/PlaceholderContext.php (1)
PlaceholderContext(10-44)src/Domain/Configuration/Service/UserPersonalizer.php (1)
__construct(18-26)
src/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolver.php (4)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Service/Placeholder/ConfirmationUrlValueResolver.php (3)
__construct(13-15)name(17-20)__invoke(22-32)src/Domain/Configuration/Service/Placeholder/ListsValueResolver.php (3)
__construct(13-17)name(19-22)__invoke(24-46)src/Domain/Configuration/Service/Placeholder/PreferencesValueResolver.php (3)
__construct(14-17)name(19-22)__invoke(24-44)
src/Domain/Configuration/Service/UserPersonalizer.php (3)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Model/Dto/PlaceholderContext.php (4)
PlaceholderContext(10-44)__construct(12-18)messageId(35-38)forwardedBy(30-33)src/Domain/Configuration/Service/PlaceholderResolver.php (3)
PlaceholderResolver(9-39)register(14-17)resolve(19-38)
src/Domain/Configuration/Service/Placeholder/SubscribeValueResolver.php (3)
src/Domain/Configuration/Model/Dto/PlaceholderContext.php (3)
PlaceholderContext(10-44)__construct(12-18)isHtml(20-23)src/Domain/Configuration/Service/Placeholder/ForwardValueResolver.php (3)
__construct(14-17)name(19-22)__invoke(24-45)src/Domain/Configuration/Service/Placeholder/SubscribeUrlValueResolver.php (3)
__construct(13-15)name(17-20)__invoke(22-31)
src/Domain/Configuration/Service/Placeholder/ListsValueResolver.php (3)
src/Domain/Configuration/Model/Dto/PlaceholderContext.php (1)
PlaceholderContext(10-44)src/Domain/Subscription/Repository/SubscriberListRepository.php (2)
SubscriberListRepository(23-94)getActiveListNamesForSubscriber(78-93)src/Domain/Configuration/Service/Placeholder/PreferencesValueResolver.php (3)
__construct(14-17)name(19-22)__invoke(24-44)
src/Domain/Configuration/Service/Placeholder/ForwardValueResolver.php (3)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Model/Dto/PlaceholderContext.php (5)
PlaceholderContext(10-44)__construct(12-18)isHtml(20-23)getUser(40-43)messageId(35-38)src/Domain/Configuration/Service/Placeholder/ForwardUrlValueResolver.php (3)
__construct(13-15)name(17-20)__invoke(22-38)
src/Domain/Messaging/Service/Constructor/MailConstructor.php (8)
src/Domain/Common/Html2Text.php (1)
Html2Text(10-85)src/Domain/Common/RemotePageFetcher.php (1)
RemotePageFetcher(17-125)src/Domain/Common/TextParser.php (1)
TextParser(7-95)src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Service/UserPersonalizer.php (1)
UserPersonalizer(16-82)src/Domain/Messaging/Exception/RemotePageFetchException.php (1)
RemotePageFetchException(9-15)src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Subscription/Repository/SubscriberRepository.php (2)
SubscriberRepository(21-218)getDataById(209-217)
src/Domain/Configuration/Service/Placeholder/ConfirmationUrlValueResolver.php (3)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Model/Dto/PlaceholderContext.php (1)
PlaceholderContext(10-44)src/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolver.php (3)
__construct(13-15)name(17-20)__invoke(22-32)
src/Domain/Configuration/Service/Placeholder/ForwardUrlValueResolver.php (3)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Configuration/Model/Dto/PlaceholderContext.php (5)
PlaceholderContext(10-44)__construct(12-18)isHtml(20-23)getUser(40-43)messageId(35-38)src/Domain/Configuration/Service/Placeholder/ForwardValueResolver.php (3)
__construct(14-17)name(19-22)__invoke(24-45)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
src/Domain/Messaging/Service/Constructor/MailConstructor.php (2)
MailConstructor(21-136)build(35-97)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
send(61-66)
src/Domain/Configuration/Service/Placeholder/UnsubscribeValueResolver.php (3)
src/Domain/Configuration/Service/LegacyUrlBuilder.php (2)
LegacyUrlBuilder(7-39)withUid(9-12)src/Domain/Configuration/Service/Placeholder/JumpoffValueResolver.php (3)
__construct(14-17)name(19-22)__invoke(24-34)src/Domain/Configuration/Service/Placeholder/PlaceholderValueResolverInterface.php (2)
name(11-11)__invoke(12-12)
🔇 Additional comments (14)
src/Domain/Configuration/Service/Placeholder/ForwardUrlValueResolver.php (1)
13-20: LGTM!Constructor and name implementation follow the established pattern for placeholder resolvers.
src/Domain/Configuration/Service/Placeholder/SubscribeValueResolver.php (1)
14-22: LGTM!Constructor and name implementation are clean and follow the established pattern.
src/Domain/Configuration/Model/Dto/PlaceholderContext.php (1)
10-44: LGTM!Clean DTO design with appropriate use of readonly properties and constructor promotion. The context provides all necessary information for format-aware placeholder resolution.
src/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolver.php (1)
13-20: LGTM!Constructor and name implementation follow the standard resolver pattern.
src/Domain/Configuration/Service/Placeholder/SubscribeUrlValueResolver.php (1)
11-32: LGTM!Clean implementation with proper HTML escaping and straightforward logic. The resolver correctly handles both HTML and text output formats.
src/Domain/Configuration/Service/Placeholder/ForwardValueResolver.php (1)
14-22: LGTM!Clean constructor with proper dependency injection and straightforward resolver name implementation.
src/Domain/Configuration/Service/Placeholder/UnsubscribeValueResolver.php (1)
15-44: LGTM!This resolver properly handles URL building via
LegacyUrlBuilder.withUid(), which ensures URL encoding. The conditional logic for forwarded contexts (switching to BlacklistUrl) is clear, and HTML escaping is correctly applied.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
204-210: TODO comment suggests incomplete implementation.The
// todo: use correct servicecomment hints this might be a work-in-progress. The flow looks solid—building email viaMailConstructorthen sending viarateLimitedCampaignMailer—but you might want to clarify what "correct service" refers to or remove the TODO if resolved.
69-69: Nice addition of MailConstructor dependency.The constructor injection looks good and follows the existing pattern in this handler. The separation of email construction from sending is a clean design choice.
src/Domain/Configuration/Service/Placeholder/ListsValueResolver.php (1)
24-46: Clean implementation following the established pattern.The HTML escaping for list names and context-aware separator logic are well done. This mirrors the approach in
PreferencesValueResolvernicely.src/Domain/Messaging/Service/Constructor/MailConstructor.php (1)
20-21: Address the TODO comment.The
// todo: check this classhints at uncertainty. Given the incompletebuild()method, this class seems to need significant work before it's ready.src/Domain/Configuration/Service/UserPersonalizer.php (3)
44-44:FORWARDFORMreturns empty string—intentional placeholder?If this is WIP, a TODO comment would help. If the feature is deprecated or not needed, consider removing it entirely.
65-67: Nice extensibility via injected placeholder resolvers.The iterable pattern allows adding new resolvers without modifying this class. Clean separation of concerns.
28-34: Good refactor to format-aware personalization.The new signature with
OutputFormatand context fields enables format-specific rendering throughout the placeholder system. ThePlaceholderContextencapsulation is tidy.Also applies to: 77-80
| <trans-unit id="PpLvt2Z" resname="Unsubscribe"> | ||
| <source>Unsubscribe</source> | ||
| <target>__Unsubscribe</target> | ||
| </trans-unit> | ||
| <trans-unit id="njfOwly" resname="This link"> | ||
| <source>This link</source> | ||
| <target>__This link</target> | ||
| </trans-unit> | ||
| <trans-unit id="7r3SSnf" resname="Confirm"> | ||
| <source>Confirm</source> | ||
| <target>__Confirm</target> | ||
| </trans-unit> | ||
| <trans-unit id="YibbDEd" resname="Update preferences"> | ||
| <source>Update preferences</source> | ||
| <target>__Update preferences</target> | ||
| </trans-unit> | ||
| <trans-unit id="BkWF7jw" resname="Sorry, you are not subscribed to any of our newsletters with this email address."> | ||
| <source>Sorry, you are not subscribed to any of our newsletters with this email address.</source> | ||
| <target>__Sorry, you are not subscribed to any of our newsletters with this email address.</target> | ||
| </trans-unit> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the translation file structure and count the __ pattern
wc -l resources/translations/messages.en.xlfRepository: phpList/core
Length of output: 98
🏁 Script executed:
# Count how many target values have the __ prefix
rg '<target>__' resources/translations/messages.en.xlf | wc -lRepository: phpList/core
Length of output: 58
🏁 Script executed:
# Check how many total target values exist
rg '<target>' resources/translations/messages.en.xlf | wc -lRepository: phpList/core
Length of output: 59
🏁 Script executed:
# Look for the specific translation units mentioned in lines 753-772
sed -n '753,772p' resources/translations/messages.en.xlfRepository: phpList/core
Length of output: 1015
🏁 Script executed:
# Search codebase for how translation strings are used/imported
fd -e ts -e js -e tsx -e jsx | head -20 | xargs rg 'messages\.en' 2>/dev/null | head -10Repository: phpList/core
Length of output: 38
🏁 Script executed:
# Look for any configuration or documentation about the __ prefix
fd -e md -e txt -e json | xargs rg '__' 2>/dev/null | grep -i "translation\|prefix\|placeholder" | head -10Repository: phpList/core
Length of output: 38
🏁 Script executed:
# Check the beginning of the translation file for any documentation
head -30 resources/translations/messages.en.xlfRepository: phpList/core
Length of output: 1545
🏁 Script executed:
# Look for other language translation files
fd -e xlf resources/translations/Repository: phpList/core
Length of output: 453
🏁 Script executed:
# Check one other language file to see if it also uses __ prefix
rg '<target>__' resources/translations/messages.*.xlf 2>/dev/nullRepository: phpList/core
Length of output: 6080
🏁 Script executed:
# Search for code that loads or processes XLIFF files
fd -e php -e ts -e js | xargs rg 'xlf|XLIFF|translation' 2>/dev/null | head -15Repository: phpList/core
Length of output: 606
🏁 Script executed:
# Look at a sample of targets without __ prefix to understand the pattern
rg '<target>(?!__)' resources/translations/messages.en.xlf | head -5Repository: phpList/core
Length of output: 281
🏁 Script executed:
# Check git history of the translation file to understand the pattern
git log --oneline -20 resources/translations/messages.en.xlfRepository: phpList/core
Length of output: 79
🏁 Script executed:
# Look for any code that processes or strips the __ prefix
rg '__' src/ --type php --type ts --type js -B 2 -A 2 | head -30Repository: phpList/core
Length of output: 2120
🏁 Script executed:
# Check if there's a pattern in how translations are loaded
rg 'xlf|Xliff|XLIFF' src/ --type php -B 2 -A 2Repository: phpList/core
Length of output: 38
🏁 Script executed:
# Check if there's any configuration or setup file that mentions this pattern
fd -e yaml -e yml -e json -e md | xargs rg '__' 2>/dev/null | grep -i "translation\|xliff\|prefix" | head -10Repository: phpList/core
Length of output: 38
🏁 Script executed:
# Look at more context around the file to understand intent
head -100 resources/translations/messages.en.xlf | tail -60Repository: phpList/core
Length of output: 3674
🏁 Script executed:
# Check if there are other language files to compare
fd --full-path 'resources/translations/' . -name '*.xlf'Repository: phpList/core
Length of output: 286
Remove the __ prefix from target values to match source strings.
The new translation units incorrectly prefix target values with __ (e.g., __Unsubscribe, __This link). For an English-to-English translation file, targets should match sources exactly. This inconsistency appears earlier in the file where entries are correctly formed—compare the new entries against lines like "Not authorized" which correctly have target matching source. If these strings are used in the application, the __ prefix will display to users as-is, breaking the UI text.
Diff showing required fixes:
- <target>__Unsubscribe</target>
+ <target>Unsubscribe</target>
- <target>__This link</target>
+ <target>This link</target>
- <target>__Confirm</target>
+ <target>Confirm</target>
- <target>__Update preferences</target>
+ <target>Update preferences</target>
- <target>__Sorry, you are not subscribed to any of our newsletters with this email address.</target>
+ <target>Sorry, you are not subscribed to any of our newsletters with this email address.</target>
🤖 Prompt for AI Agents
In @resources/translations/messages.en.xlf around lines 753 - 772, The target
strings for the translation units PpLvt2Z, njfOwly, 7r3SSnf, YibbDEd, and
BkWF7jw incorrectly include a "__" prefix; update each <target> value to exactly
match its <source> (remove the "__" prefix) so "Unsubscribe", "This link",
"Confirm", "Update preferences", and "Sorry, you are not subscribed to any of
our newsletters with this email address." are used as the target values.
| public function __invoke(PlaceholderContext $ctx): string | ||
| { | ||
| $url = $this->config->getValue(ConfigOption::SubscribeUrl) ?? ''; | ||
| $sep = !str_contains($url, '?') ? '?' : '&'; | ||
|
|
||
| if ($ctx->isHtml()) { | ||
| return sprintf('%s%suid=%s', $url, htmlspecialchars($sep), $ctx->getUser()->getUniqueId()); | ||
| } | ||
|
|
||
| return sprintf('%s%suid=%s', $url, $sep, $ctx->getUser()->getUniqueId()); | ||
| } |
There was a problem hiding this comment.
URL-encode the uid parameter.
The uid value is directly concatenated into the query string without URL encoding on both lines 28 and 31. This will produce malformed URLs if the uid contains special characters.
🔧 Suggested fix
if ($ctx->isHtml()) {
- return sprintf('%s%suid=%s', $url, htmlspecialchars($sep), $ctx->getUser()->getUniqueId());
+ return sprintf('%s%suid=%s', $url, htmlspecialchars($sep), urlencode($ctx->getUser()->getUniqueId()));
}
- return sprintf('%s%suid=%s', $url, $sep, $ctx->getUser()->getUniqueId());
+ return sprintf('%s%suid=%s', $url, $sep, urlencode($ctx->getUser()->getUniqueId()));🤖 Prompt for AI Agents
In
@src/Domain/Configuration/Service/Placeholder/ConfirmationUrlValueResolver.php
around lines 22 - 32, The uid is inserted into the query string without
URL-encoding in the __invoke method (using $ctx->getUser()->getUniqueId()),
which can break URLs; compute an encoded uid (e.g. $encodedUid =
rawurlencode($ctx->getUser()->getUniqueId())) and use that in both return paths,
and in the HTML branch also HTML-escape the encoded uid (e.g.
htmlspecialchars($encodedUid)) so the query value is both URL-safe and safe for
HTML output.
| if ($ctx->isHtml()) { | ||
| return sprintf( | ||
| '%s%suid=%s&mid=%d', | ||
| htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'), | ||
| htmlspecialchars($sep), | ||
| $ctx->getUser()->getUniqueId(), | ||
| $ctx->messageId(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add trailing space in HTML branch for consistency.
The non-HTML branch (line 37) includes a trailing space, but the HTML branch doesn't. For consistent formatting across output modes, consider adding a space after the closing tag.
Suggested fix
if ($ctx->isHtml()) {
return sprintf(
- '%s%suid=%s&mid=%d',
+ '%s%suid=%s&mid=%d ',
htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
htmlspecialchars($sep),
$ctx->getUser()->getUniqueId(),
$ctx->messageId(),
);
}📝 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 ($ctx->isHtml()) { | |
| return sprintf( | |
| '%s%suid=%s&mid=%d', | |
| htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'), | |
| htmlspecialchars($sep), | |
| $ctx->getUser()->getUniqueId(), | |
| $ctx->messageId(), | |
| ); | |
| } | |
| if ($ctx->isHtml()) { | |
| return sprintf( | |
| '%s%suid=%s&mid=%d ', | |
| htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'), | |
| htmlspecialchars($sep), | |
| $ctx->getUser()->getUniqueId(), | |
| $ctx->messageId(), | |
| ); | |
| } |
🤖 Prompt for AI Agents
In @src/Domain/Configuration/Service/Placeholder/ForwardUrlValueResolver.php
around lines 27 - 35, The HTML branch in ForwardUrlValueResolver.php (inside the
conditional using $ctx->isHtml() and the sprintf call) omits the trailing space
present in the non-HTML branch; edit the sprintf format string in that branch to
append a single trailing space (e.g., add a space after the final substitution
in the format) so the HTML output formatting matches the non-HTML branch while
preserving the existing htmlspecialchars calls and argument order.
| return sprintf( | ||
| '<a href="%s%suid=%s&mid=%d">%s</a> ', | ||
| $safeUrl, | ||
| htmlspecialchars($sep), | ||
| $ctx->getUser()->getUniqueId(), | ||
| $ctx->messageId(), | ||
| $safeLabel | ||
| ); |
There was a problem hiding this comment.
URL-encode the uid parameter to prevent broken query strings.
The uid value from $ctx->getUser()->getUniqueId() is concatenated directly into the URL without URL encoding. If the uid contains special characters like &, =, or ?, the query string will break.
Additionally, $ctx->messageId() can return null (per PlaceholderContext), but it's used with %d format specifier, which expects an integer.
🔧 Suggested fix with URL encoding and null safety
return sprintf(
- '<a href="%s%suid=%s&mid=%d">%s</a> ',
+ '<a href="%s%suid=%s&mid=%s">%s</a> ',
$safeUrl,
htmlspecialchars($sep),
- $ctx->getUser()->getUniqueId(),
- $ctx->messageId(),
+ urlencode($ctx->getUser()->getUniqueId()),
+ urlencode((string)($ctx->messageId() ?? '')),
$safeLabel
);📝 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.
| return sprintf( | |
| '<a href="%s%suid=%s&mid=%d">%s</a> ', | |
| $safeUrl, | |
| htmlspecialchars($sep), | |
| $ctx->getUser()->getUniqueId(), | |
| $ctx->messageId(), | |
| $safeLabel | |
| ); | |
| return sprintf( | |
| '<a href="%s%suid=%s&mid=%s">%s</a> ', | |
| $safeUrl, | |
| htmlspecialchars($sep), | |
| urlencode($ctx->getUser()->getUniqueId()), | |
| urlencode((string)($ctx->messageId() ?? '')), | |
| $safeLabel | |
| ); |
🤖 Prompt for AI Agents
In @src/Domain/Configuration/Service/Placeholder/ForwardValueResolver.php around
lines 34 - 41, The sprintf call in ForwardValueResolver returning the anchor
should URL-encode the uid from $ctx->getUser()->getUniqueId() and handle
nullable messageId from $ctx->messageId(); change the format to accept a string
for message id (or coalesce to a safe default) and pass
urlencode($ctx->getUser()->getUniqueId()) and a null-safe message id (e.g.,
$ctx->messageId() ?? 0 or (string)($ctx->messageId() ?? '')) so the uid won’t
break the query string and the message id won’t cause a %d type error.
| if ($ctx->isHtml()) { | ||
| return sprintf('%s%suid=%s', $url, htmlspecialchars($sep), $ctx->getUser()->getUniqueId()); | ||
| } |
There was a problem hiding this comment.
Escape the URL in HTML context for security.
The separator is being escaped but the URL itself isn't. Other resolvers like ForwardUrlValueResolver (line 30) properly escape the URL. If the configured PreferencesUrl contains special HTML characters, this could lead to rendering issues or XSS vulnerabilities.
Recommended fix
if ($ctx->isHtml()) {
- return sprintf('%s%suid=%s', $url, htmlspecialchars($sep), $ctx->getUser()->getUniqueId());
+ return sprintf(
+ '%s%suid=%s',
+ htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
+ htmlspecialchars($sep),
+ $ctx->getUser()->getUniqueId()
+ );
}🤖 Prompt for AI Agents
In @src/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolver.php
around lines 27 - 29, When rendering in HTML context inside
PreferencesUrlValueResolver (the branch checking $ctx->isHtml()), the $url is
not escaped; update the HTML branch to run the configured PreferencesUrl through
htmlspecialchars (same flags/encoding/boolean for double-encode as used in
ForwardUrlValueResolver) before formatting the final string so both the URL and
$sep are escaped and the output avoids rendering/XSS issues.
| return sprintf( | ||
| '<a href="%s%suid=%s">%s</a> ', | ||
| $safeUrl, | ||
| htmlspecialchars($sep), | ||
| $ctx->getUser()->getUniqueId(), | ||
| $safeLabel, | ||
| ); |
There was a problem hiding this comment.
URL-encode the uid parameter.
Similar to ForwardValueResolver, the uid is not URL-encoded before being concatenated into the query string. If the uid contains special URL characters, the generated link will break.
🔧 Suggested fix
return sprintf(
'<a href="%s%suid=%s">%s</a> ',
$safeUrl,
htmlspecialchars($sep),
- $ctx->getUser()->getUniqueId(),
+ urlencode($ctx->getUser()->getUniqueId()),
$safeLabel,
);Apply the same fix to line 43 for the plain text path:
- return sprintf('%s%suid=%s', $url, $sep, $ctx->getUser()->getUniqueId());
+ return sprintf('%s%suid=%s', $url, $sep, urlencode($ctx->getUser()->getUniqueId()));📝 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.
| return sprintf( | |
| '<a href="%s%suid=%s">%s</a> ', | |
| $safeUrl, | |
| htmlspecialchars($sep), | |
| $ctx->getUser()->getUniqueId(), | |
| $safeLabel, | |
| ); | |
| return sprintf( | |
| '<a href="%s%suid=%s">%s</a> ', | |
| $safeUrl, | |
| htmlspecialchars($sep), | |
| urlencode($ctx->getUser()->getUniqueId()), | |
| $safeLabel, | |
| ); |
🤖 Prompt for AI Agents
In @src/Domain/Configuration/Service/Placeholder/PreferencesValueResolver.php
around lines 34 - 40, The uid parameter is not URL-encoded when building the
link in PreferencesValueResolver (the sprintf using
$ctx->getUser()->getUniqueId()), which can break links with special characters;
update the code to URL-encode the uid (use rawurlencode(...) as done in
ForwardValueResolver) when interpolating into the query string and apply the
same change to the plain-text path occurrence on the subsequent line so both
HTML and text outputs use the encoded UID.
| if ($ctx->isHtml()) { | ||
| if ($this->emailTextCredits) { | ||
| return $this->config->getValue(ConfigOption::PoweredByText); | ||
| } | ||
|
|
||
| return preg_replace( | ||
| '/src=".*power-phplist.png"/', | ||
| 'src="powerphplist.png"', | ||
| $this->config->getValue(ConfigOption::PoweredByImage) | ||
| ); |
There was a problem hiding this comment.
Fix null handling for config values.
Lines 27 and 33 call $this->config->getValue(), which returns ?string, but the method's return type is non-nullable string. If the config values are missing, this produces a type error. Additionally, preg_replace on null (line 30-34) will trigger deprecation warnings in PHP 8.1+.
🔧 Suggested fix with null coalescing
if ($ctx->isHtml()) {
if ($this->emailTextCredits) {
- return $this->config->getValue(ConfigOption::PoweredByText);
+ return $this->config->getValue(ConfigOption::PoweredByText) ?? '';
}
return preg_replace(
'/src=".*power-phplist.png"/',
'src="powerphplist.png"',
- $this->config->getValue(ConfigOption::PoweredByImage)
+ $this->config->getValue(ConfigOption::PoweredByImage) ?? ''
);
}📝 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 ($ctx->isHtml()) { | |
| if ($this->emailTextCredits) { | |
| return $this->config->getValue(ConfigOption::PoweredByText); | |
| } | |
| return preg_replace( | |
| '/src=".*power-phplist.png"/', | |
| 'src="powerphplist.png"', | |
| $this->config->getValue(ConfigOption::PoweredByImage) | |
| ); | |
| if ($ctx->isHtml()) { | |
| if ($this->emailTextCredits) { | |
| return $this->config->getValue(ConfigOption::PoweredByText) ?? ''; | |
| } | |
| return preg_replace( | |
| '/src=".*power-phplist.png"/', | |
| 'src="powerphplist.png"', | |
| $this->config->getValue(ConfigOption::PoweredByImage) ?? '' | |
| ); |
🤖 Prompt for AI Agents
In @src/Domain/Configuration/Service/Placeholder/SignatureValueResolver.php
around lines 25 - 34, The code calls
$this->config->getValue(ConfigOption::PoweredByText) and
$this->config->getValue(ConfigOption::PoweredByImage) which return ?string but
the method expects a non-nullable string and preg_replace on null triggers PHP
8.1+ deprecation warnings; fix by coalescing those calls to a safe string (e.g.
use null coalescing to '' or a sensible default) before returning or passing
into preg_replace, e.g. replace direct calls in the $ctx->isHtml() branch with
$text = $this->config->getValue(ConfigOption::PoweredByText) ?? '' and $img =
$this->config->getValue(ConfigOption::PoweredByImage) ?? '' and use $text /
preg_replace(..., $img) so the method never receives null.
| $resolver->register( | ||
| 'SUBSCRIBEURL', | ||
| fn() => ($this->config->getValue(ConfigOption::SubscribeUrl) ?? '') . self::PHP_SPACE | ||
| name: 'CONTACTURL', | ||
| resolver: fn(PlaceholderContext $ctx) => htmlspecialchars( | ||
| $this->config->getValue(ConfigOption::VCardUrl) ?? '' | ||
| ) | ||
| ); |
There was a problem hiding this comment.
CONTACTURL applies htmlspecialchars unconditionally.
For text format output, HTML escaping isn't needed and could produce unexpected & sequences in URLs. Other resolvers check $ctx->isHtml() before escaping.
🐛 Make escaping conditional on format
$resolver->register(
name: 'CONTACTURL',
- resolver: fn(PlaceholderContext $ctx) => htmlspecialchars(
- $this->config->getValue(ConfigOption::VCardUrl) ?? ''
- )
+ resolver: fn(PlaceholderContext $ctx) => $ctx->isHtml()
+ ? htmlspecialchars($this->config->getValue(ConfigOption::VCardUrl) ?? '', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8')
+ : ($this->config->getValue(ConfigOption::VCardUrl) ?? '')
);📝 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.
| $resolver->register( | |
| 'SUBSCRIBEURL', | |
| fn() => ($this->config->getValue(ConfigOption::SubscribeUrl) ?? '') . self::PHP_SPACE | |
| name: 'CONTACTURL', | |
| resolver: fn(PlaceholderContext $ctx) => htmlspecialchars( | |
| $this->config->getValue(ConfigOption::VCardUrl) ?? '' | |
| ) | |
| ); | |
| $resolver->register( | |
| name: 'CONTACTURL', | |
| resolver: fn(PlaceholderContext $ctx) => $ctx->isHtml() | |
| ? htmlspecialchars($this->config->getValue(ConfigOption::VCardUrl) ?? '', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') | |
| : ($this->config->getValue(ConfigOption::VCardUrl) ?? '') | |
| ); |
🤖 Prompt for AI Agents
In @src/Domain/Configuration/Service/UserPersonalizer.php around lines 58 - 63,
The CONTACTURL resolver currently unconditionally applies htmlspecialchars;
change it to only escape when the placeholder context requests HTML by checking
PlaceholderContext::$ctx->isHtml(): fetch the URL via
$this->config->getValue(ConfigOption::VCardUrl) ?? '', and return
htmlspecialchars(...) only if $ctx->isHtml() is true, otherwise return the raw
URL string so text-format output isn't HTML-escaped.
|
|
||
| $mail = new Email(); | ||
| return $mail; | ||
| } |
There was a problem hiding this comment.
Critical: build() returns an empty Email without applying computed content.
All the work done to compute $htmlmessage, $textmessage, and footers is discarded—the method returns new Email() with no subject, body, recipients, or anything else configured. This will send blank emails.
🐛 The Email needs to be configured with the computed values
$mail = new Email();
+ // Apply computed content to the email
+ // $mail->subject(...);
+ // $mail->html($htmlmessage . $html['footer']);
+ // $mail->text($textmessage . $text['footer']);
+ // $mail->from(...);
+ // $mail->to($subscriber->getEmail());
return $mail;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/Domain/Messaging/Service/Constructor/MailConstructor.php around lines 94
- 97, The build() method in MailConstructor.php currently returns a bare new
Email() discarding the computed $htmlmessage, $textmessage, recipients and
footer data; update the method to apply those computed values to the Email
instance (use the Email returned from new Email() and call its setters to set
subject, from, to/cc/bcc, setHtml/setText or equivalent, add attachments and any
headers/footers) and then return that configured Email object instead of an
empty one so the generated content and recipients are actually sent.
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.
Thanks for contributing to phpList!