From 3ebedfbfce05a0a3a5b48baedbeaf7e4cafe1c38 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 26 Oct 2026 13:32:10 +0400 Subject: [PATCH 01/30] Update TemplateImageManager to set correct MIME type for resized images and add tests for logo image creation --- .../Service/Manager/TemplateImageManager.php | 8 ++- .../Manager/TemplateImageManagerTest.php | 52 +++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/Domain/Messaging/Service/Manager/TemplateImageManager.php b/src/Domain/Messaging/Service/Manager/TemplateImageManager.php index bb6c0771..15cfbc6c 100644 --- a/src/Domain/Messaging/Service/Manager/TemplateImageManager.php +++ b/src/Domain/Messaging/Service/Manager/TemplateImageManager.php @@ -211,10 +211,14 @@ public function createCachedLogoImage(int $size): void $orgLogoImage ); + // resized output is always PNG, so take the mime type from the final content + $finalSize = getimagesizefromstring($imageContent); + $mimeType = $finalSize !== false ? $finalSize['mime'] : $imgSize['mime']; + // else copy original $templateImage = (new TemplateImage()) ->setFilename('ORGANISATIONLOGO' . $size . '.png') - ->setMimetype($imgSize['mime']) + ->setMimetype($mimeType) ->setData(base64_encode($imageContent)) ->setWidth($newWidth) ->setHeight($newHeight); @@ -266,7 +270,7 @@ private function resizeImageIfNecessary( //# creates a black image (why would you want that....) $resized = imagecreatetruecolor($newWidth, $newHeight); imagesavealpha($resized, true); - //# white. All the methods to make it transparent didn't work for me @@TODO really make transparent + //# white. All the methods to make it transparent didn't work for me $transparent = imagecolorallocatealpha($resized, 255, 255, 255, 127); imagefill($resized, 0, 0, $transparent); [$sizeW, $sizeH] = [$imgSize[0], $imgSize[1]]; diff --git a/tests/Unit/Domain/Messaging/Service/Manager/TemplateImageManagerTest.php b/tests/Unit/Domain/Messaging/Service/Manager/TemplateImageManagerTest.php index 296e0c1d..4d7fb047 100644 --- a/tests/Unit/Domain/Messaging/Service/Manager/TemplateImageManagerTest.php +++ b/tests/Unit/Domain/Messaging/Service/Manager/TemplateImageManagerTest.php @@ -95,6 +95,58 @@ public function testCreateImagesFromImagePathsStoresImageDataAndReplacesExisting $this->assertSame(self::ONE_PIXEL_PNG, $capturedImage->getData()); } + public function testCreateCachedLogoImageStoresResizedLogoAsPng(): void + { + $capturedImage = $this->createCachedLogoImageFromJpeg(width: 100, height: 50, size: 20); + + $this->assertSame('ORGANISATIONLOGO20.png', $capturedImage->getFilename()); + $this->assertSame('image/png', $capturedImage->getMimeType()); + $this->assertSame(20, $capturedImage->getWidth()); + $this->assertSame(10, $capturedImage->getHeight()); + } + + public function testCreateCachedLogoImageKeepsOriginalMimeTypeWhenNotResized(): void + { + $capturedImage = $this->createCachedLogoImageFromJpeg(width: 10, height: 10, size: 500); + + $this->assertSame('image/jpeg', $capturedImage->getMimeType()); + } + + private function createCachedLogoImageFromJpeg(int $width, int $height, int $size): TemplateImage + { + $canvas = imagecreatetruecolor($width, $height); + ob_start(); + imagejpeg($canvas); + $jpeg = (string) ob_get_clean(); + + $logoImage = $this->createMock(TemplateImage::class); + $logoImage->method('getData')->willReturn(base64_encode($jpeg)); + + $this->configProvider->method('getValue')->willReturn('1'); + $this->templateImageRepository->method('findByFilename')->willReturn(null); + $this->templateImageRepository->method('findById')->with(1)->willReturn($logoImage); + + $capturedImage = null; + $this->templateImageRepository->expects($this->once()) + ->method('persist') + ->with($this->callback(static function (TemplateImage $image) use (&$capturedImage): bool { + $capturedImage = $image; + return true; + })); + + // resizing closes the buffer that was active when it started, so provide one to keep PHPUnit's intact + $obLevel = ob_get_level(); + ob_start(); + $this->manager->createCachedLogoImage($size); + while (ob_get_level() > $obLevel) { + ob_end_clean(); + } + + $this->assertInstanceOf(TemplateImage::class, $capturedImage); + + return $capturedImage; + } + public function testGuessMimeType(): void { $reflection = new \ReflectionClass($this->manager); From 7327e7277e6c3309e5066861357bd6b85355509d Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 26 Oct 2026 13:40:01 +0400 Subject: [PATCH 02/30] Refactor UserMessageBounce and UserMessageView to update TODO comments for indexing --- src/Domain/Analytics/Model/UserMessageView.php | 2 +- src/Domain/Messaging/Model/UserMessageBounce.php | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Domain/Analytics/Model/UserMessageView.php b/src/Domain/Analytics/Model/UserMessageView.php index 66240fee..f5eeb46c 100644 --- a/src/Domain/Analytics/Model/UserMessageView.php +++ b/src/Domain/Analytics/Model/UserMessageView.php @@ -15,7 +15,7 @@ #[ORM\Index(name: 'phplist_user_message_view_msgidx', columns: ['messageid'])] #[ORM\Index(name: 'phplist_user_message_view_useridx', columns: ['userid'])] #[ORM\Index(name: 'phplist_user_message_view_usermsgidx', columns: ['userid', 'messageid'])] -// todo: #[ORM\Index(name: 'phplist_user_message_view_viewedidx', columns: ['viewed'])] +// @performance #[ORM\Index(name: 'phplist_user_message_view_viewedidx', columns: ['viewed'])] class UserMessageView implements DomainModel, Identity { #[ORM\Id] diff --git a/src/Domain/Messaging/Model/UserMessageBounce.php b/src/Domain/Messaging/Model/UserMessageBounce.php index 9a350f88..efb6ea61 100644 --- a/src/Domain/Messaging/Model/UserMessageBounce.php +++ b/src/Domain/Messaging/Model/UserMessageBounce.php @@ -19,7 +19,6 @@ #[ORM\Index(name: 'phplist_user_message_bounce_msgidx', columns: ['message'])] #[ORM\Index(name: 'phplist_user_message_bounce_umbindex', columns: ['user', 'message', 'bounce'])] #[ORM\Index(name: 'phplist_user_message_bounce_useridx', columns: ['user'])] -// todo: #[ORM\Index(name: 'phplist_user_message_bounce_timeidx', columns: ['time'])] class UserMessageBounce implements DomainModel, Identity, From 2777d0e54b21ba97f5e964314295dab969ccc5b1 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 26 Oct 2026 13:44:58 +0400 Subject: [PATCH 03/30] Improve writeFileToDirectory method to handle temp file management and add tests for edge cases --- src/Domain/Common/FileHelper.php | 30 ++++++++++++--------- tests/Unit/Domain/Common/FileHelperTest.php | 17 ++++++++++++ 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/Domain/Common/FileHelper.php b/src/Domain/Common/FileHelper.php index b8995b35..e1f4da01 100644 --- a/src/Domain/Common/FileHelper.php +++ b/src/Domain/Common/FileHelper.php @@ -33,31 +33,37 @@ public function readFileContents(string $path): ?string } } - // todo: check - Handle temp-file rename and write failures. - // Appending an extension leaves the original temp file orphaned, and fopen/fwrite failures aren’t - // checked (so the method can return a path that wasn’t actually written) public function writeFileToDirectory(string $directory, string $originalFilename, string $contents): ?string { + // tempnam() silently falls back to the system temp dir when the directory is unusable + if (!is_dir($directory) || !is_writable($directory)) { + return null; + } + $pathInfo = pathinfo($originalFilename); $name = $pathInfo['filename'] === '' ? 'file' : $pathInfo['filename']; $ext = $pathInfo['extension'] ?? ''; - $newFile = tempnam($directory, $name); - if ($newFile === false) { + $tempFile = tempnam($directory, $name); + if ($tempFile === false) { return null; } + $fullPath = $directory . '/' . basename($tempFile); if ($ext !== '') { - $newFile .= '.' . $ext; - } - $relativeName = basename($newFile); + $fullPath .= '.' . $ext; + if (!rename($tempFile, $fullPath)) { + unlink($tempFile); - $fullPath = $directory . '/' . $relativeName; + return null; + } + } - $fileHandle = fopen($fullPath, 'w'); + if (file_put_contents($fullPath, $contents) !== strlen($contents)) { + unlink($fullPath); - fwrite($fileHandle, $contents); - fclose($fileHandle); + return null; + } return $fullPath; } diff --git a/tests/Unit/Domain/Common/FileHelperTest.php b/tests/Unit/Domain/Common/FileHelperTest.php index 9f652623..fae886d4 100644 --- a/tests/Unit/Domain/Common/FileHelperTest.php +++ b/tests/Unit/Domain/Common/FileHelperTest.php @@ -74,6 +74,23 @@ public function testWriteFileToDirectoryCreatesFileWithExtensionAndContents(): v $this->assertSame('payload', file_get_contents($writtenPath)); } + public function testWriteFileToDirectoryLeavesNoOrphanedTempFile(): void + { + $helper = new FileHelper(); + + $writtenPath = $helper->writeFileToDirectory($this->tmpDir, 'report.txt', 'payload'); + + $this->assertSame([$writtenPath], glob($this->tmpDir . '/*')); + } + + public function testWriteFileToDirectoryReturnsNullWhenDirectoryIsMissing(): void + { + $helper = new FileHelper(); + + $this->assertNull($helper->writeFileToDirectory($this->tmpDir . '/missing', 'report.txt', 'payload')); + $this->assertSame([], glob($this->tmpDir . '/*')); + } + public function testWriteFileToDirectoryUsesDefaultNameWhenMissing(): void { $helper = new FileHelper(); From 54e8f5f4e583e6f741b87a90d7b595c97a9f6e9f Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 26 Oct 2026 13:53:50 +0400 Subject: [PATCH 04/30] Remove TODO comment for upload directory configuration in LocalUploadStorage --- src/Domain/Common/Storage/LocalUploadStorage.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Domain/Common/Storage/LocalUploadStorage.php b/src/Domain/Common/Storage/LocalUploadStorage.php index bc731088..b08bedb4 100644 --- a/src/Domain/Common/Storage/LocalUploadStorage.php +++ b/src/Domain/Common/Storage/LocalUploadStorage.php @@ -22,7 +22,6 @@ public function __construct( public function store(UploadedFile $file, string $filename): StoredFile { - // todo: get upload_dir from configs $targetDirectory = $this->projectDir . DIRECTORY_SEPARATOR . 'public' From c52c7cb2686f16ff2e0d5a9cee1339dc9abe61fc Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 26 Oct 2026 14:09:13 +0400 Subject: [PATCH 05/30] Add methods to retrieve bounce report data and refactor database queries in UserMessageBounceElasticsearchHybridReader --- .../Messaging/Repository/BounceRepository.php | 19 +++++ .../Repository/MessageRepository.php | 21 ++++++ ...MessageBounceElasticsearchHybridReader.php | 75 +++---------------- .../Repository/UserMessageRepository.php | 15 ++++ .../Repository/SubscriberRepository.php | 24 ++++++ ...ageBounceElasticsearchHybridReaderTest.php | 9 ++- 6 files changed, 97 insertions(+), 66 deletions(-) diff --git a/src/Domain/Messaging/Repository/BounceRepository.php b/src/Domain/Messaging/Repository/BounceRepository.php index e3e0ab4f..1ff480f8 100644 --- a/src/Domain/Messaging/Repository/BounceRepository.php +++ b/src/Domain/Messaging/Repository/BounceRepository.php @@ -97,4 +97,23 @@ public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedRes lastId: $lastId, ); } + + /** + * @param int[] $ids + * @return array indexed by bounce id + */ + public function findByIdsIndexedById(array $ids): array + { + if ($ids === []) { + return []; + } + + $bouncesById = []; + /** @var Bounce $bounce */ + foreach ($this->findBy(['id' => $ids]) as $bounce) { + $bouncesById[$bounce->getId()] = $bounce; + } + + return $bouncesById; + } } diff --git a/src/Domain/Messaging/Repository/MessageRepository.php b/src/Domain/Messaging/Repository/MessageRepository.php index c5698775..1d95eb41 100644 --- a/src/Domain/Messaging/Repository/MessageRepository.php +++ b/src/Domain/Messaging/Repository/MessageRepository.php @@ -274,6 +274,27 @@ public function getNonEmptyFields(int $id): array return $message; } + /** + * Campaign ids and subjects for bounce reports, without any bounce counts. + * + * @param int|null $ownerId Limit results to campaigns owned by this admin when provided. + * @return array + */ + public function getSubjectsForBounceReport(?int $ownerId = null): array + { + $queryBuilder = $this->createQueryBuilder('m') + ->select('m.id AS messageId', 'm.content.subject AS subject') + ->orderBy('m.id', 'ASC'); + + if ($ownerId !== null) { + $queryBuilder + ->andWhere('IDENTITY(m.owner) = :ownerId') + ->setParameter('ownerId', $ownerId); + } + + return $queryBuilder->getQuery()->getArrayResult(); + } + /** * Counts active campaigns between two dates. * "Active" here means messages that were sent (or in process) during this period. diff --git a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php index 9bc94e1f..6bca9465 100644 --- a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php +++ b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php @@ -5,17 +5,15 @@ namespace PhpList\Core\Domain\Messaging\Repository; use DateTime; -use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Domain\Messaging\Model\Bounce; use PhpList\Core\Domain\Messaging\Model\Interfaces\UserMessageBounceRecordInterface; -use PhpList\Core\Domain\Messaging\Model\Message; use PhpList\Core\Domain\Messaging\Model\ReadModel\UserMessageBounceReadModel; use PhpList\Core\Domain\Messaging\Model\UserMessage; use PhpList\Core\Domain\Messaging\Model\UserMessageBounce; use PhpList\Core\Domain\Messaging\Repository\Interfaces\UserMessageBounceReportReaderInterface; use PhpList\Core\Domain\Search\Client\ElasticsearchClientInterface; use PhpList\Core\Domain\Subscription\Model\Subscriber; -use PhpList\Core\Domain\Subscription\Model\Subscription; +use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; /** * Reporting/joining queries that correlate Elasticsearch-backed bounce data with related entities @@ -33,11 +31,13 @@ */ class UserMessageBounceElasticsearchHybridReader implements UserMessageBounceReportReaderInterface { - // todo: move db queries into repositories and inject them here, rather than using the entity manager directly public function __construct( private readonly ElasticsearchClientInterface $client, private readonly string $indexPrefix, - private readonly EntityManagerInterface $entityManager, + private readonly SubscriberRepository $subscriberRepository, + private readonly MessageRepository $messageRepository, + private readonly UserMessageRepository $userMessageRepository, + private readonly BounceRepository $bounceRepository, ) { } @@ -52,21 +52,7 @@ public function __construct( */ public function getListBounceTotals(int $listId): array { - $rows = $this->entityManager->createQueryBuilder() - ->select( - 'subscriber.id AS subscriberId', - 'subscriber.email AS email', - 'subscriber.confirmed AS confirmed', - 'subscriber.blacklisted AS blacklisted' - ) - ->from(Subscriber::class, 'subscriber') - ->innerJoin(Subscription::class, 'subscription', 'ON', 'subscription.subscriber = subscriber') - ->where('IDENTITY(subscription.subscriberList) = :listId') - ->setParameter('listId', $listId) - ->groupBy('subscriber.id, subscriber.email, subscriber.confirmed, subscriber.blacklisted') - ->orderBy('subscriber.id', 'ASC') - ->getQuery() - ->getArrayResult(); + $rows = $this->subscriberRepository->getBounceReportRowsByListId($listId); if ($rows === []) { return []; @@ -99,19 +85,7 @@ public function getListBounceTotals(int $listId): array /** @return array */ public function getCampaignBounceTotals(?int $ownerId = null): array { - $queryBuilder = $this->entityManager->createQueryBuilder() - ->select('m.id AS messageId', 'm.content.subject AS subject') - ->from(Message::class, 'm') - ->orderBy('m.id', 'ASC'); - - if ($ownerId !== null) { - $queryBuilder - ->andWhere('IDENTITY(m.owner) = :ownerId') - ->setParameter('ownerId', $ownerId); - } - - /** @var array $rows */ - $rows = $queryBuilder->getQuery()->getArrayResult(); + $rows = $this->messageRepository->getSubjectsForBounceReport($ownerId); if ($rows === []) { return []; @@ -162,7 +136,7 @@ public function getPaginatedWithJoinNoRelation(int $fromId, int $limit): array $records ))); - $bouncesById = $this->findBouncesByIdIndexedById($bounceIds); + $bouncesById = $this->bounceRepository->findByIdsIndexedById($bounceIds); $result = []; foreach ($records as $record) { @@ -187,24 +161,14 @@ public function getPaginatedWithJoinNoRelation(int $fromId, int $limit): array */ public function getUserMessageHistoryWithBounces(Subscriber $subscriber): array { - /** @var UserMessage[] $userMessages */ - $userMessages = $this->entityManager->createQueryBuilder() - ->select('um') - ->from(UserMessage::class, 'um') - ->where('um.user = :userId') - ->andWhere('um.status = :status') - ->setParameter('userId', $subscriber->getId()) - ->setParameter('status', 'sent') - ->orderBy('um.createdAt', 'DESC') - ->getQuery() - ->getResult(); + $userMessages = $this->userMessageRepository->findSentByUserId((int) $subscriber->getId()); if ($userMessages === []) { return []; } $docsByMessageId = $this->groupByMessageId($this->fetchDocsByUserId((int) $subscriber->getId())); - $bouncesById = $this->findBouncesByIdIndexedById($this->bounceIdsUsedIn($docsByMessageId)); + $bouncesById = $this->bounceRepository->findByIdsIndexedById($this->bounceIdsUsedIn($docsByMessageId)); $result = []; foreach ($userMessages as $userMessage) { @@ -274,25 +238,6 @@ private function bounceIdsUsedIn(array $docsByMessageId): array return array_values(array_unique($bounceIds)); } - /** - * @param int[] $bounceIds - * @return array - */ - private function findBouncesByIdIndexedById(array $bounceIds): array - { - if ($bounceIds === []) { - return []; - } - - $bouncesById = []; - /** @var Bounce $bounce */ - foreach ($this->entityManager->getRepository(Bounce::class)->findBy(['id' => $bounceIds]) as $bounce) { - $bouncesById[$bounce->getId()] = $bounce; - } - - return $bouncesById; - } - /** @param array{_source: array} $hit */ private function hydrate(array $hit): UserMessageBounceReadModel { diff --git a/src/Domain/Messaging/Repository/UserMessageRepository.php b/src/Domain/Messaging/Repository/UserMessageRepository.php index bf307737..b92fa470 100644 --- a/src/Domain/Messaging/Repository/UserMessageRepository.php +++ b/src/Domain/Messaging/Repository/UserMessageRepository.php @@ -39,6 +39,21 @@ public function countSentBetween(DateTimeInterface $start, DateTimeInterface $en ->getSingleScalarResult(); } + /** + * @return UserMessage[] + */ + public function findSentByUserId(int $userId): array + { + return $this->createQueryBuilder('um') + ->where('um.user = :userId') + ->andWhere('um.status = :status') + ->setParameter('userId', $userId) + ->setParameter('status', UserMessageStatus::Sent->value) + ->orderBy('um.createdAt', 'DESC') + ->getQuery() + ->getResult(); + } + /** * Counts how many user messages have status "sent" since the given time. */ diff --git a/src/Domain/Subscription/Repository/SubscriberRepository.php b/src/Domain/Subscription/Repository/SubscriberRepository.php index b55164c3..4ee046c2 100644 --- a/src/Domain/Subscription/Repository/SubscriberRepository.php +++ b/src/Domain/Subscription/Repository/SubscriberRepository.php @@ -13,6 +13,7 @@ use PhpList\Core\Domain\Common\Repository\Interfaces\PaginatableRepositoryInterface; use PhpList\Core\Domain\Subscription\Model\Filter\SubscriberFilter; use PhpList\Core\Domain\Subscription\Model\Subscriber; +use PhpList\Core\Domain\Subscription\Model\Subscription; /** * Repository for Subscriber models. @@ -272,6 +273,29 @@ public function findSubscriberWithSubscriptions(int $id): ?Subscriber ->getOneOrNullResult(); } + /** + * Subscribers of a list with the data needed for bounce reports, without any bounce counts. + * + * @return array + */ + public function getBounceReportRowsByListId(int $listId): array + { + return $this->createQueryBuilder('subscriber') + ->select( + 'subscriber.id AS subscriberId', + 'subscriber.email AS email', + 'subscriber.confirmed AS confirmed', + 'subscriber.blacklisted AS blacklisted' + ) + ->innerJoin(Subscription::class, 'subscription', 'ON', 'subscription.subscriber = subscriber') + ->where('IDENTITY(subscription.subscriberList) = :listId') + ->setParameter('listId', $listId) + ->groupBy('subscriber.id, subscriber.email, subscriber.confirmed, subscriber.blacklisted') + ->orderBy('subscriber.id', 'ASC') + ->getQuery() + ->getArrayResult(); + } + public function isEmailBlacklisted(string $email): bool { $queryBuilder = $this->getEntityManager()->createQueryBuilder(); diff --git a/tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php b/tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php index 4459a4da..0a28f547 100644 --- a/tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php +++ b/tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php @@ -43,7 +43,14 @@ protected function setUp(): void $this->loadSchema(); $this->client = $this->createMock(ElasticsearchClientInterface::class); - $this->reader = new UserMessageBounceElasticsearchHybridReader($this->client, 'phplist_', $this->entityManager); + $this->reader = new UserMessageBounceElasticsearchHybridReader( + $this->client, + 'phplist_', + $this->entityManager->getRepository(Subscriber::class), + $this->entityManager->getRepository(Message::class), + $this->entityManager->getRepository(UserMessage::class), + $this->entityManager->getRepository(Bounce::class), + ); } protected function tearDown(): void From 3914068fa2d5ef437507c72628abc9d0df8a774f Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 26 Oct 2026 14:16:46 +0400 Subject: [PATCH 06/30] Refactor comments in CampaignProcessorMessageHandler and MessageForwardService to clarify logic and remove TODOs --- .../MessageHandler/CampaignProcessorMessageHandler.php | 5 ++++- src/Domain/Messaging/Service/MessageForwardService.php | 1 - 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php b/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php index af7d08d2..c3b8ad07 100644 --- a/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php +++ b/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php @@ -48,7 +48,10 @@ public function __construct( public function __invoke(CampaignProcessorMessage|SyncCampaignProcessorMessage $data): void { - // todo: recheck this stuckCampaignThresholdSeconds logic + // Note: a running campaign only proves it is alive by updating `modified` when it sends. Keep + // the threshold larger than the longest expected gap without a send (throttle sleeps, slow + // precache, large subscriber queries), otherwise a second dispatch can reclaim a live campaign + // and two workers will process it at once. 0 disables reclaiming. $campaign = $this->messageRepository->tryClaimForProcessing( $data->getMessageId(), $this->stuckCampaignThresholdSeconds diff --git a/src/Domain/Messaging/Service/MessageForwardService.php b/src/Domain/Messaging/Service/MessageForwardService.php index b93f5d76..c27af684 100644 --- a/src/Domain/Messaging/Service/MessageForwardService.php +++ b/src/Domain/Messaging/Service/MessageForwardService.php @@ -78,7 +78,6 @@ public function forward(MessageForwardDto $messageForwardDto, Message $campaign) | EmailBlacklistedException | InvalidRecipientOrSubjectException $e ) { - // todo: check if need to catch MessageCacheMissingException $forwardingRecipientResult = $this->handleFailure( campaign: $campaign, forwardingSubscriber: $forwardingSubscriber, From c09147b7d2d461fddc1acd4067f92588807ab325 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 26 Oct 2026 16:11:21 +0400 Subject: [PATCH 07/30] Validate temp file directory in FileHelper and unlink if it doesn't match --- src/Domain/Common/FileHelper.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Domain/Common/FileHelper.php b/src/Domain/Common/FileHelper.php index e1f4da01..122c804a 100644 --- a/src/Domain/Common/FileHelper.php +++ b/src/Domain/Common/FileHelper.php @@ -49,6 +49,12 @@ public function writeFileToDirectory(string $directory, string $originalFilename return null; } + if (dirname((string) realpath($tempFile)) !== realpath($directory)) { + unlink($tempFile); + + return null; + } + $fullPath = $directory . '/' . basename($tempFile); if ($ext !== '') { $fullPath .= '.' . $ext; From 93a4548a4debb734b5f4d095578a51b9e2a14980 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Tue, 27 Oct 2026 15:29:46 +0400 Subject: [PATCH 08/30] ref: MessageDataLoader and MessageRepository for improved data handling and remove TODO comments --- .../Repository/MessageRepository.php | 57 +++++++++++++------ .../Messaging/Service/MessageDataLoader.php | 22 +++---- .../Repository/MessageRepositoryTest.php | 21 +++++++ .../Service/MessageDataLoaderTest.php | 32 +++++++++++ 4 files changed, 105 insertions(+), 27 deletions(-) diff --git a/src/Domain/Messaging/Repository/MessageRepository.php b/src/Domain/Messaging/Repository/MessageRepository.php index 1d95eb41..84f4da7d 100644 --- a/src/Domain/Messaging/Repository/MessageRepository.php +++ b/src/Domain/Messaging/Repository/MessageRepository.php @@ -7,7 +7,7 @@ use DateTime; use DateTimeImmutable; use DateTimeInterface; -use Doctrine\ORM\AbstractQuery; +use Doctrine\DBAL\Exception; use PhpList\Core\Domain\Common\Model\Filter\FilterRequestInterface; use PhpList\Core\Domain\Common\Model\PaginatedResult; use PhpList\Core\Domain\Common\Repository\AbstractRepository; @@ -175,7 +175,11 @@ public function findByIdAndStatus(int $id, Message\MessageStatus $status): ?Mess public function tryClaimForProcessing(int $id, int $staleAfterSeconds = 0): ?Message { $connection = $this->getEntityManager()->getConnection(); - $table = $connection->quoteIdentifier($this->getClassMetadata()->getTableName()); + $metadata = $this->getClassMetadata(); + $table = $connection->quoteIdentifier($metadata->getTableName()); + $idColumn = $metadata->getColumnName('id'); + $statusColumn = $metadata->getColumnName('metadata.status'); + $modifiedColumn = $metadata->getColumnName('updatedAt'); $now = new DateTime(); $params = [ @@ -185,9 +189,13 @@ public function tryClaimForProcessing(int $id, int $staleAfterSeconds = 0): ?Mes 'from' => Message\MessageStatus::Submitted->value, ]; - $claimCondition = 'status = :from'; + $claimCondition = sprintf('%s = :from', $statusColumn); if ($staleAfterSeconds > 0) { - $claimCondition = '(status = :from OR (status IN (:prepared, :inProcess) AND modified < :staleBefore))'; + $claimCondition = sprintf( + '(%1$s = :from OR (%1$s IN (:prepared, :inProcess) AND %2$s < :staleBefore))', + $statusColumn, + $modifiedColumn + ); $params['prepared'] = Message\MessageStatus::Prepared->value; $params['inProcess'] = Message\MessageStatus::InProcess->value; $params['staleBefore'] = (clone $now) @@ -195,7 +203,14 @@ public function tryClaimForProcessing(int $id, int $staleAfterSeconds = 0): ?Mes ->format('Y-m-d H:i:s'); } - $sql = sprintf('UPDATE %s SET status = :to, modified = :now WHERE id = :id AND %s', $table, $claimCondition); + $sql = sprintf( + 'UPDATE %s SET %s = :to, %s = :now WHERE %s = :id AND %s', + $table, + $statusColumn, + $modifiedColumn, + $idColumn, + $claimCondition + ); $affected = $connection->executeStatement($sql, $params); @@ -257,21 +272,29 @@ public function incrementSentCounts(int $messageId, OutputFormat $sentAs): void ->execute(); } + /** + * Non-empty columns of the message row keyed by their database column names (subject, fromfield, + * message, status, ...), like the legacy `select * from message`. Entity hydration is not used on + * purpose: it would key embedded values as `content.subject` etc. + * + * @return array + * @throws Exception + */ public function getNonEmptyFields(int $id): array { - $message = $this->createQueryBuilder('m') - ->where('m.id = :id') - ->setParameter('id', $id) - ->getQuery() - ->getOneOrNullResult(AbstractQuery::HYDRATE_ARRAY) ?? []; - - foreach ($message as $key => $value) { - if ($value === null || $value === '') { - unset($message[$key]); - } - } + $connection = $this->getEntityManager()->getConnection(); + $metadata = $this->getClassMetadata(); + + $message = $connection->fetchAssociative( + sprintf( + 'SELECT * FROM %s WHERE %s = :id', + $connection->quoteIdentifier($metadata->getTableName()), + $metadata->getColumnName('id') + ), + ['id' => $id] + ); - return $message; + return array_filter($message ?: [], static fn (mixed $value): bool => $value !== null && $value !== ''); } /** diff --git a/src/Domain/Messaging/Service/MessageDataLoader.php b/src/Domain/Messaging/Service/MessageDataLoader.php index 161d302d..24a36e26 100644 --- a/src/Domain/Messaging/Service/MessageDataLoader.php +++ b/src/Domain/Messaging/Service/MessageDataLoader.php @@ -11,7 +11,6 @@ use PhpList\Core\Domain\Messaging\Repository\MessageRepository; use Psr\Log\LoggerInterface; -// todo: check types class MessageDataLoader { private const AS_FORMAT_FIELDS = ['astext', 'ashtml', 'astextandhtml', 'aspdf', 'astextandpdf']; @@ -39,7 +38,6 @@ public function __invoke(Message $message): array $this->normaliseScheduleFields($messageData); $this->populateTargetLists($messageData, $message); $this->ensureSendUrlAndMethod($messageData); - // todo: check how should $message->getOptions()->getFromField() be used here $this->normaliseFromField($messageData, $defaultFrom); $this->cleanupSelections($messageData); $this->deriveCampaignTitle($messageData); @@ -72,12 +70,13 @@ private function buildDefaultMessageData(): array 'textmessage' => '', 'rsstemplate' => '', 'embargo' => $this->getDateArray(), - 'repeatinterval' => '0', + 'repeatinterval' => 0, 'repeatuntil' => $this->getDateArray(), - 'requeueinterval' => '0', + 'requeueinterval' => 0, 'requeueuntil' => $this->getDateArray(), 'finishsending' => $this->getDateArray($finishSending), 'fromfield' => '', + 'tofield' => '', 'subject' => '', 'forwardsubject' => '', 'footer' => $this->configProvider->getValue(ConfigOption::MessageFooter), @@ -95,9 +94,8 @@ private function buildDefaultMessageData(): array value: $this->configProvider->getValue(ConfigOption::AlwaysAddGoogleTracking), filter: FILTER_VALIDATE_BOOL ), - // todo: check where this is set 'excludelist' => [], - 'sentastest' => '0', + 'sentastest' => 0, ]; } @@ -121,19 +119,23 @@ private function mergeStoredMessageData(array &$messageData, Message $message): { $storedMessageData = $this->messageDataRepository->getForMessage($message->getId()); foreach ($storedMessageData as $storedMessageDatum) { - if (str_starts_with($storedMessageDatum->getData(), 'SER:')) { - $unserialized = unserialize(substr($storedMessageDatum->getData(), 4), ['allowed_classes' => false]); + $rawData = $storedMessageDatum->getData() ?? ''; + if (str_starts_with($rawData, 'SER:')) { + $unserialized = unserialize(substr($rawData, 4), ['allowed_classes' => false]); if (!is_array($unserialized)) { $this->logger->warning('Invalid serialized data for message ID: ' . $message->getId()); continue; } array_walk_recursive($unserialized, function (&$val) { - $val = stripslashes($val); + // stored values can be ints/bools (e.g. ids stored through the API), only strings have slashes + if (is_string($val)) { + $val = stripslashes($val); + } }); $data = $unserialized; } else { - $data = stripslashes($storedMessageDatum->getData()); + $data = stripslashes($rawData); } if (!in_array($storedMessageDatum->getName(), self::AS_FORMAT_FIELDS)) { //# don't overwrite counters in the message table from the data table diff --git a/tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php b/tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php index a384457e..a496f42c 100644 --- a/tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php +++ b/tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php @@ -330,4 +330,25 @@ public function testGetFilteredAfterIdSortsDescendingAndCursorsBackward(): void self::assertCount(1, $secondPage->getItems()); self::assertSame($first->getId(), $secondPage->getItems()[0]->getId()); } + + public function testGetNonEmptyFieldsReturnsLegacyColumnNames(): void + { + $message = $this->persistMessage(Message\MessageStatus::Submitted, 'Ready to send'); + $this->entityManager->flush(); + $id = $message->getId(); + $this->entityManager->clear(); + + $fields = $this->messageRepository->getNonEmptyFields($id); + + self::assertSame('Ready to send', $fields['subject']); + self::assertSame('submitted', $fields['status']); + self::assertSame('text', $fields['sendformat']); + self::assertArrayNotHasKey('content.subject', $fields); + self::assertArrayNotHasKey('fromfield', $fields, 'empty columns are dropped'); + } + + public function testGetNonEmptyFieldsReturnsEmptyArrayForUnknownMessage(): void + { + self::assertSame([], $this->messageRepository->getNonEmptyFields(999999)); + } } diff --git a/tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php b/tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php index b3d945ae..f10a83db 100644 --- a/tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php +++ b/tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php @@ -168,6 +168,38 @@ public function testParsesFromFieldIntoNameAndEmail(string $fromField, string $n $this->assertSame($email, $result['fromemail']); } + public function testStoredSerializedDataWithNonStringValuesAndNullDataDoNotBreakLoading(): void + { + $this->config->method('getValue')->willReturnMap([ + [ConfigOption::MessageFromAddress, 'from@example.com'], + ]); + $this->messageRepository->method('getNonEmptyFields')->willReturn([]); + + $excludeList = (new MessageData()) + ->setId(10) + ->setName('excludelist') + ->setData('SER:' . serialize([3, '4', true])); + $nullData = (new MessageData())->setId(10)->setName('footer')->setData(null); + $this->messageDataRepository->method('getForMessage')->willReturn([$excludeList, $nullData]); + + $message = $this->createMock(Message::class); + $message->method('getId')->willReturn(10); + $message->method('getListMessages')->willReturn(new ArrayCollection()); + + $loader = new MessageDataLoader( + configProvider: $this->config, + messageDataRepository: $this->messageDataRepository, + messageRepository: $this->messageRepository, + logger: $this->createMock(LoggerInterface::class), + defaultMessageAge: 3600 + ); + + $result = ($loader)($message); + + $this->assertSame([3, '4', true], $result['excludelist']); + $this->assertSame('', $result['footer']); + } + public static function fromFieldProvider(): array { return [ From deff3319e06bc172768c9dcf0ca396bb038e8420 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Tue, 27 Oct 2026 15:30:00 +0400 Subject: [PATCH 09/30] feat: link tracking functionality with token generation and click tracking services --- config/services/parameters.yml | 2 + config/services/repositories.yml | 8 + config/services/services.yml | 9 + .../Analytics/Model/Dto/LinkTrackToken.php | 30 ++ .../Repository/LinkTrackForwardRepository.php | 64 +++ .../Repository/LinkTrackMlRepository.php | 98 +++++ .../LinkTrackUmlClickRepository.php | 65 ++++ .../Analytics/Service/LinkClickService.php | 104 +++++ .../Analytics/Service/LinkTrackService.php | 200 +++++++--- .../Service/LinkTrackTokenService.php | 85 ++++ .../Service/ForwardContentService.php | 12 +- .../Service/MessageProcessingPreparator.php | 46 +-- .../LinkTrackClickRepositoriesTest.php | 130 +++++++ .../Service/LinkClickServiceTest.php | 201 ++++++++++ .../Service/LinkTrackServiceTest.php | 368 +++++++++--------- .../Service/LinkTrackTokenServiceTest.php | 95 +++++ .../Service/ForwardContentServiceTest.php | 20 +- .../MessageProcessingPreparatorTest.php | 78 +--- 18 files changed, 1256 insertions(+), 359 deletions(-) create mode 100644 src/Domain/Analytics/Model/Dto/LinkTrackToken.php create mode 100644 src/Domain/Analytics/Service/LinkClickService.php create mode 100644 src/Domain/Analytics/Service/LinkTrackTokenService.php create mode 100644 tests/Integration/Domain/Analytics/Repository/LinkTrackClickRepositoriesTest.php create mode 100644 tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php create mode 100644 tests/Unit/Domain/Analytics/Service/LinkTrackTokenServiceTest.php diff --git a/config/services/parameters.yml b/config/services/parameters.yml index 18aa6ccf..9a9b4aea 100644 --- a/config/services/parameters.yml +++ b/config/services/parameters.yml @@ -14,3 +14,5 @@ parameters: notify_end_default: 'end@example.com' always_add_google_tracking: true click_track: true + # sign click-tracking links with the application secret, see LinkTrackTokenService + click_track_sign_with_hmac: false diff --git a/config/services/repositories.yml b/config/services/repositories.yml index 95952280..739b5254 100644 --- a/config/services/repositories.yml +++ b/config/services/repositories.yml @@ -11,6 +11,14 @@ services: parent: PhpList\Core\Domain\Common\Repository\AbstractRepository arguments: - PhpList\Core\Domain\Analytics\Model\LinkTrackUmlClick + PhpList\Core\Domain\Analytics\Repository\LinkTrackForwardRepository: + parent: PhpList\Core\Domain\Common\Repository\AbstractRepository + arguments: + - PhpList\Core\Domain\Analytics\Model\LinkTrackForward + PhpList\Core\Domain\Analytics\Repository\LinkTrackMlRepository: + parent: PhpList\Core\Domain\Common\Repository\AbstractRepository + arguments: + - PhpList\Core\Domain\Analytics\Model\LinkTrackMl PhpList\Core\Domain\Configuration\Repository\ConfigRepository: diff --git a/config/services/services.yml b/config/services/services.yml index ac841afa..ae06bc01 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -61,6 +61,15 @@ services: autoconfigure: true public: true + PhpList\Core\Domain\Analytics\Service\LinkTrackTokenService: + autowire: true + autoconfigure: true + + PhpList\Core\Domain\Analytics\Service\LinkClickService: + autowire: true + autoconfigure: true + public: true + PhpList\Core\Domain\Analytics\Service\AnalyticsService: autowire: true autoconfigure: true diff --git a/src/Domain/Analytics/Model/Dto/LinkTrackToken.php b/src/Domain/Analytics/Model/Dto/LinkTrackToken.php new file mode 100644 index 00000000..4b6746a6 --- /dev/null +++ b/src/Domain/Analytics/Model/Dto/LinkTrackToken.php @@ -0,0 +1,30 @@ +type === self::TYPE_HTML; + } + + public static function isValidType(string $type): bool + { + return in_array($type, [self::TYPE_HTML, self::TYPE_TEXT], true); + } +} diff --git a/src/Domain/Analytics/Repository/LinkTrackForwardRepository.php b/src/Domain/Analytics/Repository/LinkTrackForwardRepository.php index 38de3f74..85ec05eb 100644 --- a/src/Domain/Analytics/Repository/LinkTrackForwardRepository.php +++ b/src/Domain/Analytics/Repository/LinkTrackForwardRepository.php @@ -4,6 +4,9 @@ namespace PhpList\Core\Domain\Analytics\Repository; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; +use Doctrine\DBAL\Types\Types; +use PhpList\Core\Domain\Analytics\Model\LinkTrackForward; use PhpList\Core\Domain\Common\Repository\AbstractRepository; use PhpList\Core\Domain\Common\Repository\CursorPaginationTrait; use PhpList\Core\Domain\Common\Repository\Interfaces\PaginatableRepositoryInterface; @@ -11,4 +14,65 @@ class LinkTrackForwardRepository extends AbstractRepository implements PaginatableRepositoryInterface { use CursorPaginationTrait; + + public function findOneByUuid(string $uuid): ?LinkTrackForward + { + return $this->findOneBy(['uuid' => $uuid]); + } + + public function findOneByUrlHash(string $urlHash): ?LinkTrackForward + { + return $this->findOneBy(['urlHash' => $urlHash]); + } + + /** + * Returns the forward for a (cleaned) URL, creating it when it does not exist yet. Creation uses a plain + * insert, so two workers racing on the same URL end up with the one row enforced by the urlhash unique key + * instead of a closed entity manager - this is an exception of the usual workflow of creating entities in manager + * classes. + */ + public function findOrCreateByUrl(string $url, bool $personalise): LinkTrackForward + { + $urlHash = md5($url); + + $forward = $this->findOneByUrlHash($urlHash); + if ($forward === null) { + // false when a concurrent worker created it first, the select then finds that row + $this->insert($url, $urlHash, $personalise); + $forward = $this->findOneByUrlHash($urlHash); + } + + if (empty($forward->getUuid())) { + $forward->setUuid(self::generateUuid()); + $this->getEntityManager()->flush(); + } + + return $forward; + } + + private function insert(string $url, string $urlHash, bool $personalise): bool + { + $metadata = $this->getClassMetadata(); + try { + $this->getEntityManager()->getConnection()->insert( + $metadata->getTableName(), + [ + $metadata->getColumnName('url') => $url, + $metadata->getColumnName('urlHash') => $urlHash, + $metadata->getColumnName('personalise') => $personalise, + $metadata->getColumnName('uuid') => self::generateUuid(), + ], + [$metadata->getColumnName('personalise') => Types::BOOLEAN], + ); + } catch (UniqueConstraintViolationException) { + return false; + } + + return true; + } + + private static function generateUuid(): string + { + return bin2hex(random_bytes(18)); + } } diff --git a/src/Domain/Analytics/Repository/LinkTrackMlRepository.php b/src/Domain/Analytics/Repository/LinkTrackMlRepository.php index 6914105b..3e1292b4 100644 --- a/src/Domain/Analytics/Repository/LinkTrackMlRepository.php +++ b/src/Domain/Analytics/Repository/LinkTrackMlRepository.php @@ -4,8 +4,106 @@ namespace PhpList\Core\Domain\Analytics\Repository; +use DateTimeInterface; +use Doctrine\DBAL\Exception; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use PhpList\Core\Domain\Common\Repository\AbstractRepository; +/** + * Per campaign and link aggregates. Counters are changed with atomic statements so that concurrent sending + * workers and clicking subscribers cannot lose updates. Column names come from the entity mapping. + */ class LinkTrackMlRepository extends AbstractRepository { + /** Counts one more email of the campaign that contains the link. */ + public function incrementTotal(int $messageId, int $forwardId): void + { + $total = $this->column('total'); + + $this->upsert( + updateSql: sprintf('%1$s = COALESCE(%1$s, 0) + 1', $total), + updateParams: [], + insertColumns: [$total => 1], + messageId: $messageId, + forwardId: $forwardId, + ); + } + + public function recordClick(int $messageId, int $forwardId, bool $isHtml, DateTimeInterface $now): void + { + $timestamp = $now->format('Y-m-d H:i:s'); + [$clicked, $firstClick, $latestClick, $htmlClicked, $textClicked] = array_map( + $this->column(...), + ['clicked', 'firstClick', 'latestClick', 'htmlClicked', 'textClicked'] + ); + + $this->upsert( + updateSql: sprintf( + '%1$s = COALESCE(%1$s, 0) + 1, %2$s = COALESCE(%2$s, :now), %3$s = :now,' + . ' %4$s = COALESCE(%4$s, 0) + :html, %5$s = COALESCE(%5$s, 0) + :text', + $clicked, + $firstClick, + $latestClick, + $htmlClicked, + $textClicked + ), + updateParams: ['now' => $timestamp, 'html' => (int) $isHtml, 'text' => (int) !$isHtml], + insertColumns: [ + $clicked => 1, + $firstClick => $timestamp, + $latestClick => $timestamp, + $htmlClicked => (int) $isHtml, + $textClicked => (int) !$isHtml, + ], + messageId: $messageId, + forwardId: $forwardId, + ); + } + + private function column(string $field): string + { + return $this->getClassMetadata()->getColumnName($field); + } + + /** + * @param string $updateSql the assignments of the UPDATE, without SET + * @param array $updateParams + * @param array $insertColumns column => value + * @throws Exception + */ + private function upsert( + string $updateSql, + array $updateParams, + array $insertColumns, + int $messageId, + int $forwardId, + ): void { + $connection = $this->getEntityManager()->getConnection(); + $tableName = $this->getClassMetadata()->getTableName(); + $messageColumn = $this->column('messageId'); + $forwardColumn = $this->column('forwardId'); + + $sql = sprintf( + 'UPDATE %s SET %s WHERE %s = :messageId AND %s = :forwardId', + $connection->quoteIdentifier($tableName), + $updateSql, + $messageColumn, + $forwardColumn + ); + $params = $updateParams + ['messageId' => $messageId, 'forwardId' => $forwardId]; + + if ($connection->executeStatement($sql, $params) > 0) { + return; + } + + try { + $connection->insert( + $tableName, + [$messageColumn => $messageId, $forwardColumn => $forwardId] + $insertColumns + ); + } catch (UniqueConstraintViolationException) { + // inserted by a concurrent request in the meantime + $connection->executeStatement($sql, $params); + } + } } diff --git a/src/Domain/Analytics/Repository/LinkTrackUmlClickRepository.php b/src/Domain/Analytics/Repository/LinkTrackUmlClickRepository.php index 5662d672..2f3254c9 100644 --- a/src/Domain/Analytics/Repository/LinkTrackUmlClickRepository.php +++ b/src/Domain/Analytics/Repository/LinkTrackUmlClickRepository.php @@ -4,6 +4,8 @@ namespace PhpList\Core\Domain\Analytics\Repository; +use DateTimeInterface; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use PhpList\Core\Domain\Common\Repository\AbstractRepository; use PhpList\Core\Domain\Common\Repository\CursorPaginationTrait; use PhpList\Core\Domain\Common\Repository\Interfaces\PaginatableRepositoryInterface; @@ -11,4 +13,67 @@ class LinkTrackUmlClickRepository extends AbstractRepository implements PaginatableRepositoryInterface { use CursorPaginationTrait; + + /** + * Counts a click of a subscriber on a link of a campaign, creating the row on the first click. Column names + * come from the entity mapping. + */ + public function recordClick( + int $messageId, + int $userId, + int $forwardId, + bool $isHtml, + DateTimeInterface $now, + ): void { + $connection = $this->getEntityManager()->getConnection(); + $metadata = $this->getClassMetadata(); + $timestamp = $now->format('Y-m-d H:i:s'); + [$message, $user, $forward, $clicked, $firstClick, $latestClick, $htmlClicked, $textClicked] = array_map( + $metadata->getColumnName(...), + ['messageId', 'userId', 'forwardId', 'clicked', 'firstClick', 'latestClick', 'htmlClicked', 'textClicked'] + ); + + $sql = sprintf( + 'UPDATE %4$s SET %5$s = COALESCE(%5$s, 0) + 1, %6$s = COALESCE(%6$s, :now), %7$s = :now,' + . ' %8$s = COALESCE(%8$s, 0) + :html, %9$s = COALESCE(%9$s, 0) + :text' + . ' WHERE %1$s = :messageId AND %2$s = :userId AND %3$s = :forwardId', + $message, + $user, + $forward, + $connection->quoteIdentifier($metadata->getTableName()), + $clicked, + $firstClick, + $latestClick, + $htmlClicked, + $textClicked + ); + $params = [ + 'now' => $timestamp, + 'html' => (int) $isHtml, + 'text' => (int) !$isHtml, + 'messageId' => $messageId, + 'userId' => $userId, + 'forwardId' => $forwardId, + ]; + + if ($connection->executeStatement($sql, $params) > 0) { + return; + } + + try { + $connection->insert($metadata->getTableName(), [ + $message => $messageId, + $user => $userId, + $forward => $forwardId, + $firstClick => $timestamp, + $latestClick => $timestamp, + $clicked => 1, + $htmlClicked => (int) $isHtml, + $textClicked => (int) !$isHtml, + ]); + } catch (UniqueConstraintViolationException) { + // inserted by a concurrent request in the meantime + $connection->executeStatement($sql, $params); + } + } } diff --git a/src/Domain/Analytics/Service/LinkClickService.php b/src/Domain/Analytics/Service/LinkClickService.php new file mode 100644 index 00000000..75ad3d9c --- /dev/null +++ b/src/Domain/Analytics/Service/LinkClickService.php @@ -0,0 +1,104 @@ + $metadata HTTP_USER_AGENT, HTTP_REFERER and client_ip of the request + * @return string|null the url to redirect to, or null when the token is invalid or the link is unknown + */ + public function trackClick(string $tid, ?string $signature, array $metadata): ?string + { + $token = $this->tokenService->parse($tid, $signature); + if ($token === null) { + return null; + } + + $forward = $this->forwardRepository->findOneByUuid($token->forwardUuid); + $message = $this->messageRepository->findOneBy(['uuid' => $token->messageUuid]); + $subscriber = $this->subscriberRepository->findOneBy(['uuid' => $token->subscriberUuid]); + if (!$forward instanceof LinkTrackForward + || !$message instanceof Message + || !$subscriber instanceof Subscriber + ) { + return null; + } + + $userMessage = $this->userMessageRepository->findByUserAndMessage($subscriber, $message); + $url = $this->resolveUrl($forward, $subscriber, $userMessage); + if ($url === null) { + return null; + } + + $now = new DateTimeImmutable(); + $messageId = (int) $message->getId(); + $forwardId = (int) $forward->getId(); + $this->mlRepository->recordClick($messageId, $forwardId, $token->isHtml(), $now); + + // a click on a link also proves the campaign was opened, but only the first one counts as a view + if ($userMessage !== null && !$userMessage->isViewed()) { + $this->userMessageService->trackUserMessageView($subscriber->getUniqueId(), $messageId, $metadata); + } + + $this->umlClickRepository->recordClick( + $messageId, + (int) $subscriber->getId(), + $forwardId, + $token->isHtml(), + $now + ); + + return $url; + } + + private function resolveUrl(LinkTrackForward $forward, Subscriber $subscriber, ?UserMessage $userMessage): ?string + { + $url = $forward->getUrl(); + if ($url === null) { + return null; + } + + if ($forward->isPersonalise()) { + // personalised links are only valid for subscribers the campaign was actually sent to + if ($userMessage === null) { + return null; + } + $url .= (str_contains($url, '?') ? '&' : '?') . 'uid=' . $subscriber->getUniqueId(); + } + + return preg_match('~^(https?|ftp)://~i', $url) ? $url : null; + } +} diff --git a/src/Domain/Analytics/Service/LinkTrackService.php b/src/Domain/Analytics/Service/LinkTrackService.php index dc478e84..05a9baa6 100644 --- a/src/Domain/Analytics/Service/LinkTrackService.php +++ b/src/Domain/Analytics/Service/LinkTrackService.php @@ -4,19 +4,37 @@ namespace PhpList\Core\Domain\Analytics\Service; -use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Core\ParameterProvider; -use PhpList\Core\Domain\Analytics\Exception\MissingMessageIdException; -use PhpList\Core\Domain\Analytics\Model\LinkTrack; +use PhpList\Core\Domain\Analytics\Model\Dto\LinkTrackToken; +use PhpList\Core\Domain\Analytics\Repository\LinkTrackForwardRepository; +use PhpList\Core\Domain\Analytics\Repository\LinkTrackMlRepository; use PhpList\Core\Domain\Analytics\Repository\LinkTrackRepository; use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; - +use PhpList\Core\Domain\Messaging\Model\Message; +use PhpList\Core\Domain\Subscription\Model\Subscriber; + +/** + * Replaces the links of a campaign email with click-tracking links, like phplist3's sendemaillib.php. + * + * Each distinct URL is stored once as a LinkTrackForward. The replacement link carries a token that identifies + * the forward, the campaign and the subscriber (see LinkTrackTokenService); LinkClickService resolves it again. + */ class LinkTrackService { + private const MAX_URL_LENGTH = 255; + private const HTML_ANCHOR_PATTERN = '/(]*?[\s"\'])?)href=(["\'])(.*?)\2([^>]*>)(.*?)<\/a>/is'; + private const TEXT_URL_PATTERN = '#https?://[^\s<>},]+#i'; + private const PLACEHOLDER_PATTERN = '/\[[A-Z][A-Z0-9_]*(?::[^\]]*)?\]/'; + + /** @var array forwards by url hash, kept for the worker's lifetime */ + private array $forwards = []; + public function __construct( private readonly LinkTrackRepository $linkTrackRepository, + private readonly LinkTrackForwardRepository $linkTrackForwardRepository, + private readonly LinkTrackMlRepository $linkTrackMlRepository, + private readonly LinkTrackTokenService $tokenService, private readonly ParameterProvider $paramProvider, - private readonly EntityManagerInterface $entityManager, ) { } @@ -26,71 +44,165 @@ public function getUrlById(int $id): ?string return $linkTrack?->getUrl(); } - public function isExtractAndSaveLinksApplicable(): bool + public function isClickTrackingEnabled(): bool { return (bool)$this->paramProvider->get('click_track', false); } /** - * Extract links from message content and save them to the LinkTrackRepository + * Returns a copy of the content in which the links of the HTML parts (content, HTML footer) and of the text + * parts (text content, text footer) point to the click-tracking endpoint for this subscriber. * - * @return LinkTrack[] The saved LinkTrack entities - * @throws MissingMessageIdException + * Links are left alone when they are not http(s)/ftp, are longer than the forward url column, still contain + * placeholders (the redirect could not expand them), or are shown as text that itself contains a URL (the + * rewrite would look like phishing). */ - public function extractAndSaveLinks(MessagePrecacheDto $content, int $userId, ?int $messageId = null): array - { - // todo: in case of forwarded message, we need to use 'forwarded' instead of user id - if (!$this->isExtractAndSaveLinksApplicable()) { - return []; + public function trackLinks( + MessagePrecacheDto $content, + Message $campaign, + Subscriber $subscriber + ): MessagePrecacheDto { + $messageUuid = (string) $campaign->getUuid(); + $subscriberUuid = $subscriber->getUuid(); + if (!$this->isClickTrackingEnabled() || $messageUuid === '' || $subscriberUuid === '') { + return $content; + } + + $tracked = clone $content; + $forwardIds = []; + $trackUrl = function (string $url, string $type) use ($messageUuid, $subscriberUuid, &$forwardIds): ?string { + return $this->createTrackingUrl($url, $type, $messageUuid, $subscriberUuid, $forwardIds); + }; + + $tracked->content = $this->rewriteHtml($tracked->content, $trackUrl); + $tracked->htmlFooter = $this->rewriteHtml($tracked->htmlFooter, $trackUrl); + $tracked->textContent = $this->rewriteText($tracked->textContent, $trackUrl); + if ($tracked->textFooter !== null) { + $tracked->textFooter = $this->rewriteText($tracked->textFooter, $trackUrl); } - if ($messageId === null) { - throw new MissingMessageIdException(); + foreach (array_keys($forwardIds) as $forwardId) { + $this->linkTrackMlRepository->incrementTotal((int) $campaign->getId(), $forwardId); } - $links = $this->extractLinksFromHtml($content->content); + return $tracked; + } - if ($content->htmlFooter) { - $links = array_merge($links, $this->extractLinksFromHtml($content->htmlFooter)); + /** @param callable(string, string): ?string $trackUrl */ + private function rewriteHtml(string $html, callable $trackUrl): string + { + if ($html === '') { + return $html; } - $links = array_unique($links); + return (string) preg_replace_callback( + self::HTML_ANCHOR_PATTERN, + function (array $match) use ($trackUrl): string { + $linkText = strip_tags($match[5]); + if (stripos($linkText, 'http://') !== false || stripos($linkText, 'https://') !== false) { + return $match[0]; + } + + $href = html_entity_decode(trim($match[3]), ENT_QUOTES | ENT_HTML5); + $url = $trackUrl($href, LinkTrackToken::TYPE_HTML); + if ($url === null) { + return $match[0]; + } + + return $match[1] . 'href=' . $match[2] . htmlspecialchars($url, ENT_QUOTES) . $match[2] + . $match[4] . $match[5] . ''; + }, + $html + ); + } - $savedLinks = []; - foreach ($links as $url) { - $existingLinkTrack = $this->linkTrackRepository->findByUrlUserIdAndMessageId($url, $userId, $messageId); - if ($existingLinkTrack !== null) { - $savedLinks[] = $existingLinkTrack; - continue; - } - $linkTrack = new LinkTrack(); - $linkTrack->setMessageId($messageId); - $linkTrack->setUserId($userId); - $linkTrack->setUrl($url); + /** @param callable(string, string): ?string $trackUrl */ + private function rewriteText(string $text, callable $trackUrl): string + { + if ($text === '') { + return $text; + } - $this->linkTrackRepository->persist($linkTrack); - $savedLinks[] = $linkTrack; + return (string) preg_replace_callback( + self::TEXT_URL_PATTERN, + function (array $match) use ($trackUrl): string { + $trailing = str_ends_with($match[0], '.') ? '.' : ''; + $link = substr($match[0], 0, strlen($match[0]) - strlen($trailing)); + $url = $trackUrl($link, LinkTrackToken::TYPE_TEXT); + + return $url === null ? $match[0] : $url . $trailing; + }, + $text + ); + } + + /** @param array $forwardIds collects the ids of the forwards used in the email */ + private function createTrackingUrl( + string $url, + string $type, + string $messageUuid, + string $subscriberUuid, + array &$forwardIds, + ): ?string { + $personalise = false; + $cleanUrl = $this->cleanUrl($url, $personalise); + + if (!preg_match('~^(https?|ftp)://~i', $cleanUrl) + || strlen($cleanUrl) > self::MAX_URL_LENGTH + || preg_match(self::PLACEHOLDER_PATTERN, $cleanUrl) + || str_contains($cleanUrl, LinkTrackTokenService::ENDPOINT_PATH . '?tid=') + ) { + return null; } - $this->entityManager->flush(); + $forward = $this->getForward($cleanUrl, $personalise); + $forwardIds[$forward['id']] = true; + + return $this->tokenService->createUrl( + new LinkTrackToken($type, $forward['uuid'], $messageUuid, $subscriberUuid) + ); + } + + /** @return array{id: int, uuid: string} */ + private function getForward(string $cleanUrl, bool $personalise): array + { + $urlHash = md5($cleanUrl); + if (!isset($this->forwards[$urlHash])) { + $forward = $this->linkTrackForwardRepository->findOrCreateByUrl($cleanUrl, $personalise); + $this->forwards[$urlHash] = ['id' => (int) $forward->getId(), 'uuid' => (string) $forward->getUuid()]; + } - return $savedLinks; + return $this->forwards[$urlHash]; } /** - * Extract links from HTML content - * - * @return string[] The extracted links + * Removes the personal `uid` and `PHPSESSID` query parameters, which must not end up in the stored url. + * $personalise is set when a `uid` was present: the redirect then adds the subscriber's uid again. */ - private function extractLinksFromHtml(string $html): array + private function cleanUrl(string $url, bool &$personalise): string { - $links = []; + $fragment = ''; + $fragmentPosition = strpos($url, '#'); + if ($fragmentPosition !== false) { + $fragment = substr($url, $fragmentPosition); + $url = substr($url, 0, $fragmentPosition); + } - $pattern = '/]*href=(["\'])([^"\']+)\1[^>]*>/i'; - if (preg_match_all($pattern, $html, $matches)) { - $links = $matches[2]; + $queryPosition = strpos($url, '?'); + if ($queryPosition === false) { + return $url . $fragment; + } + + $kept = []; + foreach (explode('&', substr($url, $queryPosition + 1)) as $pair) { + $name = strtolower(urldecode(explode('=', $pair, 2)[0])); + if ($name === 'uid') { + $personalise = true; + } elseif ($name !== 'phpsessid' && $pair !== '') { + $kept[] = $pair; + } } - return $links; + return substr($url, 0, $queryPosition) . ($kept === [] ? '' : '?' . implode('&', $kept)) . $fragment; } } diff --git a/src/Domain/Analytics/Service/LinkTrackTokenService.php b/src/Domain/Analytics/Service/LinkTrackTokenService.php new file mode 100644 index 00000000..371f14d5 --- /dev/null +++ b/src/Domain/Analytics/Service/LinkTrackTokenService.php @@ -0,0 +1,85 @@ +/lt?tid=`), the equivalent of + * phplist3's lt.php links. + * + * The token is the url-safe base64 of `|||`. The uuids are + * random, so a token cannot be guessed. With the `click_track_sign_with_hmac` parameter enabled the token is + * additionally signed (`hm`) with the application secret, so a request can be rejected before touching the + * database. Keep it off if the secret may be rotated, as that would invalidate the links of sent campaigns. + */ +class LinkTrackTokenService +{ + public const ENDPOINT_PATH = '/lt'; + + public function __construct( + private readonly ParameterProvider $parameterProvider, + #[Autowire('%app.rest_api_base_url%')] private readonly string $restApiBaseUrl, + #[Autowire('%app.secret%')] private readonly string $secret, + ) { + } + + public function createUrl(LinkTrackToken $token): string + { + $encoded = rtrim(strtr(base64_encode(implode('|', [ + $token->type, + $token->forwardUuid, + $token->messageUuid, + $token->subscriberUuid, + ])), '+/', '-_'), '='); + + $url = rtrim($this->restApiBaseUrl, '/') . self::ENDPOINT_PATH . '?tid=' . $encoded; + if ($this->isSigningEnabled()) { + $url .= '&hm=' . $this->sign($encoded); + } + + return $url; + } + + /** Returns null for malformed tokens and, when signing is enabled, for missing or wrong signatures. */ + public function parse(string $tid, ?string $signature = null): ?LinkTrackToken + { + if ($this->isSigningEnabled() + && ($signature === null || $signature === '' || !hash_equals($this->sign($tid), $signature)) + ) { + return null; + } + + $decoded = base64_decode(strtr($tid, '-_', '+/'), true); + if ($decoded === false) { + return null; + } + + $parts = explode('|', $decoded); + if (count($parts) !== 4 || !LinkTrackToken::isValidType($parts[0])) { + return null; + } + + foreach (array_slice($parts, 1) as $uuid) { + if (!preg_match('/^[A-Za-z0-9-]{1,64}$/', $uuid)) { + return null; + } + } + + return new LinkTrackToken($parts[0], $parts[1], $parts[2], $parts[3]); + } + + private function isSigningEnabled(): bool + { + return (bool) $this->parameterProvider->get('click_track_sign_with_hmac', false); + } + + private function sign(string $encodedToken): string + { + return hash_hmac('sha256', $encodedToken, $this->secret); + } +} diff --git a/src/Domain/Messaging/Service/ForwardContentService.php b/src/Domain/Messaging/Service/ForwardContentService.php index 2fa0e17d..48320a21 100644 --- a/src/Domain/Messaging/Service/ForwardContentService.php +++ b/src/Domain/Messaging/Service/ForwardContentService.php @@ -19,7 +19,6 @@ class ForwardContentService { public function __construct( private readonly CacheInterface $cache, - private readonly MessageProcessingPreparator $messagePreparator, private readonly ForwardEmailBuilder $forwardEmailBuilder, ) { } @@ -39,18 +38,13 @@ public function getContents( throw new MessageCacheMissingException(); } - // todo: check how should links be handled in case of forwarding - $processed = $this->messagePreparator->processMessageLinks( - campaignId: $campaign->getId(), - cachedMessageDto: $messagePrecacheDto, - subscriber: $forwardingSubscriber - ); - + // links of forwarded messages are not click tracked: the forwarding subscriber would + // be credited with the clicks of the friend return $this->forwardEmailBuilder->buildForwardEmail( messageId: $campaign->getId(), friendEmail: $friendEmail, forwardedBy: $forwardingSubscriber, - data: $processed, + data: $messagePrecacheDto, htmlPref: $forwardingSubscriber->hasHtmlEmail(), fromName: $forwardDto->getFromName(), fromEmail: $forwardDto->getFromEmail(), diff --git a/src/Domain/Messaging/Service/MessageProcessingPreparator.php b/src/Domain/Messaging/Service/MessageProcessingPreparator.php index 05f044ea..cfbcc148 100644 --- a/src/Domain/Messaging/Service/MessageProcessingPreparator.php +++ b/src/Domain/Messaging/Service/MessageProcessingPreparator.php @@ -4,7 +4,6 @@ namespace PhpList\Core\Domain\Messaging\Service; -use PhpList\Core\Domain\Analytics\Model\LinkTrack; use PhpList\Core\Domain\Analytics\Service\LinkTrackService; use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; use PhpList\Core\Domain\Messaging\Repository\MessageRepository; @@ -15,9 +14,6 @@ class MessageProcessingPreparator { - // @todo: create functionality to track - public const LINK_TRACK_ENDPOINT = '/api/v2/link-track'; - public function __construct( private readonly SubscriberRepository $subscriberRepository, private readonly MessageRepository $messageRepository, @@ -57,53 +53,23 @@ public function ensureCampaignsHaveUuid(OutputInterface $output): void } /** - * Process message content to extract URLs and replace them with link track URLs + * Replaces the links of the cached message content with click-tracking links for this subscriber. + * Returns the content unchanged when click tracking is off or the campaign is unknown. */ public function processMessageLinks( int $campaignId, MessagePrecacheDto $cachedMessageDto, Subscriber $subscriber ): MessagePrecacheDto { - if (!$this->linkTrackService->isExtractAndSaveLinksApplicable()) { + if (!$this->linkTrackService->isClickTrackingEnabled()) { return $cachedMessageDto; } - $savedLinks = $this->linkTrackService->extractAndSaveLinks( - content: $cachedMessageDto, - userId: $subscriber->getId(), - messageId: $campaignId, - ); - if (empty($savedLinks)) { + $campaign = $this->messageRepository->findById($campaignId); + if ($campaign === null) { return $cachedMessageDto; } - // todo: check if getTextMessage should replace links as well - if ($cachedMessageDto->content) { - $cachedMessageDto->content = $this->replaceLinks( - savedLinks: $savedLinks, - htmlText: $cachedMessageDto->content - ); - } - - if ($cachedMessageDto->htmlFooter) { - $cachedMessageDto->htmlFooter = $this->replaceLinks( - savedLinks: $savedLinks, - htmlText: $cachedMessageDto->htmlFooter - ); - } - - return $cachedMessageDto; - } - - private function replaceLinks(array $savedLinks, string $htmlText): string - { - /** @var LinkTrack $linkTrack */ - foreach ($savedLinks as $linkTrack) { - $originalUrl = $linkTrack->getUrl(); - $trackUrl = self::LINK_TRACK_ENDPOINT . '?id=' . $linkTrack->getId(); - $htmlText = str_replace('href="' . $originalUrl . '"', 'href="' . $trackUrl . '"', $htmlText); - } - - return $htmlText; + return $this->linkTrackService->trackLinks($cachedMessageDto, $campaign, $subscriber); } } diff --git a/tests/Integration/Domain/Analytics/Repository/LinkTrackClickRepositoriesTest.php b/tests/Integration/Domain/Analytics/Repository/LinkTrackClickRepositoriesTest.php new file mode 100644 index 00000000..c20d1a1f --- /dev/null +++ b/tests/Integration/Domain/Analytics/Repository/LinkTrackClickRepositoriesTest.php @@ -0,0 +1,130 @@ +loadSchema(); + + $this->forwardRepository = self::getContainer()->get(LinkTrackForwardRepository::class); + $this->mlRepository = self::getContainer()->get(LinkTrackMlRepository::class); + $this->umlClickRepository = self::getContainer()->get(LinkTrackUmlClickRepository::class); + } + + protected function tearDown(): void + { + $schemaTool = new SchemaTool($this->entityManager); + $schemaTool->dropDatabase(); + parent::tearDown(); + } + + public function testFindOrCreateByUrlCreatesOneForwardPerUrl(): void + { + $first = $this->forwardRepository->findOrCreateByUrl('https://example.com/a', false); + $again = $this->forwardRepository->findOrCreateByUrl('https://example.com/a', false); + $other = $this->forwardRepository->findOrCreateByUrl('https://example.com/b', true); + + self::assertSame($first->getId(), $again->getId()); + self::assertNotSame($first->getId(), $other->getId()); + self::assertSame('https://example.com/a', $first->getUrl()); + self::assertSame(md5('https://example.com/a'), $first->getUrlHash()); + self::assertFalse($first->isPersonalise()); + self::assertTrue($other->isPersonalise()); + self::assertSame(36, strlen((string) $first->getUuid())); + self::assertNotSame($first->getUuid(), $other->getUuid()); + self::assertSame($first->getId(), $this->forwardRepository->findOneByUuid($first->getUuid())->getId()); + self::assertNull($this->forwardRepository->findOneByUuid('unknown')); + } + + public function testIncrementTotalCreatesAndIncrementsTheRow(): void + { + $this->mlRepository->incrementTotal(7, 11); + $this->mlRepository->incrementTotal(7, 11); + $this->mlRepository->incrementTotal(7, 12); + + self::assertSame(2, $this->findMl(7, 11)->getTotal()); + self::assertSame(1, $this->findMl(7, 12)->getTotal()); + self::assertSame(0, $this->findMl(7, 11)->getClicked()); + } + + public function testRecordClickOnMlKeepsTheFirstClickAndSplitsHtmlAndText(): void + { + $this->mlRepository->incrementTotal(7, 11); + $this->mlRepository->recordClick(7, 11, true, new DateTimeImmutable('2026-01-01 10:00:00')); + $this->mlRepository->recordClick(7, 11, false, new DateTimeImmutable('2026-01-02 11:00:00')); + $this->mlRepository->recordClick(7, 11, true, new DateTimeImmutable('2026-01-03 12:00:00')); + + $ml = $this->findMl(7, 11); + self::assertSame(1, $ml->getTotal(), 'sending total is not touched by clicks'); + self::assertSame(3, $ml->getClicked()); + self::assertSame(2, $ml->getHtmlClicked()); + self::assertSame(1, $ml->getTextClicked()); + self::assertSame('2026-01-01 10:00:00', $ml->getFirstClick()->format('Y-m-d H:i:s')); + self::assertSame('2026-01-03 12:00:00', $ml->getLatestClick()->format('Y-m-d H:i:s')); + } + + public function testRecordClickOnMlCreatesTheRowWhenNothingWasSentYet(): void + { + $this->mlRepository->recordClick(7, 11, false, new DateTimeImmutable('2026-01-01 10:00:00')); + + $ml = $this->findMl(7, 11); + self::assertSame(1, $ml->getClicked()); + self::assertSame(1, $ml->getTextClicked()); + self::assertSame(0, $ml->getHtmlClicked()); + } + + public function testRecordClickPerSubscriberIsUniquePerCampaignSubscriberAndLink(): void + { + $this->umlClickRepository->recordClick(7, 3, 11, true, new DateTimeImmutable('2026-01-01 10:00:00')); + $this->umlClickRepository->recordClick(7, 3, 11, false, new DateTimeImmutable('2026-01-02 11:00:00')); + $this->umlClickRepository->recordClick(7, 4, 11, true, new DateTimeImmutable('2026-01-02 11:00:00')); + $this->umlClickRepository->recordClick(7, 3, 12, true, new DateTimeImmutable('2026-01-02 11:00:00')); + + $click = $this->findUml(7, 3, 11); + self::assertSame(2, $click->getClicked()); + self::assertSame(1, $click->getHtmlClicked()); + self::assertSame(1, $click->getTextClicked()); + self::assertSame('2026-01-01 10:00:00', $click->getFirstClick()->format('Y-m-d H:i:s')); + self::assertSame('2026-01-02 11:00:00', $click->getLatestClick()->format('Y-m-d H:i:s')); + self::assertSame(1, $this->findUml(7, 4, 11)->getClicked()); + self::assertSame(1, $this->findUml(7, 3, 12)->getClicked()); + self::assertCount(3, $this->umlClickRepository->findAll()); + } + + private function findMl(int $messageId, int $forwardId): LinkTrackMl + { + $this->entityManager->clear(); + + return $this->mlRepository->findOneBy(['messageId' => $messageId, 'forwardId' => $forwardId]); + } + + private function findUml(int $messageId, int $userId, int $forwardId): LinkTrackUmlClick + { + $this->entityManager->clear(); + + return $this->umlClickRepository->findOneBy( + ['messageId' => $messageId, 'userId' => $userId, 'forwardId' => $forwardId] + ); + } +} diff --git a/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php b/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php new file mode 100644 index 00000000..370aefa7 --- /dev/null +++ b/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php @@ -0,0 +1,201 @@ +forwardRepository = $this->createMock(LinkTrackForwardRepository::class); + $this->mlRepository = $this->createMock(LinkTrackMlRepository::class); + $this->umlClickRepository = $this->createMock(LinkTrackUmlClickRepository::class); + $this->messageRepository = $this->createMock(MessageRepository::class); + $this->subscriberRepository = $this->createMock(SubscriberRepository::class); + $this->userMessageRepository = $this->createMock(UserMessageRepository::class); + $this->userMessageService = $this->createMock(UserMessageService::class); + $this->tokenService = new LinkTrackTokenService(new ParameterProvider([]), 'https://example.com/api/v2', 's'); + + $this->service = new LinkClickService( + $this->tokenService, + $this->forwardRepository, + $this->mlRepository, + $this->umlClickRepository, + $this->messageRepository, + $this->subscriberRepository, + $this->userMessageRepository, + $this->userMessageService, + ); + + $this->message = $this->createMock(Message::class); + $this->message->method('getId')->willReturn(7); + $this->subscriber = $this->createMock(Subscriber::class); + $this->subscriber->method('getId')->willReturn(3); + $this->subscriber->method('getUniqueId')->willReturn('uniq-3'); + } + + private function tid(string $type = 'H'): string + { + $url = $this->tokenService->createUrl(new LinkTrackToken($type, 'fwd', 'msg', 'usr')); + parse_str((string) parse_url($url, PHP_URL_QUERY), $query); + + return $query['tid']; + } + + private function givenKnownLink(string $url = 'https://example.com/a', bool $personalise = false): void + { + $forward = $this->createMock(LinkTrackForward::class); + $forward->method('getId')->willReturn(11); + $forward->method('getUrl')->willReturn($url); + $forward->method('isPersonalise')->willReturn($personalise); + + $this->forwardRepository->method('findOneByUuid')->with('fwd')->willReturn($forward); + $this->messageRepository->method('findOneBy')->with(['uuid' => 'msg'])->willReturn($this->message); + $this->subscriberRepository->method('findOneBy')->with(['uuid' => 'usr'])->willReturn($this->subscriber); + } + + private function givenUserMessage(bool $viewed): void + { + $userMessage = $this->createMock(UserMessage::class); + $userMessage->method('isViewed')->willReturn($viewed); + $this->userMessageRepository->method('findByUserAndMessage')->willReturn($userMessage); + } + + public function testRedirectsToTheStoredUrlAndCountsTheClick(): void + { + $this->givenKnownLink(); + $this->givenUserMessage(viewed: false); + + $this->mlRepository->expects(self::once()) + ->method('recordClick') + ->with(7, 11, true, self::anything()); + $this->umlClickRepository->expects(self::once()) + ->method('recordClick') + ->with(7, 3, 11, true, self::anything()); + $this->userMessageService->expects(self::once()) + ->method('trackUserMessageView') + ->with('uniq-3', 7, ['HTTP_USER_AGENT' => 'ua']); + + self::assertSame( + 'https://example.com/a', + $this->service->trackClick($this->tid(), null, ['HTTP_USER_AGENT' => 'ua']) + ); + } + + public function testTextClicksAreCountedAsText(): void + { + $this->givenKnownLink(); + $this->givenUserMessage(viewed: true); + + $this->mlRepository->expects(self::once())->method('recordClick')->with(7, 11, false, self::anything()); + $this->umlClickRepository->expects(self::once()) + ->method('recordClick') + ->with(7, 3, 11, false, self::anything()); + + self::assertNotNull($this->service->trackClick($this->tid('T'), null, [])); + } + + public function testOnlyTheFirstClickCountsAsView(): void + { + $this->givenKnownLink(); + $this->givenUserMessage(viewed: true); + + $this->userMessageService->expects(self::never())->method('trackUserMessageView'); + + self::assertNotNull($this->service->trackClick($this->tid(), null, [])); + } + + public function testPersonalisedLinkGetsTheSubscribersUid(): void + { + $this->givenKnownLink('https://example.com/p?a=1', personalise: true); + $this->givenUserMessage(viewed: true); + + self::assertSame('https://example.com/p?a=1&uid=uniq-3', $this->service->trackClick($this->tid(), null, [])); + } + + public function testPersonalisedLinkIsRefusedForSubscribersThatNeverGotTheCampaign(): void + { + $this->givenKnownLink('https://example.com/p', personalise: true); + $this->userMessageRepository->method('findByUserAndMessage')->willReturn(null); + + $this->mlRepository->expects(self::never())->method('recordClick'); + + self::assertNull($this->service->trackClick($this->tid(), null, [])); + } + + public function testNonPersonalisedLinkWorksWithoutUserMessage(): void + { + $this->givenKnownLink(); + $this->userMessageRepository->method('findByUserAndMessage')->willReturn(null); + + $this->userMessageService->expects(self::never())->method('trackUserMessageView'); + + self::assertSame('https://example.com/a', $this->service->trackClick($this->tid(), null, [])); + } + + public function testInvalidTokenIsRejected(): void + { + $this->forwardRepository->expects(self::never())->method('findOneByUuid'); + + self::assertNull($this->service->trackClick('garbage', null, [])); + } + + public function testUnknownForwardIsRejected(): void + { + $this->forwardRepository->method('findOneByUuid')->willReturn(null); + $this->mlRepository->expects(self::never())->method('recordClick'); + + self::assertNull($this->service->trackClick($this->tid(), null, [])); + } + + public function testUnknownSubscriberIsRejected(): void + { + $forward = $this->createMock(LinkTrackForward::class); + $forward->method('getUrl')->willReturn('https://example.com/a'); + $this->forwardRepository->method('findOneByUuid')->willReturn($forward); + $this->messageRepository->method('findOneBy')->willReturn($this->message); + $this->subscriberRepository->method('findOneBy')->willReturn(null); + $this->mlRepository->expects(self::never())->method('recordClick'); + + self::assertNull($this->service->trackClick($this->tid(), null, [])); + } + + public function testNeverRedirectsToNonWebUrls(): void + { + $this->givenKnownLink('javascript:alert(1)'); + $this->mlRepository->expects(self::never())->method('recordClick'); + + self::assertNull($this->service->trackClick($this->tid(), null, [])); + } +} diff --git a/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php b/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php index c7495f99..7457c0bb 100644 --- a/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php +++ b/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php @@ -4,257 +4,243 @@ namespace PhpList\Core\Tests\Unit\Domain\Analytics\Service; -use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Core\ParameterProvider; -use PhpList\Core\Domain\Analytics\Exception\MissingMessageIdException; -use PhpList\Core\Domain\Analytics\Model\LinkTrack; +use PhpList\Core\Domain\Analytics\Model\LinkTrackForward; +use PhpList\Core\Domain\Analytics\Repository\LinkTrackForwardRepository; +use PhpList\Core\Domain\Analytics\Repository\LinkTrackMlRepository; use PhpList\Core\Domain\Analytics\Repository\LinkTrackRepository; use PhpList\Core\Domain\Analytics\Service\LinkTrackService; +use PhpList\Core\Domain\Analytics\Service\LinkTrackTokenService; use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; use PhpList\Core\Domain\Messaging\Model\Message; -use PhpList\Core\Domain\Messaging\Model\Message\MessageContent; +use PhpList\Core\Domain\Subscription\Model\Subscriber; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; class LinkTrackServiceTest extends TestCase { - private LinkTrackService $subject; - private LinkTrackRepository|MockObject $linkTrackRepository; + private LinkTrackForwardRepository&MockObject $forwardRepository; + private LinkTrackMlRepository&MockObject $mlRepository; + private LinkTrackTokenService $tokenService; + private Message&MockObject $campaign; + private Subscriber $subscriber; + /** @var array */ + private array $createdForwards = []; protected function setUp(): void { - $this->linkTrackRepository = $this->createMock(LinkTrackRepository::class); - $paramProvider = $this->createMock(ParameterProvider::class); + $this->forwardRepository = $this->createMock(LinkTrackForwardRepository::class); + $this->mlRepository = $this->createMock(LinkTrackMlRepository::class); + $this->tokenService = new LinkTrackTokenService( + new ParameterProvider([]), + 'https://example.com/api/v2', + 'secret' + ); + + $this->campaign = $this->createMock(Message::class); + $this->campaign->method('getId')->willReturn(7); + $this->campaign->method('getUuid')->willReturn('campaign-uuid'); + + $this->subscriber = new Subscriber('alice@example.com'); + $this->subscriber->setUuid('subscriber-uuid'); - $paramProvider->method('get') - ->with('click_track', false) - ->willReturn(true); + $this->forwardRepository->method('findOrCreateByUrl')->willReturnCallback( + function (string $url, bool $personalise): LinkTrackForward { + $this->createdForwards[$url] = ['url' => $url, 'personalise' => $personalise]; + $id = array_search($url, array_keys($this->createdForwards), true) + 1; - $this->subject = new LinkTrackService( - linkTrackRepository: $this->linkTrackRepository, - paramProvider: $paramProvider, - entityManager: $this->createMock(EntityManagerInterface::class) + return $this->createForward($id, $url); + } ); } - public function testExtractAndSaveLinksWithNoLinks(): void + private function createForward(int $id, string $url): LinkTrackForward&MockObject { - $messageId = 123; - $userId = 456; + $forward = $this->createMock(LinkTrackForward::class); + $forward->method('getId')->willReturn($id); + $forward->method('getUuid')->willReturn('forward-uuid-' . $id); + $forward->method('getUrl')->willReturn($url); - $messageContent = new MessagePrecacheDto(); - $messageContent->subject = 'Test Subject'; - $messageContent->content = 'No links here'; + return $forward; + } - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn($messageId); - $message->method('getContent')->willReturn(new MessageContent('Test Subject', 'No links here')); + private function createService(bool $clickTrack = true): LinkTrackService + { + return new LinkTrackService( + $this->createMock(LinkTrackRepository::class), + $this->forwardRepository, + $this->mlRepository, + $this->tokenService, + new ParameterProvider(['click_track' => $clickTrack]), + ); + } - $this->linkTrackRepository->expects(self::never())->method('persist'); + /** @return string[] the uuids found in all tracking links of the text, in order */ + private function trackedForwardUuids(string $text): array + { + preg_match_all('~/lt\?tid=([A-Za-z0-9_-]+)~', $text, $matches); - $result = $this->subject->extractAndSaveLinks($messageContent, $userId, $messageId); + return array_map( + fn (string $tid): string => $this->tokenService->parse($tid)->forwardUuid, + $matches[1] + ); + } - self::assertEmpty($result); + public function testIsClickTrackingEnabledFollowsTheParameter(): void + { + self::assertTrue($this->createService(true)->isClickTrackingEnabled()); + self::assertFalse($this->createService(false)->isClickTrackingEnabled()); } - public function testExtractAndSaveLinksWithLinks(): void + public function testContentIsReturnedUntouchedWhenClickTrackingIsDisabled(): void { - $messageId = 123; - $userId = 456; - $htmlContent = '

Check out this link and ' - . 'this one.

'; - - $messageContent = new MessageContent('Test Subject', $htmlContent); - $messagePrecacheDto = new MessagePrecacheDto(); - $messagePrecacheDto->subject = 'Test Subject'; - $messagePrecacheDto->content = $htmlContent; - - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn($messageId); - $message->method('getContent')->willReturn($messageContent); - - $this->linkTrackRepository->expects(self::exactly(2)) - ->method('persist') - ->willReturnCallback(function (LinkTrack $linkTrack) use ($messageId, $userId) { - self::assertSame($messageId, $linkTrack->getMessageId()); - self::assertSame($userId, $linkTrack->getUserId()); - self::assertContains($linkTrack->getUrl(), ['https://example.com', 'https://test.com']); - return null; - }); + $content = new MessagePrecacheDto(); + $content->content = 'x'; + + $this->forwardRepository->expects(self::never())->method('findOrCreateByUrl'); - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + $result = $this->createService(false)->trackLinks($content, $this->campaign, $this->subscriber); - self::assertCount(2, $result); - self::assertSame('https://example.com', $result[0]->getUrl()); - self::assertSame('https://test.com', $result[1]->getUrl()); + self::assertSame($content, $result); } - public function testExtractAndSaveLinksWithFooter(): void + public function testContentIsReturnedUntouchedWhenCampaignHasNoUuid(): void { - $messageId = 123; - $userId = 456; - $htmlContent = '

Main content with a link.

'; - $footerContent = '

Footer with another link.

'; - - $messageContent = new MessageContent('Test Subject', $htmlContent, null, $footerContent); - $messagePrecacheDto = new MessagePrecacheDto(); - $messagePrecacheDto->subject = 'Test Subject'; - $messagePrecacheDto->content = $htmlContent; - $messagePrecacheDto->htmlFooter = $footerContent; - - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn($messageId); - $message->method('getContent')->willReturn($messageContent); - - $this->linkTrackRepository->expects(self::exactly(2)) - ->method('persist') - ->willReturnCallback(function (LinkTrack $linkTrack) use ($messageId, $userId) { - self::assertSame($messageId, $linkTrack->getMessageId()); - self::assertSame($userId, $linkTrack->getUserId()); - self::assertContains($linkTrack->getUrl(), ['https://example.com', 'https://footer.com']); - return null; - }); - - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + $campaign = $this->createMock(Message::class); + $campaign->method('getUuid')->willReturn(null); + $content = new MessagePrecacheDto(); + $content->content = 'x'; - self::assertCount(2, $result); - self::assertSame('https://example.com', $result[0]->getUrl()); - self::assertSame('https://footer.com', $result[1]->getUrl()); + self::assertSame($content, $this->createService()->trackLinks($content, $campaign, $this->subscriber)); } - public function testExtractAndSaveLinksWithDuplicateLinks(): void + public function testHtmlLinksAreReplacedWithTrackingLinks(): void { - $messageId = 123; - $userId = 456; - $htmlContent = '

Link 1 and Link 2.

'; - - $messageContent = new MessageContent('Test Subject', $htmlContent); - $messagePrecacheDto = new MessagePrecacheDto(); - $messagePrecacheDto->subject = 'Test Subject'; - $messagePrecacheDto->content = $htmlContent; - - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn($messageId); - $message->method('getContent')->willReturn($messageContent); - - $this->linkTrackRepository->expects(self::once()) - ->method('persist') - ->willReturnCallback(function (LinkTrack $linkTrack) use ($messageId, $userId) { - self::assertSame($messageId, $linkTrack->getMessageId()); - self::assertSame($userId, $linkTrack->getUserId()); - self::assertSame('https://example.com', $linkTrack->getUrl()); - return null; - }); + $content = new MessagePrecacheDto(); + $content->content = '

A ' + . "

"; + + $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); + + self::assertStringContainsString('A', $result->content); + self::assertStringContainsString("content); + self::assertArrayHasKey('https://example.com/a?x=1&y=2', $this->createdForwards, 'stored url is decoded'); + self::assertArrayHasKey('http://example.org/b', $this->createdForwards); + } - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + public function testTrackingLinkIdentifiesForwardCampaignAndSubscriber(): void + { + $content = new MessagePrecacheDto(); + $content->content = 'A'; + + $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); + + preg_match('~tid=([A-Za-z0-9_-]+)~', $result->content, $match); + $token = $this->tokenService->parse($match[1]); + self::assertNotNull($token); + self::assertTrue($token->isHtml()); + self::assertSame('forward-uuid-1', $token->forwardUuid); + self::assertSame('campaign-uuid', $token->messageUuid); + self::assertSame('subscriber-uuid', $token->subscriberUuid); + } - self::assertCount(1, $result); - self::assertSame('https://example.com', $result[0]->getUrl()); + public function testLinksThatMustNotBeTrackedAreLeftAlone(): void + { + $html = implode(' ', [ + 'mail', + 'unsubscribe', + 'placeholder', + 'script', + 'https://example.com/other', + 'too long', + 'already tracked', + 'no href', + ]); + $content = new MessagePrecacheDto(); + $content->content = $html; + + $this->forwardRepository->expects(self::never())->method('findOrCreateByUrl'); + $this->mlRepository->expects(self::never())->method('incrementTotal'); + + $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); + + self::assertSame($html, $result->content); } - public function testExtractAndSaveLinksWithNullText(): void + public function testPersonalUidAndSessionParametersAreStrippedAndMarkTheForwardPersonal(): void { - $messageId = 123; - $userId = 456; - $footerContent = '

Footer with a link.

'; - - $messageContent = new MessageContent('Test Subject', null, null, $footerContent); - $messagePrecacheDto = new MessagePrecacheDto(); - $messagePrecacheDto->subject = 'Test Subject'; - $messagePrecacheDto->htmlFooter = $footerContent; - - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn($messageId); - $message->method('getContent')->willReturn($messageContent); - - $this->linkTrackRepository->expects(self::once()) - ->method('persist') - ->willReturnCallback(function (LinkTrack $linkTrack) use ($messageId, $userId) { - self::assertSame($messageId, $linkTrack->getMessageId()); - self::assertSame($userId, $linkTrack->getUserId()); - self::assertSame('https://footer.com', $linkTrack->getUrl()); - return null; - }); + $content = new MessagePrecacheDto(); + $content->content = 'P'; - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); - self::assertCount(1, $result); - self::assertSame('https://footer.com', $result[0]->getUrl()); + self::assertSame( + ['https://example.com/p?a=1#top' => ['url' => 'https://example.com/p?a=1#top', 'personalise' => true]], + $this->createdForwards + ); } - public function testExtractAndSaveLinksWithMessageWithoutId(): void + public function testTextUrlsAreReplacedWithTextTokens(): void { - $userId = 456; - $htmlContent = '

Link

'; + $content = new MessagePrecacheDto(); + $content->textContent = "Read https://example.com/a. Or see http://example.org/b?x=1\nbye"; + $content->textFooter = 'More: https://example.com/a'; - $messageContent = new MessageContent('Test Subject', $htmlContent); - $messagePrecacheDto = new MessagePrecacheDto(); - $messagePrecacheDto->subject = 'Test Subject'; - $messagePrecacheDto->content = $htmlContent; + $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn(null); - $message->method('getContent')->willReturn($messageContent); - - $this->expectException(MissingMessageIdException::class); - $this->expectExceptionMessage('Message must have an ID'); - - $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $message->getId()); + self::assertMatchesRegularExpression( + '~^Read https://example.com/api/v2/lt\?tid=[A-Za-z0-9_-]+\. Or see https://example.com/api/v2/lt\?tid=~', + $result->textContent + ); + preg_match('~tid=([A-Za-z0-9_-]+)~', $result->textContent, $match); + self::assertSame('T', $this->tokenService->parse($match[1])->type); + self::assertSame( + [$this->trackedForwardUuids($result->textContent)[0]], + $this->trackedForwardUuids((string) $result->textFooter), + 'the same url gets the same forward' + ); + self::assertSame("\nbye", substr($result->textContent, -4)); } - public function testIsExtractAndSaveLinksApplicableWhenClickTrackIsTrue(): void + public function testFootersAreTrackedToo(): void { - self::assertTrue($this->subject->isExtractAndSaveLinksApplicable()); - } + $content = new MessagePrecacheDto(); + $content->htmlFooter = 'Footer'; - public function testIsExtractAndSaveLinksApplicableWhenClickTrackIsFalse(): void - { - $paramProvider = $this->createMock(ParameterProvider::class); - $paramProvider->method('get') - ->with('click_track', false) - ->willReturn(false); - - $subject = new LinkTrackService( - linkTrackRepository: $this->linkTrackRepository, - paramProvider: $paramProvider, - entityManager: $this->createMock(EntityManagerInterface::class) - ); + $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); - self::assertFalse($subject->isExtractAndSaveLinksApplicable()); + self::assertSame(['forward-uuid-1'], $this->trackedForwardUuids($result->htmlFooter)); } - public function testExtractAndSaveLinksWithExistingLink(): void + public function testTotalIsIncrementedOncePerDistinctLinkOfTheEmail(): void { - $messageId = 123; - $userId = 456; - $url = 'https://example.com'; - $htmlContent = '

Check out this link.

'; - - $messageContent = new MessageContent('Test Subject', $htmlContent); - - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn($messageId); - $message->method('getContent')->willReturn($messageContent); - $messagePrecacheDto = new MessagePrecacheDto(); - $messagePrecacheDto->subject = 'Test Subject'; - $messagePrecacheDto->content = $htmlContent; + $content = new MessagePrecacheDto(); + $content->content = '12' + . '3'; + $content->textContent = 'https://example.com/a'; + + $incremented = []; + $this->mlRepository->expects(self::exactly(2)) + ->method('incrementTotal') + ->willReturnCallback(function (int $messageId, int $forwardId) use (&$incremented): void { + $incremented[] = [$messageId, $forwardId]; + }); - $existingLinkTrack = new LinkTrack(); - $existingLinkTrack->setMessageId($messageId); - $existingLinkTrack->setUserId($userId); - $existingLinkTrack->setUrl($url); + $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); - $this->linkTrackRepository->expects(self::once()) - ->method('findByUrlUserIdAndMessageId') - ->with($url, $userId, $messageId) - ->willReturn($existingLinkTrack); + self::assertSame([[7, 1], [7, 2]], $incremented); + } - $this->linkTrackRepository->expects(self::never()) - ->method('persist'); + public function testTheGivenContentIsNotModified(): void + { + $content = new MessagePrecacheDto(); + $content->content = 'A'; - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); - self::assertCount(1, $result); - self::assertSame($existingLinkTrack, $result[0]); + self::assertNotSame($content, $result); + self::assertSame('A', $content->content); } } diff --git a/tests/Unit/Domain/Analytics/Service/LinkTrackTokenServiceTest.php b/tests/Unit/Domain/Analytics/Service/LinkTrackTokenServiceTest.php new file mode 100644 index 00000000..e97f1fc5 --- /dev/null +++ b/tests/Unit/Domain/Analytics/Service/LinkTrackTokenServiceTest.php @@ -0,0 +1,95 @@ + $sign]), + 'https://example.com/api/v2/', + $secret + ); + } + + /** @return array */ + private function queryOf(string $url): array + { + parse_str((string) parse_url($url, PHP_URL_QUERY), $query); + + return $query; + } + + public function testCreatesAbsoluteUrlToTheLinkTrackEndpoint(): void + { + $url = $this->createService()->createUrl(new LinkTrackToken('H', 'fwd-1', 'msg-2', 'usr-3')); + + self::assertStringStartsWith('https://example.com/api/v2/lt?tid=', $url); + self::assertSame(['tid'], array_keys($this->queryOf($url))); + } + + public function testParsesWhatItCreated(): void + { + $service = $this->createService(); + $url = $service->createUrl(new LinkTrackToken('T', 'fwd-1', 'msg-2', 'usr-3')); + + $token = $service->parse($this->queryOf($url)['tid']); + + self::assertNotNull($token); + self::assertSame('T', $token->type); + self::assertFalse($token->isHtml()); + self::assertSame('fwd-1', $token->forwardUuid); + self::assertSame('msg-2', $token->messageUuid); + self::assertSame('usr-3', $token->subscriberUuid); + } + + /** @dataProvider malformedTokenProvider */ + public function testRejectsMalformedTokens(string $tid): void + { + self::assertNull($this->createService()->parse($tid)); + } + + public static function malformedTokenProvider(): array + { + $encode = static fn (string $value): string => rtrim(strtr(base64_encode($value), '+/', '-_'), '='); + + return [ + 'not base64' => ['!!!'], + 'too few parts' => [$encode('H|a|b')], + 'too many parts' => [$encode('H|a|b|c|d')], + 'unknown type' => [$encode('X|a|b|c')], + 'illegal characters' => [$encode("H|a|b|c'; drop table")], + 'empty uuid' => [$encode('H||b|c')], + ]; + } + + public function testSignedUrlsAreVerified(): void + { + $service = $this->createService(sign: true); + $query = $this->queryOf($service->createUrl(new LinkTrackToken('H', 'f', 'm', 'u'))); + + self::assertArrayHasKey('hm', $query); + self::assertNotNull($service->parse($query['tid'], $query['hm'])); + self::assertNull($service->parse($query['tid']), 'missing signature'); + self::assertNull($service->parse($query['tid'], 'wrong'), 'wrong signature'); + self::assertNull( + $this->createService(sign: true, secret: 'other')->parse($query['tid'], $query['hm']), + 'signature of another secret' + ); + } + + public function testUnsignedUrlsHaveNoSignature(): void + { + $query = $this->queryOf($this->createService()->createUrl(new LinkTrackToken('H', 'f', 'm', 'u'))); + + self::assertArrayNotHasKey('hm', $query); + } +} diff --git a/tests/Unit/Domain/Messaging/Service/ForwardContentServiceTest.php b/tests/Unit/Domain/Messaging/Service/ForwardContentServiceTest.php index 28891f78..65bcc57e 100644 --- a/tests/Unit/Domain/Messaging/Service/ForwardContentServiceTest.php +++ b/tests/Unit/Domain/Messaging/Service/ForwardContentServiceTest.php @@ -11,7 +11,6 @@ use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; use PhpList\Core\Domain\Messaging\Model\Message; use PhpList\Core\Domain\Messaging\Service\ForwardContentService; -use PhpList\Core\Domain\Messaging\Service\MessageProcessingPreparator; use PhpList\Core\Domain\Messaging\Service\Builder\ForwardEmailBuilder; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PHPUnit\Framework\MockObject\MockObject; @@ -22,13 +21,11 @@ class ForwardContentServiceTest extends TestCase { private CacheInterface&MockObject $cache; - private MessageProcessingPreparator&MockObject $preparator; private ForwardEmailBuilder&MockObject $builder; protected function setUp(): void { $this->cache = $this->createMock(CacheInterface::class); - $this->preparator = $this->createMock(MessageProcessingPreparator::class); $this->builder = $this->createMock(ForwardEmailBuilder::class); } @@ -36,7 +33,6 @@ public function testThrowsWhenCacheMissing(): void { $service = new ForwardContentService( cache: $this->cache, - messagePreparator: $this->preparator, forwardEmailBuilder: $this->builder, ); @@ -66,11 +62,10 @@ public function testThrowsWhenCacheMissing(): void ); } - public function testProcessesLinksAndDelegatesToBuilder(): void + public function testDelegatesCachedContentToBuilderWithoutClickTracking(): void { $service = new ForwardContentService( cache: $this->cache, - messagePreparator: $this->preparator, forwardEmailBuilder: $this->builder, ); @@ -80,7 +75,6 @@ public function testProcessesLinksAndDelegatesToBuilder(): void $subscriber->setHtmlEmail(true); $cached = new MessagePrecacheDto(); - $processed = new MessagePrecacheDto(); $this->cache ->expects(self::once()) @@ -88,16 +82,6 @@ public function testProcessesLinksAndDelegatesToBuilder(): void ->with('messaging.message.base.42.1.0') ->willReturn($cached); - $this->preparator - ->expects(self::once()) - ->method('processMessageLinks') - ->with( - 42, - $cached, - $subscriber - ) - ->willReturn($processed); - $expectedEmail = new Email(); $this->builder ->expects(self::once()) @@ -106,7 +90,7 @@ public function testProcessesLinksAndDelegatesToBuilder(): void 42, 'f@example.com', $subscriber, - $processed, + $cached, true, 'From Name', 'from@example.com', diff --git a/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php b/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php index 4cd2e800..08ffa892 100644 --- a/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php +++ b/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php @@ -4,7 +4,6 @@ namespace PhpList\Core\Tests\Unit\Domain\Messaging\Service; -use PhpList\Core\Domain\Analytics\Model\LinkTrack; use PhpList\Core\Domain\Analytics\Service\LinkTrackService; use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; use PhpList\Core\Domain\Messaging\Repository\MessageRepository; @@ -116,87 +115,52 @@ public function testEnsureCampaignsHaveUuidWithCampaigns(): void $this->preparator->ensureCampaignsHaveUuid($this->output); } - public function testProcessMessageLinksWhenLinkTrackingNotApplicable(): void + public function testProcessMessageLinksWhenClickTrackingIsDisabled(): void { $messageContent = new MessagePrecacheDto(); $subscriber = $this->createMock(Subscriber::class); - $subscriber->method('getId')->willReturn(123); - $subscriber->method('getEmail')->willReturn('test@example.com'); $this->linkTrackService->expects($this->once()) - ->method('isExtractAndSaveLinksApplicable') + ->method('isClickTrackingEnabled') ->willReturn(false); - - $this->linkTrackService->expects($this->never()) - ->method('extractAndSaveLinks'); + $this->messageRepository->expects($this->never())->method('findById'); + $this->linkTrackService->expects($this->never())->method('trackLinks'); $result = $this->preparator->processMessageLinks(1, $messageContent, $subscriber); $this->assertSame($messageContent, $result); } - public function testProcessMessageLinksWhenNoLinksExtracted(): void + public function testProcessMessageLinksWhenCampaignIsUnknown(): void { - $messageId = 1; $messageContent = new MessagePrecacheDto(); $subscriber = $this->createMock(Subscriber::class); - $subscriber->method('getId')->willReturn(123); - $subscriber->method('getEmail')->willReturn('test@example.com'); - - $this->linkTrackService->expects($this->once()) - ->method('isExtractAndSaveLinksApplicable') - ->willReturn(true); - $this->linkTrackService->expects($this->once()) - ->method('extractAndSaveLinks') - ->with($messageContent, 123, $messageId) - ->willReturn([]); + $this->linkTrackService->method('isClickTrackingEnabled')->willReturn(true); + $this->messageRepository->expects($this->once())->method('findById')->with(1)->willReturn(null); + $this->linkTrackService->expects($this->never())->method('trackLinks'); - $result = $this->preparator->processMessageLinks($messageId, $messageContent, $subscriber); + $result = $this->preparator->processMessageLinks(1, $messageContent, $subscriber); $this->assertSame($messageContent, $result); } - public function testProcessMessageLinksWithLinksExtracted(): void + public function testProcessMessageLinksReturnsTheContentWithTrackedLinks(): void { - $content = new MessagePrecacheDto(); + $messageContent = new MessagePrecacheDto(); + $tracked = new MessagePrecacheDto(); $subscriber = $this->createMock(Subscriber::class); - $subscriber->method('getId')->willReturn(123); - $subscriber->method('getEmail')->willReturn('test@example.com'); - - $linkTrack1 = $this->createMock(LinkTrack::class); - $linkTrack1->method('getId')->willReturn(1); - $linkTrack1->method('getUrl')->willReturn('https://example.com'); + $campaign = $this->createMock(Message::class); - $linkTrack2 = $this->createMock(LinkTrack::class); - $linkTrack2->method('getId')->willReturn(2); - $linkTrack2->method('getUrl')->willReturn('https://example.org'); - - $savedLinks = [$linkTrack1, $linkTrack2]; - - $this->linkTrackService->method('isExtractAndSaveLinksApplicable')->willReturn(true); - $this->linkTrackService - ->method('extractAndSaveLinks') - ->with($content, 123, 1) - ->willReturn($savedLinks); - - $content->content = 'Link 1 Link 2'; - $content->htmlFooter = 'Footer Link'; + $this->linkTrackService->method('isClickTrackingEnabled')->willReturn(true); + $this->messageRepository->method('findById')->with(1)->willReturn($campaign); + $this->linkTrackService->expects($this->once()) + ->method('trackLinks') + ->with($messageContent, $campaign, $subscriber) + ->willReturn($tracked); - $result = $this->preparator->processMessageLinks(1, $content, $subscriber); + $result = $this->preparator->processMessageLinks(1, $messageContent, $subscriber); - $this->assertSame($content, $result); - $this->assertStringContainsString( - MessageProcessingPreparator::LINK_TRACK_ENDPOINT . '?id=1', - $content->content - ); - $this->assertStringContainsString( - MessageProcessingPreparator::LINK_TRACK_ENDPOINT . '?id=2', - $content->content - ); - $this->assertStringContainsString( - MessageProcessingPreparator::LINK_TRACK_ENDPOINT . '?id=1', - $content->htmlFooter - ); + $this->assertSame($tracked, $result); } } From ee412fd3af8a415bb286527ed482ab2fb35c5de4 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Tue, 27 Oct 2026 15:39:08 +0400 Subject: [PATCH 10/30] ref: improve security in FileHelper --- src/Domain/Common/FileHelper.php | 35 ++++++++++----------- tests/Unit/Domain/Common/FileHelperTest.php | 2 +- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/src/Domain/Common/FileHelper.php b/src/Domain/Common/FileHelper.php index 122c804a..27a7ed93 100644 --- a/src/Domain/Common/FileHelper.php +++ b/src/Domain/Common/FileHelper.php @@ -35,37 +35,36 @@ public function readFileContents(string $path): ?string public function writeFileToDirectory(string $directory, string $originalFilename, string $contents): ?string { - // tempnam() silently falls back to the system temp dir when the directory is unusable if (!is_dir($directory) || !is_writable($directory)) { return null; } - $pathInfo = pathinfo($originalFilename); - $name = $pathInfo['filename'] === '' ? 'file' : $pathInfo['filename']; - $ext = $pathInfo['extension'] ?? ''; - - $tempFile = tempnam($directory, $name); - if ($tempFile === false) { + $realDirectory = realpath($directory); + if ($realDirectory === false) { return null; } - if (dirname((string) realpath($tempFile)) !== realpath($directory)) { - unlink($tempFile); - + // Untrusted writers could swap entries in the directory between checks and writes + $perms = fileperms($realDirectory); + if ($perms === false || (($perms & 0022) !== 0 && ($perms & 01000) === 0)) { return null; } - $fullPath = $directory . '/' . basename($tempFile); - if ($ext !== '') { - $fullPath .= '.' . $ext; - if (!rename($tempFile, $fullPath)) { - unlink($tempFile); + $pathInfo = pathinfo($originalFilename); + $name = $pathInfo['filename'] === '' ? 'file' : $pathInfo['filename']; + $ext = $pathInfo['extension'] ?? ''; - return null; - } + // Exclusive create (O_EXCL) never follows a symlink or reuses an existing entry, + // so the file is created and written in a single step without rename. + $fullPath = $realDirectory . '/' . $name . bin2hex(random_bytes(6)) . ($ext !== '' ? '.' . $ext : ''); + $handle = fopen($fullPath, 'xb'); + if ($handle === false) { + return null; } - if (file_put_contents($fullPath, $contents) !== strlen($contents)) { + $written = fwrite($handle, $contents); + fclose($handle); + if ($written !== strlen($contents)) { unlink($fullPath); return null; diff --git a/tests/Unit/Domain/Common/FileHelperTest.php b/tests/Unit/Domain/Common/FileHelperTest.php index fae886d4..f9f231d9 100644 --- a/tests/Unit/Domain/Common/FileHelperTest.php +++ b/tests/Unit/Domain/Common/FileHelperTest.php @@ -14,7 +14,7 @@ class FileHelperTest extends TestCase protected function setUp(): void { $this->tmpDir = sys_get_temp_dir() . '/phplist-filehelper-' . bin2hex(random_bytes(6)); - mkdir($this->tmpDir, 0777, true); + mkdir($this->tmpDir, 0700, true); } protected function tearDown(): void From 6cbdced3ed15e73c5bd16005713faf5592c47331 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Tue, 27 Oct 2026 15:48:03 +0400 Subject: [PATCH 11/30] ref: use executeStatement instead of flush --- .../Repository/LinkTrackForwardRepository.php | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/Domain/Analytics/Repository/LinkTrackForwardRepository.php b/src/Domain/Analytics/Repository/LinkTrackForwardRepository.php index 85ec05eb..1db61194 100644 --- a/src/Domain/Analytics/Repository/LinkTrackForwardRepository.php +++ b/src/Domain/Analytics/Repository/LinkTrackForwardRepository.php @@ -43,8 +43,21 @@ public function findOrCreateByUrl(string $url, bool $personalise): LinkTrackForw } if (empty($forward->getUuid())) { - $forward->setUuid(self::generateUuid()); - $this->getEntityManager()->flush(); + $metadata = $this->getClassMetadata(); + $uuidColumn = $metadata->getColumnName('uuid'); + // only fills an empty uuid, so a worker that got there first is never overwritten + $this->getEntityManager()->getConnection()->executeStatement( + sprintf( + 'UPDATE %s SET %s = :uuid WHERE %s = :id AND (%s IS NULL OR %s = \'\')', + $metadata->getTableName(), + $uuidColumn, + $metadata->getColumnName('id'), + $uuidColumn, + $uuidColumn + ), + ['uuid' => self::generateUuid(), 'id' => $forward->getId()], + ); + $this->getEntityManager()->refresh($forward); } return $forward; From 8f307403de301fce96189afcf15b2c9fa285a4a2 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Tue, 27 Oct 2026 15:49:59 +0400 Subject: [PATCH 12/30] fix: ensure UID is added before fragment in personalized links --- .../Analytics/Service/LinkClickService.php | 6 ++++- .../Service/LinkClickServiceTest.php | 22 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/Domain/Analytics/Service/LinkClickService.php b/src/Domain/Analytics/Service/LinkClickService.php index 75ad3d9c..538f2dd3 100644 --- a/src/Domain/Analytics/Service/LinkClickService.php +++ b/src/Domain/Analytics/Service/LinkClickService.php @@ -96,7 +96,11 @@ private function resolveUrl(LinkTrackForward $forward, Subscriber $subscriber, ? if ($userMessage === null) { return null; } - $url .= (str_contains($url, '?') ? '&' : '?') . 'uid=' . $subscriber->getUniqueId(); + // the uid has to go before any fragment, which is never sent to the server + $fragmentPos = strpos($url, '#'); + $fragment = $fragmentPos === false ? '' : substr($url, $fragmentPos); + $base = $fragmentPos === false ? $url : substr($url, 0, $fragmentPos); + $url = $base . (str_contains($base, '?') ? '&' : '?') . 'uid=' . $subscriber->getUniqueId() . $fragment; } return preg_match('~^(https?|ftp)://~i', $url) ? $url : null; diff --git a/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php b/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php index 370aefa7..5b59986d 100644 --- a/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php +++ b/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php @@ -144,6 +144,28 @@ public function testPersonalisedLinkGetsTheSubscribersUid(): void self::assertSame('https://example.com/p?a=1&uid=uniq-3', $this->service->trackClick($this->tid(), null, [])); } + public function testPersonalisedLinkPutsTheUidBeforeTheFragment(): void + { + $this->givenKnownLink('https://example.com/p?a=1#top', personalise: true); + $this->givenUserMessage(viewed: true); + + self::assertSame( + 'https://example.com/p?a=1&uid=uniq-3#top', + $this->service->trackClick($this->tid(), null, []) + ); + } + + public function testPersonalisedLinkWithOnlyAFragmentStartsTheQueryBeforeIt(): void + { + $this->givenKnownLink('https://example.com/p#a?b', personalise: true); + $this->givenUserMessage(viewed: true); + + self::assertSame( + 'https://example.com/p?uid=uniq-3#a?b', + $this->service->trackClick($this->tid(), null, []) + ); + } + public function testPersonalisedLinkIsRefusedForSubscribersThatNeverGotTheCampaign(): void { $this->givenKnownLink('https://example.com/p', personalise: true); From e37ba434743b858e9a3c32c1379f8d9b41ee17aa Mon Sep 17 00:00:00 2001 From: Tatevik Date: Tue, 27 Oct 2026 15:51:40 +0400 Subject: [PATCH 13/30] feat: add LIST_FIELDS constant and handle null data for target and exclude lists --- .../Messaging/Service/MessageDataLoader.php | 8 ++++- .../Service/MessageDataLoaderTest.php | 34 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/Domain/Messaging/Service/MessageDataLoader.php b/src/Domain/Messaging/Service/MessageDataLoader.php index 24a36e26..c1db86b5 100644 --- a/src/Domain/Messaging/Service/MessageDataLoader.php +++ b/src/Domain/Messaging/Service/MessageDataLoader.php @@ -14,6 +14,7 @@ class MessageDataLoader { private const AS_FORMAT_FIELDS = ['astext', 'ashtml', 'astextandhtml', 'aspdf', 'astextandpdf']; + private const LIST_FIELDS = ['targetlist', 'excludelist']; private const SCHEDULE_FIELDS = ['embargo', 'repeatuntil', 'requeueuntil']; private ?string $messageFromAddress; @@ -119,7 +120,12 @@ private function mergeStoredMessageData(array &$messageData, Message $message): { $storedMessageData = $this->messageDataRepository->getForMessage($message->getId()); foreach ($storedMessageData as $storedMessageDatum) { - $rawData = $storedMessageDatum->getData() ?? ''; + $rawData = $storedMessageDatum->getData(); + if ($rawData === null && in_array($storedMessageDatum->getName(), self::LIST_FIELDS, true)) { + // keep the array defaults, a string here would break populateTargetLists + continue; + } + $rawData ??= ''; if (str_starts_with($rawData, 'SER:')) { $unserialized = unserialize(substr($rawData, 4), ['allowed_classes' => false]); if (!is_array($unserialized)) { diff --git a/tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php b/tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php index f10a83db..b1446640 100644 --- a/tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php +++ b/tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php @@ -200,6 +200,40 @@ public function testStoredSerializedDataWithNonStringValuesAndNullDataDoNotBreak $this->assertSame('', $result['footer']); } + public function testNullTargetAndExcludeListDataKeepArrayDefaults(): void + { + $this->config->method('getValue')->willReturnMap([ + [ConfigOption::MessageFromAddress, 'from@example.com'], + ]); + $this->messageRepository->method('getNonEmptyFields')->willReturn([]); + + $targetList = (new MessageData())->setId(11)->setName('targetlist')->setData(null); + $excludeList = (new MessageData())->setId(11)->setName('excludelist')->setData(null); + $this->messageDataRepository->method('getForMessage')->willReturn([$targetList, $excludeList]); + + $listMock = $this->createMock(SubscriberList::class); + $listMock->method('getId')->willReturn(42); + $listMessageMock = $this->createMock(ListMessage::class); + $listMessageMock->method('getList')->willReturn($listMock); + + $message = $this->createMock(Message::class); + $message->method('getId')->willReturn(11); + $message->method('getListMessages')->willReturn(new ArrayCollection([$listMessageMock])); + + $loader = new MessageDataLoader( + configProvider: $this->config, + messageDataRepository: $this->messageDataRepository, + messageRepository: $this->messageRepository, + logger: $this->createMock(LoggerInterface::class), + defaultMessageAge: 3600 + ); + + $result = ($loader)($message); + + $this->assertSame([42 => 1], $result['targetlist']); + $this->assertSame([], $result['excludelist']); + } + public static function fromFieldProvider(): array { return [ From 4e4c6d01d3c20c004f8e24bf0b8eb12f45a22c3e Mon Sep 17 00:00:00 2001 From: Tatevik Date: Tue, 27 Oct 2026 15:54:18 +0400 Subject: [PATCH 14/30] ref: to fix phpmd --- .../Service/LinkTrackTokenService.php | 30 ++++++++++++---- src/Domain/Common/FileHelper.php | 34 ++++++++++++------- 2 files changed, 45 insertions(+), 19 deletions(-) diff --git a/src/Domain/Analytics/Service/LinkTrackTokenService.php b/src/Domain/Analytics/Service/LinkTrackTokenService.php index 371f14d5..796432ea 100644 --- a/src/Domain/Analytics/Service/LinkTrackTokenService.php +++ b/src/Domain/Analytics/Service/LinkTrackTokenService.php @@ -48,9 +48,7 @@ public function createUrl(LinkTrackToken $token): string /** Returns null for malformed tokens and, when signing is enabled, for missing or wrong signatures. */ public function parse(string $tid, ?string $signature = null): ?LinkTrackToken { - if ($this->isSigningEnabled() - && ($signature === null || $signature === '' || !hash_equals($this->sign($tid), $signature)) - ) { + if (!$this->hasValidSignature($tid, $signature)) { return null; } @@ -60,17 +58,35 @@ public function parse(string $tid, ?string $signature = null): ?LinkTrackToken } $parts = explode('|', $decoded); - if (count($parts) !== 4 || !LinkTrackToken::isValidType($parts[0])) { + if (count($parts) !== 4 + || !LinkTrackToken::isValidType($parts[0]) + || !$this->areValidUuids(array_slice($parts, 1)) + ) { return null; } - foreach (array_slice($parts, 1) as $uuid) { + return new LinkTrackToken($parts[0], $parts[1], $parts[2], $parts[3]); + } + + private function hasValidSignature(string $tid, ?string $signature): bool + { + if (!$this->isSigningEnabled()) { + return true; + } + + return $signature !== null && $signature !== '' && hash_equals($this->sign($tid), $signature); + } + + /** @param string[] $uuids */ + private function areValidUuids(array $uuids): bool + { + foreach ($uuids as $uuid) { if (!preg_match('/^[A-Za-z0-9-]{1,64}$/', $uuid)) { - return null; + return false; } } - return new LinkTrackToken($parts[0], $parts[1], $parts[2], $parts[3]); + return true; } private function isSigningEnabled(): bool diff --git a/src/Domain/Common/FileHelper.php b/src/Domain/Common/FileHelper.php index 27a7ed93..7073abf8 100644 --- a/src/Domain/Common/FileHelper.php +++ b/src/Domain/Common/FileHelper.php @@ -35,18 +35,8 @@ public function readFileContents(string $path): ?string public function writeFileToDirectory(string $directory, string $originalFilename, string $contents): ?string { - if (!is_dir($directory) || !is_writable($directory)) { - return null; - } - - $realDirectory = realpath($directory); - if ($realDirectory === false) { - return null; - } - - // Untrusted writers could swap entries in the directory between checks and writes - $perms = fileperms($realDirectory); - if ($perms === false || (($perms & 0022) !== 0 && ($perms & 01000) === 0)) { + $realDirectory = $this->resolveTrustedDirectory($directory); + if ($realDirectory === null) { return null; } @@ -72,4 +62,24 @@ public function writeFileToDirectory(string $directory, string $originalFilename return $fullPath; } + + private function resolveTrustedDirectory(string $directory): ?string + { + if (!is_dir($directory) || !is_writable($directory)) { + return null; + } + + $realDirectory = realpath($directory); + if ($realDirectory === false) { + return null; + } + + // Untrusted writers could swap entries in the directory between checks and writes + $perms = fileperms($realDirectory); + if ($perms === false || (($perms & 0022) !== 0 && ($perms & 01000) === 0)) { + return null; + } + + return $realDirectory; + } } From b7eb0a82f7b5600a2ff3422a9ec8a7ae4aeeaf34 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Sun, 1 Nov 2026 20:19:15 +0400 Subject: [PATCH 15/30] feat: add APP_WEBSITE to environment configuration for server domain setting --- .env.dist | 3 +++ config/parameters.yml | 1 + config/services/providers.yml | 2 ++ .../Configuration/Model/ConfigOption.php | 1 - .../Provider/DefaultConfigProvider.php | 21 +++++++++++++++++-- .../Provider/DefaultConfigProviderTest.php | 8 +++++++ 6 files changed, 33 insertions(+), 3 deletions(-) diff --git a/.env.dist b/.env.dist index 07a41b6a..80161df9 100644 --- a/.env.dist +++ b/.env.dist @@ -24,6 +24,9 @@ APP_POWERED_BY_PHPLIST=0 PREFERENCEPAGE_SHOW_PRIVATE_LISTS=0 API_BASE_URL=http://api.phplist.local/ +# Website address without scheme (e.g. www.example.com). Used as the default of the "website" setting; +# the "domain" setting defaults to the same value without a leading "www.". Both are editable in the admin UI. +APP_WEBSITE=phplist.local FRONT_END_BASE_URL=http://frontend.phplist.local PARALLEL_USE_WITH_PHPLIST3=0 diff --git a/config/parameters.yml b/config/parameters.yml index 28d4ed9c..36860be3 100644 --- a/config/parameters.yml +++ b/config/parameters.yml @@ -23,6 +23,7 @@ parameters: app.rest_api_base_url: '%env(API_BASE_URL)%/api/v2' app.api_base_url: '%env(API_BASE_URL)%' app.frontend_base_url: '%env(FRONT_END_BASE_URL)%' + app.website: '%env(APP_WEBSITE)%' app.parallel_use_with_phplist3: '%env(PARALLEL_USE_WITH_PHPLIST3)%' diff --git a/config/services/providers.yml b/config/services/providers.yml index 481b23a3..830abb14 100644 --- a/config/services/providers.yml +++ b/config/services/providers.yml @@ -16,6 +16,8 @@ services: PhpList\Core\Domain\Configuration\Service\Provider\DefaultConfigProvider: autowire: true + arguments: + $website: '%app.website%' PhpList\Core\Domain\Configuration\Service\Provider\ConfigProvider: autowire: true diff --git a/src/Domain/Configuration/Model/ConfigOption.php b/src/Domain/Configuration/Model/ConfigOption.php index 1c4a3376..34a2b306 100644 --- a/src/Domain/Configuration/Model/ConfigOption.php +++ b/src/Domain/Configuration/Model/ConfigOption.php @@ -15,7 +15,6 @@ enum ConfigOption: string case ConfirmationUrl = 'confirmationurl'; case PreferencesUrl = 'preferencesurl'; case SubscribeUrl = 'subscribeurl'; - // todo: check where is this defined case Domain = 'domain'; case Website = 'website'; case MessageFromAddress = 'message_from_address'; diff --git a/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php b/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php index 3222acc9..7d8b0f3d 100644 --- a/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php +++ b/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php @@ -16,8 +16,10 @@ class DefaultConfigProvider */ private array $defaults = []; - public function __construct(private TranslatorInterface $translator) - { + public function __construct( + private readonly TranslatorInterface $translator, + private readonly string $website = '', + ) { } /** @SuppressWarnings("PHPMD.ExcessiveMethodLength") */ @@ -31,6 +33,21 @@ private function init(): void $pageRoot = '/api/v2'; $this->defaults = [ + 'website' => [ + 'value' => $this->website, + 'description' => $this->translator->trans('Website address (without http://)'), + 'infoicon' => true, + 'type' => 'text', + 'allowempty' => false, + 'category' => 'general', + ], + 'domain' => [ + 'value' => preg_replace('/^www\./i', '', $this->website), + 'description' => $this->translator->trans('Domain Name of your server (for email)'), + 'type' => 'text', + 'allowempty' => false, + 'category' => 'general', + ], 'admin_address' => [ 'value' => 'webmaster@[DOMAIN]', 'description' => $this->translator->trans('Person in charge of this system (one email address)'), diff --git a/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php b/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php index 1d79369a..c296ccea 100644 --- a/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php +++ b/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php @@ -25,6 +25,14 @@ protected function setUp(): void $this->provider = new DefaultConfigProvider($this->translator); } + public function testWebsiteAndDomainDefaultsComeFromConfiguredWebsite(): void + { + $provider = new DefaultConfigProvider($this->translator, 'www.example.com'); + + $this->assertSame('www.example.com', $provider->get(ConfigOption::Website)['value']); + $this->assertSame('example.com', $provider->get(ConfigOption::Domain)['value']); + } + public function testHasReturnsTrueForKnownKey(): void { $this->assertTrue($this->provider->has(ConfigOption::AdminAddress)); From ba200910c790b4891c0331839e1f47c1d8d065e1 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Sun, 1 Nov 2026 20:32:33 +0400 Subject: [PATCH 16/30] fix: clarify envelope sender configuration in SystemNotificationMailer --- src/Domain/Messaging/Service/SystemNotificationMailer.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Domain/Messaging/Service/SystemNotificationMailer.php b/src/Domain/Messaging/Service/SystemNotificationMailer.php index 62c60628..160ad669 100644 --- a/src/Domain/Messaging/Service/SystemNotificationMailer.php +++ b/src/Domain/Messaging/Service/SystemNotificationMailer.php @@ -40,9 +40,8 @@ public function send(int $messageId, string $toEmail, string $subject, string $c return false; } - // todo: check if from name should be from config $envelope = new Envelope( - sender: new Address($this->bounceEmail, 'PHPList'), + sender: new Address($this->bounceEmail), recipients: [new Address($email->getTo()[0]->getAddress())], ); $this->mailer->send(message: $email, envelope: $envelope); From 3ec60da8e40d8e9b848f577f867c303a23e16377 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Sun, 1 Nov 2026 20:42:36 +0400 Subject: [PATCH 17/30] feat: integrate user blacklist check in SubscriberCsvImporter for email validation --- .../Service/SubscriberCsvImporter.php | 22 +++-- .../Service/SubscriberCsvImporterTest.php | 97 ++++++++++++++++++- 2 files changed, 109 insertions(+), 10 deletions(-) diff --git a/src/Domain/Subscription/Service/SubscriberCsvImporter.php b/src/Domain/Subscription/Service/SubscriberCsvImporter.php index f9db822a..d82b0c4d 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvImporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvImporter.php @@ -13,6 +13,7 @@ use PhpList\Core\Domain\Subscription\Model\Dto\SubscriberImportOptions; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; +use PhpList\Core\Domain\Subscription\Repository\UserBlacklistRepository; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberAttributeManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberManager; @@ -44,6 +45,7 @@ public function __construct( private readonly TranslatorInterface $translator, private readonly MessageBusInterface $messageBus, private readonly SubscriberHistoryManager $subscriberHistoryManager, + private readonly UserBlacklistRepository $blacklistRepository, ) { } @@ -187,13 +189,16 @@ private function processRow( $this->attributeManager->processAttributes($subscriber, $dto->extraAttributes); - [$listLines, $addedNewSubscriberToList] = $this->getHistoryListLines($subscriber, $options); - - // todo: use isBlacklisted logic (grace time) not function from Subscriber class - if ($subscriber->isBlacklisted()) { + // as in phplist3, only the blacklist table decides; a blacklisted flag from the CSV alone does not + $isBlacklisted = $this->blacklistRepository->isEmailBlacklisted($subscriber->getEmail()); + if ($isBlacklisted) { + // make sure the flag reflects the blacklist table + $subscriber->setBlacklisted(true); $stats['blacklisted']++; } + [$listLines, $addedNewSubscriberToList] = $this->getHistoryListLines($subscriber, $options, $isBlacklisted); + $this->subscriberHistoryManager->addHistoryFromImport( subscriber: $subscriber, listLines: $listLines, @@ -273,11 +278,14 @@ private function prepareConfirmationMessage( return null; } - private function getHistoryListLines(Subscriber $subscriber, SubscriberImportOptions $options): array - { + private function getHistoryListLines( + Subscriber $subscriber, + SubscriberImportOptions $options, + bool $isBlacklisted, + ): array { $addedNewSubscriberToList = false; $listLines = []; - if (!$subscriber->isBlacklisted() && count($options->listIds) > 0) { + if (!$isBlacklisted && count($options->listIds) > 0) { foreach ($options->listIds as $listId) { $created = $this->subscriptionManager->addSubscriberToAList($subscriber, $listId); if ($created) { diff --git a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php index 1b2a7bf6..d429c5ca 100644 --- a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php +++ b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php @@ -17,6 +17,7 @@ use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriptionManager; +use PhpList\Core\Domain\Subscription\Repository\UserBlacklistRepository; use PhpList\Core\Domain\Subscription\Service\SubscriberCsvImporter; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -31,28 +32,34 @@ class SubscriberCsvImporterTest extends TestCase private SubscriberRepository&MockObject $subscriberRepositoryMock; private CsvToDtoImporter&MockObject $csvImporterMock; private SubscriberAttributeDefinitionRepository&MockObject $attributeDefinitionRepositoryMock; + private UserBlacklistRepository&MockObject $blacklistRepositoryMock; + private SubscriptionManager&MockObject $subscriptionManagerMock; + private MessageBusInterface&MockObject $messageBusMock; private SubscriberCsvImporter $subject; protected function setUp(): void { $this->subscriberManagerMock = $this->createMock(SubscriberManager::class); $this->attributeManagerMock = $this->createMock(SubscriberAttributeManager::class); - $subscriptionManagerMock = $this->createMock(SubscriptionManager::class); + $this->subscriptionManagerMock = $this->createMock(SubscriptionManager::class); $this->subscriberRepositoryMock = $this->createMock(SubscriberRepository::class); $this->csvImporterMock = $this->createMock(CsvToDtoImporter::class); $this->attributeDefinitionRepositoryMock = $this->createMock(SubscriberAttributeDefinitionRepository::class); + $this->messageBusMock = $this->createMock(MessageBusInterface::class); $entityManager = $this->createMock(EntityManagerInterface::class); + $this->blacklistRepositoryMock = $this->createMock(UserBlacklistRepository::class); $this->subject = new SubscriberCsvImporter( subscriberManager: $this->subscriberManagerMock, attributeManager: $this->attributeManagerMock, - subscriptionManager: $subscriptionManagerMock, + subscriptionManager: $this->subscriptionManagerMock, subscriberRepository: $this->subscriberRepositoryMock, csvToDtoImporter: $this->csvImporterMock, entityManager: $entityManager, translator: new Translator('en'), - messageBus: $this->createMock(MessageBusInterface::class), + messageBus: $this->messageBusMock, subscriberHistoryManager: $this->createMock(SubscriberHistoryManager::class), + blacklistRepository: $this->blacklistRepositoryMock, ); } @@ -499,4 +506,88 @@ public function testImportCreatesNewWhenNeitherEmailNorForeignKeyFound(): void unlink($tempFile); } + + public function testImportCountsSubscriberWhoseEmailIsInBlacklistTable(): void + { + $tempFile = tempnam(sys_get_temp_dir(), 'csv_test_blacklist'); + file_put_contents($tempFile, "email,confirmed,html_email,blacklisted,disabled\nbl@example.com,0,1,0,0\n"); + + $uploadedFile = $this->createMock(UploadedFile::class); + $uploadedFile->method('getRealPath')->willReturn($tempFile); + + $this->subscriberRepositoryMock->method('findOneByEmail')->willReturn(null); + + $dto = new ImportSubscriberDto( + email: 'bl@example.com', + confirmed: false, + blacklisted: false, + htmlEmail: true, + disabled: false, + ); + $this->csvImporterMock->method('parseAndValidate')->willReturn(['valid' => [$dto], 'errors' => []]); + + $created = $this->createMock(Subscriber::class); + $created->method('getId')->willReturn(7); + $created->method('getEmail')->willReturn('bl@example.com'); + $created->method('isBlacklisted')->willReturn(false); + $created->expects($this->once())->method('setBlacklisted')->with(true); + $this->subscriberManagerMock->method('createFromImport')->willReturn($created); + + $this->blacklistRepositoryMock + ->expects($this->once()) + ->method('isEmailBlacklisted') + ->with('bl@example.com') + ->willReturn(true); + + $this->subscriptionManagerMock->expects($this->never())->method('addSubscriberToAList'); + $this->messageBusMock->expects($this->never())->method('dispatch'); + + $result = $this->subject->importFromCsv( + $uploadedFile, + new SubscriberImportOptions(updateExisting: false, listIds: [1, 2]) + ); + + $this->assertSame(1, $result['blacklisted']); + + unlink($tempFile); + } + + public function testImportIgnoresBlacklistedFlagWhenEmailIsNotInBlacklistTable(): void + { + $tempFile = tempnam(sys_get_temp_dir(), 'csv_test_flag_only'); + file_put_contents($tempFile, "email,confirmed,html_email,blacklisted,disabled\nflag@example.com,1,1,1,0\n"); + + $uploadedFile = $this->createMock(UploadedFile::class); + $uploadedFile->method('getRealPath')->willReturn($tempFile); + + $this->subscriberRepositoryMock->method('findOneByEmail')->willReturn(null); + + $dto = new ImportSubscriberDto( + email: 'flag@example.com', + confirmed: true, + blacklisted: true, + htmlEmail: true, + disabled: false, + ); + $this->csvImporterMock->method('parseAndValidate')->willReturn(['valid' => [$dto], 'errors' => []]); + + $created = $this->createMock(Subscriber::class); + $created->method('getId')->willReturn(8); + $created->method('getEmail')->willReturn('flag@example.com'); + $created->method('isBlacklisted')->willReturn(true); + $this->subscriberManagerMock->method('createFromImport')->willReturn($created); + + $this->blacklistRepositoryMock->method('isEmailBlacklisted')->willReturn(false); + + $this->subscriptionManagerMock->expects($this->once())->method('addSubscriberToAList'); + + $result = $this->subject->importFromCsv( + $uploadedFile, + new SubscriberImportOptions(updateExisting: false, listIds: [1]) + ); + + $this->assertSame(0, $result['blacklisted']); + + unlink($tempFile); + } } From c517d5332578148dc02f2ea711a10facef0cfc12 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Sun, 1 Nov 2026 20:53:51 +0400 Subject: [PATCH 18/30] feat: blacklisting logic in SubscriberCsvExporter to re-verify against the blacklist table --- src/Domain/Subscription/Model/Subscriber.php | 1 - .../Subscription/Service/SubscriberCsvExporter.php | 12 +++++++++++- .../Service/SubscriberCsvExporterTest.php | 4 +++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/Domain/Subscription/Model/Subscriber.php b/src/Domain/Subscription/Model/Subscriber.php index e29a9b7d..7e8ce0e4 100644 --- a/src/Domain/Subscription/Model/Subscriber.php +++ b/src/Domain/Subscription/Model/Subscriber.php @@ -174,7 +174,6 @@ public function setConfirmed(bool $confirmed): self return $this; } - // todo: check where is this used and if should be replaced by isBlacklisted with grace time public function isBlacklisted(): bool { return $this->blacklisted; diff --git a/src/Domain/Subscription/Service/SubscriberCsvExporter.php b/src/Domain/Subscription/Service/SubscriberCsvExporter.php index 48802fe0..9429349b 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvExporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvExporter.php @@ -9,6 +9,7 @@ use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition; use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeDefinitionRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; +use PhpList\Core\Domain\Subscription\Repository\UserBlacklistRepository; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberAttributeManager; use PhpList\Core\Domain\Subscription\Service\Resolver\AttributeValueResolver; use Psr\Log\LoggerInterface; @@ -27,6 +28,7 @@ public function __construct( private readonly SubscriberRepository $subscriberRepository, private readonly SubscriberAttributeDefinitionRepository $definitionRepository, private readonly LoggerInterface $logger, + private readonly UserBlacklistRepository $blacklistRepository, ) { } @@ -253,6 +255,14 @@ private function configureResponse(BinaryFileResponse $response): Response return $response; } + /** + * re-verify the flag against the blacklist table so stale flags are not exported. + */ + private function isBlacklisted(Subscriber $subscriber): bool + { + return $subscriber->isBlacklisted() || $this->blacklistRepository->isEmailBlacklisted($subscriber->getEmail()); + } + /** @SuppressWarnings("CyclomaticComplexity") */ private function normalizerSubscriberData(Subscriber $subscriber): array { @@ -260,7 +270,7 @@ private function normalizerSubscriberData(Subscriber $subscriber): array 'id' => $subscriber->getId(), 'email' => $subscriber->getEmail(), 'confirmed' => $subscriber->isConfirmed() ? '1' : '0', - 'blacklisted' => $subscriber->isBlacklisted() ? '1' : '0', + 'blacklisted' => $this->isBlacklisted($subscriber) ? '1' : '0', 'bounceCount' => $subscriber->getBounceCount(), 'createdAt' => $subscriber->getCreatedAt()?->format('Y-m-d H:i:s') ?? '', 'updatedAt' => $subscriber->getUpdatedAt()->format('Y-m-d H:i:s'), diff --git a/tests/Unit/Domain/Subscription/Service/SubscriberCsvExporterTest.php b/tests/Unit/Domain/Subscription/Service/SubscriberCsvExporterTest.php index 3c7ae4b3..5bb70054 100644 --- a/tests/Unit/Domain/Subscription/Service/SubscriberCsvExporterTest.php +++ b/tests/Unit/Domain/Subscription/Service/SubscriberCsvExporterTest.php @@ -11,6 +11,7 @@ use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeValue; use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeDefinitionRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; +use PhpList\Core\Domain\Subscription\Repository\UserBlacklistRepository; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberAttributeManager; use PhpList\Core\Domain\Subscription\Service\Resolver\AttributeValueResolver; use PhpList\Core\Domain\Subscription\Service\SubscriberCsvExporter; @@ -37,7 +38,8 @@ protected function setUp(): void $this->createMock(AttributeValueResolver::class), $this->subscriberRepositoryMock, $this->attributeDefinitionRepositoryMock, - $this->createMock(LoggerInterface::class) + $this->createMock(LoggerInterface::class), + $this->createMock(UserBlacklistRepository::class), ); } From 5e71eb4357564f679b6135020a9875ed03ac9a07 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Sun, 1 Nov 2026 20:57:46 +0400 Subject: [PATCH 19/30] fix: test --- .../Provider/DefaultConfigProviderTest.php | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php b/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php index c296ccea..b0aab919 100644 --- a/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php +++ b/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php @@ -8,7 +8,6 @@ use PhpList\Core\Domain\Configuration\Service\Provider\DefaultConfigProvider; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; -use ReflectionClass; use Symfony\Contracts\Translation\TranslatorInterface; /** @@ -87,24 +86,23 @@ public function testUnsubscribeUrlDefaults(): void public function testTranslatorIsUsedOnlyOnFirstInit(): void { + $calls = 0; $this->translator - ->expects($this->atLeastOnce()) ->method('trans') - ->willReturnArgument(0); + ->willReturnCallback(function (string $id) use (&$calls): string { + $calls++; + + return $id; + }); $this->provider->get(ConfigOption::AdminAddress); + $callsAfterInit = $calls; + $this->assertGreaterThan(0, $callsAfterInit); // Subsequent calls should not trigger init again - $translator = $this->createMock(TranslatorInterface::class); - $translator - ->expects($this->never()) - ->method('trans'); - - $reflection = new ReflectionClass($this->provider); - $prop = $reflection->getProperty('translator'); - - $prop->setValue($this->provider, $translator); $this->provider->get(ConfigOption::UnsubscribeUrl); $this->provider->has(ConfigOption::BlacklistUrl); + + $this->assertSame($callsAfterInit, $calls); } public function testKnownKeysHaveReasonableTypes(): void From 1277dec6e133affff2d62cfadfe210c2328888b6 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Sun, 1 Nov 2026 21:00:16 +0400 Subject: [PATCH 20/30] ref: update comments in ExternalImageService and RemotePageFetcher for clarity --- src/Domain/Common/RemotePageFetcher.php | 2 +- .../Common/Service/ExternalImageService.php | 2 +- .../Service/Manager/MessageDataManager.php | 28 +------------------ 3 files changed, 3 insertions(+), 29 deletions(-) diff --git a/src/Domain/Common/RemotePageFetcher.php b/src/Domain/Common/RemotePageFetcher.php index fdeb01db..5605c96d 100644 --- a/src/Domain/Common/RemotePageFetcher.php +++ b/src/Domain/Common/RemotePageFetcher.php @@ -52,7 +52,7 @@ public function __invoke(string $url, array $userData): string $cacheUrl = $this->urlCacheRepository->findByUrlAndLastModified($url, $lastModified); $content = $cacheUrl?->getContent(); if ($cacheUrl) { - // todo: check what the page should be for this log + // no page context here; phplist3 logs 'unknown page' in that case $this->eventLogManager->log(page: 'unknown page', entry: $url . ' was cached in database'); } else { $content = $this->fetchUrlDirect($url); diff --git a/src/Domain/Common/Service/ExternalImageService.php b/src/Domain/Common/Service/ExternalImageService.php index 63d9f35c..4f2314e2 100644 --- a/src/Domain/Common/Service/ExternalImageService.php +++ b/src/Domain/Common/Service/ExternalImageService.php @@ -91,7 +91,7 @@ private function removeOldFilesInCache(): void if ($cacheFile === false) { break; } - // todo: make sure that this is what we need + // skip hidden files and the . and .. entries, as phplist3 does if (!str_starts_with($cacheFile, '.')) { // phpcs:ignore Generic.PHP.NoSilencedErrors $cfmt = @filemtime($this->externalCacheDir . '/' . $cacheFile); diff --git a/src/Domain/Messaging/Service/Manager/MessageDataManager.php b/src/Domain/Messaging/Service/Manager/MessageDataManager.php index 760fda60..b8dab1c2 100644 --- a/src/Domain/Messaging/Service/Manager/MessageDataManager.php +++ b/src/Domain/Messaging/Service/Manager/MessageDataManager.php @@ -34,33 +34,7 @@ public function setMessageData(Message $campaign, string $name, mixed $value): v $value = $this->normalizeValueByName($name, $value); - // todo: remove this once we have a proper way to handle targetlists -// if ($name === 'targetlist' && is_array($value)) { -// $this->listMessageRepository->removeAllListAssociationsForMessage($campaign); -// -// if (!empty($value['all']) || !empty($value['allactive'])) { -// // todo: should be with $GLOBALS['subselect'] filter for access control -// foreach ($this->subscriberListRepository->getAllActive() as $list) { -// $listMessage = (new ListMessage()) -// ->setMessage($campaign) -// ->setList($list); -// $this->listMessageRepository->persist($listMessage); -// } -// // once we used "all" to set all, unset it, to avoid confusion trying to unselect lists -// unset($value['all']); -// } else { -// foreach ($value as $listId => $val) { -// // see #16940 - ignore a list called "unselect" which is there to allow unselecting all -// if ($listId !== 'unselect') { -// $list = $this->subscriberListRepository->find($listId); -// $listMessage = (new ListMessage()) -// ->setMessage($campaign) -// ->setList($list); -// $this->listMessageRepository->persist($listMessage); -// } -// } -// } -// } + // todo: targetLists (not only here) should be with $GLOBALS['subselect'] filter for access control if (is_array($value) || is_object($value)) { $value = 'SER:' . serialize($value); From cbdf76c863c846f7ed400eb9d8152ea950c243b9 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 2 Nov 2026 15:50:15 +0400 Subject: [PATCH 21/30] feat: implement link tracking in CampaignMailContentBuilder after placeholders are expanded --- .../Analytics/Service/LinkTrackService.php | 38 +++--- .../CampaignProcessorTestMessageHandler.php | 13 +- .../Messaging/Service/CampaignEmailSender.php | 10 +- .../CampaignMailContentBuilder.php | 31 ++++- .../Service/MessageProcessingPreparator.php | 25 ---- .../Service/LinkTrackServiceTest.php | 114 +++++++++--------- .../Service/CampaignEmailSenderTest.php | 6 - .../CampaignMailContentBuilderTest.php | 56 +++++++++ .../MessageProcessingPreparatorTest.php | 54 --------- 9 files changed, 162 insertions(+), 185 deletions(-) diff --git a/src/Domain/Analytics/Service/LinkTrackService.php b/src/Domain/Analytics/Service/LinkTrackService.php index 05a9baa6..5eda1067 100644 --- a/src/Domain/Analytics/Service/LinkTrackService.php +++ b/src/Domain/Analytics/Service/LinkTrackService.php @@ -9,8 +9,7 @@ use PhpList\Core\Domain\Analytics\Repository\LinkTrackForwardRepository; use PhpList\Core\Domain\Analytics\Repository\LinkTrackMlRepository; use PhpList\Core\Domain\Analytics\Repository\LinkTrackRepository; -use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; -use PhpList\Core\Domain\Messaging\Model\Message; +use PhpList\Core\Domain\Messaging\Repository\MessageRepository; use PhpList\Core\Domain\Subscription\Model\Subscriber; /** @@ -33,6 +32,7 @@ public function __construct( private readonly LinkTrackRepository $linkTrackRepository, private readonly LinkTrackForwardRepository $linkTrackForwardRepository, private readonly LinkTrackMlRepository $linkTrackMlRepository, + private readonly MessageRepository $messageRepository, private readonly LinkTrackTokenService $tokenService, private readonly ParameterProvider $paramProvider, ) { @@ -50,42 +50,42 @@ public function isClickTrackingEnabled(): bool } /** - * Returns a copy of the content in which the links of the HTML parts (content, HTML footer) and of the text - * parts (text content, text footer) point to the click-tracking endpoint for this subscriber. + * Replaces the links of the final HTML and text bodies with click-tracking links for this subscriber. It has to + * run once all placeholders are expanded, so that template links and links coming from placeholders (unsubscribe, + * preferences, ...) are tracked too, like phplist3 does. * * Links are left alone when they are not http(s)/ftp, are longer than the forward url column, still contain * placeholders (the redirect could not expand them), or are shown as text that itself contains a URL (the * rewrite would look like phishing). + * + * @return array{0: string, 1: string} the tracked HTML and text */ - public function trackLinks( - MessagePrecacheDto $content, - Message $campaign, - Subscriber $subscriber - ): MessagePrecacheDto { + public function trackLinks(string $html, string $text, int $campaignId, Subscriber $subscriber): array + { + $campaign = $this->isClickTrackingEnabled() ? $this->messageRepository->findById($campaignId) : null; + if ($campaign === null) { + return [$html, $text]; + } + $messageUuid = (string) $campaign->getUuid(); $subscriberUuid = $subscriber->getUuid(); if (!$this->isClickTrackingEnabled() || $messageUuid === '' || $subscriberUuid === '') { - return $content; + return [$html, $text]; } - $tracked = clone $content; $forwardIds = []; $trackUrl = function (string $url, string $type) use ($messageUuid, $subscriberUuid, &$forwardIds): ?string { return $this->createTrackingUrl($url, $type, $messageUuid, $subscriberUuid, $forwardIds); }; - $tracked->content = $this->rewriteHtml($tracked->content, $trackUrl); - $tracked->htmlFooter = $this->rewriteHtml($tracked->htmlFooter, $trackUrl); - $tracked->textContent = $this->rewriteText($tracked->textContent, $trackUrl); - if ($tracked->textFooter !== null) { - $tracked->textFooter = $this->rewriteText($tracked->textFooter, $trackUrl); - } + $html = $this->rewriteHtml($html, $trackUrl); + $text = $this->rewriteText($text, $trackUrl); foreach (array_keys($forwardIds) as $forwardId) { - $this->linkTrackMlRepository->incrementTotal((int) $campaign->getId(), $forwardId); + $this->linkTrackMlRepository->incrementTotal($campaignId, $forwardId); } - return $tracked; + return [$html, $text]; } /** @param callable(string, string): ?string $trackUrl */ diff --git a/src/Domain/Messaging/MessageHandler/CampaignProcessorTestMessageHandler.php b/src/Domain/Messaging/MessageHandler/CampaignProcessorTestMessageHandler.php index 61f47384..21895457 100644 --- a/src/Domain/Messaging/MessageHandler/CampaignProcessorTestMessageHandler.php +++ b/src/Domain/Messaging/MessageHandler/CampaignProcessorTestMessageHandler.php @@ -18,7 +18,6 @@ use PhpList\Core\Domain\Messaging\Service\MailSizeChecker; use PhpList\Core\Domain\Messaging\Service\MessageDataLoader; use PhpList\Core\Domain\Messaging\Service\MessagePrecacheService; -use PhpList\Core\Domain\Messaging\Service\MessageProcessingPreparator; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Service\Provider\SubscriberProvider; use Psr\Log\LoggerInterface; @@ -41,7 +40,6 @@ class CampaignProcessorTestMessageHandler public function __construct( private readonly MailerInterface $mailer, private readonly SubscriberProvider $subscriberProvider, - private readonly MessageProcessingPreparator $messagePreparator, private readonly LoggerInterface $logger, private readonly CacheInterface $cache, private readonly TranslatorInterface $translator, @@ -89,17 +87,10 @@ private function handleEmailSending( Subscriber $subscriber, MessagePrecacheDto $precachedContent, ): void { - // 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 - ); - try { $result = $this->campaignEmailBuilder->buildCampaignEmail( messageId: $campaign->getId(), - data: $processed, + data: $precachedContent, toEmail: $subscriber->getEmail(), skipBlacklistCheck: false, inBlast: true, @@ -132,7 +123,7 @@ private function handleEmailSending( ); $envelope = new Envelope( - sender: new Address($this->bounceEmail, 'PHPList'), + sender: new Address($this->bounceEmail), recipients: [new Address($email->getTo()[0]->getAddress())], ); $this->mailer->send(message: $email, envelope: $envelope); diff --git a/src/Domain/Messaging/Service/CampaignEmailSender.php b/src/Domain/Messaging/Service/CampaignEmailSender.php index 7cb590b0..e4babbfb 100644 --- a/src/Domain/Messaging/Service/CampaignEmailSender.php +++ b/src/Domain/Messaging/Service/CampaignEmailSender.php @@ -36,7 +36,6 @@ public function __construct( private readonly EmailBuilder $campaignEmailBuilder, private readonly RateLimitedCampaignMailer $rateLimitedCampaignMailer, private readonly MailSizeChecker $mailSizeChecker, - private readonly MessageProcessingPreparator $messagePreparator, private readonly MessageRepository $messageRepository, private readonly MessageStatusUpdater $messageStatusUpdater, private readonly SystemNotificationMailer $notificationMailer, @@ -54,17 +53,10 @@ public function send( UserMessage $userMessage, MessagePrecacheDto $precachedContent, ): void { - // 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 - ); - try { $result = $this->campaignEmailBuilder->buildCampaignEmail( messageId: $campaign->getId(), - data: $processed, + data: $precachedContent, toEmail: $subscriber->getEmail(), skipBlacklistCheck: false, inBlast: true, diff --git a/src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php b/src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php index c7398a20..44ed2eb5 100644 --- a/src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php +++ b/src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Domain\Messaging\Service\Constructor; +use PhpList\Core\Domain\Analytics\Service\LinkTrackService; use PhpList\Core\Domain\Common\Html2Text; use PhpList\Core\Domain\Common\RemotePageFetcher; use PhpList\Core\Domain\Common\TextParser; @@ -28,6 +29,7 @@ public function __construct( private readonly Html2Text $html2Text, private readonly TextParser $textParser, private readonly MessagePlaceholderProcessor $placeholderProcessor, + private readonly LinkTrackService $linkTrackService, #[Autowire('%phplist.forward_personal_note_size%')] private readonly int $forwardPersonalNoteSize = 0, ) { } @@ -90,8 +92,15 @@ public function __invoke( forwardedBy: $forwardedBy, ); + [$htmlMessage, $textMessage] = $this->trackLinks( + htmlMessage: $htmlMessage, + textMessage: $textMessage, + receiver: $receiver, + campaignId: $campaignId, + forwardedBy: $forwardedBy, + ); + $htmlMessage = $this->ensureHtmlFormating(content: $htmlMessage, addDefaultStyle: $addDefaultStyle); - // todo: add link CLICKTRACK to $htmlMessage [$htmlMessage, $textMessage] = $this->ensureNoteInCaseOfForwarded( htmlMessage: $htmlMessage, @@ -102,6 +111,26 @@ public function __invoke( return [$htmlMessage, $textMessage]; } + /** + * As in phplist3, links are tracked once all placeholders are expanded, but not in forwarded messages as the + * receivers are not necessarily subscribers. + * + * @return array{0: string, 1: string} + */ + private function trackLinks( + string $htmlMessage, + string $textMessage, + Subscriber $receiver, + ?int $campaignId, + ?Subscriber $forwardedBy, + ): array { + if ($campaignId === null || $forwardedBy !== null) { + return [$htmlMessage, $textMessage]; + } + + return $this->linkTrackService->trackLinks($htmlMessage, $textMessage, $campaignId, $receiver); + } + private function replaceUserSpecificRemoteContent( MessagePrecacheDto $messagePrecacheDto, Subscriber $subscriber, diff --git a/src/Domain/Messaging/Service/MessageProcessingPreparator.php b/src/Domain/Messaging/Service/MessageProcessingPreparator.php index cfbcc148..acdd1914 100644 --- a/src/Domain/Messaging/Service/MessageProcessingPreparator.php +++ b/src/Domain/Messaging/Service/MessageProcessingPreparator.php @@ -4,10 +4,7 @@ namespace PhpList\Core\Domain\Messaging\Service; -use PhpList\Core\Domain\Analytics\Service\LinkTrackService; -use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; use PhpList\Core\Domain\Messaging\Repository\MessageRepository; -use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Contracts\Translation\TranslatorInterface; @@ -17,7 +14,6 @@ class MessageProcessingPreparator public function __construct( private readonly SubscriberRepository $subscriberRepository, private readonly MessageRepository $messageRepository, - private readonly LinkTrackService $linkTrackService, private readonly TranslatorInterface $translator, ) { } @@ -51,25 +47,4 @@ public function ensureCampaignsHaveUuid(OutputInterface $output): void } } } - - /** - * Replaces the links of the cached message content with click-tracking links for this subscriber. - * Returns the content unchanged when click tracking is off or the campaign is unknown. - */ - public function processMessageLinks( - int $campaignId, - MessagePrecacheDto $cachedMessageDto, - Subscriber $subscriber - ): MessagePrecacheDto { - if (!$this->linkTrackService->isClickTrackingEnabled()) { - return $cachedMessageDto; - } - - $campaign = $this->messageRepository->findById($campaignId); - if ($campaign === null) { - return $cachedMessageDto; - } - - return $this->linkTrackService->trackLinks($cachedMessageDto, $campaign, $subscriber); - } } diff --git a/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php b/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php index 7457c0bb..ace74a03 100644 --- a/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php +++ b/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php @@ -11,8 +11,8 @@ use PhpList\Core\Domain\Analytics\Repository\LinkTrackRepository; use PhpList\Core\Domain\Analytics\Service\LinkTrackService; use PhpList\Core\Domain\Analytics\Service\LinkTrackTokenService; -use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; use PhpList\Core\Domain\Messaging\Model\Message; +use PhpList\Core\Domain\Messaging\Repository\MessageRepository; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -21,6 +21,7 @@ class LinkTrackServiceTest extends TestCase { private LinkTrackForwardRepository&MockObject $forwardRepository; private LinkTrackMlRepository&MockObject $mlRepository; + private MessageRepository&MockObject $messageRepository; private LinkTrackTokenService $tokenService; private Message&MockObject $campaign; private Subscriber $subscriber; @@ -31,6 +32,7 @@ protected function setUp(): void { $this->forwardRepository = $this->createMock(LinkTrackForwardRepository::class); $this->mlRepository = $this->createMock(LinkTrackMlRepository::class); + $this->messageRepository = $this->createMock(MessageRepository::class); $this->tokenService = new LinkTrackTokenService( new ParameterProvider([]), 'https://example.com/api/v2', @@ -40,6 +42,7 @@ protected function setUp(): void $this->campaign = $this->createMock(Message::class); $this->campaign->method('getId')->willReturn(7); $this->campaign->method('getUuid')->willReturn('campaign-uuid'); + $this->messageRepository->method('findById')->with(7)->willReturn($this->campaign); $this->subscriber = new Subscriber('alice@example.com'); $this->subscriber->setUuid('subscriber-uuid'); @@ -70,6 +73,7 @@ private function createService(bool $clickTrack = true): LinkTrackService $this->createMock(LinkTrackRepository::class), $this->forwardRepository, $this->mlRepository, + $this->messageRepository, $this->tokenService, new ParameterProvider(['click_track' => $clickTrack]), ); @@ -94,50 +98,59 @@ public function testIsClickTrackingEnabledFollowsTheParameter(): void public function testContentIsReturnedUntouchedWhenClickTrackingIsDisabled(): void { - $content = new MessagePrecacheDto(); - $content->content = 'x'; + $html = 'x'; $this->forwardRepository->expects(self::never())->method('findOrCreateByUrl'); - $result = $this->createService(false)->trackLinks($content, $this->campaign, $this->subscriber); + $result = $this->createService(false)->trackLinks($html, 'https://example.com/', 7, $this->subscriber); - self::assertSame($content, $result); + self::assertSame([$html, 'https://example.com/'], $result); } public function testContentIsReturnedUntouchedWhenCampaignHasNoUuid(): void { $campaign = $this->createMock(Message::class); $campaign->method('getUuid')->willReturn(null); - $content = new MessagePrecacheDto(); - $content->content = 'x'; + $repository = $this->createMock(MessageRepository::class); + $repository->method('findById')->willReturn($campaign); + $service = new LinkTrackService( + $this->createMock(LinkTrackRepository::class), + $this->forwardRepository, + $this->mlRepository, + $repository, + $this->tokenService, + new ParameterProvider(['click_track' => true]), + ); + $html = 'x'; - self::assertSame($content, $this->createService()->trackLinks($content, $campaign, $this->subscriber)); + self::assertSame([$html, ''], $service->trackLinks($html, '', 7, $this->subscriber)); } public function testHtmlLinksAreReplacedWithTrackingLinks(): void { - $content = new MessagePrecacheDto(); - $content->content = '

A ' + $html = '

A ' . "

"; - $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); + [$result] = $this->createService()->trackLinks($html, '', 7, $this->subscriber); - self::assertStringContainsString('A', $result->content); - self::assertStringContainsString("content); + self::assertStringContainsString('A', $result); + self::assertStringContainsString("createdForwards, 'stored url is decoded'); self::assertArrayHasKey('http://example.org/b', $this->createdForwards); } public function testTrackingLinkIdentifiesForwardCampaignAndSubscriber(): void { - $content = new MessagePrecacheDto(); - $content->content = 'A'; - - $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); + [$result] = $this->createService()->trackLinks( + 'A', + '', + 7, + $this->subscriber + ); - preg_match('~tid=([A-Za-z0-9_-]+)~', $result->content, $match); + preg_match('~tid=([A-Za-z0-9_-]+)~', $result, $match); $token = $this->tokenService->parse($match[1]); self::assertNotNull($token); self::assertTrue($token->isHtml()); @@ -158,23 +171,23 @@ public function testLinksThatMustNotBeTrackedAreLeftAlone(): void 'already tracked', 'no href', ]); - $content = new MessagePrecacheDto(); - $content->content = $html; $this->forwardRepository->expects(self::never())->method('findOrCreateByUrl'); $this->mlRepository->expects(self::never())->method('incrementTotal'); - $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); + [$result] = $this->createService()->trackLinks($html, '', 7, $this->subscriber); - self::assertSame($html, $result->content); + self::assertSame($html, $result); } public function testPersonalUidAndSessionParametersAreStrippedAndMarkTheForwardPersonal(): void { - $content = new MessagePrecacheDto(); - $content->content = 'P'; - - $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); + $this->createService()->trackLinks( + 'P', + '', + 7, + $this->subscriber + ); self::assertSame( ['https://example.com/p?a=1#top' => ['url' => 'https://example.com/p?a=1#top', 'personalise' => true]], @@ -184,42 +197,34 @@ public function testPersonalUidAndSessionParametersAreStrippedAndMarkTheForwardP public function testTextUrlsAreReplacedWithTextTokens(): void { - $content = new MessagePrecacheDto(); - $content->textContent = "Read https://example.com/a. Or see http://example.org/b?x=1\nbye"; - $content->textFooter = 'More: https://example.com/a'; + $text = "Read https://example.com/a. Or see http://example.org/b?x=1\nbye\nMore: https://example.com/a"; - $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); + [, $result] = $this->createService()->trackLinks('', $text, 7, $this->subscriber); self::assertMatchesRegularExpression( '~^Read https://example.com/api/v2/lt\?tid=[A-Za-z0-9_-]+\. Or see https://example.com/api/v2/lt\?tid=~', - $result->textContent + $result ); - preg_match('~tid=([A-Za-z0-9_-]+)~', $result->textContent, $match); + preg_match('~tid=([A-Za-z0-9_-]+)~', $result, $match); self::assertSame('T', $this->tokenService->parse($match[1])->type); - self::assertSame( - [$this->trackedForwardUuids($result->textContent)[0]], - $this->trackedForwardUuids((string) $result->textFooter), - 'the same url gets the same forward' - ); - self::assertSame("\nbye", substr($result->textContent, -4)); + $uuids = $this->trackedForwardUuids($result); + self::assertSame($uuids[0], $uuids[2], 'the same url gets the same forward'); + self::assertSame("\nbye\n", substr($result, strpos($result, "\n"), 5)); } - public function testFootersAreTrackedToo(): void + public function testLinksFromExpandedPlaceholdersAreTracked(): void { - $content = new MessagePrecacheDto(); - $content->htmlFooter = 'Footer'; + $html = 'Unsubscribe'; - $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); + [$result] = $this->createService()->trackLinks($html, '', 7, $this->subscriber); - self::assertSame(['forward-uuid-1'], $this->trackedForwardUuids($result->htmlFooter)); + self::assertSame(['forward-uuid-1'], $this->trackedForwardUuids($result)); } public function testTotalIsIncrementedOncePerDistinctLinkOfTheEmail(): void { - $content = new MessagePrecacheDto(); - $content->content = '12' + $html = '12' . '3'; - $content->textContent = 'https://example.com/a'; $incremented = []; $this->mlRepository->expects(self::exactly(2)) @@ -228,19 +233,8 @@ public function testTotalIsIncrementedOncePerDistinctLinkOfTheEmail(): void $incremented[] = [$messageId, $forwardId]; }); - $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); + $this->createService()->trackLinks($html, 'https://example.com/a', 7, $this->subscriber); self::assertSame([[7, 1], [7, 2]], $incremented); } - - public function testTheGivenContentIsNotModified(): void - { - $content = new MessagePrecacheDto(); - $content->content = 'A'; - - $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); - - self::assertNotSame($content, $result); - self::assertSame('A', $content->content); - } } diff --git a/tests/Unit/Domain/Messaging/Service/CampaignEmailSenderTest.php b/tests/Unit/Domain/Messaging/Service/CampaignEmailSenderTest.php index b673f442..22ef8593 100644 --- a/tests/Unit/Domain/Messaging/Service/CampaignEmailSenderTest.php +++ b/tests/Unit/Domain/Messaging/Service/CampaignEmailSenderTest.php @@ -19,7 +19,6 @@ use PhpList\Core\Domain\Messaging\Service\Builder\EmailBuilder; use PhpList\Core\Domain\Messaging\Service\CampaignEmailSender; use PhpList\Core\Domain\Messaging\Service\MailSizeChecker; -use PhpList\Core\Domain\Messaging\Service\MessageProcessingPreparator; use PhpList\Core\Domain\Messaging\Service\MessageStatusUpdater; use PhpList\Core\Domain\Messaging\Service\RateLimitedCampaignMailer; use PhpList\Core\Domain\Messaging\Service\SystemNotificationMailer; @@ -37,7 +36,6 @@ class CampaignEmailSenderTest extends TestCase private EmailBuilder|MockObject $campaignEmailBuilder; private RateLimitedCampaignMailer|MockObject $rateLimitedCampaignMailer; private MailSizeChecker|MockObject $mailSizeChecker; - private MessageProcessingPreparator|MockObject $messagePreparator; private MessageRepository|MockObject $messageRepository; private MessageStatusUpdater|MockObject $messageStatusUpdater; private SystemNotificationMailer|MockObject $notificationMailer; @@ -57,7 +55,6 @@ protected function setUp(): void $this->campaignEmailBuilder = $this->createMock(EmailBuilder::class); $this->rateLimitedCampaignMailer = $this->createMock(RateLimitedCampaignMailer::class); $this->mailSizeChecker = $this->createMock(MailSizeChecker::class); - $this->messagePreparator = $this->createMock(MessageProcessingPreparator::class); $this->messageRepository = $this->createMock(MessageRepository::class); $this->messageStatusUpdater = $this->createMock(MessageStatusUpdater::class); $this->notificationMailer = $this->createMock(SystemNotificationMailer::class); @@ -72,7 +69,6 @@ protected function setUp(): void $this->campaignEmailBuilder, $this->rateLimitedCampaignMailer, $this->mailSizeChecker, - $this->messagePreparator, $this->messageRepository, $this->messageStatusUpdater, $this->notificationMailer, @@ -92,8 +88,6 @@ protected function setUp(): void $this->subscriber->method('getEmail')->willReturn('test@example.com'); $this->userMessage = $this->createMock(UserMessage::class); - - $this->messagePreparator->method('processMessageLinks')->willReturn($this->precached); } public function testSendMarksSentAndIncrementsCountsOnSuccess(): void diff --git a/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php b/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php index df8afd18..ad4cde61 100644 --- a/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php +++ b/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Tests\Unit\Domain\Messaging\Service\Constructor; +use PhpList\Core\Domain\Analytics\Service\LinkTrackService; use PhpList\Core\Domain\Common\Html2Text; use PhpList\Core\Domain\Common\RemotePageFetcher; use PhpList\Core\Domain\Common\TextParser; @@ -28,6 +29,7 @@ class CampaignMailContentBuilderTest extends TestCase private Html2Text&MockObject $html2Text; private TextParser&MockObject $textParser; private MessagePlaceholderProcessor&MockObject $placeholderProcessor; + private LinkTrackService&MockObject $linkTrackService; protected function setUp(): void { @@ -47,6 +49,10 @@ protected function setUp(): void ->onlyMethods(['__invoke']) ->getMock(); $this->placeholderProcessor = $this->createMock(MessagePlaceholderProcessor::class); + $this->linkTrackService = $this->createMock(LinkTrackService::class); + $this->linkTrackService->method('trackLinks')->willReturnCallback( + static fn (string $html, string $text): array => [$html, $text] + ); $this->configProvider ->method('getValue') @@ -67,6 +73,7 @@ private function makeBuilder(): CampaignMailContentBuilder html2Text: $this->html2Text, textParser: $this->textParser, placeholderProcessor: $this->placeholderProcessor, + linkTrackService: $this->linkTrackService, ); } @@ -276,4 +283,53 @@ static function (...$args): string { 'No text content provided and html2text not used when htmlFormatted and template present' ); } + + private function makeSubscriber(): Subscriber&MockObject + { + $subscriber = $this->getMockBuilder(Subscriber::class) + ->disableOriginalConstructor() + ->onlyMethods(['getId', 'getEmail']) + ->getMock(); + $subscriber->method('getId')->willReturn(5); + $subscriber->method('getEmail')->willReturn('user@example.com'); + $this->placeholderProcessor + ->method('process') + ->willReturnCallback(static fn (...$args): string => (string) $args[0] . '[expanded]'); + + return $subscriber; + } + + public function testLinksAreTrackedAfterPlaceholdersAreExpanded(): void + { + $subscriber = $this->makeSubscriber(); + $this->linkTrackService = $this->createMock(LinkTrackService::class); + $this->linkTrackService->expects($this->once()) + ->method('trackLinks') + ->with('

Hi

[expanded]', 'Hi[expanded]', 9, $subscriber) + ->willReturn(['

tracked

', 'tracked text']); + + $dto = new MessagePrecacheDto(); + $dto->content = '

Hi

'; + $dto->textContent = 'Hi'; + $dto->htmlFormatted = true; + + [$html, $text] = ($this->makeBuilder())($dto, $subscriber, 9); + + $this->assertStringContainsString('

tracked

', $html); + $this->assertSame('tracked text', $text); + } + + public function testLinksAreNotTrackedInForwardedMessages(): void + { + $subscriber = $this->makeSubscriber(); + $this->linkTrackService = $this->createMock(LinkTrackService::class); + $this->linkTrackService->expects($this->never())->method('trackLinks'); + + $dto = new MessagePrecacheDto(); + $dto->content = '

Hi

'; + $dto->textContent = 'Hi'; + $dto->htmlFormatted = true; + + ($this->makeBuilder())($dto, $subscriber, 9, $this->createMock(Subscriber::class)); + } } diff --git a/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php b/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php index 08ffa892..f598d636 100644 --- a/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php +++ b/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php @@ -4,8 +4,6 @@ namespace PhpList\Core\Tests\Unit\Domain\Messaging\Service; -use PhpList\Core\Domain\Analytics\Service\LinkTrackService; -use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; use PhpList\Core\Domain\Messaging\Repository\MessageRepository; use PhpList\Core\Domain\Messaging\Service\MessageProcessingPreparator; use PhpList\Core\Domain\Subscription\Model\Subscriber; @@ -20,7 +18,6 @@ class MessageProcessingPreparatorTest extends TestCase { private SubscriberRepository&MockObject $subscriberRepository; private MessageRepository&MockObject $messageRepository; - private LinkTrackService&MockObject $linkTrackService; private OutputInterface&MockObject $output; private MessageProcessingPreparator $preparator; @@ -28,13 +25,11 @@ protected function setUp(): void { $this->subscriberRepository = $this->createMock(SubscriberRepository::class); $this->messageRepository = $this->createMock(MessageRepository::class); - $this->linkTrackService = $this->createMock(LinkTrackService::class); $this->output = $this->createMock(OutputInterface::class); $this->preparator = new MessageProcessingPreparator( subscriberRepository: $this->subscriberRepository, messageRepository: $this->messageRepository, - linkTrackService: $this->linkTrackService, translator: new Translator('en'), ); } @@ -114,53 +109,4 @@ public function testEnsureCampaignsHaveUuidWithCampaigns(): void $this->preparator->ensureCampaignsHaveUuid($this->output); } - - public function testProcessMessageLinksWhenClickTrackingIsDisabled(): void - { - $messageContent = new MessagePrecacheDto(); - $subscriber = $this->createMock(Subscriber::class); - - $this->linkTrackService->expects($this->once()) - ->method('isClickTrackingEnabled') - ->willReturn(false); - $this->messageRepository->expects($this->never())->method('findById'); - $this->linkTrackService->expects($this->never())->method('trackLinks'); - - $result = $this->preparator->processMessageLinks(1, $messageContent, $subscriber); - - $this->assertSame($messageContent, $result); - } - - public function testProcessMessageLinksWhenCampaignIsUnknown(): void - { - $messageContent = new MessagePrecacheDto(); - $subscriber = $this->createMock(Subscriber::class); - - $this->linkTrackService->method('isClickTrackingEnabled')->willReturn(true); - $this->messageRepository->expects($this->once())->method('findById')->with(1)->willReturn(null); - $this->linkTrackService->expects($this->never())->method('trackLinks'); - - $result = $this->preparator->processMessageLinks(1, $messageContent, $subscriber); - - $this->assertSame($messageContent, $result); - } - - public function testProcessMessageLinksReturnsTheContentWithTrackedLinks(): void - { - $messageContent = new MessagePrecacheDto(); - $tracked = new MessagePrecacheDto(); - $subscriber = $this->createMock(Subscriber::class); - $campaign = $this->createMock(Message::class); - - $this->linkTrackService->method('isClickTrackingEnabled')->willReturn(true); - $this->messageRepository->method('findById')->with(1)->willReturn($campaign); - $this->linkTrackService->expects($this->once()) - ->method('trackLinks') - ->with($messageContent, $campaign, $subscriber) - ->willReturn($tracked); - - $result = $this->preparator->processMessageLinks(1, $messageContent, $subscriber); - - $this->assertSame($tracked, $result); - } } From 3efa03d1d909c0ca30c7ba200a541c94bffe022c Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 2 Nov 2026 15:50:30 +0400 Subject: [PATCH 22/30] feat: add analytics tracking service for campaign links and integrate with link tracking --- config/services/services.yml | 6 + .../Service/AnalyticsTrackingService.php | 128 ++++++++++++ .../Analytics/Service/LinkClickService.php | 3 +- src/Domain/Analytics/Service/LinkRewriter.php | 71 +++++++ .../Analytics/Service/LinkTrackService.php | 89 ++++----- .../Service/Query/AnalyticsQueryInterface.php | 30 +++ .../Service/Query/GoogleAnalyticsQuery.php | 32 +++ .../Service/Query/MatomoAnalyticsQuery.php | 32 +++ .../Configuration/Model/ConfigOption.php | 1 + .../Model/Dto/MessagePrecacheDto.php | 2 + .../CampaignMailContentBuilder.php | 20 +- .../Service/MessagePrecacheService.php | 17 ++ .../Service/AnalyticsTrackingServiceTest.php | 182 ++++++++++++++++++ .../Service/LinkClickServiceTest.php | 30 +++ .../Service/LinkTrackServiceTest.php | 44 +++++ .../CampaignMailContentBuilderTest.php | 26 ++- 16 files changed, 648 insertions(+), 65 deletions(-) create mode 100644 src/Domain/Analytics/Service/AnalyticsTrackingService.php create mode 100644 src/Domain/Analytics/Service/LinkRewriter.php create mode 100644 src/Domain/Analytics/Service/Query/AnalyticsQueryInterface.php create mode 100644 src/Domain/Analytics/Service/Query/GoogleAnalyticsQuery.php create mode 100644 src/Domain/Analytics/Service/Query/MatomoAnalyticsQuery.php create mode 100644 tests/Unit/Domain/Analytics/Service/AnalyticsTrackingServiceTest.php diff --git a/config/services/services.yml b/config/services/services.yml index ae06bc01..bf40c389 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -61,6 +61,12 @@ services: autoconfigure: true public: true + PhpList\Core\Domain\Analytics\Service\LinkRewriter: + autowire: true + + PhpList\Core\Domain\Analytics\Service\AnalyticsTrackingService: + autowire: true + PhpList\Core\Domain\Analytics\Service\LinkTrackTokenService: autowire: true autoconfigure: true diff --git a/src/Domain/Analytics/Service/AnalyticsTrackingService.php b/src/Domain/Analytics/Service/AnalyticsTrackingService.php new file mode 100644 index 00000000..1525176d --- /dev/null +++ b/src/Domain/Analytics/Service/AnalyticsTrackingService.php @@ -0,0 +1,128 @@ +messageDataLoader)($campaign); + if (empty($messageData['google_track'])) { + return $url; + } + + return $this->addToUrl($url, $isHtml ? self::FORMAT_HTML : self::FORMAT_TEXT, $messageData); + } + + /** + * For a message that is sent without click tracking: adds the parameters to all links. + * + * @param array $messageData the campaign's analytics settings, see MessagePrecacheDto + * @return array{0: string, 1: string} the HTML and the text + */ + public function addToBodies(string $html, string $text, array $messageData): array + { + $html = $this->linkRewriter->rewriteHtml( + $html, + fn (string $url): string => $this->addToUrl($url, self::FORMAT_HTML, $messageData) + ); + $text = $this->linkRewriter->rewriteText( + $text, + fn (string $url): string => $this->addToUrl($url, self::FORMAT_TEXT, $messageData) + ); + + return [$html, $text]; + } + + /** + * Only http and https urls get parameters. + * + * @param array $messageData + */ + public function addToUrl(string $url, string $format, array $messageData): string + { + if (preg_match('/^https?:/i', $url) !== 1) { + return $url; + } + + $query = $this->getQuery(); + // a value the campaign never set gets its default, as phplist3's form would have saved it; a value that + // was set to empty stays empty, which leaves that parameter out + foreach ($query->editableParameters($messageData) as $name => $default) { + if (!array_key_exists($name, $messageData)) { + $messageData[$name] = $default; + } + } + + return $this->withParameters( + $url, + $query->trackingParameters($format, $messageData), + $query->prefix() + ); + } + + /** + * Adds the parameters to the url, taking off tracking parameters that are already on it. Empty parameters are + * not added. A fragment stays at the end. + * + * @param array $trackingParameters + */ + public function withParameters(string $url, array $trackingParameters, string $prefix): string + { + $fragment = ''; + $position = strpos($url, '#'); + if ($position !== false) { + $fragment = substr($url, $position); + $url = substr($url, 0, $position); + } + + if (str_contains($url, $prefix)) { + // the expression can leave a trailing ? or & behind + $pattern = sprintf('/%s.+?=[^&]+(&|$)/', preg_quote($prefix, '/')); + $url = rtrim((string) preg_replace($pattern, '', $url), '?&'); + } + + $trackingCode = http_build_query( + array_filter($trackingParameters, static fn ($value): bool => $value !== null && $value !== '') + ); + if ($trackingCode === '') { + return $url . $fragment; + } + + return $url . (str_contains($url, '?') ? '&' : '?') . $trackingCode . $fragment; + } + + private function getQuery(): AnalyticsQueryInterface + { + return $this->configProvider->getValue(ConfigOption::AnalyticTracker) === 'matomo' + ? new MatomoAnalyticsQuery() + : new GoogleAnalyticsQuery(); + } +} diff --git a/src/Domain/Analytics/Service/LinkClickService.php b/src/Domain/Analytics/Service/LinkClickService.php index 538f2dd3..2eb48dc0 100644 --- a/src/Domain/Analytics/Service/LinkClickService.php +++ b/src/Domain/Analytics/Service/LinkClickService.php @@ -31,6 +31,7 @@ public function __construct( private readonly SubscriberRepository $subscriberRepository, private readonly UserMessageRepository $userMessageRepository, private readonly UserMessageService $userMessageService, + private readonly AnalyticsTrackingService $analyticsTrackingService, ) { } @@ -81,7 +82,7 @@ public function trackClick(string $tid, ?string $signature, array $metadata): ?s $now ); - return $url; + return $this->analyticsTrackingService->addToClickedUrl($url, $message, $token->isHtml()); } private function resolveUrl(LinkTrackForward $forward, Subscriber $subscriber, ?UserMessage $userMessage): ?string diff --git a/src/Domain/Analytics/Service/LinkRewriter.php b/src/Domain/Analytics/Service/LinkRewriter.php new file mode 100644 index 00000000..c0accbea --- /dev/null +++ b/src/Domain/Analytics/Service/LinkRewriter.php @@ -0,0 +1,71 @@ +]*?[\s"\'])?)href=(["\'])(.*?)\2([^>]*>)(.*?)<\/a>/is'; + private const TEXT_URL_PATTERN = '#https?://[^\s<>},]+#i'; + + /** + * Rewrites the href of every anchor. Anchors whose visible text itself contains a URL are left alone, as + * pointing them elsewhere would look like phishing. + * + * @param callable(string): ?string $rewrite gets the decoded href, returns the new url or null to keep the link + */ + public function rewriteHtml(string $html, callable $rewrite): string + { + if ($html === '') { + return $html; + } + + return (string) preg_replace_callback( + self::HTML_ANCHOR_PATTERN, + function (array $match) use ($rewrite): string { + $linkText = strip_tags($match[5]); + if (stripos($linkText, 'http://') !== false || stripos($linkText, 'https://') !== false) { + return $match[0]; + } + + $href = html_entity_decode(trim($match[3]), ENT_QUOTES | ENT_HTML5); + $url = $rewrite($href); + if ($url === null) { + return $match[0]; + } + + return $match[1] . 'href=' . $match[2] . htmlspecialchars($url, ENT_QUOTES) . $match[2] + . $match[4] . $match[5] . ''; + }, + $html + ); + } + + /** + * Rewrites every http(s) URL in the text, leaving a trailing dot (the end of a sentence) where it is. + * + * @param callable(string): ?string $rewrite gets the url, returns the new url or null to keep it + */ + public function rewriteText(string $text, callable $rewrite): string + { + if ($text === '') { + return $text; + } + + return (string) preg_replace_callback( + self::TEXT_URL_PATTERN, + function (array $match) use ($rewrite): string { + $trailing = str_ends_with($match[0], '.') ? '.' : ''; + $link = substr($match[0], 0, strlen($match[0]) - strlen($trailing)); + $url = $rewrite($link); + + return $url === null ? $match[0] : $url . $trailing; + }, + $text + ); + } +} diff --git a/src/Domain/Analytics/Service/LinkTrackService.php b/src/Domain/Analytics/Service/LinkTrackService.php index 5eda1067..7153c40a 100644 --- a/src/Domain/Analytics/Service/LinkTrackService.php +++ b/src/Domain/Analytics/Service/LinkTrackService.php @@ -21,8 +21,6 @@ class LinkTrackService { private const MAX_URL_LENGTH = 255; - private const HTML_ANCHOR_PATTERN = '/(]*?[\s"\'])?)href=(["\'])(.*?)\2([^>]*>)(.*?)<\/a>/is'; - private const TEXT_URL_PATTERN = '#https?://[^\s<>},]+#i'; private const PLACEHOLDER_PATTERN = '/\[[A-Z][A-Z0-9_]*(?::[^\]]*)?\]/'; /** @var array forwards by url hash, kept for the worker's lifetime */ @@ -35,6 +33,8 @@ public function __construct( private readonly MessageRepository $messageRepository, private readonly LinkTrackTokenService $tokenService, private readonly ParameterProvider $paramProvider, + private readonly LinkRewriter $linkRewriter, + private readonly AnalyticsTrackingService $analyticsTrackingService, ) { } @@ -58,18 +58,37 @@ public function isClickTrackingEnabled(): bool * placeholders (the redirect could not expand them), or are shown as text that itself contains a URL (the * rewrite would look like phishing). * + * Without click tracking, the analytics parameters (when given) are added straight onto the links instead, like + * phplist3 does. With click tracking they are added when the link is followed, see LinkClickService. + * + * Forwarded messages are not click tracked, as their receivers are not necessarily subscribers. + * + * @param array|null $analyticsParameters the campaign's analytics settings, null when it has + * no analytics tracking * @return array{0: string, 1: string} the tracked HTML and text */ - public function trackLinks(string $html, string $text, int $campaignId, Subscriber $subscriber): array - { - $campaign = $this->isClickTrackingEnabled() ? $this->messageRepository->findById($campaignId) : null; + public function trackLinks( + string $html, + string $text, + int $campaignId, + Subscriber $subscriber, + ?array $analyticsParameters = null, + bool $forwarded = false, + ): array { + if (!$this->isClickTrackingEnabled()) { + return $analyticsParameters === null + ? [$html, $text] + : $this->analyticsTrackingService->addToBodies($html, $text, $analyticsParameters); + } + + $campaign = $forwarded ? null : $this->messageRepository->findById($campaignId); if ($campaign === null) { return [$html, $text]; } $messageUuid = (string) $campaign->getUuid(); $subscriberUuid = $subscriber->getUuid(); - if (!$this->isClickTrackingEnabled() || $messageUuid === '' || $subscriberUuid === '') { + if ($messageUuid === '' || $subscriberUuid === '') { return [$html, $text]; } @@ -78,8 +97,14 @@ public function trackLinks(string $html, string $text, int $campaignId, Subscrib return $this->createTrackingUrl($url, $type, $messageUuid, $subscriberUuid, $forwardIds); }; - $html = $this->rewriteHtml($html, $trackUrl); - $text = $this->rewriteText($text, $trackUrl); + $html = $this->linkRewriter->rewriteHtml( + $html, + fn (string $href): ?string => $trackUrl($href, LinkTrackToken::TYPE_HTML) + ); + $text = $this->linkRewriter->rewriteText( + $text, + fn (string $url): ?string => $trackUrl($url, LinkTrackToken::TYPE_TEXT) + ); foreach (array_keys($forwardIds) as $forwardId) { $this->linkTrackMlRepository->incrementTotal($campaignId, $forwardId); @@ -88,54 +113,6 @@ public function trackLinks(string $html, string $text, int $campaignId, Subscrib return [$html, $text]; } - /** @param callable(string, string): ?string $trackUrl */ - private function rewriteHtml(string $html, callable $trackUrl): string - { - if ($html === '') { - return $html; - } - - return (string) preg_replace_callback( - self::HTML_ANCHOR_PATTERN, - function (array $match) use ($trackUrl): string { - $linkText = strip_tags($match[5]); - if (stripos($linkText, 'http://') !== false || stripos($linkText, 'https://') !== false) { - return $match[0]; - } - - $href = html_entity_decode(trim($match[3]), ENT_QUOTES | ENT_HTML5); - $url = $trackUrl($href, LinkTrackToken::TYPE_HTML); - if ($url === null) { - return $match[0]; - } - - return $match[1] . 'href=' . $match[2] . htmlspecialchars($url, ENT_QUOTES) . $match[2] - . $match[4] . $match[5] . ''; - }, - $html - ); - } - - /** @param callable(string, string): ?string $trackUrl */ - private function rewriteText(string $text, callable $trackUrl): string - { - if ($text === '') { - return $text; - } - - return (string) preg_replace_callback( - self::TEXT_URL_PATTERN, - function (array $match) use ($trackUrl): string { - $trailing = str_ends_with($match[0], '.') ? '.' : ''; - $link = substr($match[0], 0, strlen($match[0]) - strlen($trailing)); - $url = $trackUrl($link, LinkTrackToken::TYPE_TEXT); - - return $url === null ? $match[0] : $url . $trailing; - }, - $text - ); - } - /** @param array $forwardIds collects the ids of the forwards used in the email */ private function createTrackingUrl( string $url, diff --git a/src/Domain/Analytics/Service/Query/AnalyticsQueryInterface.php b/src/Domain/Analytics/Service/Query/AnalyticsQueryInterface.php new file mode 100644 index 00000000..befaab1a --- /dev/null +++ b/src/Domain/Analytics/Service/Query/AnalyticsQueryInterface.php @@ -0,0 +1,30 @@ + $messageData the campaign's message data + * @return array query parameters as name => value, empty values are not added to links + */ + public function trackingParameters(string $emailFormat, array $messageData): array; + + /** + * The parameters a campaign can set, with the values to use when it has not set them. + * + * @param array $messageData + * @return array + */ + public function editableParameters(array $messageData): array; + + /** Prefix shared by the parameter names, used to take off tracking parameters that are already on a link. */ + public function prefix(): string; +} diff --git a/src/Domain/Analytics/Service/Query/GoogleAnalyticsQuery.php b/src/Domain/Analytics/Service/Query/GoogleAnalyticsQuery.php new file mode 100644 index 00000000..1ad2a4c2 --- /dev/null +++ b/src/Domain/Analytics/Service/Query/GoogleAnalyticsQuery.php @@ -0,0 +1,32 @@ + $messageData['utm_source'] ?? null, + 'utm_medium' => $messageData['utm_medium'] ?? null, + 'utm_campaign' => $messageData['utm_campaign'] ?? null, + 'utm_content' => $emailFormat, + ]; + } + + public function editableParameters(array $messageData): array + { + return [ + 'utm_source' => 'phpList', + 'utm_medium' => 'email', + 'utm_campaign' => (string) ($messageData['subject'] ?? ''), + ]; + } + + public function prefix(): string + { + return 'utm_'; + } +} diff --git a/src/Domain/Analytics/Service/Query/MatomoAnalyticsQuery.php b/src/Domain/Analytics/Service/Query/MatomoAnalyticsQuery.php new file mode 100644 index 00000000..ce975b67 --- /dev/null +++ b/src/Domain/Analytics/Service/Query/MatomoAnalyticsQuery.php @@ -0,0 +1,32 @@ + $messageData['pk_source'] ?? null, + 'pk_medium' => $messageData['pk_medium'] ?? null, + 'pk_campaign' => $messageData['pk_campaign'] ?? null, + 'pk_content' => $emailFormat, + ]; + } + + public function editableParameters(array $messageData): array + { + return [ + 'pk_source' => 'phpList', + 'pk_medium' => 'email', + 'pk_campaign' => (string) ($messageData['subject'] ?? ''), + ]; + } + + public function prefix(): string + { + return 'pk_'; + } +} diff --git a/src/Domain/Configuration/Model/ConfigOption.php b/src/Domain/Configuration/Model/ConfigOption.php index 34a2b306..41ec18dc 100644 --- a/src/Domain/Configuration/Model/ConfigOption.php +++ b/src/Domain/Configuration/Model/ConfigOption.php @@ -22,6 +22,7 @@ enum ConfigOption: string case MessageReplyToAddress = 'message_replyto_address'; case SystemMessageTemplate = 'systemmessagetemplate'; case AlwaysAddGoogleTracking = 'always_add_googletracking'; + case AnalyticTracker = 'analytic_tracker'; case AdminAddress = 'admin_address'; case AdminAddresses = 'admin_addresses'; case DefaultMessageTemplate = 'defaultmessagetemplate'; diff --git a/src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php b/src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php index 57cbc32b..928a5262 100644 --- a/src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php +++ b/src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php @@ -24,5 +24,7 @@ class MessagePrecacheDto public ?int $templateId = null; public bool $userSpecificUrl = false; public bool $googleTrack = false; + /** @var array the campaign's analytics settings (utm_*, pk_*) */ + public array $analyticsParameters = []; public array $adminAttributes = []; } diff --git a/src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php b/src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php index 44ed2eb5..3d752bb2 100644 --- a/src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php +++ b/src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php @@ -97,7 +97,8 @@ public function __invoke( textMessage: $textMessage, receiver: $receiver, campaignId: $campaignId, - forwardedBy: $forwardedBy, + messagePrecacheDto: $messagePrecacheDto, + forwarded: $forwardedBy !== null, ); $htmlMessage = $this->ensureHtmlFormating(content: $htmlMessage, addDefaultStyle: $addDefaultStyle); @@ -112,8 +113,7 @@ public function __invoke( } /** - * As in phplist3, links are tracked once all placeholders are expanded, but not in forwarded messages as the - * receivers are not necessarily subscribers. + * As in phplist3, links are tracked once all placeholders are expanded. * * @return array{0: string, 1: string} */ @@ -122,13 +122,21 @@ private function trackLinks( string $textMessage, Subscriber $receiver, ?int $campaignId, - ?Subscriber $forwardedBy, + MessagePrecacheDto $messagePrecacheDto, + bool $forwarded, ): array { - if ($campaignId === null || $forwardedBy !== null) { + if ($campaignId === null) { return [$htmlMessage, $textMessage]; } - return $this->linkTrackService->trackLinks($htmlMessage, $textMessage, $campaignId, $receiver); + return $this->linkTrackService->trackLinks( + html: $htmlMessage, + text: $textMessage, + campaignId: $campaignId, + subscriber: $receiver, + analyticsParameters: $messagePrecacheDto->googleTrack ? $messagePrecacheDto->analyticsParameters : null, + forwarded: $forwarded, + ); } private function replaceUserSpecificRemoteContent( diff --git a/src/Domain/Messaging/Service/MessagePrecacheService.php b/src/Domain/Messaging/Service/MessagePrecacheService.php index 59641c5f..86ed5b31 100644 --- a/src/Domain/Messaging/Service/MessagePrecacheService.php +++ b/src/Domain/Messaging/Service/MessagePrecacheService.php @@ -77,6 +77,7 @@ public function precacheMessage( } $messagePrecacheDto->googleTrack = (bool) $loadedMessageData['google_track']; + $messagePrecacheDto->analyticsParameters = $this->extractAnalyticsParameters($loadedMessageData); $this->applyBasicReplacements($messagePrecacheDto, $loadedMessageData); $this->populateAdminAttributes($messagePrecacheDto, $campaign); @@ -257,4 +258,20 @@ private function populateAdminAttributes(MessagePrecacheDto $messagePrecacheDto, $messagePrecacheDto->adminAttributes['LISTOWNER.' . $attr['name']] = $attr['value']; } } + + /** + * The analytics settings of the campaign: the Google (utm_*) and Matomo (pk_*) parameters it has set, and its + * subject, which is their default campaign name. + */ + private function extractAnalyticsParameters(array $loadedMessageData): array + { + $parameters = ['subject' => (string) ($loadedMessageData['subject'] ?? '')]; + foreach ($loadedMessageData as $name => $value) { + if (is_string($name) && preg_match('/^(utm|pk)_/', $name) === 1) { + $parameters[$name] = $value; + } + } + + return $parameters; + } } diff --git a/tests/Unit/Domain/Analytics/Service/AnalyticsTrackingServiceTest.php b/tests/Unit/Domain/Analytics/Service/AnalyticsTrackingServiceTest.php new file mode 100644 index 00000000..db8b2c73 --- /dev/null +++ b/tests/Unit/Domain/Analytics/Service/AnalyticsTrackingServiceTest.php @@ -0,0 +1,182 @@ +configProvider = $this->createMock(ConfigProvider::class); + $this->messageDataLoader = $this->createMock(MessageDataLoader::class); + $this->service = new AnalyticsTrackingService( + $this->configProvider, + $this->messageDataLoader, + new LinkRewriter(), + ); + } + + private function useTracker(?string $tracker): void + { + $this->configProvider->method('getValue')->with(ConfigOption::AnalyticTracker)->willReturn($tracker); + } + + public function testGoogleParametersAreAddedWithTheEmailFormat(): void + { + $this->useTracker('google'); + + $url = $this->service->addToUrl( + 'https://example.com/page', + 'HTML', + ['utm_source' => 'news', 'utm_medium' => 'mail', 'utm_campaign' => 'spring'] + ); + + self::assertSame( + 'https://example.com/page?utm_source=news&utm_medium=mail&utm_campaign=spring&utm_content=HTML', + $url + ); + } + + public function testMatomoParametersAreUsedWhenConfigured(): void + { + $this->useTracker('matomo'); + + $url = $this->service->addToUrl( + 'https://example.com/page?a=1', + 'text', + ['pk_source' => 'news', 'pk_medium' => 'mail', 'pk_campaign' => 'spring', 'utm_source' => 'ignored'] + ); + + self::assertSame( + 'https://example.com/page?a=1&pk_source=news&pk_medium=mail&pk_campaign=spring&pk_content=text', + $url + ); + } + + public function testCampaignWithoutOwnValuesGetsTheDefaults(): void + { + $this->useTracker('google'); + + $url = $this->service->addToUrl('https://example.com/', 'HTML', ['subject' => 'Hello World']); + + self::assertSame( + 'https://example.com/?utm_source=phpList&utm_medium=email&utm_campaign=Hello%20World&utm_content=HTML', + str_replace('+', '%20', $url) + ); + } + + public function testParameterSetToEmptyIsLeftOutInsteadOfGettingItsDefault(): void + { + $this->useTracker('google'); + + $url = $this->service->addToUrl( + 'https://example.com/', + 'HTML', + ['utm_source' => '', 'subject' => 'Hi'] + ); + + self::assertSame('https://example.com/?utm_medium=email&utm_campaign=Hi&utm_content=HTML', $url); + } + + public function testUnknownTrackerFallsBackToGoogle(): void + { + $this->useTracker(null); + + self::assertStringContainsString( + 'utm_content=HTML', + $this->service->addToUrl('https://example.com/', 'HTML', []) + ); + } + + public function testOnlyHttpUrlsGetParameters(): void + { + $this->useTracker('google'); + + foreach (['mailto:a@example.com', 'ftp://example.com/f', '[UNSUBSCRIBEURL]', '#top'] as $url) { + self::assertSame($url, $this->service->addToUrl($url, 'HTML', [])); + } + } + + public function testExistingTrackingParametersAreReplacedAndTheFragmentStaysLast(): void + { + $url = $this->service->withParameters( + 'https://example.com/p?a=1&utm_source=old&utm_medium=old&b=2#section', + ['utm_source' => 'new', 'utm_content' => 'HTML'], + 'utm_' + ); + + self::assertSame('https://example.com/p?a=1&b=2&utm_source=new&utm_content=HTML#section', $url); + } + + public function testEmptyParametersAreLeftOut(): void + { + self::assertSame( + 'https://example.com/?utm_content=text', + $this->service->withParameters( + 'https://example.com/', + ['utm_source' => '', 'utm_medium' => null, 'utm_content' => 'text'], + 'utm_' + ) + ); + self::assertSame( + 'https://example.com/#x', + $this->service->withParameters('https://example.com/#x', ['utm_source' => ''], 'utm_') + ); + } + + public function testBodiesGetParametersOnHtmlLinksAndTextUrls(): void + { + $this->useTracker('google'); + $data = ['utm_source' => 'news', 'utm_medium' => 'mail', 'utm_campaign' => 'c']; + + [$html, $text] = $this->service->addToBodies( + 'A m', + 'Read https://example.com/b. Thanks', + $data + ); + + self::assertStringContainsString( + 'href="https://example.com/a?x=1&y=2&utm_source=news&utm_medium=mail' + . '&utm_campaign=c&utm_content=HTML"', + $html + ); + self::assertStringContainsString('href="mailto:a@example.com"', $html); + self::assertSame( + 'Read https://example.com/b?utm_source=news&utm_medium=mail&utm_campaign=c&utm_content=text. Thanks', + $text + ); + } + + public function testClickedUrlGetsParametersOnlyWhenTheCampaignTracksAnalytics(): void + { + $this->useTracker('google'); + $campaign = $this->createMock(Message::class); + $this->messageDataLoader->method('__invoke')->with($campaign)->willReturnOnConsecutiveCalls( + ['google_track' => false, 'utm_source' => 'news'], + ['google_track' => true, 'utm_source' => 'news', 'utm_medium' => 'm', 'utm_campaign' => 'c'], + ); + + self::assertSame( + 'https://example.com/', + $this->service->addToClickedUrl('https://example.com/', $campaign, true) + ); + self::assertSame( + 'https://example.com/?utm_source=news&utm_medium=m&utm_campaign=c&utm_content=text', + $this->service->addToClickedUrl('https://example.com/', $campaign, false) + ); + } +} diff --git a/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php b/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php index 5b59986d..1f1ada68 100644 --- a/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php +++ b/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php @@ -10,6 +10,7 @@ use PhpList\Core\Domain\Analytics\Repository\LinkTrackForwardRepository; use PhpList\Core\Domain\Analytics\Repository\LinkTrackMlRepository; use PhpList\Core\Domain\Analytics\Repository\LinkTrackUmlClickRepository; +use PhpList\Core\Domain\Analytics\Service\AnalyticsTrackingService; use PhpList\Core\Domain\Analytics\Service\LinkClickService; use PhpList\Core\Domain\Analytics\Service\LinkTrackTokenService; use PhpList\Core\Domain\Analytics\Service\UserMessageService; @@ -31,6 +32,7 @@ class LinkClickServiceTest extends TestCase private SubscriberRepository&MockObject $subscriberRepository; private UserMessageRepository&MockObject $userMessageRepository; private UserMessageService&MockObject $userMessageService; + private AnalyticsTrackingService&MockObject $analyticsTrackingService; private LinkTrackTokenService $tokenService; private LinkClickService $service; private Message&MockObject $message; @@ -45,6 +47,8 @@ protected function setUp(): void $this->subscriberRepository = $this->createMock(SubscriberRepository::class); $this->userMessageRepository = $this->createMock(UserMessageRepository::class); $this->userMessageService = $this->createMock(UserMessageService::class); + $this->analyticsTrackingService = $this->createMock(AnalyticsTrackingService::class); + $this->analyticsTrackingService->method('addToClickedUrl')->willReturnArgument(0); $this->tokenService = new LinkTrackTokenService(new ParameterProvider([]), 'https://example.com/api/v2', 's'); $this->service = new LinkClickService( @@ -56,6 +60,7 @@ protected function setUp(): void $this->subscriberRepository, $this->userMessageRepository, $this->userMessageService, + $this->analyticsTrackingService, ); $this->message = $this->createMock(Message::class); @@ -113,6 +118,31 @@ public function testRedirectsToTheStoredUrlAndCountsTheClick(): void ); } + public function testAnalyticsParametersAreAddedToTheRedirectUrl(): void + { + $this->givenKnownLink(); + $this->givenUserMessage(viewed: true); + + $analytics = $this->createMock(AnalyticsTrackingService::class); + $analytics->expects(self::once()) + ->method('addToClickedUrl') + ->with('https://example.com/a', $this->message, true) + ->willReturn('https://example.com/a?utm_source=phpList'); + $service = new LinkClickService( + $this->tokenService, + $this->forwardRepository, + $this->mlRepository, + $this->umlClickRepository, + $this->messageRepository, + $this->subscriberRepository, + $this->userMessageRepository, + $this->userMessageService, + $analytics, + ); + + self::assertSame('https://example.com/a?utm_source=phpList', $service->trackClick($this->tid(), null, [])); + } + public function testTextClicksAreCountedAsText(): void { $this->givenKnownLink(); diff --git a/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php b/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php index ace74a03..d6cfc5c8 100644 --- a/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php +++ b/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php @@ -9,6 +9,8 @@ use PhpList\Core\Domain\Analytics\Repository\LinkTrackForwardRepository; use PhpList\Core\Domain\Analytics\Repository\LinkTrackMlRepository; use PhpList\Core\Domain\Analytics\Repository\LinkTrackRepository; +use PhpList\Core\Domain\Analytics\Service\AnalyticsTrackingService; +use PhpList\Core\Domain\Analytics\Service\LinkRewriter; use PhpList\Core\Domain\Analytics\Service\LinkTrackService; use PhpList\Core\Domain\Analytics\Service\LinkTrackTokenService; use PhpList\Core\Domain\Messaging\Model\Message; @@ -22,6 +24,7 @@ class LinkTrackServiceTest extends TestCase private LinkTrackForwardRepository&MockObject $forwardRepository; private LinkTrackMlRepository&MockObject $mlRepository; private MessageRepository&MockObject $messageRepository; + private AnalyticsTrackingService&MockObject $analyticsTrackingService; private LinkTrackTokenService $tokenService; private Message&MockObject $campaign; private Subscriber $subscriber; @@ -33,6 +36,7 @@ protected function setUp(): void $this->forwardRepository = $this->createMock(LinkTrackForwardRepository::class); $this->mlRepository = $this->createMock(LinkTrackMlRepository::class); $this->messageRepository = $this->createMock(MessageRepository::class); + $this->analyticsTrackingService = $this->createMock(AnalyticsTrackingService::class); $this->tokenService = new LinkTrackTokenService( new ParameterProvider([]), 'https://example.com/api/v2', @@ -76,6 +80,8 @@ private function createService(bool $clickTrack = true): LinkTrackService $this->messageRepository, $this->tokenService, new ParameterProvider(['click_track' => $clickTrack]), + new LinkRewriter(), + $this->analyticsTrackingService, ); } @@ -120,6 +126,8 @@ public function testContentIsReturnedUntouchedWhenCampaignHasNoUuid(): void $repository, $this->tokenService, new ParameterProvider(['click_track' => true]), + new LinkRewriter(), + $this->analyticsTrackingService, ); $html = 'x'; @@ -237,4 +245,40 @@ public function testTotalIsIncrementedOncePerDistinctLinkOfTheEmail(): void self::assertSame([[7, 1], [7, 2]], $incremented); } + + public function testAnalyticsParametersAreAddedStraightToTheLinksWithoutClickTracking(): void + { + $this->analyticsTrackingService->expects(self::once()) + ->method('addToBodies') + ->with('x', 'text', ['utm_source' => 'news']) + ->willReturn(['html with utm', 'text with utm']); + $this->forwardRepository->expects(self::never())->method('findOrCreateByUrl'); + + $result = $this->createService(false)->trackLinks( + 'x', + 'text', + 7, + $this->subscriber, + ['utm_source' => 'news'] + ); + + self::assertSame(['html with utm', 'text with utm'], $result); + } + + public function testAnalyticsParametersAreNotAddedByTheSendWhenClickTrackingIsOn(): void + { + $this->analyticsTrackingService->expects(self::never())->method('addToBodies'); + + $this->createService(true)->trackLinks('x', '', 7, $this->subscriber, []); + } + + public function testForwardedMessagesAreNotClickTracked(): void + { + $html = 'x'; + $this->forwardRepository->expects(self::never())->method('findOrCreateByUrl'); + + $result = $this->createService()->trackLinks($html, '', 7, $this->subscriber, null, true); + + self::assertSame([$html, ''], $result); + } } diff --git a/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php b/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php index ad4cde61..d463ac9e 100644 --- a/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php +++ b/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php @@ -319,11 +319,14 @@ public function testLinksAreTrackedAfterPlaceholdersAreExpanded(): void $this->assertSame('tracked text', $text); } - public function testLinksAreNotTrackedInForwardedMessages(): void + public function testForwardedMessagesAreMarkedAsForwardedForLinkTracking(): void { $subscriber = $this->makeSubscriber(); $this->linkTrackService = $this->createMock(LinkTrackService::class); - $this->linkTrackService->expects($this->never())->method('trackLinks'); + $this->linkTrackService->expects($this->once()) + ->method('trackLinks') + ->with('

Hi

[expanded]', 'Hi[expanded]', 9, $subscriber, null, true) + ->willReturn(['

Hi

', 'Hi']); $dto = new MessagePrecacheDto(); $dto->content = '

Hi

'; @@ -332,4 +335,23 @@ public function testLinksAreNotTrackedInForwardedMessages(): void ($this->makeBuilder())($dto, $subscriber, 9, $this->createMock(Subscriber::class)); } + + public function testAnalyticsParametersAreHandedOverWhenTheCampaignTracksAnalytics(): void + { + $subscriber = $this->makeSubscriber(); + $this->linkTrackService = $this->createMock(LinkTrackService::class); + $this->linkTrackService->expects($this->once()) + ->method('trackLinks') + ->with('

Hi

[expanded]', 'Hi[expanded]', 9, $subscriber, ['utm_source' => 'news'], false) + ->willReturn(['

Hi

', 'Hi']); + + $dto = new MessagePrecacheDto(); + $dto->content = '

Hi

'; + $dto->textContent = 'Hi'; + $dto->htmlFormatted = true; + $dto->googleTrack = true; + $dto->analyticsParameters = ['utm_source' => 'news']; + + ($this->makeBuilder())($dto, $subscriber, 9); + } } From 0a8e6c375511e395fdbaf5bcfc8510c19591a507 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 2 Nov 2026 16:15:02 +0400 Subject: [PATCH 23/30] feat: add fallback for APP_WEBSITE in parameters.yml to support existing .env files --- config/parameters.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/parameters.yml b/config/parameters.yml index 36860be3..aa00f865 100644 --- a/config/parameters.yml +++ b/config/parameters.yml @@ -23,6 +23,8 @@ parameters: app.rest_api_base_url: '%env(API_BASE_URL)%/api/v2' app.api_base_url: '%env(API_BASE_URL)%' app.frontend_base_url: '%env(FRONT_END_BASE_URL)%' + # falls back to empty so existing .env files without APP_WEBSITE keep working (the value may live in the database) + env(APP_WEBSITE): '' app.website: '%env(APP_WEBSITE)%' app.parallel_use_with_phplist3: '%env(PARALLEL_USE_WITH_PHPLIST3)%' From b7e40ee6401ddf7b7761f6d2bc4da7a77f97d743 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 2 Nov 2026 16:16:54 +0400 Subject: [PATCH 24/30] feat: add restrictive umask when creating files in FileHelper to ensure privacy --- src/Domain/Common/FileHelper.php | 8 +++++++- tests/Unit/Domain/Common/FileHelperTest.php | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/Domain/Common/FileHelper.php b/src/Domain/Common/FileHelper.php index 7073abf8..fed3649f 100644 --- a/src/Domain/Common/FileHelper.php +++ b/src/Domain/Common/FileHelper.php @@ -47,7 +47,13 @@ public function writeFileToDirectory(string $directory, string $originalFilename // Exclusive create (O_EXCL) never follows a symlink or reuses an existing entry, // so the file is created and written in a single step without rename. $fullPath = $realDirectory . '/' . $name . bin2hex(random_bytes(6)) . ($ext !== '' ? '.' . $ext : ''); - $handle = fopen($fullPath, 'xb'); + // Restrictive umask so the file is private from creation (like tempnam()), whatever the process umask is + $previousUmask = umask(0077); + try { + $handle = fopen($fullPath, 'xb'); + } finally { + umask($previousUmask); + } if ($handle === false) { return null; } diff --git a/tests/Unit/Domain/Common/FileHelperTest.php b/tests/Unit/Domain/Common/FileHelperTest.php index f9f231d9..4e1f3d7e 100644 --- a/tests/Unit/Domain/Common/FileHelperTest.php +++ b/tests/Unit/Domain/Common/FileHelperTest.php @@ -65,13 +65,19 @@ public function testWriteFileToDirectoryCreatesFileWithExtensionAndContents(): v { $helper = new FileHelper(); - $writtenPath = $helper->writeFileToDirectory($this->tmpDir, 'report.txt', 'payload'); + $previousUmask = umask(0022); + try { + $writtenPath = $helper->writeFileToDirectory($this->tmpDir, 'report.txt', 'payload'); + } finally { + umask($previousUmask); + } $this->assertNotNull($writtenPath); $this->assertStringStartsWith($this->tmpDir . '/', $writtenPath); $this->assertTrue(is_file($writtenPath)); $this->assertStringEndsWith('.txt', basename($writtenPath)); $this->assertSame('payload', file_get_contents($writtenPath)); + $this->assertSame(0600, fileperms($writtenPath) & 0777); } public function testWriteFileToDirectoryLeavesNoOrphanedTempFile(): void From b09f08a13ac2d64bf17d6c2354e48eafe22a570f Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 2 Nov 2026 16:19:41 +0400 Subject: [PATCH 25/30] feat: refactor UserTrackValueResolver to use absolute REST API base URL and simplify domain handling --- .../Placeholder/UserTrackValueResolver.php | 8 ++- .../Provider/DefaultConfigProvider.php | 6 ++- .../UserTrackValueResolverTest.php | 52 +++---------------- 3 files changed, 14 insertions(+), 52 deletions(-) diff --git a/src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php b/src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php index 8a5b626d..f99efa2c 100644 --- a/src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php +++ b/src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php @@ -4,15 +4,12 @@ namespace PhpList\Core\Domain\Configuration\Service\Placeholder; -use PhpList\Core\Domain\Configuration\Model\ConfigOption; -use PhpList\Core\Domain\Configuration\Service\Provider\ConfigProvider; use PhpList\Core\Domain\Configuration\Model\Dto\PlaceholderContext; use Symfony\Component\DependencyInjection\Attribute\Autowire; final class UserTrackValueResolver implements PlaceholderValueResolverInterface { public function __construct( - private readonly ConfigProvider $config, #[Autowire('%app.rest_api_base_url%')] private readonly string $restApiBaseUrl, ) { } @@ -24,9 +21,10 @@ public function name(): string public function __invoke(PlaceholderContext $ctx): string { - $base = $this->config->getValue(ConfigOption::Domain) ?? $this->restApiBaseUrl; + // The stored domain is scheme-free, which would make the pixel src relative, so always use the absolute base + $base = rtrim($this->restApiBaseUrl, '/'); - if ($ctx->isText() || empty($base)) { + if ($ctx->isText() || $base === '') { return ''; } $uid = $ctx->forwardedBy() ? 'forwarded' : $ctx->getUser()->getUniqueId(); diff --git a/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php b/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php index 7d8b0f3d..c1aea578 100644 --- a/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php +++ b/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php @@ -29,6 +29,10 @@ private function init(): void return; } + // the website may contain a port or path, the email domain is the bare host without a leading www. + $host = parse_url('//' . $this->website, PHP_URL_HOST); + $domain = preg_replace('/^www\./i', '', is_string($host) ? $host : $this->website); + $publicSchema = 'http'; $pageRoot = '/api/v2'; @@ -42,7 +46,7 @@ private function init(): void 'category' => 'general', ], 'domain' => [ - 'value' => preg_replace('/^www\./i', '', $this->website), + 'value' => $domain, 'description' => $this->translator->trans('Domain Name of your server (for email)'), 'type' => 'text', 'allowempty' => false, diff --git a/tests/Unit/Domain/Configuration/Service/Placeholder/UserTrackValueResolverTest.php b/tests/Unit/Domain/Configuration/Service/Placeholder/UserTrackValueResolverTest.php index 19caaada..6c10f0dd 100644 --- a/tests/Unit/Domain/Configuration/Service/Placeholder/UserTrackValueResolverTest.php +++ b/tests/Unit/Domain/Configuration/Service/Placeholder/UserTrackValueResolverTest.php @@ -4,24 +4,14 @@ namespace PhpList\Core\Tests\Unit\Domain\Configuration\Service\Placeholder; -use PhpList\Core\Domain\Configuration\Model\ConfigOption; use PhpList\Core\Domain\Configuration\Model\Dto\PlaceholderContext; use PhpList\Core\Domain\Configuration\Model\OutputFormat; use PhpList\Core\Domain\Configuration\Service\Placeholder\UserTrackValueResolver; -use PhpList\Core\Domain\Configuration\Service\Provider\ConfigProvider; use PhpList\Core\Domain\Subscription\Model\Subscriber; -use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; final class UserTrackValueResolverTest extends TestCase { - private ConfigProvider&MockObject $config; - - protected function setUp(): void - { - $this->config = $this->createMock(ConfigProvider::class); - } - private function makeUser(string $uid = 'U-42'): Subscriber { $u = new Subscriber('user@example.com'); @@ -31,24 +21,20 @@ private function makeUser(string $uid = 'U-42'): Subscriber public function testName(): void { - $resolver = new UserTrackValueResolver($this->config, 'https://api.example'); + $resolver = new UserTrackValueResolver('https://api.example'); $this->assertSame('USERTRACK', $resolver->name()); } public function testReturnsEmptyForTextFormat(): void { - $resolver = new UserTrackValueResolver($this->config, 'https://api.example'); + $resolver = new UserTrackValueResolver('https://api.example'); $ctx = new PlaceholderContext(user: $this->makeUser(), format: OutputFormat::Text); $this->assertSame('', $resolver($ctx)); } - public function testHtmlUsesConfigDomainWhenAvailable(): void + public function testHtmlUsesAbsoluteRestApiBaseUrl(): void { - $this->config->method('getValue') - ->with(ConfigOption::Domain) - ->willReturn('example.com'); - - $resolver = new UserTrackValueResolver($this->config, 'https://api.example'); + $resolver = new UserTrackValueResolver('https://api.example/api/v2/'); $ctx = new PlaceholderContext( user: $this->makeUser('UID-XYZ'), @@ -59,33 +45,7 @@ public function testHtmlUsesConfigDomainWhenAvailable(): void messageId: 99, ); - $result = $resolver($ctx); - - $expected = ''; - // Normalize double quotes for comparison - $this->assertSame($expected, $result); - } - - public function testHtmlFallsBackToRestApiDomainWhenConfigMissing(): void - { - $this->config->method('getValue') - ->with(ConfigOption::Domain) - ->willReturn(null); - - $resolver = new UserTrackValueResolver($this->config, 'https://api.example'); - - $ctx = new PlaceholderContext( - user: $this->makeUser('U1'), - format: OutputFormat::Html, - messagePrecacheDto: null, - locale: 'en', - forwardedBy: null, - messageId: 7, - ); - - $result = $resolver($ctx); - - $expected = ''; - $this->assertSame($expected, $result); + $expected = ''; + $this->assertSame($expected, $resolver($ctx)); } } From f8b558fc293cff7125c52c99f4e4f4fc5bc49fab Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 2 Nov 2026 16:22:19 +0400 Subject: [PATCH 26/30] feat: enhance SubscriberCsvExporter to include blacklisted email checks and optimize data normalization --- .../Repository/UserBlacklistRepository.php | 24 ++++++++++++ .../Service/SubscriberCsvExporter.php | 38 +++++++++++++++---- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/src/Domain/Subscription/Repository/UserBlacklistRepository.php b/src/Domain/Subscription/Repository/UserBlacklistRepository.php index 9dabc9ff..5651c20c 100644 --- a/src/Domain/Subscription/Repository/UserBlacklistRepository.php +++ b/src/Domain/Subscription/Repository/UserBlacklistRepository.php @@ -58,4 +58,28 @@ public function isEmailBlacklisted(string $email, ?bool $immediate = true): bool ->getQuery() ->getOneOrNullResult() !== null; } + + /** + * Batch variant of isEmailBlacklisted(): returns the given emails (lowercased) that are blacklisted. + * + * @param string[] $emails + * @return string[] + */ + public function findBlacklistedEmails(array $emails): array + { + if ($emails === []) { + return []; + } + + $rows = $this->createQueryBuilder('ub') + ->select('ub.email') + ->where('ub.email IN (:emails)') + ->andWhere('ub.added < :cutoff') + ->setParameter('emails', array_values($emails)) + ->setParameter('cutoff', new DateTimeImmutable()) + ->getQuery() + ->getScalarResult(); + + return array_map(static fn(array $row): string => strtolower($row['email']), $rows); + } } diff --git a/src/Domain/Subscription/Service/SubscriberCsvExporter.php b/src/Domain/Subscription/Service/SubscriberCsvExporter.php index 9429349b..1420fe9e 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvExporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvExporter.php @@ -180,11 +180,16 @@ private function exportSubscribers( 'count' => $subscriberCount ]); + $blacklistedEmails = in_array('blacklisted', $headers, true) + ? $this->findBlacklistedEmails($subscribers) + : []; + foreach ($subscribers as $subscriber) { $row = $this->getSubscriberRow( subscriber: $subscriber, attributeDefinitions: $attributeDefinitions, headers: $headers, + blacklistedEmails: $blacklistedEmails, ); fputcsv($handle, $row); $lastId = $subscriber->getId(); @@ -211,11 +216,16 @@ private function exportSubscribers( * * @param Subscriber $subscriber The subscriber * @param SubscriberAttributeDefinition[] $attributeDefinitions Attribute definitions + * @param string[] $blacklistedEmails Lowercased emails of the batch found in the blacklist table * @return array Row data */ - private function getSubscriberRow(Subscriber $subscriber, array $attributeDefinitions, array $headers): array - { - $row = $this->normalizerSubscriberData($subscriber); + private function getSubscriberRow( + Subscriber $subscriber, + array $attributeDefinitions, + array $headers, + array $blacklistedEmails, + ): array { + $row = $this->normalizerSubscriberData($subscriber, $blacklistedEmails); foreach ($attributeDefinitions as $definition) { $attrValue = $this->attributeManager->getSubscriberAttribute( @@ -256,21 +266,33 @@ private function configureResponse(BinaryFileResponse $response): Response } /** - * re-verify the flag against the blacklist table so stale flags are not exported. + * Re-verifies the flag against the blacklist table with one query per batch, so stale flags are not exported. + * Subscribers whose stored flag is already set are not looked up. + * + * @param Subscriber[] $subscribers + * @return string[] lowercased blacklisted emails */ - private function isBlacklisted(Subscriber $subscriber): bool + private function findBlacklistedEmails(array $subscribers): array { - return $subscriber->isBlacklisted() || $this->blacklistRepository->isEmailBlacklisted($subscriber->getEmail()); + $emails = []; + foreach ($subscribers as $subscriber) { + if (!$subscriber->isBlacklisted()) { + $emails[] = $subscriber->getEmail(); + } + } + + return $this->blacklistRepository->findBlacklistedEmails($emails); } /** @SuppressWarnings("CyclomaticComplexity") */ - private function normalizerSubscriberData(Subscriber $subscriber): array + private function normalizerSubscriberData(Subscriber $subscriber, array $blacklistedEmails): array { return [ 'id' => $subscriber->getId(), 'email' => $subscriber->getEmail(), 'confirmed' => $subscriber->isConfirmed() ? '1' : '0', - 'blacklisted' => $this->isBlacklisted($subscriber) ? '1' : '0', + 'blacklisted' => $subscriber->isBlacklisted() + || in_array(strtolower($subscriber->getEmail()), $blacklistedEmails, true) ? '1' : '0', 'bounceCount' => $subscriber->getBounceCount(), 'createdAt' => $subscriber->getCreatedAt()?->format('Y-m-d H:i:s') ?? '', 'updatedAt' => $subscriber->getUpdatedAt()->format('Y-m-d H:i:s'), From bf29cc591060d18b5469de5c11cceb5f8a03b9a7 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 2 Nov 2026 16:28:27 +0400 Subject: [PATCH 27/30] feat: implement live blacklist status filtering in SubscriberRepository --- .../Repository/SubscriberRepository.php | 26 +++++++++++++++--- .../Repository/SubscriberRepositoryTest.php | 27 +++++++++++++++++++ .../UserTrackValueResolverTest.php | 3 ++- 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/src/Domain/Subscription/Repository/SubscriberRepository.php b/src/Domain/Subscription/Repository/SubscriberRepository.php index 4ee046c2..706e7692 100644 --- a/src/Domain/Subscription/Repository/SubscriberRepository.php +++ b/src/Domain/Subscription/Repository/SubscriberRepository.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Domain\Subscription\Repository; +use DateTimeImmutable; use DateTimeInterface; use Doctrine\ORM\QueryBuilder; use InvalidArgumentException; @@ -14,6 +15,7 @@ use PhpList\Core\Domain\Subscription\Model\Filter\SubscriberFilter; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\Subscription; +use PhpList\Core\Domain\Subscription\Model\UserBlacklist; /** * Repository for Subscriber models. @@ -146,9 +148,7 @@ public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedRes } if ($filter->getIsBlacklisted() !== null) { - $queryBuilder - ->andWhere('subscriber.blacklisted = :isBlacklisted') - ->setParameter('isBlacklisted', $filter->getIsBlacklisted()); + $this->applyBlacklistedFilter($queryBuilder, $filter->getIsBlacklisted()); } if ($filter->getFindColumn() && $filter->getFindValue()) { @@ -296,6 +296,26 @@ public function getBounceReportRowsByListId(int $listId): array ->getArrayResult(); } + /** + * Matches the live blacklist status (stored flag or an entry in the blacklist table), the same status the CSV + * export writes, so a stale stored flag does not make the filter and the exported value disagree. + */ + private function applyBlacklistedFilter(QueryBuilder $queryBuilder, bool $isBlacklisted): void + { + $inBlacklistTable = sprintf( + 'EXISTS (SELECT 1 FROM %s ub WHERE ub.email = subscriber.email AND ub.added < :blacklistNow)', + UserBlacklist::class + ); + + $queryBuilder + ->andWhere( + $isBlacklisted + ? '(subscriber.blacklisted = true OR ' . $inBlacklistTable . ')' + : '(subscriber.blacklisted = false AND NOT ' . $inBlacklistTable . ')' + ) + ->setParameter('blacklistNow', new DateTimeImmutable()); + } + public function isEmailBlacklisted(string $email): bool { $queryBuilder = $this->getEntityManager()->createQueryBuilder(); diff --git a/tests/Integration/Domain/Subscription/Repository/SubscriberRepositoryTest.php b/tests/Integration/Domain/Subscription/Repository/SubscriberRepositoryTest.php index c835207c..48677399 100644 --- a/tests/Integration/Domain/Subscription/Repository/SubscriberRepositoryTest.php +++ b/tests/Integration/Domain/Subscription/Repository/SubscriberRepositoryTest.php @@ -11,6 +11,7 @@ use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberList; use PhpList\Core\Domain\Subscription\Model\Subscription; +use PhpList\Core\Domain\Subscription\Model\UserBlacklist; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriptionRepository; use PhpList\Core\TestingSupport\Traits\DatabaseTestTrait; @@ -303,6 +304,32 @@ public function testGetSubscribersBySubscribedListIdsReturnsEmptyArrayForEmptyIn self::assertSame([], $this->subscriberRepository->getSubscribersBySubscribedListIds([])); } + public function testGetFilteredAfterIdBlacklistedFilterUsesLiveBlacklistStatus(): void + { + $flagged = (new Subscriber('flagged@example.com'))->setBlacklisted(true); + $stale = new Subscriber('stale@example.com'); + $clean = new Subscriber('clean@example.com'); + foreach ([$flagged, $stale, $clean] as $subscriber) { + $this->entityManager->persist($subscriber); + } + $this->entityManager->persist((new UserBlacklist('stale@example.com'))->setAdded(new DateTime('-1 day'))); + $this->entityManager->flush(); + + $emailsFor = function (bool $isBlacklisted): array { + $result = $this->subscriberRepository->getFilteredAfterId( + new SubscriberFilter(isBlacklisted: $isBlacklisted) + ); + + return array_map( + static fn (Subscriber $subscriber): string => $subscriber->getEmail(), + $result->getItems() + ); + }; + + self::assertEqualsCanonicalizing(['flagged@example.com', 'stale@example.com'], $emailsFor(true)); + self::assertSame(['clean@example.com'], $emailsFor(false)); + } + public function testGetFilteredAfterIdWithoutListFilterReturnsAllSubscribersOnce(): void { $list = (new SubscriberList())->setName('list'); diff --git a/tests/Unit/Domain/Configuration/Service/Placeholder/UserTrackValueResolverTest.php b/tests/Unit/Domain/Configuration/Service/Placeholder/UserTrackValueResolverTest.php index 6c10f0dd..21992a4a 100644 --- a/tests/Unit/Domain/Configuration/Service/Placeholder/UserTrackValueResolverTest.php +++ b/tests/Unit/Domain/Configuration/Service/Placeholder/UserTrackValueResolverTest.php @@ -45,7 +45,8 @@ public function testHtmlUsesAbsoluteRestApiBaseUrl(): void messageId: 99, ); - $expected = ''; + $expected = ''; $this->assertSame($expected, $resolver($ctx)); } } From d4d78c8f9a73e6b47181fa93b3dc76f2bc15c410 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 2 Nov 2026 16:57:38 +0400 Subject: [PATCH 28/30] feat: update SubscriberCsvImporter to ignore CSV blacklisted flag and ensure blacklist status is determined by the repository --- .../Subscription/Model/Dto/ImportSubscriberDto.php | 5 ----- src/Domain/Subscription/Service/ArrayToDtoMapper.php | 2 +- .../Subscription/Service/Manager/SubscriberManager.php | 2 -- .../Subscription/Service/SubscriberCsvImporter.php | 4 ++-- .../Service/SubscriberCsvImportManagerTest.php | 4 ++-- .../Subscription/Service/SubscriberCsvImporterTest.php | 10 +--------- 6 files changed, 6 insertions(+), 21 deletions(-) diff --git a/src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php b/src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php index ee468fa1..c76c3430 100644 --- a/src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php +++ b/src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php @@ -20,9 +20,6 @@ class ImportSubscriberDto #[Assert\Type('bool')] public bool $confirmed; - #[Assert\Type('bool')] - public bool $blacklisted; - #[Assert\Type('bool')] public bool $htmlEmail; @@ -39,7 +36,6 @@ class ImportSubscriberDto public function __construct( string $email, bool $confirmed, - bool $blacklisted, bool $htmlEmail, bool $disabled, ?string $extraData = null, @@ -49,7 +45,6 @@ public function __construct( $this->email = $email; $this->confirmed = $confirmed; $this->sendConfirmation = !$confirmed; - $this->blacklisted = $blacklisted; $this->htmlEmail = $htmlEmail; $this->disabled = $disabled; $this->extraData = $extraData; diff --git a/src/Domain/Subscription/Service/ArrayToDtoMapper.php b/src/Domain/Subscription/Service/ArrayToDtoMapper.php index dcd2f6e8..52e8be47 100644 --- a/src/Domain/Subscription/Service/ArrayToDtoMapper.php +++ b/src/Domain/Subscription/Service/ArrayToDtoMapper.php @@ -9,6 +9,7 @@ class ArrayToDtoMapper { private const FK_HEADER = 'foreignkey'; + // 'blacklisted' stays a known header so the column is ignored instead of becoming an attribute private const KNOWN_HEADERS = [ 'email', 'confirmed', 'blacklisted', 'html_email', 'disabled', 'extra_data', 'foreignkey', ]; @@ -31,7 +32,6 @@ public function map(array $row): ImportSubscriberDto return new ImportSubscriberDto( email: $email, confirmed: filter_var($normalizedRow['confirmed'] ?? false, FILTER_VALIDATE_BOOLEAN), - blacklisted: filter_var($normalizedRow['blacklisted'] ?? false, FILTER_VALIDATE_BOOLEAN), htmlEmail: filter_var($normalizedRow['html_email'] ?? false, FILTER_VALIDATE_BOOLEAN), disabled: filter_var($normalizedRow['disabled'] ?? false, FILTER_VALIDATE_BOOLEAN), extraData: $normalizedRow['extra_data'] ?? null, diff --git a/src/Domain/Subscription/Service/Manager/SubscriberManager.php b/src/Domain/Subscription/Service/Manager/SubscriberManager.php index 189a2881..6c9e6f6d 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberManager.php @@ -110,7 +110,6 @@ public function createFromImport(ImportSubscriberDto $subscriberDto): Subscriber { $subscriber = new Subscriber($subscriberDto->email); $subscriber->setConfirmed($subscriberDto->confirmed); - $subscriber->setBlacklisted($subscriberDto->blacklisted); $subscriber->setHtmlEmail($subscriberDto->htmlEmail); $subscriber->setDisabled($subscriberDto->disabled); $subscriber->setExtraData($subscriberDto->extraData); @@ -127,7 +126,6 @@ public function updateFromImport(Subscriber $existingSubscriber, ImportSubscribe { $existingSubscriber->setEmail($subscriberDto->email); $existingSubscriber->setConfirmed($subscriberDto->confirmed); - $existingSubscriber->setBlacklisted($subscriberDto->blacklisted); $existingSubscriber->setHtmlEmail($subscriberDto->htmlEmail); $existingSubscriber->setDisabled($subscriberDto->disabled); $existingSubscriber->setExtraData($subscriberDto->extraData); diff --git a/src/Domain/Subscription/Service/SubscriberCsvImporter.php b/src/Domain/Subscription/Service/SubscriberCsvImporter.php index d82b0c4d..18ee26a1 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvImporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvImporter.php @@ -189,10 +189,10 @@ private function processRow( $this->attributeManager->processAttributes($subscriber, $dto->extraAttributes); - // as in phplist3, only the blacklist table decides; a blacklisted flag from the CSV alone does not + // as in phplist3, the CSV blacklisted column is ignored; only the blacklist table decides, and the flag is + // only ever raised, never cleared, by an import $isBlacklisted = $this->blacklistRepository->isEmailBlacklisted($subscriber->getEmail()); if ($isBlacklisted) { - // make sure the flag reflects the blacklist table $subscriber->setBlacklisted(true); $stats['blacklisted']++; } diff --git a/tests/Integration/Domain/Subscription/Service/SubscriberCsvImportManagerTest.php b/tests/Integration/Domain/Subscription/Service/SubscriberCsvImportManagerTest.php index 9c27c1c9..914cdde8 100644 --- a/tests/Integration/Domain/Subscription/Service/SubscriberCsvImportManagerTest.php +++ b/tests/Integration/Domain/Subscription/Service/SubscriberCsvImportManagerTest.php @@ -86,7 +86,7 @@ public function testImportFromCsvCreatesNewSubscribers(): void self::assertInstanceOf(Subscriber::class, $subscriber2); self::assertFalse($subscriber2->isConfirmed()); self::assertFalse($subscriber2->hasHtmlEmail()); - self::assertTrue($subscriber2->isBlacklisted()); + self::assertFalse($subscriber2->isBlacklisted()); self::assertTrue($subscriber2->isDisabled()); self::assertSame('More data', $subscriber2->getExtraData()); @@ -130,7 +130,7 @@ public function testImportFromCsvUpdatesExistingSubscribers(): void self::assertInstanceOf(Subscriber::class, $updatedSubscriber); self::assertTrue($updatedSubscriber->isConfirmed()); self::assertTrue($updatedSubscriber->hasHtmlEmail()); - self::assertFalse($updatedSubscriber->isBlacklisted()); + self::assertTrue($updatedSubscriber->isBlacklisted()); self::assertFalse($updatedSubscriber->isDisabled()); self::assertSame('Updated data', $updatedSubscriber->getExtraData()); diff --git a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php index d429c5ca..ec46f9c1 100644 --- a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php +++ b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php @@ -104,7 +104,6 @@ public function testImportFromCsvCreatesNewSubscribers(): void $importDto1 = new ImportSubscriberDto( email: 'test@example.com', confirmed: true, - blacklisted: false, htmlEmail: true, disabled: false, ); @@ -115,7 +114,6 @@ public function testImportFromCsvCreatesNewSubscribers(): void $importDto2 = new ImportSubscriberDto( email: 'another@example.com', confirmed: false, - blacklisted: true, htmlEmail: false, disabled: true ); @@ -211,7 +209,6 @@ public function testImportFromCsvUpdatesExistingSubscribers(): void $importDto = new ImportSubscriberDto( email: 'existing@example.com', confirmed: true, - blacklisted: false, htmlEmail: true, disabled: false, ); @@ -281,7 +278,6 @@ public function testImportResolvesByForeignKeyWhenProvidedAndMatches(): void $dto = new ImportSubscriberDto( email: 'user@example.com', confirmed: true, - blacklisted: false, htmlEmail: true, disabled: false, extraData: null, @@ -346,7 +342,6 @@ public function testImportConflictWhenEmailAndForeignKeyReferToDifferentSubscrib $dto = new ImportSubscriberDto( email: 'conflict@example.com', confirmed: true, - blacklisted: false, htmlEmail: true, disabled: false, extraData: null, @@ -406,7 +401,6 @@ public function testImportResolvesByEmailWhenForeignKeyNotFound(): void $dto = new ImportSubscriberDto( email: 'existing@example.com', confirmed: true, - blacklisted: false, htmlEmail: true, disabled: false, extraData: null, @@ -466,7 +460,6 @@ public function testImportCreatesNewWhenNeitherEmailNorForeignKeyFound(): void $dto = new ImportSubscriberDto( email: 'new@example.com', confirmed: false, - blacklisted: false, htmlEmail: true, disabled: false, extraData: null, @@ -520,7 +513,6 @@ public function testImportCountsSubscriberWhoseEmailIsInBlacklistTable(): void $dto = new ImportSubscriberDto( email: 'bl@example.com', confirmed: false, - blacklisted: false, htmlEmail: true, disabled: false, ); @@ -565,7 +557,6 @@ public function testImportIgnoresBlacklistedFlagWhenEmailIsNotInBlacklistTable() $dto = new ImportSubscriberDto( email: 'flag@example.com', confirmed: true, - blacklisted: true, htmlEmail: true, disabled: false, ); @@ -575,6 +566,7 @@ public function testImportIgnoresBlacklistedFlagWhenEmailIsNotInBlacklistTable() $created->method('getId')->willReturn(8); $created->method('getEmail')->willReturn('flag@example.com'); $created->method('isBlacklisted')->willReturn(true); + $created->expects($this->never())->method('setBlacklisted'); $this->subscriberManagerMock->method('createFromImport')->willReturn($created); $this->blacklistRepositoryMock->method('isEmailBlacklisted')->willReturn(false); From 15579b1a739bc143c483728553255dece61f397c Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 2 Nov 2026 18:15:46 +0400 Subject: [PATCH 29/30] feat: implement method to remove query parameters with specified prefix from URLs in AnalyticsTrackingService --- .../Service/AnalyticsTrackingService.php | 32 ++++++++++++++++--- .../Service/AnalyticsTrackingServiceTest.php | 16 ++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/Domain/Analytics/Service/AnalyticsTrackingService.php b/src/Domain/Analytics/Service/AnalyticsTrackingService.php index 1525176d..a9e19ecf 100644 --- a/src/Domain/Analytics/Service/AnalyticsTrackingService.php +++ b/src/Domain/Analytics/Service/AnalyticsTrackingService.php @@ -103,11 +103,7 @@ public function withParameters(string $url, array $trackingParameters, string $p $url = substr($url, 0, $position); } - if (str_contains($url, $prefix)) { - // the expression can leave a trailing ? or & behind - $pattern = sprintf('/%s.+?=[^&]+(&|$)/', preg_quote($prefix, '/')); - $url = rtrim((string) preg_replace($pattern, '', $url), '?&'); - } + $url = $this->withoutPrefixedParameters($url, $prefix); $trackingCode = http_build_query( array_filter($trackingParameters, static fn ($value): bool => $value !== null && $value !== '') @@ -119,6 +115,32 @@ public function withParameters(string $url, array $trackingParameters, string $p return $url . (str_contains($url, '?') ? '&' : '?') . $trackingCode . $fragment; } + /** + * Takes off the query parameters whose name starts with the prefix, leaving the path and the other parameters + * as they are. The url is left untouched when nothing is removed. + */ + private function withoutPrefixedParameters(string $url, string $prefix): string + { + $queryPosition = strpos($url, '?'); + if ($prefix === '' || $queryPosition === false) { + return $url; + } + + $base = substr($url, 0, $queryPosition); + $pairs = explode('&', substr($url, $queryPosition + 1)); + $kept = array_filter( + $pairs, + static fn (string $pair): bool => !str_starts_with(urldecode(explode('=', $pair, 2)[0]), $prefix) + ); + if (count($kept) === count($pairs)) { + return $url; + } + + $kept = array_filter($kept, static fn (string $pair): bool => $pair !== ''); + + return $kept === [] ? $base : $base . '?' . implode('&', $kept); + } + private function getQuery(): AnalyticsQueryInterface { return $this->configProvider->getValue(ConfigOption::AnalyticTracker) === 'matomo' diff --git a/tests/Unit/Domain/Analytics/Service/AnalyticsTrackingServiceTest.php b/tests/Unit/Domain/Analytics/Service/AnalyticsTrackingServiceTest.php index db8b2c73..2aa6e120 100644 --- a/tests/Unit/Domain/Analytics/Service/AnalyticsTrackingServiceTest.php +++ b/tests/Unit/Domain/Analytics/Service/AnalyticsTrackingServiceTest.php @@ -122,6 +122,22 @@ public function testExistingTrackingParametersAreReplacedAndTheFragmentStaysLast self::assertSame('https://example.com/p?a=1&b=2&utm_source=new&utm_content=HTML#section', $url); } + public function testOnlyQueryParametersStartingWithThePrefixAreRemoved(): void + { + $this->assertSame( + 'https://example.com/utm_page?x_utm_a=1&b=2', + $this->service->withParameters( + 'https://example.com/utm_page?x_utm_a=1&utm_source=old&&b=2&utm_medium=old', + [], + 'utm_' + ) + ); + $this->assertSame( + 'https://example.com/utm_page', + $this->service->withParameters('https://example.com/utm_page?utm_source=old', [], 'utm_') + ); + } + public function testEmptyParametersAreLeftOut(): void { self::assertSame( From 8d8ef6a7cae4c14b3b6e8290e1d6eeea8fe6c0a0 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 2 Nov 2026 18:18:23 +0400 Subject: [PATCH 30/30] feat: enhance LinkRewriter to handle PCRE errors gracefully and add unit tests --- src/Domain/Analytics/Service/LinkRewriter.php | 10 ++-- .../Analytics/Service/LinkRewriterTest.php | 53 +++++++++++++++++++ 2 files changed, 59 insertions(+), 4 deletions(-) create mode 100644 tests/Unit/Domain/Analytics/Service/LinkRewriterTest.php diff --git a/src/Domain/Analytics/Service/LinkRewriter.php b/src/Domain/Analytics/Service/LinkRewriter.php index c0accbea..8da1e305 100644 --- a/src/Domain/Analytics/Service/LinkRewriter.php +++ b/src/Domain/Analytics/Service/LinkRewriter.php @@ -24,7 +24,8 @@ public function rewriteHtml(string $html, callable $rewrite): string return $html; } - return (string) preg_replace_callback( + // null means a PCRE error (e.g. the backtrack limit on a large body): keep the original instead of emptying it + return preg_replace_callback( self::HTML_ANCHOR_PATTERN, function (array $match) use ($rewrite): string { $linkText = strip_tags($match[5]); @@ -42,7 +43,7 @@ function (array $match) use ($rewrite): string { . $match[4] . $match[5] . ''; }, $html - ); + ) ?? $html; } /** @@ -56,7 +57,8 @@ public function rewriteText(string $text, callable $rewrite): string return $text; } - return (string) preg_replace_callback( + // null means a PCRE error: keep the original instead of emptying the text + return preg_replace_callback( self::TEXT_URL_PATTERN, function (array $match) use ($rewrite): string { $trailing = str_ends_with($match[0], '.') ? '.' : ''; @@ -66,6 +68,6 @@ function (array $match) use ($rewrite): string { return $url === null ? $match[0] : $url . $trailing; }, $text - ); + ) ?? $text; } } diff --git a/tests/Unit/Domain/Analytics/Service/LinkRewriterTest.php b/tests/Unit/Domain/Analytics/Service/LinkRewriterTest.php new file mode 100644 index 00000000..942f665b --- /dev/null +++ b/tests/Unit/Domain/Analytics/Service/LinkRewriterTest.php @@ -0,0 +1,53 @@ +backtrackLimit = (string) ini_get('pcre.backtrack_limit'); + } + + protected function tearDown(): void + { + ini_set('pcre.backtrack_limit', $this->backtrackLimit); + } + + public function testHtmlIsRewritten(): void + { + $result = (new LinkRewriter())->rewriteHtml( + '

link

', + static fn (string $url): string => $url . '?x=1' + ); + + $this->assertSame('

link

', $result); + } + + public function testHtmlIsKeptWhenThePatternFailsWithAPcreError(): void + { + $html = '' . str_repeat('word ', 200) . ''; + ini_set('pcre.backtrack_limit', '10'); + + $result = (new LinkRewriter())->rewriteHtml($html, static fn (string $url): string => $url . '?x=1'); + + $this->assertSame($html, $result); + } + + public function testTextIsRewritten(): void + { + $result = (new LinkRewriter())->rewriteText( + 'see https://example.com/a.', + static fn (string $url): string => $url . '?x=1' + ); + + $this->assertSame('see https://example.com/a?x=1.', $result); + } +}