Feat/email building with attachments #375
Conversation
📝 WalkthroughWalkthroughIntroduces a context-aware placeholder system, DTOs/enums, mail content builders and constructors, attachment/pdf support, extensive DI/service rewiring, message-format refactors, repository helpers, new exceptions/helpers, many tests updated, and a new composer runtime dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Proc as CampaignProcessor
participant Pre as MessagePrecacheService
participant Builder as CampaignMailContentBuilder
participant Placeholder as MessagePlaceholderProcessor
participant EmailB as SystemEmailBuilder / EmailBuilder
participant Mailer as RateLimitedCampaignMailer
participant Repo as SubscriberRepository
Proc->>Pre: request precache -> MessagePrecacheDto
Pre->>Builder: build html + text (may fetch remote content)
Builder->>Repo: load subscriber data
Builder-->>Pre: return [html, text]
Proc->>Placeholder: resolve placeholders (PlaceholderContext per subscriber)
Placeholder->>Repo: load attributes/supporting data
Placeholder-->>Proc: resolved content
Proc->>EmailB: buildPhplistEmail(messageId, MessagePrecacheDto, htmlPref,...)
EmailB-->>Mailer: send(Email, OutputFormat)
Mailer->>Mailer: awaitTurn (rate limiter) / dispatch
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 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; }
| public function __invoke(PlaceholderContext $ctx): string | ||
| { | ||
| $base = $this->config->getValue(ConfigOption::BlacklistUrl) ?? ''; | ||
| $url = $this->urlBuilder->withEmail($base, $ctx->getUser()->getEmail()); | ||
|
|
||
| if ($ctx->isHtml()) { | ||
| return htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); | ||
| } | ||
|
|
||
| return $url; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other URL value resolvers handle empty base URLs
ast-grep --pattern $'$this->config->getValue($_) ?? $_'
# Look for similar resolvers to compare implementation patterns
rg -n -A5 'implements PlaceholderValueResolverInterface' --type phpRepository: phpList/core
Length of output: 16190
🏁 Script executed:
#!/bin/bash
# Find the LegacyUrlBuilder class and check the withEmail method
fd -e php -x grep -l 'LegacyUrlBuilder' {} \;Repository: phpList/core
Length of output: 746
🏁 Script executed:
#!/bin/bash
# Check the ConfigOption enum to see all available config options
fd ConfigOption.php --type f | head -5Repository: phpList/core
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Search for how BlacklistUrl is used elsewhere in the codebase
rg -n 'BlacklistUrl' --type php -B2 -A2Repository: phpList/core
Length of output: 3024
🏁 Script executed:
#!/bin/bash
# Read the LegacyUrlBuilder class to understand withEmail behavior
cat src/Domain/Configuration/Service/LegacyUrlBuilder.phpRepository: phpList/core
Length of output: 1235
🏁 Script executed:
#!/bin/bash
# Compare similar resolvers like UnsubscribeUrlValueResolver and PreferencesUrlValueResolver
cat src/Domain/Configuration/Service/Placeholder/UnsubscribeUrlValueResolver.phpRepository: phpList/core
Length of output: 1115
🏁 Script executed:
#!/bin/bash
# Check the full BlacklistValueResolver implementation
cat src/Domain/Configuration/Service/Placeholder/BlacklistValueResolver.phpRepository: phpList/core
Length of output: 1443
Add a guard to handle empty base URL gracefully.
The concern is valid. When ConfigOption::BlacklistUrl isn't configured, LegacyUrlBuilder::withEmail() produces a malformed URL like https://?email=user@example.com because the host defaults to an empty string. This pattern affects other URL resolvers in the codebase as well (e.g., UnsubscribeUrlValueResolver, BlacklistValueResolver).
Consider returning an empty string when the base URL is not configured, or adding a check before calling withEmail():
$base = $this->config->getValue(ConfigOption::BlacklistUrl) ?? '';
if (!$base) {
return '';
}
$url = $this->urlBuilder->withEmail($base, $ctx->getUser()->getEmail());🤖 Prompt for AI Agents
In `@src/Domain/Configuration/Service/Placeholder/BlacklistUrlValueResolver.php`
around lines 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.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Thanks for contributing to phpList!