Feat/user personalizer #375
Conversation
📝 WalkthroughWalkthroughConfiguration parameters added to support preference page private list visibility. New OutputFormat enum introduced for HTML/Text formatting. UserPersonalizer enhanced with format-aware URL and list name resolution. SubscriberListRepository extended with list-name retrieval methods. Multiple message handlers refactored to use constructor property promotion. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Thanks for contributing to phpList!