Skip to content

Refactor CampaignProcessorMessageHandler #374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TatevikGr wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Refactor CampaignProcessorMessageHandler #374

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a72d2e9
UserPersonalizer in CampaignProcessorMessageHandler
tatevikg1 Dec 11, 2025
d40dedd
HtmlToText
tatevikg1 Dec 11, 2025
613a196
MessageDataLoader
tatevikg1 Dec 11, 2025
759d8e0
TextParser
tatevikg1 Dec 11, 2025
d94f825
RemotePageFetcher
tatevikg1 Dec 13, 2025
3b9267f
Use repo methods
tatevikg1 Dec 14, 2025
5fc8637
Use MessagePrecacheDto
tatevikg1 Dec 14, 2025
69884a8
Refactor
tatevikg1 Dec 14, 2025
077bc63
Todo
tatevikg1 Dec 15, 2025
492e1d0
SystemMailConstructor
tatevikg1 Dec 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
namespace PhpList\Core\Domain\Messaging\MessageHandler;

use Doctrine\ORM\EntityManagerInterface;
use PhpList\Core\Domain\Configuration\Service\UserPersonalizer;
use PhpList\Core\Domain\Messaging\Exception\MessageSizeLimitExceededException;
use PhpList\Core\Domain\Messaging\Message\CampaignProcessorMessage;
use PhpList\Core\Domain\Messaging\Message\SyncCampaignProcessorMessage;
Expand Down Expand Up @@ -56,6 +57,7 @@ public function __construct(
private readonly EventLogManager $eventLogManager,
private readonly MessageDataManager $messageDataManager,
private readonly MessagePrecacheService $precacheService,
private readonly UserPersonalizer $userPersonalizer,
?int $maxMailSize = null,
) {
$this->maxMailSize = $maxMailSize ?? 0;
Expand Down Expand Up @@ -159,6 +161,8 @@ private function handleEmailSending(
Message\MessageContent $precachedContent,
): void {
$processed = $this->messagePreparator->processMessageLinks($campaign->getId(), $precachedContent, $subscriber);
$processed->setText($this->userPersonalizer->personalize($processed->getText(), $subscriber->getEmail()));
$processed->setFooter($this->userPersonalizer->personalize($processed->getFooter(), $subscriber->getEmail()));
Copy link

@coderabbitai coderabbitai bot Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for UserPersonalizer class
fd -t f "UserPersonalizer" --type f

Repository: phpList/core

Length of output: 174

🏁 Script executed:

#!/bin/bash
# Search for the personalize method implementation
rg -t php "class UserPersonalizer" -A 20

Repository: phpList/core

Length of output: 4188

🏁 Script executed:

#!/bin/bash
# Search for personalize method definition with more context
rg -t php "personalize\(" --type-add "php:*.php" -B 2 -A 10

Repository: phpList/core

Length of output: 9578

🏁 Script executed:

#!/bin/bash
# Find the ProcessedMessage or Message class used at lines 169-170
rg -t php "class.*Message" src/Domain/Messaging --type-add "php:*.php" -B 2 -A 5 | grep -A 20 "ProcessedMessage\|class.*Message {" | head -40

Repository: phpList/core

Length of output: 38

🏁 Script executed:

#!/bin/bash
# Search for getText and getFooter methods
rg -t php "getText|getFooter" src/Domain --type-add "php:*.php" -B 2 -A 3

Repository: phpList/core

Length of output: 6212

Fix type mismatch: getText() and getFooter() return nullable strings, but personalize() expects non-nullable string.

The getText() and getFooter() methods return ?string, but UserPersonalizer::personalize() has a non-nullable string $value parameter. Pass null-safe values using the null-coalescing operator:

$processed->setText($this->userPersonalizer->personalize($processed->getText() ?? '', $subscriber->getEmail()));
$processed->setFooter($this->userPersonalizer->personalize($processed->getFooter() ?? '', $subscriber->getEmail()));

Alternatively, add null checks before calling personalize() if empty strings are not the desired fallback.

🤖 Prompt for AI Agents 
In src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
around lines 169-170, getText() and getFooter() return ?string but
UserPersonalizer::personalize() requires a non-nullable string; update the calls
to pass a non-null value (for example use the null-coalescing operator to pass
an empty string when null) or add explicit null checks to skip personalization
when the value is null so personalize() never receives null.


try {
$email = $this->mailer->composeEmail($campaign, $subscriber, $processed);
Expand Down
7 changes: 1 addition & 6 deletions src/Domain/Messaging/Service/MessageProcessingPreparator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
namespace PhpList\Core\Domain\Messaging\Service;

use PhpList\Core\Domain\Analytics\Service\LinkTrackService;
use PhpList\Core\Domain\Configuration\Service\UserPersonalizer;
use PhpList\Core\Domain\Messaging\Model\Message;
use PhpList\Core\Domain\Messaging\Model\Message\MessageContent;
use PhpList\Core\Domain\Messaging\Repository\MessageRepository;
use PhpList\Core\Domain\Subscription\Model\Subscriber;
Expand All @@ -24,7 +22,6 @@ public function __construct(
private readonly MessageRepository $messageRepository,
private readonly LinkTrackService $linkTrackService,
private readonly TranslatorInterface $translator,
private readonly UserPersonalizer $userPersonalizer,
) {
}

Expand Down Expand Up @@ -78,16 +75,14 @@ public function processMessageLinks(

$htmlText = $content->getText();
$footer = $content->getFooter();
// todo: check other configured data that should be used in mail formatting/creation
// todo: check if getTextMessage should replace links as well
if ($htmlText !== null) {
$htmlText = $this->replaceLinks($savedLinks, $htmlText);
$htmlText = $this->userPersonalizer->personalize($htmlText, $subscriber->getEmail());
$content->setText($htmlText);
}

if ($footer !== null) {
$footer = $this->replaceLinks($savedLinks, $footer);
$footer = $this->userPersonalizer->personalize($footer, $subscriber->getEmail());
$content->setFooter($footer);
}

Expand Down
21 changes: 12 additions & 9 deletions src/Domain/Messaging/Service/RateLimitedCampaignMailer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,26 @@ public function __construct(MailerInterface $mailer, SendRateLimiter $limiter)
$this->limiter = $limiter;
}

public function composeEmail(Message $processed, Subscriber $subscriber, Message\MessageContent $content): Email
{
public function composeEmail(
Message $message,
Subscriber $subscriber,
Message\MessageContent $processedContent,
): Email {
$email = new Email();
if ($processed->getOptions()->getFromField() !== '') {
$email->from($processed->getOptions()->getFromField());
if ($message->getOptions()->getFromField() !== '') {
$email->from($message->getOptions()->getFromField());
}

if ($processed->getOptions()->getReplyTo() !== '') {
$email->replyTo($processed->getOptions()->getReplyTo());
if ($message->getOptions()->getReplyTo() !== '') {
$email->replyTo($message->getOptions()->getReplyTo());
}

return $email
->to($subscriber->getEmail())
->subject($content->getSubject())
->subject($processedContent->getSubject())
// todo: check HTML2Text functionality
->text($content->getTextMessage())
->html($content->getText());
->text($processedContent->getTextMessage())
->html($processedContent->getText());
}

/**
Expand Down
16 changes: 16 additions & 0 deletions tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Doctrine\ORM\EntityManagerInterface;
use Exception;
use PhpList\Core\Domain\Configuration\Service\Manager\EventLogManager;
use PhpList\Core\Domain\Configuration\Service\UserPersonalizer;
use PhpList\Core\Domain\Messaging\Message\CampaignProcessorMessage;
use PhpList\Core\Domain\Messaging\MessageHandler\CampaignProcessorMessageHandler;
use PhpList\Core\Domain\Messaging\Model\Message;
Expand Down Expand Up @@ -57,10 +58,18 @@ protected function setUp(): void
$requeueHandler = $this->createMock(RequeueHandler::class);
$this->translator = $this->createMock(Translator::class);
$this->precacheService = $this->createMock(MessagePrecacheService::class);
$userPersonalizer = $this->createMock(UserPersonalizer::class);

$timeLimiter->method('start');
$timeLimiter->method('shouldStop')->willReturn(false);

// Ensure personalization returns original text so assertions on replaced links remain valid
$userPersonalizer
->method('personalize')
->willReturnCallback(function (string $text) {
return $text;
});

$this->handler = new CampaignProcessorMessageHandler(
mailer: $this->mailer,
entityManager: $this->entityManager,
Expand All @@ -77,6 +86,7 @@ protected function setUp(): void
eventLogManager: $this->createMock(EventLogManager::class),
messageDataManager: $this->createMock(MessageDataManager::class),
precacheService: $this->precacheService,
userPersonalizer: $userPersonalizer,
maxMailSize: 0,
);
}
Expand Down Expand Up @@ -166,6 +176,8 @@ public function testInvokeWithValidSubscriberEmail(): void
{
$campaign = $this->createMock(Message::class);
$content = $this->createContentMock();
$content->method('getText')->willReturn('<p>Test HTML message</p>');
$content->method('getFooter')->willReturn('<p>Test footer message</p>');
$campaign->method('getContent')->willReturn($content);
$metadata = $this->createMock(MessageMetadata::class);
$campaign->method('getMetadata')->willReturn($metadata);
Expand Down Expand Up @@ -225,6 +237,8 @@ public function testInvokeWithMailerException(): void
{
$campaign = $this->createMock(Message::class);
$content = $this->createContentMock();
$content->method('getText')->willReturn('<p>Test HTML message</p>');
$content->method('getFooter')->willReturn('<p>Test footer message</p>');
$metadata = $this->createMock(MessageMetadata::class);
$campaign->method('getContent')->willReturn($content);
$campaign->method('getMetadata')->willReturn($metadata);
Expand Down Expand Up @@ -278,6 +292,8 @@ public function testInvokeWithMultipleSubscribers(): void
{
$campaign = $this->createCampaignMock();
$content = $this->createContentMock();
$content->method('getText')->willReturn('<p>Test HTML message</p>');
$content->method('getFooter')->willReturn('<p>Test footer message</p>');
$metadata = $this->createMock(MessageMetadata::class);
$campaign->method('getMetadata')->willReturn($metadata);
$campaign->method('getId')->willReturn(1);
Expand Down
10 changes: 0 additions & 10 deletions tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

use PhpList\Core\Domain\Analytics\Model\LinkTrack;
use PhpList\Core\Domain\Analytics\Service\LinkTrackService;
use PhpList\Core\Domain\Configuration\Service\UserPersonalizer;
use PhpList\Core\Domain\Messaging\Model\Message\MessageContent;
use PhpList\Core\Domain\Messaging\Repository\MessageRepository;
use PhpList\Core\Domain\Messaging\Service\MessageProcessingPreparator;
Expand All @@ -23,7 +22,6 @@ class MessageProcessingPreparatorTest extends TestCase
private SubscriberRepository&MockObject $subscriberRepository;
private MessageRepository&MockObject $messageRepository;
private LinkTrackService&MockObject $linkTrackService;
private UserPersonalizer&MockObject $userPersonalizer;
private OutputInterface&MockObject $output;
private MessageProcessingPreparator $preparator;

Expand All @@ -32,21 +30,13 @@ protected function setUp(): void
$this->subscriberRepository = $this->createMock(SubscriberRepository::class);
$this->messageRepository = $this->createMock(MessageRepository::class);
$this->linkTrackService = $this->createMock(LinkTrackService::class);
$this->userPersonalizer = $this->createMock(UserPersonalizer::class);
// Ensure personalization returns original text so assertions on replaced links remain valid
$this->userPersonalizer
->method('personalize')
->willReturnCallback(function (string $text) {
return $text;
});
$this->output = $this->createMock(OutputInterface::class);

$this->preparator = new MessageProcessingPreparator(
subscriberRepository: $this->subscriberRepository,
messageRepository: $this->messageRepository,
linkTrackService: $this->linkTrackService,
translator: new Translator('en'),
userPersonalizer: $this->userPersonalizer,
);
}

Expand Down
Loading