Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 4 additions & 8 deletions src/Domain/Analytics/Service/LinkTrackService.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,10 @@

class LinkTrackService
{
private LinkTrackRepository $linkTrackRepository;
private ParameterProvider $paramProvider;

public function __construct(LinkTrackRepository $linkTrackRepository, ParameterProvider $paramProvider)
{
$this->linkTrackRepository = $linkTrackRepository;
$this->paramProvider = $paramProvider;
public function __construct(
private readonly LinkTrackRepository $linkTrackRepository,
private readonly ParameterProvider $paramProvider,
) {
}

public function getUrlById(int $id): ?string
Expand Down Expand Up @@ -58,7 +55,6 @@ public function extractAndSaveLinks(MessagePrecacheDto $content, int $userId, ?i
$links = array_unique($links);

$savedLinks = [];

foreach ($links as $url) {
$existingLinkTrack = $this->linkTrackRepository->findByUrlUserIdAndMessageId($url, $userId, $messageId);
if ($existingLinkTrack !== null) {
Expand Down
5 changes: 2 additions & 3 deletions src/Domain/Identity/Service/PermissionChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use PhpList\Core\Domain\Common\Model\Interfaces\OwnableInterface;
use PhpList\Core\Domain\Identity\Model\Administrator;
use PhpList\Core\Domain\Identity\Model\PrivilegeFlag;
use PhpList\Core\Domain\Messaging\Model\ListMessage;
use PhpList\Core\Domain\Messaging\Model\Message;
use PhpList\Core\Domain\Subscription\Model\Subscriber;
use PhpList\Core\Domain\Subscription\Model\SubscriberList;
Expand Down Expand Up @@ -70,9 +71,7 @@ private function resolveRelatedEntity(DomainModel $resource, string $relatedClas
}

if ($resource instanceof Message && $relatedClass === SubscriberList::class) {
// todo: check which one is correct
// return $resource->getListMessages()->map(fn(ListMessage $lm) => $lm->getList())->toArray();
return $resource->getListMessages()->map(fn($lm) => $lm->getSubscriberList())->toArray();
return $resource->getListMessages()->map(fn(ListMessage $lm) => $lm->getList())->toArray();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Filter nullable list relations before ownership checks.

At Line 74, ListMessage::getList() is nullable, so this array may contain null. That can break owns(...) checks in checkRelatedResources() or cause false negatives.

Suggested patch
-            return $resource->getListMessages()->map(fn(ListMessage $lm) => $lm->getList())->toArray();
+            return $resource->getListMessages()
+                ->map(fn (ListMessage $lm) => $lm->getList())
+                ->filter(fn (?SubscriberList $list) => $list !== null)
+                ->toArray();
📝 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.

Suggested change
return $resource->getListMessages()->map(fn(ListMessage $lm) => $lm->getList())->toArray();
return $resource->getListMessages()
->map(fn (ListMessage $lm) => $lm->getList())
->filter(fn (?SubscriberList $list) => $list !== null)
->toArray();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Domain/Identity/Service/PermissionChecker.php` at line 74, The returned
array from PermissionChecker:: (the expression using
$resource->getListMessages()->map(fn(ListMessage $lm) =>
$lm->getList())->toArray()) can include null because ListMessage::getList() is
nullable; update this to filter out null list relations before returning so
checkRelatedResources() and the owns(...) checks never receive nulls — e.g.,
only include non-null results of getList() (or call ->filter(...) on
getListMessages() or apply array_filter after mapping) so all elements passed
into owns(...) are valid list objects.

}

return [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,12 +199,13 @@ private function handleEmailSending(
UserMessage $userMessage,
MessagePrecacheDto $precachedContent,
): void {
// todo: check at which point link tracking should be applied (maybe after constructing ful text?)
// todo: check at which point link tracking should be applied (maybe after constructing full text?)
$processed = $this->messagePreparator->processMessageLinks(
campaignId: $campaign->getId(),
cachedMessageDto: $precachedContent,
subscriber: $subscriber
);
$this->entityManager->flush();

try {
Comment on lines +208 to 210
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Put the new flush inside the try block.

At Line 208, flush() can throw before control reaches the catch handlers, so the user-message recovery path won’t run.

Suggested patch
-        $this->entityManager->flush();
-
-        try {
+        try {
+            $this->entityManager->flush();
             $result = $this->campaignEmailBuilder->buildCampaignEmail(
                 messageId: $campaign->getId(),
                 data: $processed,
📝 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.

Suggested change
$this->entityManager->flush();
try {
try {
$this->entityManager->flush();
$result = $this->campaignEmailBuilder->buildCampaignEmail(
messageId: $campaign->getId(),
data: $processed,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php`
around lines 208 - 210, The flush() call currently occurs before the try/catch
in CampaignProcessorMessageHandler (inside the message handling method), so
exceptions from $this->entityManager->flush() bypass the catch handlers; move
the $this->entityManager->flush() invocation into the existing try block (so it
executes inside the try that contains the recovery/catch logic) to ensure any
flush exceptions are caught and the existing catch handlers and user-message
recovery path run as intended.

$result = $this->campaignEmailBuilder->buildCampaignEmail(
Expand Down
6 changes: 6 additions & 0 deletions src/Domain/Messaging/Model/Message.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ class Message implements DomainModel, Identity, ModificationDate, OwnableInterfa
#[ORM\JoinColumn(name: 'template', referencedColumnName: 'id', nullable: true, onDelete: 'SET NULL')]
private ?Template $template = null;

/**
* @var Collection<int, ListMessage>
*/
#[ORM\OneToMany(targetEntity: ListMessage::class, mappedBy: 'message')]
private Collection $listMessages;

Expand Down Expand Up @@ -190,6 +193,9 @@ public function setOptions(MessageOptions $options): self
return $this;
}

/**
* @return Collection<int, ListMessage>
*/
public function getListMessages(): Collection
{
return $this->listMessages;
Expand Down
5 changes: 3 additions & 2 deletions src/Domain/Messaging/Model/Message/MessageStatus.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ enum MessageStatus: string
public function allowedTransitions(): array
{
return match ($this) {
self::Draft, self::Suspended => [self::Submitted],
self::Submitted => [self::Prepared, self::InProcess],
self::Draft => [self::Prepared, self::Submitted],
self::Suspended => [self::Submitted],
self::Submitted => [self::Prepared, self::InProcess, self::Suspended],
self::Prepared => [self::InProcess],
self::InProcess => [self::Sent, self::Suspended, self::Submitted],
self::Requeued => [self::InProcess, self::Suspended],
Expand Down
33 changes: 26 additions & 7 deletions src/Domain/Messaging/Service/Manager/MessageManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,16 @@
use PhpList\Core\Domain\Messaging\Model\Dto\MessageContext;
use PhpList\Core\Domain\Messaging\Model\Dto\MessageDtoInterface;
use PhpList\Core\Domain\Messaging\Model\Message;
use PhpList\Core\Domain\Messaging\Model\Message\MessageMetadata;
use PhpList\Core\Domain\Messaging\Repository\MessageRepository;
use PhpList\Core\Domain\Messaging\Service\Builder\MessageBuilder;

class MessageManager
{
private MessageRepository $messageRepository;
private MessageBuilder $messageBuilder;

public function __construct(MessageRepository $messageRepository, MessageBuilder $messageBuilder)
{
$this->messageRepository = $messageRepository;
$this->messageBuilder = $messageBuilder;
public function __construct(
private readonly MessageRepository $messageRepository,
private readonly MessageBuilder $messageBuilder,
) {
}

public function createMessage(MessageDtoInterface $createMessageDto, Administrator $authUser): Message
Expand All @@ -31,6 +29,27 @@ public function createMessage(MessageDtoInterface $createMessageDto, Administrat
return $message;
}

public function copyAsDraftMessage(Message $message, Administrator $authUser): Message
{
$newMessage = new Message(
format: new Message\MessageFormat(
htmlFormatted: $message->getFormat()->isHtmlFormatted(),
sendFormat: $message->getFormat()->getSendFormat()
),
schedule: clone $message->getSchedule(),
metadata: new MessageMetadata(status: Message\MessageStatus::Draft),
content: clone $message->getContent(),
options: clone $message->getOptions(),
owner: $authUser,
template: $message->getTemplate(),
);
$newMessage->setUuid(bin2hex(random_bytes(18)));

$this->messageRepository->persist($newMessage);

return $newMessage;
}

public function updateMessage(
MessageDtoInterface $updateMessageDto,
Message $message,
Expand Down
2 changes: 1 addition & 1 deletion src/Domain/Messaging/Service/MessageDataLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ private function normaliseScheduleFields(array &$messageData): void
private function populateTargetLists(array &$messageData, Message $message): void
{
foreach ($message->getListMessages() as $listMessage) {
$messageData['targetlist'][$listMessage->getListId()] = 1;
$messageData['targetlist'][$listMessage->getList()->getId()] = 1;
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/Domain/Messaging/Service/MessagePrecacheService.php
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public function precacheMessage(Message $campaign, array $loadedMessageData, ?bo
return false;
}

$messagePrecacheDto->googleTrack = $loadedMessageData['google_track'];
$messagePrecacheDto->googleTrack = (bool) $loadedMessageData['google_track'];

$this->applyBasicReplacements($messagePrecacheDto, $loadedMessageData);
$this->populateAdminAttributes($messagePrecacheDto, $campaign);
Expand Down
43 changes: 43 additions & 0 deletions tests/Unit/Domain/Messaging/Service/Manager/MessageManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace PhpList\Core\Tests\Unit\Domain\Messaging\Service\Manager;

use DateTime;
use PhpList\Core\Domain\Identity\Model\Administrator;
use PhpList\Core\Domain\Messaging\Model\Dto\CreateMessageDto;
use PhpList\Core\Domain\Messaging\Model\Dto\Message\MessageContentDto;
Expand All @@ -13,6 +14,10 @@
use PhpList\Core\Domain\Messaging\Model\Dto\Message\MessageScheduleDto;
use PhpList\Core\Domain\Messaging\Model\Dto\UpdateMessageDto;
use PhpList\Core\Domain\Messaging\Model\Message;
use PhpList\Core\Domain\Messaging\Model\Message\MessageFormat;
use PhpList\Core\Domain\Messaging\Model\Message\MessageMetadata;
use PhpList\Core\Domain\Messaging\Model\Message\MessageOptions;
use PhpList\Core\Domain\Messaging\Model\Message\MessageSchedule;
use PhpList\Core\Domain\Messaging\Model\Message\MessageContent;
use PhpList\Core\Domain\Messaging\Repository\MessageRepository;
use PhpList\Core\Domain\Messaging\Service\Builder\MessageBuilder;
Expand All @@ -21,6 +26,44 @@

class MessageManagerTest extends TestCase
{
public function testCopyAsDraftMessagePersistsClonedDraftMessage(): void
{
$messageRepository = $this->createMock(MessageRepository::class);
$messageBuilder = $this->createMock(MessageBuilder::class);
$manager = new MessageManager($messageRepository, $messageBuilder);

$message = new Message(
format: new MessageFormat(true, 'html'),
schedule: new MessageSchedule(
repeatInterval: 0,
repeatUntil: null,
requeueInterval: 0,
requeueUntil: null,
embargo: new DateTime('2025-04-17T09:00:00+00:00')
),
metadata: new MessageMetadata(Message\MessageStatus::Submitted),
content: new MessageContent('Subject', 'Full text', 'Short text', 'Footer'),
options: new MessageOptions('from@example.com', 'to@example.com', 'reply@example.com', 'all-users'),
owner: null
);

$messageRepository->expects($this->once())
->method('persist')
->with($this->callback(function (Message $persistedMessage) use ($message): bool {
$this->assertNotSame($message, $persistedMessage);
$this->assertSame(Message\MessageStatus::Draft, $persistedMessage->getMetadata()->getStatus());
$this->assertTrue($persistedMessage->getFormat()->isHtmlFormatted());
$this->assertSame('html', $persistedMessage->getFormat()->getSendFormat());

return true;
}));

$result = $manager->copyAsDraftMessage($message, $this->createMock(Administrator::class));

$this->assertSame(Message\MessageStatus::Draft, $result->getMetadata()->getStatus());
$this->assertNotSame($message, $result);
}

public function testCreateMessageReturnsPersistedMessage(): void
{
$messageRepository = $this->createMock(MessageRepository::class);
Expand Down
18 changes: 8 additions & 10 deletions tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
use Doctrine\Common\Collections\ArrayCollection;
use PhpList\Core\Domain\Configuration\Model\ConfigOption;
use PhpList\Core\Domain\Configuration\Service\Provider\ConfigProvider;
use PhpList\Core\Domain\Messaging\Model\ListMessage;
use PhpList\Core\Domain\Messaging\Model\Message;
use PhpList\Core\Domain\Messaging\Model\MessageData;
use PhpList\Core\Domain\Messaging\Repository\MessageDataRepository;
use PhpList\Core\Domain\Messaging\Repository\MessageRepository;
use PhpList\Core\Domain\Messaging\Service\MessageDataLoader;
use PhpList\Core\Domain\Subscription\Model\SubscriberList;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use Psr\Log\LoggerInterface;
Expand Down Expand Up @@ -61,6 +63,11 @@ public function testLoadsMessageDataMergesAndParses(): void
$md2 = (new MessageData())->setId($messageId)->setName('criteria_match')->setData('any');
$md3 = (new MessageData())->setId($messageId)->setName('embargo')->setData('string');

$listMock = $this->createMock(SubscriberList::class);
$listMock->method('getId')->willReturn(42);
$listMessageMock = $this->createMock(ListMessage::class);
$listMessageMock->method('getList')->willReturn($listMock);

$this->messageDataRepository
->method('getForMessage')
->with($messageId)
Expand All @@ -69,16 +76,7 @@ public function testLoadsMessageDataMergesAndParses(): void
// Use a Message mock instead of an anonymous stub
$message = $this->createMock(Message::class);
$message->method('getId')->willReturn($messageId);
$message->method('getListMessages')->willReturn(
new ArrayCollection([
new class {
public function getListId(): int
{
return 42;
}
},
])
);
$message->method('getListMessages')->willReturn(new ArrayCollection([$listMessageMock]));

$loader = new MessageDataLoader(
configProvider: $this->config,
Expand Down
Loading