Feat/email building with attachments #375
Conversation
📝 WalkthroughWalkthroughIntroduces a context-aware placeholder/resolver system, DTOs/enums, mail content builders/constructors, attachment/pdf support, many DI/service rewrites, message-format refactors, repository helpers, new exceptions/helpers, extensive unit tests, and adds setasign/fpdf runtime dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Proc as CampaignProcessor
participant Pre as MessagePrecacheService
participant Builder as CampaignMailContentBuilder
participant Placeholder as MessagePlaceholderProcessor
participant SysBuilder as SystemEmailBuilder / EmailBuilder
participant Mailer as RateLimitedCampaignMailer
participant Repo as SubscriberRepository
participant Remote as RemotePageFetcher
Proc->>Pre: request precache (messageId)
Pre->>Remote: fetch remote content if tokens present
Remote-->>Pre: remote content (or error)
Pre-->>Proc: MessagePrecacheDto (html/text, tokens resolved)
Proc->>Builder: build content for subscriber (MessagePrecacheDto)
Builder->>Repo: load subscriber by email
Builder->>Placeholder: process placeholders (PlaceholderContext)
Placeholder->>Repo: load attributes/supporting data
Placeholder-->>Builder: resolved html/text
Proc->>SysBuilder: buildPhplistEmail(messageId, MessagePrecacheDto,...)
SysBuilder-->>SysBuilder: applyContentAndFormatting (embed images, pdf/attachments)
SysBuilder-->>Mailer: send(Email)
Mailer->>Mailer: rate limiter awaitTurn / send / afterSend
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
TatevikGr
changed the title
TatevikGr
force-pushed
the
feat/user-personalizer
branch
from
0e21e56 to
5fd4e4a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
TatevikGr
force-pushed
the
feat/user-personalizer
branch
from
6a31c89 to
19f224b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@config/parameters.yml.dist`:
- Around line 135-138: The two parameter entries phplist.attachment_download_url
and phplist.attachment_repository_path use single %env(...)% placeholders while
the rest of the template uses the escaped form %%env(...)%%; update those
entries to use the double-percent escaping (i.e., change
%env(PHPLIST_ATTACHMENT_DOWNLOAD_URL)% ->
%%env(PHPLIST_ATTACHMENT_DOWNLOAD_URL)%% and
%env(PHPLIST_ATTACHMENT_REPOSITORY_PATH)% ->
%%env(PHPLIST_ATTACHMENT_REPOSITORY_PATH)%%) so the template remains consistent
with the parameter handler’s expected escaping.
In `@resources/translations/messages.en.xlf`:
- Around line 777-780: Remove the duplicate translation unit for the message
with resname="Insufficient memory to add attachment to campaign %campaignId%
%totalSize% - %memLimit%": locate the two <trans-unit> entries with ids QLlIQQh
and 28lDkjt and delete one of them (keep the other intact) so only a single
translation unit with that resname/source/target remains; ensure the remaining
entry preserves the existing <source> and <target> values and that XML remains
well-formed after removal.
In `@src/Domain/Common/FileHelper.php`:
- Around line 36-59: In writeFileToDirectory: avoid orphaning the temp file
created by tempnam by either renaming the temp file to the extension-added name
(use rename(tempPath, newPath)) or generate the final filename first and create
that directly; open the file in binary mode ('wb') instead of 'w'; check return
values of fopen, fwrite and fclose (ensure fopen did not return false, check
fwrite bytes written equals strlen($contents)), delete any created temp/orphan
file on failure, and only return the full path when write and close succeed;
reference tempnam, rename/basename, fopen, fwrite and fclose to locate and
update the logic.
In `@src/Domain/Configuration/Service/Placeholder/ContactUrlValueResolver.php`:
- Around line 22-28: The HTML escaping in ContactUrlValueResolver::__invoke uses
htmlspecialchars() with defaults; update the non-text branch to call
htmlspecialchars($this->config->getValue(ConfigOption::VCardUrl) ?? '',
ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') so escaping flags and encoding are
explicit and consistent with other URL resolvers (leave the text branch
returning the raw value via PlaceholderContext::isText as-is).
In `@src/Domain/Messaging/Service/AttachmentAdder.php`:
- Around line 177-196: copyAttachmentToRepository in AttachmentAdder stores the
full path returned by writeFileToDirectory into $att->setFilename(), causing
attachFromRepository to later prepend the repository path again and produce
duplicated paths; change copyAttachmentToRepository to extract and store only
the basename/filename (not the full path) returned by writeFileToDirectory
before calling $att->setFilename(), preserve existing error handling via
handleCopyFailure, and ensure attachFromRepository continues to reconstruct the
full path by concatenating $this->attachmentRepositoryPath and the stored
filename.
♻️ Duplicate comments (6)
src/Domain/Subscription/Repository/SubscriberRepository.php (1)
209-217: Unsafe array access will throw an exception when subscriber doesn't exist.Accessing
[0]ongetArrayResult()will cause anUndefined array key 0error if no subscriber matches the ID. This is a runtime crash waiting to happen.🐛 Proposed fix using getOneOrNullResult
public function getDataById(int $subscriberId): array { - return $this->createQueryBuilder('s') + $result = $this->createQueryBuilder('s') ->select('s') ->where('s.id = :subscriberId') ->setParameter('subscriberId', $subscriberId) ->getQuery() - ->getArrayResult()[0]; + ->getOneOrNullResult(\Doctrine\ORM\Query::HYDRATE_ARRAY); + + return $result ?? []; }resources/translations/messages.en.xlf (2)
753-772: The__prefix issue was already flagged.This was covered in a previous review comment. The target values should match the source values exactly for an English-to-English translation file.
773-812: Remove the__prefix from remaining target values.Same issue as the earlier entries - these target values incorrectly prefix with
__. For an English-to-English translation file (wheresource-language="en"andtarget-language="en"), targets should match sources. The__prefix will display to users as-is in the UI.Suggested fix
<trans-unit id="6LBFI8k" resname="This message contains attachments that can be viewed with a webbrowser"> <source>This message contains attachments that can be viewed with a webbrowser</source> - <target>__This message contains attachments that can be viewed with a webbrowser</target> + <target>This message contains attachments that can be viewed with a webbrowser</target> </trans-unit> <trans-unit id="QLlIQQh" resname="Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%"> <source>Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</source> - <target>__Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</target> + <target>Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</target> </trans-unit>Apply the same pattern (remove
__prefix) to all target values in lines 781-812.src/Domain/Messaging/Service/AttachmentAdder.php (1)
59-63: Guard against emptyTolist before indexing.
Line 60 assumes at least one recipient; an empty array will throw.🐛 Defensive fix
- $userEmail = $email->getTo()[0]->getAddress(); + $toRecipients = $email->getTo(); + if (empty($toRecipients)) { + $this->eventLogManager->log( + page: '', + entry: $this->translator->trans( + 'Cannot add text attachment link - no recipient set for campaign %campaignId%', + ['%campaignId%' => $campaignId] + ) + ); + return false; + } + $userEmail = $toRecipients[0]->getAddress();src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
233-256: Missing null check on system email inAttachmentCopyExceptionhandler.The
systemEmailBuilder->buildPhplistEmail()call at lines 243-248 can returnnull, but line 252 accesses$email->getTo()[0]->getAddress()without checking. This will throw aTypeErrorifProposed fix
$email = $this->systemEmailBuilder->buildPhplistEmail( messageId: $campaign->getId(), data: $data, inBlast: false, htmlPref: true, ); + + if ($email === null) { + $this->logger->error('Failed to build system error email for attachment exception'); + throw $e; + } $envelope = new Envelope( sender: new Address($this->bounceEmail, 'PHPList'), recipients: [new Address($email->getTo()[0]->getAddress())], );src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
117-137: Copy-paste bug:$adminAddressuses wrong config key.Line 122 fetches the admin address using
ConfigOption::UnsubscribeUrlinstead ofConfigOption::AdminAddress. This meansList-Ownerheader will contain the unsubscribe URL rather than an admin email address.Proposed fix
- $adminAddress = $this->configProvider->getValue(ConfigOption::UnsubscribeUrl) ?? ''; + $adminAddress = $this->configProvider->getValue(ConfigOption::AdminAddress) ?? '';
🧹 Nitpick comments (3)
src/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandler.php (1)
65-76: N+1 query pattern — repository already has a batch method.This loops and hits the DB for each list ID. The
SubscriberListRepositoryalready provides agetListNames(array $listIds)method that fetches all names in a single query.♻️ Proposed fix using the existing batch method
private function getListNames(array $listIds): string { - $names = []; - foreach ($listIds as $listId) { - $list = $this->subscriberListRepository->find($listId); - if ($list !== null) { - $names[] = $list->getName(); - } - } - - return implode(', ', $names); + $names = $this->subscriberListRepository->getListNames($listIds); + return implode(', ', $names); }src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php (1)
91-93: TODO for CLICKTRACK still pending.
Line 92 leaves a TODO in the send path—worth tracking to avoid regressions later.If you want, I can draft the CLICKTRACK insertion logic or open a tracking issue.
src/Domain/Messaging/Service/Builder/SystemEmailBuilder.php (1)
52-91: Looks good overall, but consider cleaning up commented code.The
buildPhplistEmailmethod flow is solid. However, lines 69-70 have commented-out reply-to logic. If this is intentional (not needed for system emails), consider removing it to keep the code clean. If it's a TODO, maybe add a comment explaining why.
| <trans-unit id="QLlIQQh" resname="Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%"> | ||
| <source>Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</source> | ||
| <target>__Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</target> | ||
| </trans-unit> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the translation file
fd "messages.en.xlf" -type f
# Check the size and examine the specific lines mentioned
wc -l resources/translations/messages.en.xlfRepository: phpList/core
Length of output: 286
🏁 Script executed:
# Now let's check for both duplicate entries in the file
cat -n resources/translations/messages.en.xlf | sed -n '770,810p'Repository: phpList/core
Length of output: 3685
🏁 Script executed:
# Search for usage of both IDs in the codebase (all file types, not just PHP)
echo "Searching for usage of ID QLlIQQh:"
rg -n "QLlIQQh" -t php -t js -t ts
echo ""
echo "Searching for usage of ID 28lDkjt:"
rg -n "28lDkjt" -t php -t js -t ts
echo ""
echo "Searching for references to translation keys (broader search):"
rg -n "QLlIQQh|28lDkjt" --type-list | head -20Repository: phpList/core
Length of output: 597
🏁 Script executed:
# Search more broadly - check if these resnames or IDs are used anywhere in the codebase
echo "Searching for resname pattern in all files:"
rg -n "Insufficient memory to add attachment to campaign"
echo ""
echo "Searching for the ID pattern in all files:"
rg -n "QLlIQQh|28lDkjt"Repository: phpList/core
Length of output: 1627
Remove duplicate translation entry.
Two identical translation units exist for the same message ("Insufficient memory to add attachment to campaign...") with different IDs: QLlIQQh (line 777) and 28lDkjt (line 797). Both have the same resname, source, and target. The message is used in the codebase but neither ID is directly referenced—only the string content matters. Remove one of these entries to eliminate confusion.
Suggested fix - remove duplicate
- <trans-unit id="28lDkjt" resname="Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%">
- <source>Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</source>
- <target>__Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</target>
- </trans-unit>🤖 Prompt for AI Agents
In `@resources/translations/messages.en.xlf` around lines 777 - 780, Remove the
duplicate translation unit for the message with resname="Insufficient memory to
add attachment to campaign %campaignId% %totalSize% - %memLimit%": locate the
two <trans-unit> entries with ids QLlIQQh and 28lDkjt and delete one of them
(keep the other intact) so only a single translation unit with that
resname/source/target remains; ensure the remaining entry preserves the existing
<source> and <target> values and that XML remains well-formed after removal.
| public function __invoke(PlaceholderContext $ctx): string | ||
| { | ||
| if ($ctx->isText()) { | ||
| return $this->config->getValue(ConfigOption::VCardUrl) ?? ''; | ||
| } | ||
|
|
||
| return htmlspecialchars($this->config->getValue(ConfigOption::VCardUrl) ?? ''); |
There was a problem hiding this comment.
Harden HTML escaping to match other URL resolvers.
This uses htmlspecialchars() with default flags/encoding, which is inconsistent with other resolvers and can produce unsafe or incorrect escaping depending on ini defaults. Use explicit flags/UTF‑8 like the rest of the URL resolvers.
🔧 Proposed fix
public function __invoke(PlaceholderContext $ctx): string
{
- if ($ctx->isText()) {
- return $this->config->getValue(ConfigOption::VCardUrl) ?? '';
- }
+ $url = $this->config->getValue(ConfigOption::VCardUrl) ?? '';
+ if ($ctx->isText()) {
+ return $url;
+ }
- return htmlspecialchars($this->config->getValue(ConfigOption::VCardUrl) ?? '');
+ return htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
}🤖 Prompt for AI Agents
In `@src/Domain/Configuration/Service/Placeholder/ContactUrlValueResolver.php`
around lines 22 - 28, The HTML escaping in ContactUrlValueResolver::__invoke
uses htmlspecialchars() with defaults; update the non-text branch to call
htmlspecialchars($this->config->getValue(ConfigOption::VCardUrl) ?? '',
ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') so escaping flags and encoding are
explicit and consistent with other URL resolvers (leave the text branch
returning the raw value via PlaceholderContext::isText as-is).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@resources/translations/messages.en.xlf`:
- Around line 773-812: Multiple new trans-unit targets (ids 6LBFI8k, QLlIQQh,
HhEWxtR, TNQR4ZC, _DfkneV, VXRuG2l, 28lDkjt, DmqU7IE, 021aois, FbYZdLJ)
mistakenly include a "__" prefix; remove the leading "__" from each <target>
value so the target text exactly matches the <source> text for those trans-units
(e.g., update trans-unit id 6LBFI8k target to "This message contains attachments
that can be viewed with a webbrowser" and likewise for the others). Ensure you
update every listed trans-unit target (not just the first few) so none retain
the "__" prefix.
In `@src/Domain/Configuration/Service/Placeholder/SignatureValueResolver.php`:
- Around line 31-35: The return from preg_replace in SignatureValueResolver::
(the line returning preg_replace(...
$this->config->getValue(ConfigOption::PoweredByImage) ?? '')) can be null on
error and the method is declared to return string; update the return so you
defensively ensure a string (e.g., capture the preg_replace result into a
variable and return (is_string($result) ? $result : '') or coalesce with an
empty string) so the method never returns null while keeping the existing
replacement logic intact.
In `@src/Domain/Messaging/Service/Builder/BaseEmailBuilder.php`:
- Around line 36-55: The validateRecipientAndSubject method currently only
checks for "\n" characters, leaving "\r" vulnerability for header injection;
update the checks in validateRecipientAndSubject (used in BaseEmailBuilder) to
detect both CR and LF characters (e.g. use a pattern that matches "\r" or "\n")
for both $to and $subject and keep the same eventLogManager->log messages and
early returns when invalid values are found.
♻️ Duplicate comments (10)
src/Domain/Configuration/Service/Placeholder/PreferencesValueResolver.php (1)
35-45: URL-encode theuidparameter to prevent broken links.The
uidvalue from$ctx->getUser()->getUniqueId()is concatenated directly into the URL without encoding. If the uid contains URL-special characters (&,=,?, etc.), the link will be malformed.🔧 Suggested fix
- return '<a href="' - . $safeUrl - . htmlspecialchars($sep) - . 'uid=' - . $ctx->getUser()->getUniqueId() - . '">' - . $safeLabel - . '</a> '; + return '<a href="' + . $safeUrl + . htmlspecialchars($sep) + . 'uid=' + . rawurlencode($ctx->getUser()->getUniqueId()) + . '">' + . $safeLabel + . '</a> '; } - return sprintf('%s%suid=%s', $url, $sep, $ctx->getUser()->getUniqueId()); + return sprintf('%s%suid=%s', $url, $sep, rawurlencode($ctx->getUser()->getUniqueId()));src/Domain/Common/FileHelper.php (1)
36-60: Binary attachments still at risk: orphan temp file and text-mode write.This method has interconnected bugs that can corrupt binary attachments and leave orphan files:
Orphan temp file:
tempnam()creates a real file on disk at line 42. Appending the extension at line 48 only modifies the$newFilevariable—the original temp file remains. Then you write to a different path constructed at line 52, leaving the original orphaned.Text-mode write: Line 54 uses
'w'instead of'wb', which can corrupt binary data on Windows due to\r\nconversion.Unchecked I/O:
fopen()andfwrite()return values aren't checked—if either fails, you still return the path as if it succeeded.🔧 Suggested fix
public function writeFileToDirectory(string $directory, string $originalFilename, string $contents): ?string { $pathInfo = pathinfo($originalFilename); $name = $pathInfo['filename'] === '' ? 'file' : $pathInfo['filename']; $ext = $pathInfo['extension'] ?? ''; - $newFile = tempnam($directory, $name); - if ($newFile === false) { + $tmpFile = tempnam($directory, $name); + if ($tmpFile === false) { return null; } + $fullPath = $tmpFile; if ($ext !== '') { - $newFile .= '.' . $ext; + $fullPath = $tmpFile . '.' . $ext; + if (!rename($tmpFile, $fullPath)) { + `@unlink`($tmpFile); + return null; + } } - $relativeName = basename($newFile); - - $fullPath = $directory . '/' . $relativeName; - $fileHandle = fopen($fullPath, 'w'); - - fwrite($fileHandle, $contents); - fclose($fileHandle); + $fileHandle = fopen($fullPath, 'wb'); + if ($fileHandle === false) { + `@unlink`($fullPath); + return null; + } + + try { + if (fwrite($fileHandle, $contents) === false) { + `@unlink`($fullPath); + return null; + } + } finally { + fclose($fileHandle); + } return $fullPath; }config/parameters.yml.dist (1)
135-138: Use the same env-placeholder escaping as the rest of the template.
These two entries still use single%env(...)%while the file consistently uses%%env(...)%%.🔧 Suggested fix
- phplist.attachment_download_url: '%env(PHPLIST_ATTACHMENT_DOWNLOAD_URL)%' + phplist.attachment_download_url: '%%env(PHPLIST_ATTACHMENT_DOWNLOAD_URL)%%' env(PHPLIST_ATTACHMENT_DOWNLOAD_URL): 'https://example.com/download/' - phplist.attachment_repository_path: '%env(PHPLIST_ATTACHMENT_REPOSITORY_PATH)%' + phplist.attachment_repository_path: '%%env(PHPLIST_ATTACHMENT_REPOSITORY_PATH)%%' env(PHPLIST_ATTACHMENT_REPOSITORY_PATH): '/tmp'src/Domain/Messaging/Service/Builder/BaseEmailBuilder.php (1)
126-139: Avoid duplicateList-Unsubscribeheaders for campaign sends.
Campaign emails also add a UID-basedList-Unsubscribeheader inapplyCampaignHeaders, so the base header causes duplicates. Also guard for empty config values.🔧 Gate base header to non-blast + non-empty URL
- $removeUrl = $this->configProvider->getValue(ConfigOption::UnsubscribeUrl); - $sep = !str_contains($removeUrl, '?') ? '?' : '&'; - $email->getHeaders()->addTextHeader( - 'List-Unsubscribe', - sprintf( - '<%s%s%s>', - $removeUrl, - $sep, - http_build_query([ - 'email' => $destinationEmail, - 'jo' => 1, - ]) - ) - ); + $removeUrl = $this->configProvider->getValue(ConfigOption::UnsubscribeUrl); + if (!$inBlast && !empty($removeUrl)) { + $sep = !str_contains($removeUrl, '?') ? '?' : '&'; + $email->getHeaders()->addTextHeader( + 'List-Unsubscribe', + sprintf( + '<%s%s%s>', + $removeUrl, + $sep, + http_build_query([ + 'email' => $destinationEmail, + 'jo' => 1, + ]) + ) + ); + }src/Domain/Messaging/Service/AttachmentAdder.php (2)
59-68: Guard against empty recipients before indexinggetTo().
$email->getTo()[0]will throw if no recipients are set.🔧 Defensive fix
- $userEmail = $email->getTo()[0]->getAddress(); + $toRecipients = $email->getTo(); + if (empty($toRecipients)) { + $this->eventLogManager->log( + page: '', + entry: $this->translator->trans( + 'Cannot add text attachment link - no recipient set for campaign %campaignId%', + ['%campaignId%' => $campaignId] + ) + ); + return false; + } + $userEmail = $toRecipients[0]->getAddress(); // todo: add endpoint in rest-api project $viewUrl = $this->attachmentDownloadUrl . '/?id=' . $att->getId() . '&uid=' . $userEmail;
184-195: Store only the filename, not the full path.
writeFileToDirectory()returns a full path, which later gets prefixed again.🔧 Suggested fix
- $relativeName = $this->fileHelper->writeFileToDirectory( + $fullPath = $this->fileHelper->writeFileToDirectory( directory: $this->attachmentRepositoryPath, originalFilename: $remoteFile, contents: $contents ); - if ($relativeName === null) { + if ($fullPath === null) { $this->handleCopyFailure($remoteFile, $campaignId, $key); return; } - $att->setFilename($relativeName); + $att->setFilename(basename($fullPath));src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
119-123: Admin address uses the wrong config key.
$adminAddressshould come fromConfigOption::AdminAddress, notUnsubscribeUrl.🔧 Fix the config key
- $adminAddress = $this->configProvider->getValue(ConfigOption::UnsubscribeUrl) ?? ''; + $adminAddress = $this->configProvider->getValue(ConfigOption::AdminAddress) ?? '';resources/translations/messages.en.xlf (2)
753-772: Remove the__prefix from target values.These new translation units have
__prefixed to their target values (e.g.,__Unsubscribe,__This link). For an English-to-English translation file, targets should match their sources exactly. The__prefix will display to users as-is, breaking UI text.Examples needing fixes
- <target>__Unsubscribe</target> + <target>Unsubscribe</target> - <target>__This link</target> + <target>This link</target> - <target>__Confirm</target> + <target>Confirm</target>
777-780: Remove duplicate translation entry.There are two identical translation units for "Insufficient memory to add attachment to campaign..." with different IDs (
QLlIQQhat line 777 and28lDkjtat line 797). They have the same resname, source, and target. Remove one to eliminate confusion.Suggested fix - remove the duplicate
- <trans-unit id="28lDkjt" resname="Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%"> - <source>Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</source> - <target>__Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</target> - </trans-unit>Also applies to: 797-800
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
243-254: Missing null check before accessing$email->getTo()in AttachmentCopyException handler.Hey, so the
systemEmailBuilder->buildPhplistEmail()call at lines 243-248 can returnnull, but you're accessing$email->getTo()[0]->getAddress()at line 252 without checking first. This will throw aTypeErrorifThe past review comment mentioned this was addressed, but looking at the current code, it seems the fix didn't make it in.
Suggested fix
$email = $this->systemEmailBuilder->buildPhplistEmail( messageId: $campaign->getId(), data: $data, inBlast: false, htmlPref: true, ); + if ($email === null) { + $this->logger->error('Failed to build system error email for attachment exception'); + throw $e; + } $envelope = new Envelope( sender: new Address($this->bounceEmail, 'PHPList'), recipients: [new Address($email->getTo()[0]->getAddress())], );
🧹 Nitpick comments (3)
src/Domain/Common/FileHelper.php (1)
11-14: Clarify intent: empty files are considered invalid.
filesize($path)returns0for empty files, which is falsy. SoisValidFilereturnsfalsefor zero-byte files. If that's intentional (attachments shouldn't be empty), this is fine. Otherwise, consider usingfilesize($path) !== falseif you just want to check the file exists and is readable.src/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandler.php (1)
65-76: N+1 query: use the existing batch method instead.You're calling
find($listId)in a loop, which executes N separate queries. TheSubscriberListRepositoryalready has agetListNames(array $listIds)method that fetches all names in a single query.♻️ Suggested refactor
private function getListNames(array $listIds): string { - $names = []; - foreach ($listIds as $listId) { - $list = $this->subscriberListRepository->find($listId); - if ($list !== null) { - $names[] = $list->getName(); - } - } - - return implode(', ', $names); + return implode(', ', $this->subscriberListRepository->getListNames($listIds)); }src/Domain/Configuration/Service/MessagePlaceholderProcessor.php (1)
46-63:FORWARDFORMis a stubbed placeholder with no implementation.The hardcoded empty string at line 50 is the only reference in the codebase. If intentional (planned for later or handled elsewhere), add a comment explaining the intent. Otherwise, consider marking it
TODOor removing it if it's legacy code.
| <trans-unit id="6LBFI8k" resname="This message contains attachments that can be viewed with a webbrowser"> | ||
| <source>This message contains attachments that can be viewed with a webbrowser</source> | ||
| <target>__This message contains attachments that can be viewed with a webbrowser</target> | ||
| </trans-unit> | ||
| <trans-unit id="QLlIQQh" resname="Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%"> | ||
| <source>Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</source> | ||
| <target>__Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</target> | ||
| </trans-unit> | ||
| <trans-unit id="HhEWxtR" resname="Add us to your address book"> | ||
| <source>Add us to your address book</source> | ||
| <target>__Add us to your address book</target> | ||
| </trans-unit> | ||
| <trans-unit id="TNQR4ZC" resname="phpList system error"> | ||
| <source>phpList system error</source> | ||
| <target>__phpList system error</target> | ||
| </trans-unit> | ||
| <trans-unit id="_DfkneV" resname="Error, when trying to send campaign %campaignId% the attachment (%remoteFile%) could not be copied to the repository. Check for permissions."> | ||
| <source>Error, when trying to send campaign %campaignId% the attachment (%remoteFile%) could not be copied to the repository. Check for permissions.</source> | ||
| <target>__Error, when trying to send campaign %campaignId% the attachment (%remoteFile%) could not be copied to the repository. Check for permissions.</target> | ||
| </trans-unit> | ||
| <trans-unit id="VXRuG2l" resname="failed to open attachment (%remoteFile%) to add to campaign %campaignId%"> | ||
| <source>failed to open attachment (%remoteFile%) to add to campaign %campaignId%</source> | ||
| <target>__failed to open attachment (%remoteFile%) to add to campaign %campaignId%</target> | ||
| </trans-unit> | ||
| <trans-unit id="28lDkjt" resname="Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%"> | ||
| <source>Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</source> | ||
| <target>__Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</target> | ||
| </trans-unit> | ||
| <trans-unit id="DmqU7IE" resname="Attachment %remoteFile% does not exist"> | ||
| <source>Attachment %remoteFile% does not exist</source> | ||
| <target>__Attachment %remoteFile% does not exist</target> | ||
| </trans-unit> | ||
| <trans-unit id="021aois" resname="Error, when trying to send campaign %campaignId% the attachment (%remoteFile%) could not be found in the repository."> | ||
| <source>Error, when trying to send campaign %campaignId% the attachment (%remoteFile%) could not be found in the repository.</source> | ||
| <target>__Error, when trying to send campaign %campaignId% the attachment (%remoteFile%) could not be found in the repository.</target> | ||
| </trans-unit> | ||
| <trans-unit id="FbYZdLJ" resname="Location"> | ||
| <source>Location</source> | ||
| <target>__Location</target> | ||
| </trans-unit> |
There was a problem hiding this comment.
All new attachment/system error translations have the __ prefix issue too.
Just a heads up - all the newly added translations from line 773-812 have the same __ prefix problem. When fixing, make sure to hit all of them, not just the first batch.
🤖 Prompt for AI Agents
In `@resources/translations/messages.en.xlf` around lines 773 - 812, Multiple new
trans-unit targets (ids 6LBFI8k, QLlIQQh, HhEWxtR, TNQR4ZC, _DfkneV, VXRuG2l,
28lDkjt, DmqU7IE, 021aois, FbYZdLJ) mistakenly include a "__" prefix; remove the
leading "__" from each <target> value so the target text exactly matches the
<source> text for those trans-units (e.g., update trans-unit id 6LBFI8k target
to "This message contains attachments that can be viewed with a webbrowser" and
likewise for the others). Ensure you update every listed trans-unit target (not
just the first few) so none retain the "__" prefix.
| protected function validateRecipientAndSubject(?string $to, ?string $subject): bool | ||
| { | ||
| if (!$to || trim($to) === '') { | ||
| $this->eventLogManager->log('', sprintf('Error: empty To: in message with subject %s to send', $subject)); | ||
|
|
||
| return false; | ||
| } | ||
| if (!$subject || trim($subject) === '') { | ||
| $this->eventLogManager->log('', sprintf('Error: empty Subject: in message to send to %s', $to)); | ||
|
|
||
| return false; | ||
| } | ||
| if (preg_match("/\n/", $to)) { | ||
| $this->eventLogManager->log('', 'Error: invalid recipient, containing newlines, email blocked'); | ||
|
|
||
| return false; | ||
| } | ||
| if (preg_match("/\n/", $subject)) { | ||
| $this->eventLogManager->log('', 'Error: invalid subject, containing newlines, email blocked'); | ||
|
|
There was a problem hiding this comment.
Block CRLF too in recipient/subject validation.
Only \n is checked, so \r can still slip through header injection.
🔧 Tighten validation
- if (preg_match("/\n/", $to)) {
+ if (preg_match("/[\r\n]/", $to)) {
$this->eventLogManager->log('', 'Error: invalid recipient, containing newlines, email blocked');
return false;
}
- if (preg_match("/\n/", $subject)) {
+ if (preg_match("/[\r\n]/", $subject)) {
$this->eventLogManager->log('', 'Error: invalid subject, containing newlines, email blocked');
return false;
}🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/Builder/BaseEmailBuilder.php` around lines 36 -
55, The validateRecipientAndSubject method currently only checks for "\n"
characters, leaving "\r" vulnerability for header injection; update the checks
in validateRecipientAndSubject (used in BaseEmailBuilder) to detect both CR and
LF characters (e.g. use a pattern that matches "\r" or "\n") for both $to and
$subject and keep the same eventLogManager->log messages and early returns when
invalid values are found.
19f224b to
f90f636
Compare
TatevikGr
force-pushed
the
feat/user-personalizer
branch
from
f90f636 to
8647fcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Domain/Configuration/Service/Placeholder/BlacklistUrlValueResolver.php`:
- Around line 25-35: In BlacklistUrlValueResolver::__invoke, guard against an
empty base by checking the value returned from
$this->config->getValue(ConfigOption::BlacklistUrl) before calling
$this->urlBuilder->withEmail(); if the base is empty or null, return an empty
string (respecting the method return type) so you don't call
LegacyUrlBuilder::withEmail() with an empty host and produce malformed URLs;
keep the existing htmlspecialchars branch unchanged but ensure it only runs when
a non-empty URL was generated.
♻️ Duplicate comments (10)
src/Domain/Configuration/Service/Placeholder/ContactUrlValueResolver.php (1)
24-28: Harden HTML escaping to match other URL resolvers.
htmlspecialchars()defaults can be unsafe/inconsistent. Use explicit flags/UTF‑8 and reuse the URL value.🔧 Proposed fix
public function __invoke(PlaceholderContext $ctx): string { - if ($ctx->isText()) { - return $this->config->getValue(ConfigOption::VCardUrl) ?? ''; - } + $url = $this->config->getValue(ConfigOption::VCardUrl) ?? ''; + if ($ctx->isText()) { + return $url; + } - return htmlspecialchars($this->config->getValue(ConfigOption::VCardUrl) ?? ''); + return htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); }src/Domain/Configuration/Service/Placeholder/SignatureValueResolver.php (1)
31-35: Handle potentialnullreturn frompreg_replace.The null coalescing for the config value is in place, nice. However,
preg_replaceitself can returnnullon error (e.g., regex failure), and the method's return type isstring. Worth adding a fallback.Suggested fix
- return preg_replace( + return preg_replace( '/src=".*power-phplist.png"/', 'src="powerphplist.png"', $this->config->getValue(ConfigOption::PoweredByImage) ?? '' - ); + ) ?? '';resources/translations/messages.en.xlf (2)
753-812: Strip the leading__from the new English targets (Line 753-812).These targets should mirror their sources in
messages.en.xlf; otherwise the prefix will render in UI/email text.🔧 Example fix
- <target>__Unsubscribe</target> + <target>Unsubscribe</target>Apply the same removal for all new
<target>values in this block (including CDATA targets if any).
777-800: Remove the duplicate “Insufficient memory…” translation entry (Line 777-800).The message appears twice with different IDs (
QLlIQQhand28lDkjt). Keep one to avoid ambiguity.🔧 Example fix (remove the duplicate)
- <trans-unit id="28lDkjt" resname="Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%"> - <source>Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</source> - <target>__Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit%</target> - </trans-unit>src/Domain/Messaging/Service/Builder/BaseEmailBuilder.php (2)
48-54: Block\rin header injection validation.Only
\nis checked;\rcan still be used for header injection.🔧 Tighten validation
- if (preg_match("/\n/", $to)) { + if (preg_match("/[\r\n]/", $to)) { $this->eventLogManager->log('', 'Error: invalid recipient, containing newlines, email blocked'); return false; } - if (preg_match("/\n/", $subject)) { + if (preg_match("/[\r\n]/", $subject)) { $this->eventLogManager->log('', 'Error: invalid subject, containing newlines, email blocked'); return false; }
126-139: Guard missing Unsubscribe URL and avoid duplicate header in campaigns.
getValue()is nullable;str_contains(null, ...)will throw. Also, campaign emails already add aList-Unsubscribeheader inEmailBuilder::applyCampaignHeaders, so this duplicates it.🛠️ Suggested fix
- $removeUrl = $this->configProvider->getValue(ConfigOption::UnsubscribeUrl); - $sep = !str_contains($removeUrl, '?') ? '?' : '&'; - $email->getHeaders()->addTextHeader( - 'List-Unsubscribe', - sprintf( - '<%s%s%s>', - $removeUrl, - $sep, - http_build_query([ - 'email' => $destinationEmail, - 'jo' => 1, - ]) - ) - ); + $removeUrl = $this->configProvider->getValue(ConfigOption::UnsubscribeUrl) ?? ''; + if (!$inBlast && $removeUrl !== '') { + $sep = !str_contains($removeUrl, '?') ? '?' : '&'; + $email->getHeaders()->addTextHeader( + 'List-Unsubscribe', + sprintf( + '<%s%s%s>', + $removeUrl, + $sep, + http_build_query([ + 'email' => $destinationEmail, + 'jo' => 1, + ]) + ) + ); + }src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
243-254: Null‑safe system error email before dereferencing recipients.
buildPhplistEmail()can returnnull; dereferencing$email->getTo()[0]will crash.🩹 Proposed guard
$email = $this->systemEmailBuilder->buildPhplistEmail( messageId: $campaign->getId(), data: $data, inBlast: false, htmlPref: true, ); + if ($email === null || empty($email->getTo())) { + $this->logger->error('Failed to build system error email for attachment exception'); + throw $e; + } $envelope = new Envelope( sender: new Address($this->bounceEmail, 'PHPList'), recipients: [new Address($email->getTo()[0]->getAddress())], );src/Domain/Messaging/Service/AttachmentAdder.php (2)
59-68: Guard empty recipients before building attachment URLs.
$email->getTo()[0]will throw if no recipients are set.🛡️ Safer handling
case OutputFormat::Text: - $userEmail = $email->getTo()[0]->getAddress(); + $toRecipients = $email->getTo(); + if (empty($toRecipients)) { + $this->eventLogManager->log( + page: '', + entry: $this->translator->trans( + 'Cannot add text attachment link - no recipient set for campaign %campaignId%', + ['%campaignId%' => $campaignId] + ) + ); + return false; + } + $userEmail = $toRecipients[0]->getAddress();
184-196: Store only the filename, not the full path.
writeFileToDirectory()returns a full path; later you prefix the repository path again, creating a broken path.🔧 Suggested fix
- $relativeName = $this->fileHelper->writeFileToDirectory( + $fullPath = $this->fileHelper->writeFileToDirectory( directory: $this->attachmentRepositoryPath, originalFilename: $remoteFile, contents: $contents ); - if ($relativeName === null) { + if ($fullPath === null) { $this->handleCopyFailure($remoteFile, $campaignId, $key); return; } - $att->setFilename($relativeName); + $att->setFilename(basename($fullPath));src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
119-123: UseAdminAddressforList-Owner.
$adminAddressis pulled fromUnsubscribeUrl, which is the wrong config key for the List-Owner header.🔧 Fix
- $adminAddress = $this->configProvider->getValue(ConfigOption::UnsubscribeUrl) ?? ''; + $adminAddress = $this->configProvider->getValue(ConfigOption::AdminAddress) ?? '';
🧹 Nitpick comments (3)
src/Domain/Configuration/Service/Placeholder/ForwardUrlValueResolver.php (1)
24-37: Useurlencode()for query parameters to match the codebase pattern and improve defensiveness.While
uniqueIdis currently generated as hex-only (bin2hex(random_bytes(16))), other similar handlers—PasswordResetMessageHandlerandSubscriberConfirmationMessageHandler—already wrap it withurlencode()when building URLs. For consistency and defensive programming, apply the same here:$uid = urlencode($ctx->getUser()->getUniqueId()); $mid = (int)$ctx->messageId(); if ($ctx->isHtml()) { return htmlspecialchars( sprintf('%s%suid=%s&mid=%d', $url, $sep, $uid, $mid), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8' ); } return sprintf('%s%suid=%s&mid=%d', $url, $sep, $uid, $mid);src/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandler.php (1)
65-75: Avoid N+1 list lookups ingetListNames.Looping
find()per ID will issue one query per list. The repository already has a bulk query helper.♻️ Suggested refactor
- $names = []; - foreach ($listIds as $listId) { - $list = $this->subscriberListRepository->find($listId); - if ($list !== null) { - $names[] = $list->getName(); - } - } - - return implode(', ', $names); + $names = $this->subscriberListRepository->getListNames($listIds); + return implode(', ', $names);src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
210-219: Handle null build result with status/logging.If
buildPhplistEmail()returnsnull, the user message staysActiveand no trace is logged. Consider marking asNotSent(or similar) and logging.💡 Possible tweak
$result = $this->campaignEmailBuilder->buildPhplistEmail( messageId: $campaign->getId(), data: $processed, skipBlacklistCheck: false, inBlast: true, htmlPref: $subscriber->hasHtmlEmail(), ); if ($result === null) { + $this->updateUserMessageStatus($userMessage, UserMessageStatus::NotSent); + $this->logger->warning($this->translator->trans('Email could not be built for: %email%', [ + '%email%' => $subscriber->getEmail(), + ])); return; }
src/Domain/Configuration/Service/Placeholder/BlacklistUrlValueResolver.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php`:
- Around line 33-39: The returned tracking IMG markup concatenates raw $base,
$ctx->getUser()->getUniqueId() and $ctx->messageId() into an attribute, risking
broken attributes or XSS; in UserTrackValueResolver (the method that builds this
return string) URL-encode the query parameters (use rawurlencode or urlencode)
for the UID and message ID, ensure you normalize/validate $base (e.g. allowlist
or parse and rebuild) and then escape the final URL when embedding in HTML (e.g.
htmlspecialchars) so the src attribute cannot be injected or broken.
In `@tests/Unit/Domain/Common/FileHelperTest.php`:
- Around line 14-18: The setUp method creates a temporary directory but doesn't
check mkdir's result; update setUp() to fail fast if directory creation fails by
checking the return value of mkdir (or verifying is_dir after the call) and
throwing an exception or calling $this->fail with a clear message referencing
$this->tmpDir so tests stop immediately on mkdir failure; target the setUp()
method and the $this->tmpDir property to implement this guard.
In
`@tests/Unit/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolverTest.php`:
- Around line 77-79: The test expectation escapes HTML differently than the
resolver; update the assertion in PreferencesUrlValueResolverTest so both uses
of htmlspecialchars match the resolver's flags and encoding (use ENT_QUOTES |
ENT_SUBSTITUTE and 'UTF-8') when escaping $raw and '&' in the sprintf, ensuring
the expected string is produced by the same escaping behavior as the resolver.
In
`@tests/Unit/Domain/Configuration/Service/Placeholder/PreferencesValueResolverTest.php`:
- Around line 84-85: The test includes a redundant assertion comparing the
locally-built $expectedHref to a hardcoded string; instead make the assertion
verify the resolver output by replacing or changing the assertion so it asserts
the resolver result ($out) equals the expected value (e.g.,
assertSame($expectedHref, $out)) or simply remove the redundant local-to-literal
check; target the assertion in PreferencesValueResolverTest where $expectedHref
and $out are used.
♻️ Duplicate comments (2)
src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php (1)
33-38: Guard against null messageId before appendingm=.
Still seeing the same nullable issue noted previously; an emptym=is possible here.src/Domain/Configuration/Service/Placeholder/ForwardValueResolver.php (1)
33-48: Encodeuidand handle nullablemessageIdto avoid broken URLs.
uidis injected raw into the query string, andmessageId()can be null while%dexpects an int. This can break URLs or emit warnings.🔧 Proposed fix
if ($ctx->isHtml()) { $label = $this->translator->trans('This link'); + $uid = urlencode($ctx->getUser()->getUniqueId()); + $mid = urlencode((string) ($ctx->messageId() ?? '')); return '<a href="' . htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . htmlspecialchars($sep) . 'uid=' - . $ctx->getUser()->getUniqueId() + . $uid . '&mid=' - . $ctx->messageId() + . $mid . '">' . htmlspecialchars($label, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . '</a> '; } - return sprintf('%s%suid=%s&mid=%d ', $url, $sep, $ctx->getUser()->getUniqueId(), $ctx->messageId()); + $uid = urlencode($ctx->getUser()->getUniqueId()); + $mid = urlencode((string) ($ctx->messageId() ?? '')); + return sprintf('%s%suid=%s&mid=%s ', $url, $sep, $uid, $mid);
🧹 Nitpick comments (12)
tests/Unit/Domain/Configuration/Service/Placeholder/SubscribeUrlValueResolverTest.php (1)
64-73: Optional: Consider using a data provider for format variants.Testing both
HtmlandTextformats in one method works fine here, but if you want stricter isolation (one assertion per test), you could use a@dataProvider. Totally optional given the simplicity.💡 Example with data provider (optional)
+ /** + * `@dataProvider` nullConfigFormatProvider + */ + public function testReturnsEmptyStringWhenConfigNull(OutputFormat $format): void { $this->config->method('getValue') ->with(ConfigOption::SubscribeUrl) ->willReturn(null); $resolver = new SubscribeUrlValueResolver($this->config); - $this->assertSame('', $resolver(new PlaceholderContext(user: $this->makeUser(), format: OutputFormat::Html))); - $this->assertSame('', $resolver(new PlaceholderContext(user: $this->makeUser(), format: OutputFormat::Text))); + $this->assertSame('', $resolver(new PlaceholderContext(user: $this->makeUser(), format: $format))); + } + + public static function nullConfigFormatProvider(): array + { + return [ + 'Html format' => [OutputFormat::Html], + 'Text format' => [OutputFormat::Text], + ]; }src/Domain/Configuration/Service/Placeholder/JumpoffValueResolver.php (1)
25-38: Consider usingLegacyUrlBuilderfor thejoparam as well.The
uidparam is added viaLegacyUrlBuilder->withUid(), butjo=1is appended via string concatenation. While this works (sincewithUidguarantees a query string exists), it's inconsistent and could break if the URL building logic changes.💡 Optional: Add a method to LegacyUrlBuilder for jo param
// In LegacyUrlBuilder.php, add: + public function withJumpoff(string $baseUrl): string + { + return $this->withQueryParam($baseUrl, 'jo', '1'); + } // Then in JumpoffValueResolver.php: - return $url . '&jo=1'; + return $this->urlBuilder->withJumpoff($url);tests/Unit/Domain/Configuration/Service/Placeholder/JumpoffValueResolverTest.php (1)
42-58: Optional: Add edge case test for empty config.The resolver returns
''whenUnsubscribeUrlis empty (lines 28-30 in the implementation). A test confirming this behavior would complete the coverage.🧪 Suggested test for empty config
public function testReturnsEmptyStringWhenUnsubscribeUrlIsEmpty(): void { $this->config->method('getValue') ->with(ConfigOption::UnsubscribeUrl) ->willReturn(''); $this->urlBuilder->expects($this->never()) ->method('withUid'); $ctx = new PlaceholderContext(user: $this->makeUser(), format: OutputFormat::Text); $resolver = new JumpoffValueResolver($this->config, $this->urlBuilder); $this->assertSame('', $resolver($ctx)); }tests/Unit/Domain/Common/FileHelperTest.php (1)
20-29: Optional: make cleanup recursive.If any future test creates subdirectories, the current cleanup will leave leftovers. A recursive delete keeps teardown robust.
🧹 Example recursive cleanup
protected function tearDown(): void { if (is_dir($this->tmpDir)) { - foreach (glob($this->tmpDir . '/*') ?: [] as $path) { - if (is_file($path) || is_link($path)) { - unlink($path); - } - } - rmdir($this->tmpDir); + $it = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($this->tmpDir, \FilesystemIterator::SKIP_DOTS), + \RecursiveIteratorIterator::CHILD_FIRST + ); + foreach ($it as $path) { + $path->isDir() ? rmdir($path->getPathname()) : unlink($path->getPathname()); + } + rmdir($this->tmpDir); } }tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php (1)
87-129: Consider a helper for Subscriber mocks to reduce repetition.
Same Subscriber mock setup repeats across tests; a small helper would cut noise.♻️ Optional refactor sketch
+ private function makeSubscriber(int $id, string $email): Subscriber&MockObject + { + $subscriber = $this->getMockBuilder(Subscriber::class) + ->disableOriginalConstructor() + ->onlyMethods(['getId', 'getEmail']) + ->getMock(); + $subscriber->method('getId')->willReturn($id); + $subscriber->method('getEmail')->willReturn($email); + + return $subscriber; + }Then in tests:
- $subscriber = $this->getMockBuilder(Subscriber::class) - ->disableOriginalConstructor() - ->onlyMethods(['getId', 'getEmail']) - ->getMock(); - $subscriber->method('getId')->willReturn(123); - $subscriber->method('getEmail')->willReturn('user@example.com'); + $subscriber = $this->makeSubscriber(123, 'user@example.com');src/Domain/Configuration/Service/Placeholder/ForwardMessageIdValueResolver.php (1)
42-42: Non-numeric message ID silently becomes 0.If someone writes
[FORWARD:abc], the(int)cast will silently producemid=0. Might be worth validating that$forwardMessageis actually numeric before casting, or at least documenting this behavior.Not blocking, just something to be aware of for debugging purposes.
Optional: Add numeric validation
$forwardMessage = trim($forwardMessage); if ($forwardMessage === '') { return ''; } + if (!ctype_digit($forwardMessage)) { + return ''; // or log a warning + } $messageId = (int) $forwardMessage;tests/Unit/Domain/Common/PdfGeneratorTest.php (1)
39-44: UseassertStringContainsStringfor clearer intent.
It reads more naturally thanstrpos+assertNotFalse, and sidesteps the quirk wherestrposreturns 0 (falsy but not false) when the substring is at the start.tests/Unit/Domain/Messaging/Service/Builder/EmailBuilderTest.php (1)
360-392: Consider a TODO or follow-up ticket for the List-Owner header bug.
The test currently asserts the known bug as-is. If a fix is planned, adding a TODO (or a separate regression test for the corrected behavior) would keep this from becoming sticky.tests/Unit/Domain/Configuration/Service/Placeholder/BlacklistValueResolverTest.php (1)
18-96: Solid test coverage for BlacklistValueResolver.The tests properly validate HTML escaping, translated labels, and text output paths. Nice job including special characters in the translation mock to verify escaping behavior.
One small gap: the production code has an early return for empty
BlacklistUrlconfig (returns''), but there's no test covering that edge case. Consider adding a test similar totestReturnsEmptyStringWhenConfigNullOrEmptyseen in other resolver tests in this PR for consistency.tests/Unit/Domain/Configuration/Service/Placeholder/ConfirmationUrlValueResolverTest.php (1)
16-103: Good coverage for URL query string handling.The tests properly validate both
?uid=and&uid=scenarios for HTML and Text formats. Thehtml_entity_decodeassertion on line 72 is a nice touch for verifying round-trip correctness.Missing: A test for when
ConfirmationUrlconfig is null/empty (production returns''). Would be good to add for parity with other resolver tests in this PR.tests/Unit/Domain/Configuration/Service/Placeholder/PreferencesValueResolverTest.php (2)
56-62: Minor: Unusual escaping approach and trailing space.The
htmlspecialchars('?')on line 57 will just return'?'unchanged since?isn't a special HTML character. This works but is slightly misleading. The trailing space in the expected output ('</a> ') on line 61 is a bit unusual—worth confirming this matches the productionPreferencesValueResolverbehavior.
17-109: Missing test for empty/null config.For consistency with other resolver tests in this PR (e.g.,
SubscribeValueResolverTest,UnsubscribeUrlValueResolverTest), consider adding a test case for whenPreferencesUrlconfig returns null/empty.
| return '<img src="' | ||
| . $base | ||
| . '/ut.php?u=' | ||
| . $ctx->getUser()->getUniqueId() | ||
| . '&m=' | ||
| . $ctx->messageId() | ||
| . '" width="1" height="1" border="0" alt="" />'; |
There was a problem hiding this comment.
Escape + URL‑encode the tracking URL before embedding in HTML.
$base/UID can contain characters that break attributes or enable injection; also params aren’t URL‑encoded. Please encode params and escape the final URL.
🔧 Suggested fix
- return '<img src="'
- . $base
- . '/ut.php?u='
- . $ctx->getUser()->getUniqueId()
- . '&m='
- . $ctx->messageId()
- . '" width="1" height="1" border="0" alt="" />';
+ $uid = rawurlencode($ctx->getUser()->getUniqueId());
+ $messageId = rawurlencode((string) $ctx->messageId());
+ $url = sprintf('%s/ut.php?u=%s&m=%s', $base, $uid, $messageId);
+ $safeUrl = htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
+
+ return '<img src="'
+ . $safeUrl
+ . '" width="1" height="1" border="0" alt="" />';🤖 Prompt for AI Agents
In `@src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php`
around lines 33 - 39, The returned tracking IMG markup concatenates raw $base,
$ctx->getUser()->getUniqueId() and $ctx->messageId() into an attribute, risking
broken attributes or XSS; in UserTrackValueResolver (the method that builds this
return string) URL-encode the query parameters (use rawurlencode or urlencode)
for the UID and message ID, ensure you normalize/validate $base (e.g. allowlist
or parse and rebuild) and then escape the final URL when embedding in HTML (e.g.
htmlspecialchars) so the src attribute cannot be injected or broken.
| protected function setUp(): void | ||
| { | ||
| $this->tmpDir = sys_get_temp_dir() . '/phplist-filehelper-' . bin2hex(random_bytes(6)); | ||
| mkdir($this->tmpDir, 0777, true); | ||
| } |
There was a problem hiding this comment.
Make temp dir creation failure explicit.
If mkdir fails (permissions, race, disk issues), later assertions will fail in less obvious ways. Consider asserting success or failing fast for clearer diagnostics.
🛠️ Suggested tweak
protected function setUp(): void
{
$this->tmpDir = sys_get_temp_dir() . '/phplist-filehelper-' . bin2hex(random_bytes(6));
- mkdir($this->tmpDir, 0777, true);
+ if (!mkdir($this->tmpDir, 0777, true) && !is_dir($this->tmpDir)) {
+ $this->fail('Failed to create temp directory: ' . $this->tmpDir);
+ }
}🤖 Prompt for AI Agents
In `@tests/Unit/Domain/Common/FileHelperTest.php` around lines 14 - 18, The setUp
method creates a temporary directory but doesn't check mkdir's result; update
setUp() to fail fast if directory creation fails by checking the return value of
mkdir (or verifying is_dir after the call) and throwing an exception or calling
$this->fail with a clear message referencing $this->tmpDir so tests stop
immediately on mkdir failure; target the setUp() method and the $this->tmpDir
property to implement this guard.
| $this->assertSame( | ||
| sprintf('%s%suid=%s', htmlspecialchars($raw), htmlspecialchars('&'), 'UID-PREF'), | ||
| $result |
There was a problem hiding this comment.
Align HTML escaping flags with resolver to avoid a false failure.
The resolver escapes with ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8', but the test uses default htmlspecialchars($raw); with the single quote in Line 67 this mismatches. Align the flags/encoding so the expectation matches production behavior.
✅ Suggested fix
- $this->assertSame(
- sprintf('%s%suid=%s', htmlspecialchars($raw), htmlspecialchars('&'), 'UID-PREF'),
- $result
- );
+ $this->assertSame(
+ sprintf(
+ '%s%suid=%s',
+ htmlspecialchars($raw, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
+ htmlspecialchars('&'),
+ 'UID-PREF'
+ ),
+ $result
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $this->assertSame( | |
| sprintf('%s%suid=%s', htmlspecialchars($raw), htmlspecialchars('&'), 'UID-PREF'), | |
| $result | |
| $this->assertSame( | |
| sprintf( | |
| '%s%suid=%s', | |
| htmlspecialchars($raw, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'), | |
| htmlspecialchars('&'), | |
| 'UID-PREF' | |
| ), | |
| $result | |
| ); |
🤖 Prompt for AI Agents
In
`@tests/Unit/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolverTest.php`
around lines 77 - 79, The test expectation escapes HTML differently than the
resolver; update the assertion in PreferencesUrlValueResolverTest so both uses
of htmlspecialchars match the resolver's flags and encoding (use ENT_QUOTES |
ENT_SUBSTITUTE and 'UTF-8') when escaping $raw and '&' in the sprintf, ensuring
the expected string is produced by the same escaping behavior as the resolver.
| $this->assertSame('https://example.com/prefs.php?a=1&uid=U-2', $expectedHref); | ||
| } |
There was a problem hiding this comment.
Redundant assertion on a local variable.
Line 84 asserts that $expectedHref equals a hardcoded string, but $expectedHref is a local variable you just constructed. This doesn't test the resolver—it tests your own string concatenation. You likely meant to assert $out contains the expected href, which is already covered by line 83.
Suggested fix
- $this->assertSame('<a href="' . $expectedHref . '">' . $expectedLabel . '</a> ', $out);
- $this->assertSame('https://example.com/prefs.php?a=1&uid=U-2', $expectedHref);
+ $this->assertSame('<a href="' . $expectedHref . '">' . $expectedLabel . '</a> ', $out);🤖 Prompt for AI Agents
In
`@tests/Unit/Domain/Configuration/Service/Placeholder/PreferencesValueResolverTest.php`
around lines 84 - 85, The test includes a redundant assertion comparing the
locally-built $expectedHref to a hardcoded string; instead make the assertion
verify the resolver output by replacing or changing the assertion so it asserts
the resolver result ($out) equals the expected value (e.g.,
assertSame($expectedHref, $out)) or simply remove the redundant local-to-literal
check; target the assertion in PreferencesValueResolverTest where $expectedHref
and $out are used.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Thanks for contributing to phpList!