Feat: message sending-forwarding #374
Conversation
📝 WalkthroughWalkthroughReworks the messaging pipeline around a MessagePrecacheDto: introduces precaching, message data loading, modular placeholder resolution, new email builders/constructors, forwarding orchestration (guards, delivery, stats), image/attachment utilities, mail-size checks, many repository/DTO additions, DI wiring, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(63,81,181,0.5)
participant Handler as CampaignProcessorMessageHandler
end
rect rgba(0,150,136,0.5)
participant Loader as MessageDataLoader
participant Precacher as MessagePrecacheService
end
rect rgba(255,193,7,0.5)
participant Builder as EmailBuilder
participant Mailer as RateLimitedCampaignMailer
end
rect rgba(233,30,99,0.5)
participant DB as Database
end
Handler->>Loader: load consolidated message data
Loader-->>Handler: messageData
Handler->>Precacher: precacheMessage(campaign, messageData)
Precacher-->>Handler: MessagePrecacheDto
Handler->>Handler: handleAdminNotifications()
loop per subscriber
Handler->>Builder: buildCampaignEmail(messageId, MessagePrecacheDto)
Builder->>Builder: validate recipient & blacklist
Builder->>Builder: applyContentAndFormatting (embed/attach/pdf)
alt email built
Builder-->>Handler: Email + OutputFormat
Handler->>Mailer: send(Email)
Mailer-->>Handler: send result
Handler->>DB: persist subscriber status
else failure
Handler->>DB: mark NotSent/Failed
end
end
sequenceDiagram
rect rgba(63,81,181,0.5)
participant ForwardSvc as MessageForwardService
end
rect rgba(0,150,136,0.5)
participant Guard as ForwardingGuard
participant Precache as MessagePrecacheService
participant Content as ForwardContentService
end
rect rgba(255,193,7,0.5)
participant Builder as ForwardEmailBuilder
participant Delivery as ForwardDeliveryService
participant Stats as ForwardingStatsService
end
rect rgba(233,30,99,0.5)
participant Repo as UserMessageForwardRepository
participant Admin as AdminNotifier
end
ForwardSvc->>Guard: assertCanForward(uid, campaign)
Guard-->>ForwardSvc: Subscriber or throw
ForwardSvc->>Precache: precacheMessage(campaign)
ForwardSvc->>Content: getContents(campaign, subscriber, friendEmail, dto)
Content->>Builder: buildForwardEmail(...)
Builder-->>Content: Email or null
alt Email present
ForwardSvc->>Delivery: send(Email)
Delivery->>Repo: persist forward record (sent)
ForwardSvc->>Stats: incrementFriendsCount(subscriber)
ForwardSvc->>Admin: notifyForwardSucceeded(...)
else failure
ForwardSvc->>Repo: persist forward record (failed)
ForwardSvc->>Admin: notifyForwardFailed(...)
end
ForwardSvc->>Stats: updateFriendsCount(subscriber)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
157-166: Guard personalization against null text/footer to avoid runtime errorsRight now you call
personalize()ongetText()andgetFooter()unconditionally. GivenMessageContent::getText()/getFooter()are treated as nullable elsewhere, this can blow up with aTypeErrorif either isnull(and tests hide this by forcing non-null mocks).A small null-check keeps this safe:
private function handleEmailSending( Message $campaign, Subscriber $subscriber, UserMessage $userMessage, Message\MessageContent $precachedContent, ): void { - $processed = $this->messagePreparator->processMessageLinks($campaign->getId(), $precachedContent, $subscriber); - $processed->setText($this->userPersonalizer->personalize($processed->getText(), $subscriber->getEmail())); - $processed->setFooter($this->userPersonalizer->personalize($processed->getFooter(), $subscriber->getEmail())); + $processed = $this->messagePreparator->processMessageLinks( + $campaign->getId(), + $precachedContent, + $subscriber + ); + + $htmlText = $processed->getText(); + if ($htmlText !== null) { + $processed->setText( + $this->userPersonalizer->personalize($htmlText, $subscriber->getEmail()) + ); + } + + $footer = $processed->getFooter(); + if ($footer !== null) { + $processed->setFooter( + $this->userPersonalizer->personalize($footer, $subscriber->getEmail()) + ); + }This matches how
MessageProcessingPreparatoralready treats these fields and avoids surprising failures when campaigns have no HTML or footer.
🧹 Nitpick comments (1)
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
175-205: Content mocks align with new personalization flow; consider adding a null‑footer caseStubbing
getText()/getFooter()in these tests to return concrete HTML/footer strings is a good way to keep the new personalization path exercised without changing expectations.Once you add null-guards in the handler, it’d be worth adding a small test where
getFooter()(and/orgetText()) returnsnullto lock in that behavior and prevent regressions.Also applies to: 236-269, 291-325
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php(3 hunks)src/Domain/Messaging/Service/MessageProcessingPreparator.php(1 hunks)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php(1 hunks)tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php(6 hunks)tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php(0 hunks)
💤 Files with no reviewable changes (1)
- tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/Service/RateLimitedCampaignMailer.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/MessageProcessingPreparator.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (1)
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
src/Domain/Messaging/Message/AsyncEmailMessage.php (1)
getReplyTo(49-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (4)
src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
76-87: No behavioral change here; TODO still relevantThis is just a comment tweak; existing logic around
getText()/getFooter()and link replacement remains consistent and safe. All good.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
7-9: UserPersonalizer wiring looks goodImporting and injecting
UserPersonalizerinto the handler keeps responsibilities in the message handler (where persistence and orchestration already live) and lines up with the new personalization flow. No issues here.Also applies to: 44-62
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
10-11: Test wiring for UserPersonalizer is solidInjecting a
UserPersonalizermock and stubbingpersonalize()to be identity keeps existing assertions unchanged while exercising the new dependency. The constructor args line up with the handler changes, so this looks good.Also applies to: 48-92
src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (1)
23-42: composeEmail refactor improves clarity and matches new pipelineUsing
$messagefor options and$processedContentfor subject/text/HTML matches the new “preprocess then personalize” flow and keeps the method signature self-explanatory. The handler call site aligns with this, so the change looks clean.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
5fe45ab to
492e1d0
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.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (4)
101-113: Missing return type declaration (still unresolved).This method still lacks a return type and parameter type hint. Add
string $contentand: stringfor type safety.- public function parseLogoPlaceholders($content) + public function parseLogoPlaceholders(string $content): string
122-125: Null pointer dereference risk (still unresolved).
findByFilename()can returnnull, but line 123 calls->getData()without a null check. This will throw a fatal error.$orgLogoImage = $this->templateImageRepository->findByFilename("ORGANISATIONLOGO$size.png"); - if (!empty($orgLogoImage->getData())) { + if ($orgLogoImage !== null && !empty($orgLogoImage->getData())) { return; }
127-128: Null pointer dereference risk (still unresolved).
findById()can returnnull, but line 128 calls->getData()without checking. Add a null guard before decoding.$logoImage = $this->templateImageRepository->findById((int) $logoImageId); + if ($logoImage === null) { + return; + } $imageContent = base64_decode($logoImage->getData());
154-176: Output buffer handling is fragile (still unresolved).The buffer capture (lines 169-175) grabs content before calling
ob_start(), then echoes it back after capturing the image. This can corrupt output. Use a local buffer instead:- //# rather convoluted way to get the image contents - $buffer = ob_get_contents(); - ob_end_clean(); - ob_start(); - imagepng($resized); - $imageContent = ob_get_contents(); - ob_end_clean(); - echo $buffer; + ob_start(); + imagepng($resized); + $imageContent = ob_get_clean();composer.json (1)
82-85: Past review comment still applies.The addition of these three PHP extensions is correct for the new functionality, but as noted in the previous review, please update the PR description to document why these extensions are required.
🧹 Nitpick comments (5)
src/Domain/Common/ExternalImageService.php (5)
50-55: Avoid error suppression; handle mkdir failures explicitly.Using
@mkdirhides potential errors. Consider checking if the directory creation succeeds and logging or returning an error if it fails.Apply this diff:
- if (!file_exists($this->externalCacheDir)) { - @mkdir($this->externalCacheDir); - } - - if (!file_exists($this->externalCacheDir) || !is_writable($this->externalCacheDir)) { - return false; - } + if (!file_exists($this->externalCacheDir)) { + if (!mkdir($this->externalCacheDir) && !is_dir($this->externalCacheDir)) { + return false; + } + } + + if (!is_writable($this->externalCacheDir)) { + return false; + }
64-66: Remove redundant curl_init check.Since
ext-curlis now a required dependency in composer.json, thefunction_exists('curl_init')check is unnecessary.Apply this diff:
- if (function_exists('curl_init')) { - $cacheFileContent = $this->downloadUsingCurl($filename); - } + $cacheFileContent = $this->downloadUsingCurl($filename);
76-84: Avoid error suppression on file operations.Using
@fopensilently ignores errors. Handle the failure case explicitly instead.
94-117: Address error suppression and TODO comment.This method uses
@suppression extensively (opendir, readdir, filemtime, unlink, closedir). Consider proper error handling or at least logging failures.Also, the TODO at line 102 should be resolved or tracked in an issue.
Would you like me to open an issue to track the TODO comment?
10-177: Consider moving to Infrastructure layer.This service performs filesystem and network I/O operations, which are side effects that typically belong in the Infrastructure layer rather than the Domain layer. According to the coding guidelines for domain purity, domain code should avoid persistence and transaction side effects.
While this service doesn't use Doctrine persistence, it does interact with external systems (filesystem and HTTP) which may violate domain purity principles. Consider moving this to
src/Infrastructure/instead.Based on coding guidelines, domain layer should be pure and free of side effects.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composer.json(1 hunks)config/parameters.yml.dist(2 hunks)src/Domain/Common/ExternalImageService.php(1 hunks)src/Domain/Messaging/Service/Manager/TemplateImageManager.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/parameters.yml.dist
🧰 Additional context used
📓 Path-based instructions (1)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow persistence or transaction side effects here for normal domain models.
Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/Service/Manager/TemplateImageManager.phpsrc/Domain/Common/ExternalImageService.php
🧬 Code graph analysis (2)
src/Domain/Messaging/Service/Manager/TemplateImageManager.php (2)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)src/Domain/Messaging/Repository/TemplateImageRepository.php (3)
TemplateImageRepository(12-35)findByFilename(16-24)findById(26-34)
src/Domain/Common/ExternalImageService.php (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
ConfigProvider(12-82)
🔇 Additional comments (3)
src/Domain/Common/ExternalImageService.php (3)
14-22: Constructor looks good.Clean dependency injection and simple initialization logic.
24-38: Clean cache retrieval logic.Proper validation and error handling with null returns.
119-126: Filename sanitization logic is correct.The regex patterns work but could be documented for clarity. Consider adding a comment explaining what characters are being removed/replaced.
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
ref/campaign-processing
branch
from
8e75515 to
25ef84a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
TatevikGr
changed the title
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
28b0824 to
823e006
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/Unit/Domain/Common/HtmlUrlRewriterTest.php (1)
10-121: Consider adding edge case tests.The current test suite provides solid coverage of the main functionality. You might optionally add tests for edge cases like empty strings, malformed HTML, or invalid base URLs to increase robustness, but the existing tests are comprehensive for typical usage scenarios.
tests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php (1)
101-102: Consider strengthening the callback assertion.The callback only validates that the parameter is a string. You could make this more robust by checking for specific content (e.g., that it contains the powered-by image or signature placeholder).
💡 Example of more specific validation
- ->with($this->callback(fn ($html) => is_string($html))) + ->with($this->callback(fn ($html) => is_string($html) && str_contains($html, 'src=')))tests/Unit/Domain/Messaging/Service/TemplateImageEmbedderTest.php (5)
38-43: Optional: Config setup duplication.The default config values set here in
setUp()are re-defined in most individual test methods (e.g., lines 95-100, 133-138). While this doesn't cause issues and allows each test to be self-contained, you could reduce duplication by relying on these defaults and only overriding specific values in tests that need different configs.
150-160: Test coverage: Verify template ID parameter.The mock callback for
findByTemplateIdAndFilenamechecks the filename but doesn't verify the$tplIdparameter. Since the test sets template ID to'42'(line 134) but calls the embedder with campaignId7(line 163), consider asserting the expected template ID values within the callback to ensure the correct IDs are passed.🔎 Example: Add assertions for template ID
$this->templateImageRepository->method('findByTemplateIdAndFilename') ->willReturnCallback(function (int $tplId, string $filename) use ($tplImg1, $tplImg2) { if ($filename === '/assets/logo.jpg') { - // In current implementation, first pass checks templateId as provided + $this->assertSame(42, $tplId, 'Expected template ID 42 for logo.jpg'); return $tplImg1; } if ($filename === 'powerphplist.png') { + $this->assertSame(0, $tplId, 'Expected template ID 0 for powerphplist.png'); return $tplImg2; } return null; });
188-189: Use more precise mock expectation.Using
atLeastOnce()for thecreate()expectation is less precise thanonce()orexactly(1). Consider using a more specific expectation to catch potential bugs if the method is called an unexpected number of times.🔎 Suggested change
-$this->configManager->expects($this->atLeastOnce()) +$this->configManager->expects($this->once()) ->method('create');
225-225: Type inconsistency in config value.The
ConfigOption::SystemMessageTemplateis set to integer0here, while other tests use string'0'(lines 39, 96, 134, etc.). Unless this is intentionally testing type handling, consider using consistent types across all tests.
105-111: Future consideration:withConsecutive()deprecation.The
withConsecutive()method used here is deprecated in PHPUnit 10+. Since you're currently on PHPUnit 9.5.0, this works fine, but when upgrading to PHPUnit 10+, you'll need to refactor these expectations using parameter matchers or thewillReturnCallback()approach.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
phpunit.xml.disttests/Unit/Domain/Common/HtmlUrlRewriterTest.phptests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.phptests/Unit/Domain/Messaging/Service/TemplateImageEmbedderTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Unit/Domain/Common/HtmlUrlRewriterTest.php (1)
src/Domain/Common/HtmlUrlRewriter.php (3)
HtmlUrlRewriter(11-208)addAbsoluteResources(13-73)absolutizeUrl(84-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (15)
phpunit.xml.dist (1)
10-14: Configuration is good. Thetestsdirectory exists and is properly set up with test files across Integration, Unit, System, and Support subdirectories.tests/Unit/Domain/Common/HtmlUrlRewriterTest.php (7)
1-17: Clean test setup.The class structure and setUp are straightforward and appropriate for unit testing the HtmlUrlRewriter.
19-33: Good coverage of basic attributes.The test validates absolutization of common HTML attributes (src, href, action, background) with both relative and root-relative paths.
35-49: Solid edge case coverage.Validates that special schemes (hash anchors, mailto, javascript, data URIs) are correctly preserved without modification.
51-59: Protocol-relative URL handling looks good.Correctly validates that protocol-relative URLs adopt the base scheme while preserving their host.
61-71: Srcset handling validated correctly.Tests that srcset candidates are properly rewritten for relative URLs while preserving already-absolute URLs.
73-105: Comprehensive CSS URL rewriting test.Validates CSS url() and @import rewriting in both style blocks and inline attributes. The regex patterns appropriately handle optional quotes and various @import syntaxes.
107-120: Direct method testing with good edge case coverage.Validates dot segment resolution and port/query/fragment preservation through direct absolutizeUrl calls.
tests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php (7)
1-23: LGTM!Clean setup with proper type hints and all necessary dependencies mocked.
24-36: Mocking strategy looks solid.Good use of
getMockBuilderwhere specific method control is needed andcreateMockfor simpler cases.
38-53: Nice helper pattern.Centralizing the constructor setup with configurable
poweredByPhplistkeeps tests clean and focused.
55-67: Test coverage looks good.Properly validates that plain text isn't unnecessarily processed through Html2Text, and that linkification and newline handling work as expected.
69-82: Clean test of HTML conversion path.Validates the Html2Text integration correctly converts HTML to text while preserving the original HTML.
160-167: Test assertions validate the powered-by text rendering path.Good coverage of the HTML insertion before
</body>and text appending behavior when no signature placeholder is present.
120-122: No action needed — the src transformation is correctly implemented.The
SystemMailConstructoractually performs the path transformation on line 44-48 usingpreg_replace, which convertssrc="/assets/power-phplist.png"tosrc="powerphplist.png". This happens beforeparseLogoPlaceholdersis called, so the test expectation at line 122 is correct and the actual behavior matches.
| $this->externalImageService->expects($this->exactly(2)) | ||
| ->method('cache') | ||
| ->withConsecutive( | ||
| ['https://cdn.other.org/pic.jpg', 111], | ||
| ['https://example.com/local.jpg', 111] | ||
| ) | ||
| ->willReturnOnConsecutiveCalls(true, false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the TemplateImageEmbedder class implementation
git ls-files | grep -i templateimageembedder | head -20Repository: phpList/core
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Search for the cache method in TemplateImageEmbedder
rg -l 'class TemplateImageEmbedder' --type=phpRepository: phpList/core
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Read the TemplateImageEmbedder implementation
cat -n src/Domain/Messaging/Service/TemplateImageEmbedder.phpRepository: phpList/core
Length of output: 13369
🏁 Script executed:
#!/bin/bash
# Read the test file to understand the test setup and expectations
sed -n '1,130p' tests/Unit/Domain/Messaging/Service/TemplateImageEmbedderTest.phpRepository: phpList/core
Length of output: 5099
Fix mock expectations: cache() should be called only once.
The embedExternalImages() method uses a regex pattern with a negative lookahead (?!https://example.com) to exclude same-host URLs before calling cache(). As a result, cache() will only be called for the external image (line 237 in the implementation), not for the same-host URL. Update the mock to expect exactly one call to cache() with only ['https://cdn.other.org/pic.jpg', 111], and remove the second consecutive call expectation.
🤖 Prompt for AI Agents
In tests/Unit/Domain/Messaging/Service/TemplateImageEmbedderTest.php around
lines 105 to 111, the mock expectation wrongly expects two calls to
externalImageService->cache(); update it to expect exactly one call with the
single argument pair ['https://cdn.other.org/pic.jpg', 111] and return true for
that call, removing the second consecutive expectation for the same-host URL.
7d62a59 to
7f21c58
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
202-206: Nullablefootermay cause TypeError.
MessagePrecacheDto::footeris?string, butUserPersonalizer::personalize()expects a non-nullable string. Iffooteris null, this throws a TypeError.- $processed->footer = $this->userPersonalizer->personalize($processed->footer, $subscriber->getEmail()); + $processed->footer = $this->userPersonalizer->personalize($processed->footer ?? '', $subscriber->getEmail());
🧹 Nitpick comments (2)
src/Domain/Messaging/Service/MailSizeChecker.php (1)
36-44: Potential null fromcache->get()after cache miss path.After setting the cache on line 39, line 42 retrieves the value. If the cache fails to store or expires immediately,
cache->get()could returnnull, causing the comparison on line 43 to behave unexpectedly (null <= int evaluates to true in PHP 8).Consider using the calculated
$sizedirectly in the cache-miss branch:🔎 Suggested fix
if (!$this->cache->has($cacheKey)) { $size = $this->calculateEmailSize($email); $this->messageDataManager->setMessageData($campaign, $sizeName, $size); $this->cache->set($cacheKey, $size); + } else { + $size = $this->cache->get($cacheKey); } - $size = $this->cache->get($cacheKey); if ($size <= $this->maxMailSize) { return; }tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
364-370:withConsecutiveis deprecated in PHPUnit 10+ and removed in PHPUnit 11.This project currently uses PHPUnit
^9.5, so no immediate change needed. When upgrading to PHPUnit 10+, consider replacing with a callback matcher orwillReturnOnConsecutiveCalls()combined with argument validation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/MailSizeChecker.phptests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.phptests/Unit/Domain/Messaging/Service/MailSizeCheckerTest.php
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow, flag ANY DB write / finalization:
$entityManager->flush(...),$this->entityManager->flush(...)$em->beginTransaction(),$em->commit(),$em->rollback(),$em->transactional(...)$em->getConnection()->executeStatement(...)for DML/DDL (INSERT/UPDATE/DELETE/ALTER/...)✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes. Accessing Doctrine persistence APIs
persist, remove, etc.) is acceptable, allow scheduling changes in the UnitOfWork (no DB writes)✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/MailSizeChecker.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (3)
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (4)
src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
EmailBuilder(21-214)src/Domain/Messaging/Service/MailSizeChecker.php (1)
MailSizeChecker(15-78)src/Domain/Messaging/Service/MessageDataLoader.php (1)
MessageDataLoader(14-235)
tests/Unit/Domain/Messaging/Service/MailSizeCheckerTest.php (2)
src/Domain/Messaging/Exception/MessageSizeLimitExceededException.php (1)
MessageSizeLimitExceededException(9-31)src/Domain/Messaging/Service/MailSizeChecker.php (2)
MailSizeChecker(15-78)__invoke(29-66)
src/Domain/Messaging/Service/MailSizeChecker.php (3)
src/Domain/Messaging/Exception/MessageSizeLimitExceededException.php (1)
MessageSizeLimitExceededException(9-31)src/Domain/Messaging/Service/Manager/MessageDataManager.php (2)
MessageDataManager(13-96)setMessageData(29-71)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
__construct(51-72)
🔇 Additional comments (16)
tests/Unit/Domain/Messaging/Service/MailSizeCheckerTest.php (5)
18-31: Good test setup with proper mock initialization.The test class is well-structured with clearly defined mock objects and a clean
setUp()method. The intersection types for mocks (EventLogManager&MockObject) are a nice touch for type safety.
54-72: Solid test for the disabled path.Good coverage of the early-return behavior when
maxMailSizeis 0, ensuring no side effects occur.
74-111: Nice coverage of the cache-miss flow.The test correctly verifies that on cache miss, the size is calculated, stored in both
MessageDataand cache, and then retrieved for comparison.
113-147: Good exception path coverage.Properly verifies that when the cached size exceeds the limit, appropriate logging occurs and
MessageSizeLimitExceededExceptionis thrown.
149-170: Clean happy-path test.Verifies that when the cached size is within limits, the method returns normally without logging.
src/Domain/Messaging/Service/MailSizeChecker.php (2)
15-27: Constructor looks good.Clean dependency injection with sensible defaults. The null-coalescing to 0 for
maxMailSizeis a nice defensive pattern.
68-77: Size calculation looks solid.Straightforward approach using
Email::toIterable()to get the serialized representation and summing the lengths.tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (4)
50-101: Test setup updated correctly for new dependencies.The new mocks for
CacheInterface,MailerInterface,UserPersonalizer,EmailBuilder,MessageDataLoader, andMailSizeCheckerare properly wired into the handler. The passthrough behavior foruserPersonalizer->personalize()is sensible for these tests.
103-119: Unchanged test for campaign-not-found case.This test correctly verifies the early exit when no campaign is found.
121-152: Test properly updated for precaching flow.Added the expected
precacheMessagecall and switched to verifysymfonyMailer->sendis never called when no subscribers exist.
194-263: Good DTO-based test for valid subscriber flow.The test correctly sets up the
MessagePrecacheDto, mocks the cache to return it, and verifies the email composition and sending path.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (5)
51-72: Constructor wiring is clean.All new dependencies are properly injected. The handler now has comprehensive control over the email building and sending pipeline.
86-111: Data loading and precaching flow looks correct.The cache key now includes the
forwardContentparameter (hardcoded to 0), addressing the previous cache key mismatch issue. The commented-out legacy code could be removed in a follow-up cleanup.
231-272: Admin notifications implementation is solid.Good defensive handling with the null check on
start_notifiedentries viaUniqueConstraintViolationException. The persistence here is appropriate for the MessageHandler layer per the coding guidelines.
274-308: Subscriber processing loop is well-structured.The null check for
$messagePrecacheDtoon line 301-303 properly handles the cache-miss scenario by throwingMessageCacheMissingException. The time limiter integration and early-continue patterns are clean.
159-166: Send-start timestamp initialization is a nice touch.Good addition to track when the campaign actually started processing. Minor note: you're using
DateTimehere butDateTimeImmutableelsewhere (line 261) - consider consistency, but not blocking.
| try { | ||
| $email = $this->mailer->composeEmail($campaign, $subscriber, $processed); | ||
| $email = $this->rateLimitedCampaignMailer->composeEmail($campaign, $subscriber, $processed); | ||
| $this->mailer->send($email); | ||
| $this->checkMessageSizeOrSuspendCampaign($campaign, $email, $subscriber->hasHtmlEmail()); | ||
| ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail()); | ||
| $this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent); |
There was a problem hiding this comment.
Size check happens after the email is already sent.
The mailSizeChecker is invoked on line 211 after $this->mailer->send($email) on line 210. If the email exceeds the size limit, it's already been dispatched before the exception is thrown.
Move the size check before sending:
🔎 Proposed fix
try {
$email = $this->rateLimitedCampaignMailer->composeEmail($campaign, $subscriber, $processed);
+ ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail());
$this->mailer->send($email);
- ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail());
$this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent);🤖 Prompt for AI Agents
In src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
around lines 208 to 212, the mailSizeChecker is invoked after
$this->mailer->send($email) so oversized messages are sent before the check
throws; move the call to ($this->mailSizeChecker)($campaign, $email,
$subscriber->hasHtmlEmail()) to immediately after composing the email and before
$this->mailer->send($email) so the size check can prevent sending, and ensure
$this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent) remains
after a successful send.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (2)
209-211: Critical: Mail size check executes after email is already sent.The
mailSizeCheckeris invoked on line 211 after$this->mailer->send($email)on line 210. If the email exceeds the configured size limit, it will have already been dispatched before the exception is thrown. This defeats the purpose of the size check.🔎 Proposed fix: Move size check before send
try { $email = $this->rateLimitedCampaignMailer->composeEmail($campaign, $subscriber, $processed); + ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail()); $this->mailer->send($email); - ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail()); $this->updateUserMessageStatus($userMessage, UserMessageStatus::Sent);
202-206: Critical: Nullable fields passed to personalize() require non-null string.Lines 202-206 pass
$processed->textContentand$processed->footertopersonalize(), but both fields are declared as?stringinMessagePrecacheDto(see lines 7-30 of MessagePrecacheDto.php). TheUserPersonalizer::personalize()method signature expects a non-nullablestring $valueparameter (noted in past review comments). If either field is null, this will throw aTypeError.🔎 Proposed fix using null-coalescing operator
- $processed->textContent = $this->userPersonalizer->personalize( - $processed->textContent, - $subscriber->getEmail(), - ); - $processed->footer = $this->userPersonalizer->personalize($processed->footer, $subscriber->getEmail()); + $processed->textContent = $this->userPersonalizer->personalize( + $processed->textContent ?? '', + $subscriber->getEmail(), + ); + $processed->footer = $this->userPersonalizer->personalize($processed->footer ?? '', $subscriber->getEmail());Alternatively, add null checks before calling
personalize()if empty strings aren't the desired fallback.
🧹 Nitpick comments (1)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
86-105: Consider removing large commented code blocks.Lines 86-105 and 120-139 contain significant blocks of commented-out legacy code. If this code is no longer needed, removing it would improve readability. If it might be needed for reference, consider documenting why it's commented or moving it to a separate doc/ADR.
Also applies to: 120-139
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
config/services/messenger.ymlconfig/services/services.ymlsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Service/MailSizeChecker.phptests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.phptests/Unit/Domain/Messaging/Service/MailSizeCheckerTest.php
💤 Files with no reviewable changes (1)
- config/services/messenger.yml
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
❌ Do not allow, flag ANY DB write / finalization:
$entityManager->flush(...),$this->entityManager->flush(...)$em->beginTransaction(),$em->commit(),$em->rollback(),$em->transactional(...)$em->getConnection()->executeStatement(...)for DML/DDL (INSERT/UPDATE/DELETE/ALTER/...)✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes. Accessing Doctrine persistence APIs
persist, remove, etc.) is acceptable, allow scheduling changes in the UnitOfWork (no DB writes)✅ Relaxed rule for DynamicListAttr-related code:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (
persist,remove,flush, etc.)
as part of managing DynamicListAttr data and schema.- Do not flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.
⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Repositories in Domain should be abstractions without side effects; they should express intent,
not perform flush/transactional logic.
Files:
src/Domain/Messaging/Service/MailSizeChecker.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence and schema management.
- ✅ Allow
$entityManager->flush()when the job is the orchestration boundary.- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.
Files:
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
🧬 Code graph analysis (2)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (10)
src/Domain/Messaging/Model/Message/MessageMetadata.php (1)
setSendStart(127-131)src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php (1)
MessagePrecacheDto(8-31)src/Domain/Messaging/Repository/MessageRepository.php (1)
MessageRepository(16-119)src/Domain/Messaging/Service/Builder/EmailBuilder.php (2)
EmailBuilder(21-214)buildPhplistEmail(40-77)src/Domain/Messaging/Service/MailSizeChecker.php (1)
MailSizeChecker(15-78)src/Domain/Messaging/Service/MessageDataLoader.php (1)
MessageDataLoader(14-235)src/Domain/Messaging/Service/MessagePrecacheService.php (1)
MessagePrecacheService(22-241)src/Domain/Messaging/Service/MessageProcessingPreparator.php (1)
MessageProcessingPreparator(15-101)src/Domain/Messaging/Service/RateLimitedCampaignMailer.php (3)
RateLimitedCampaignMailer(14-56)composeEmail(24-45)send(50-55)src/Domain/Subscription/Service/Provider/SubscriberProvider.php (1)
SubscriberProvider(12-45)
tests/Unit/Domain/Messaging/Service/MailSizeCheckerTest.php (2)
src/Domain/Messaging/Exception/MessageSizeLimitExceededException.php (1)
MessageSizeLimitExceededException(9-31)src/Domain/Messaging/Service/MailSizeChecker.php (2)
MailSizeChecker(15-78)__invoke(29-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (12)
tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (3)
68-78: Smart test setup for personalization.The mock ensures
personalizereturns the input unchanged, which preserves link-replacement assertions in downstream tests. This is a solid testing pattern.
80-100: Constructor wiring updated correctly.All new dependencies (symfonyMailer, cache, userPersonalizer, messageDataLoader, emailBuilder, mailSizeChecker, messageEnvelope) are properly mocked and wired into the handler. The test setup aligns with the broader DTO-based precaching workflow.
197-250: MessagePrecacheDto usage validated thoroughly.The test creates a proper DTO, retrieves it from cache, passes it through
processMessageLinks, and validates it reachescomposeEmail. The callback assertions confirm the DTO flows correctly through the processing pipeline.config/services/services.yml (1)
137-141: MailSizeChecker service wiring looks good.The service definition properly autowires dependencies and binds the
maxMailSizeparameter to the configuration value. This follows the existing service definition patterns in the file.tests/Unit/Domain/Messaging/Service/MailSizeCheckerTest.php (2)
54-111: Comprehensive test coverage for cache miss scenario.The test validates the full flow: cache miss triggers size calculation, stores result in both MessageData and cache, and completes without exception when size is within limit. The callback-based assertions are well-structured.
113-147: Exception path properly tested.The test confirms that oversized messages trigger warning logs, event logs (exactly 2), and throw
MessageSizeLimitExceededException. This validates the size-enforcement behavior.src/Domain/Messaging/Service/MailSizeChecker.php (3)
29-45: Cache-backed size check implemented correctly.The logic properly handles disabled checks (maxMailSize <= 0), cache misses (calculate + store), and within-limit cases (early return). The cache key includes both campaign ID and size type (html/text), ensuring proper isolation.
47-66: Size limit enforcement with comprehensive logging.When size exceeds the limit, the code logs a warning, records two event-log entries for observability, and throws an exception to halt processing. This provides good visibility into size-related suspensions.
68-77: Email size calculation uses MIME iteration.Calculating size via
email->toIterable()measures the actual serialized MIME content, which is the correct approach for enforcing wire-format size limits.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (3)
161-163: SendStart timestamp initialization looks good.Setting
SendStartwhen transitioning toInProcess(if not already set) ensures campaigns have a proper start timestamp. This aligns well with the message lifecycle.
231-272: Admin notification flow implemented correctly.The method properly checks for
notify_startandstart_notified, builds emails with null-checking, constructs envelopes, sends notifications, and persists thestart_notifiedmarker with duplicate-handling viaUniqueConstraintViolationException. The logic is sound.
300-303: Cache retrieval includes proper null check.Retrieving the precached DTO from cache now includes a null check and throws
MessageCacheMissingExceptionif missing. This addresses the concern from past reviews about cache misses.
New Features
PDF generation for messages, per-subscriber remote-content fetching, tracking-pixel user tracking, and richer attachment handling with downloadable copies.
Improvements
Unified email builder flow with consistent composition and multi-format output (HTML/Text/PDF); expanded, context-aware placeholder personalization (many URL/list resolvers); improved remote-content precaching and output formatting; new configurable parameters and translations.
---------
Co-authored-by: Tatevik <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Domain/Messaging/Service/EmailService.php (1)
67-70: Consider guarding against empty recipients.Line 69 accesses
$email->getTo()[0]without checking if the array is non-empty. If somehow an email without aToaddress makes it here, you'd get an undefined index error.Might be worth a quick guard or assertion, though if callers always set recipients this could be fine in practice.
Suggested defensive check
+ $toAddresses = $email->getTo(); + if (empty($toAddresses)) { + throw new \InvalidArgumentException('Email must have at least one recipient'); + } + $envelope = new Envelope( sender: new Address($this->bounceEmail, 'PHPList Bounce'), - recipients: [new Address($email->getTo()[0]->getAddress())] + recipients: [new Address($toAddresses[0]->getAddress())] );tests/Unit/Domain/Messaging/MessageHandler/SubscriptionConfirmationMessageHandlerTest.php (1)
63-70: Fix test inconsistency: second test should mockfind()instead ofgetListNames().Both tests need to mock the same repository method. The handler's private
getListNames()method loops through list IDs and calls$subscriberListRepository->find($listId)individually—it never uses the repository's batchgetListNames()method. The first test (line 63-70) correctly mocksfind(), but the second test at line 128 should do the same instead of mockinggetListNames().Suggested fix
- $listRepo->method('getListNames')->with([42])->willReturn([]); + $listRepo->method('find') + ->willReturnCallback(function (int $id) { + return null; + });src/Domain/Messaging/MessageHandler/PasswordResetMessageHandler.php (1)
24-27: Minor: DocBlock mentions "subscriber confirmation" but this handles password reset.The comment says "Process a subscriber confirmation message" but the handler processes
PasswordResetMessage. Small copy-paste oversight.📝 Suggested fix
/** - * Process a subscriber confirmation message by sending the confirmation email + * Process a password reset message by sending the reset email */ public function __invoke(PasswordResetMessage $message): void
🤖 Fix all issues with AI agents
In `@resources/translations/messages.en.xlf`:
- Around line 777-780: Remove the duplicate translation trans-unit by keeping a
single canonical entry for the source "Insufficient memory to add attachment to
campaign %campaignId% %totalSize% - %memLimit%" and deleting the redundant
trans-unit (either id="QLlIQQh" or id="28lDkjt"); ensure only one trans-unit
remains with that source/target text and update any tooling or references that
expect that id if you choose to keep the other id.
In `@src/Domain/Common/FileHelper.php`:
- Around line 36-59: The writeFileToDirectory method can leave the original temp
file orphaned when you append an extension and currently does not check
fopen/fwrite results; update writeFileToDirectory to write to the temp file
returned by tempnam (use file_put_contents($tempPath, $contents, LOCK_EX) or
check fwrite/fflush returns) and verify the write succeeded, then if an
extension is required perform a rename($tempPath, $targetPath) and check its
return value; on any failure (write or rename) unlink the temp file and return
null, and only return $fullPath when write+rename (or write alone when no rename
needed) have succeeded. Target symbols: writeFileToDirectory, tempnam, rename,
unlink, file_put_contents/fopen/fwrite/fflush/fclose.
In `@src/Domain/Common/RemotePageFetcher.php`:
- Around line 65-70: RemotePageFetcher is performing persistence side effects by
calling $this->entityManager->remove(...) and then
$this->urlCacheRepository->persist(...); remove direct EntityManager usage from
the Domain class and push the logic into the repository/infrastructure layer:
add a single repository method (e.g., UrlCacheRepository::replaceCacheForUrl or
purgeAndSave) that encapsulates getByUrl, removal of existing UrlCache entities,
and persisting the new UrlCache, then update RemotePageFetcher to call that
repository method (passing $url, $content, $lastModified) and delete any
EntityManager references from RemotePageFetcher.
- Around line 111-124: The prepareUrl method is converting the final URL to
ISO-8859-1 which breaks percent-encoded UTF-8 sequences; remove the
mb_convert_encoding($url, 'ISO-8859-1', 'UTF-8') call in prepareUrl so the URL
remains UTF-8 after rawurlencode(), and return the result via expandUrl($url)
unchanged; ensure no other code in prepareUrl replaces or re-encodes the URL.
In `@src/Domain/Configuration/Service/Placeholder/ContactUrlValueResolver.php`:
- Around line 22-28: In ContactUrlValueResolver::__invoke, avoid calling
$this->config->getValue(ConfigOption::VCardUrl) twice; fetch it once into a
local variable (e.g., $url = $this->config->getValue(ConfigOption::VCardUrl) ??
''), then return $url for text contexts and htmlspecialchars($url) for non-text
contexts, preserving the existing logic with a single config lookup.
In
`@src/Domain/Configuration/Service/Placeholder/ForwardMessageIdValueResolver.php`:
- Around line 37-43: The current code in ForwardMessageIdValueResolver casts
$forwardMessage to (int) producing 0 for non-numeric inputs; before casting,
validate $forwardMessage is an integer string (e.g., use
ctype_digit($forwardMessage) or preg_match('/^\d+$/')) and if it fails return ''
(and optionally emit a debug/warning via the resolver's logger if present)
instead of proceeding to create $messageId = 0; update the block around
$forwardMessage/$messageId to perform this check and only cast when validation
passes.
In `@src/Domain/Configuration/Service/Placeholder/ForwardUrlValueResolver.php`:
- Around line 30-38: In ForwardUrlValueResolver.php inside the HTML branch
(where $ctx->isHtml() is true), ensure the user UID is HTML-escaped (use
htmlspecialchars on $ctx->getUser()->getUniqueId()) and avoid emitting mid=0
when $ctx->messageId() is null by only appending the mid parameter when
messageId() !== null (build the query fragment conditionally rather than always
using '%d' in sprintf). Update the sprintf/concatenation accordingly so $url,
$sep and the escaped uid are consistently escaped and mid is included only when
present.
In `@src/Domain/Configuration/Service/Placeholder/JumpoffValueResolver.php`:
- Around line 25-37: In JumpoffValueResolver::__invoke, when appending the jo=1
flag to the URL produced by $this->urlBuilder->withUid(...) you must preserve
any URL fragment (the part after '#') by inserting the query parameter before
the fragment; split the URL on '#' into $basePart and $fragment, append the
parameter to $basePart (using '?' or '&' depending on whether $basePart already
contains a query string), then reattach '#'.$fragment if present; keep all
existing query parameters intact and return the reconstructed URL instead of
naively concatenating '&jo=1'.
In
`@src/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolver.php`:
- Around line 22-39: The PreferencesUrlValueResolver::__invoke currently
concatenates the raw UID into the URL; update it to URL-encode the UID and to
HTML-escape the separator with explicit flags: use
rawurlencode($ctx->getUser()->getUniqueId()) when building both plain and HTML
URLs, and in the HTML branch call htmlspecialchars($sep, ENT_QUOTES |
ENT_SUBSTITUTE, 'UTF-8') (and ensure the already-escaped $url keeps its existing
flags). This fixes potential injection from arbitrary UIDs while keeping proper
HTML escaping for the separator; locate PlaceholderContext usage and
ConfigOption::PreferencesUrl handling inside __invoke to apply the changes.
In `@src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php`:
- Around line 238-254: The code assumes buildPhplistEmail() returns an Email and
dereferences $email->getTo()[0], which can be null and mask the original
exception; update CampaignProcessorMessageHandler to check the return of
systemEmailBuilder->buildPhplistEmail(...) (and that getTo() has at least one
recipient) before constructing Envelope and calling mailer->send; if
buildPhplistEmail() returns null or no recipients, log a clear error (including
$e->getMessage() context) or rethrow the original exception instead of
proceeding to send so the original failure is not hidden.
In `@src/Domain/Messaging/Service/AttachmentAdder.php`:
- Around line 59-62: In AttachmentAdder (in the method handling the
OutputFormat::Text case) avoid directly accessing $email->getTo()[0]; first
check that $email->getTo() is non-empty (e.g. empty() or count()>0) before
calling ->getAddress(); if no recipients exist, choose a safe fallback (empty
string, null, or skip building the $viewUrl / log and return/throw as
appropriate) so $att->getId() is not combined with an undefined index—update the
branch that builds $viewUrl to use this defensive check and create the URL only
when a valid $userEmail is available.
In `@src/Domain/Messaging/Service/Builder/BaseEmailBuilder.php`:
- Around line 48-55: The current newline checks in BaseEmailBuilder (the
preg_match calls that validate $to and $subject) only match "\n" and miss "\r",
allowing header-injection via "\r\n"; update both validations to use a regex
that matches either CR or LF (e.g. /[\r\n]/ or /(\r|\n)/) so any CR or LF in $to
or $subject triggers the existing $this->eventLogManager->log(...) and returns
false, leaving the rest of the logic unchanged.
- Around line 126-139: The code calls
$this->configProvider->getValue(ConfigOption::UnsubscribeUrl) into $removeUrl
and immediately uses str_contains($removeUrl, '?') which will throw if null;
update BaseEmailBuilder to guard the value: check that $removeUrl is a non-empty
string before calling str_contains and before adding the 'List-Unsubscribe'
header (e.g. skip header creation when getValue returns null/empty), and
reference the same symbols ($removeUrl, getValue, ConfigOption::UnsubscribeUrl,
str_contains, List-Unsubscribe) so the header logic only runs when a valid
unsubscribe URL exists.
In `@src/Domain/Messaging/Service/Builder/EmailBuilder.php`:
- Line 122: The code assigns $adminAddress from the wrong config option; change
the call in EmailBuilder where $adminAddress is set (currently
$this->configProvider->getValue(ConfigOption::UnsubscribeUrl)) to use the admin
address config key (e.g. ConfigOption::AdminAddress or the correct enum value)
so the value used when building the List-Owner header comes from the intended
config; update the lookup in the EmailBuilder class (the $adminAddress
assignment) to use the AdminAddress config option and keep the fallback behavior
unchanged.
In `@src/Domain/Messaging/Service/Builder/SystemEmailBuilder.php`:
- Around line 93-109: The applyContentAndFormatting method calls Email::text()
with nullable $textMessage which can cause a runtime error; guard against null
by ensuring a string is passed to Email::text() (for example coalesce
$textMessage to an empty string or derive plain text from $htmlMessage after
embedding via $this->templateImageEmbedder) before calling $email->text(),
keeping the existing html handling via ($this->templateImageEmbedder)(html:
$htmlMessage, messageId: $messageId) and only passing a non-null string to
Email::text().
In `@src/Domain/Messaging/Service/MessagePrecacheService.php`:
- Around line 188-196: The log message is referencing the wrong URL variable
which is misleading: when ($this->remotePageFetcher) is called with $regs[1] and
returns falsy (the $remoteContent check in MessagePrecacheService), update the
$this->eventLogManager->log call to reference the actual fetched URL ($regs[1])
instead of $loadedMessageData['sendurl'] — optionally include both values for
context (fetched URL $regs[1] and original sendurl
$loadedMessageData['sendurl']) so the log clearly shows which URL failed; adjust
the log entry in the remoteContent falsy branch accordingly.
In `@src/Domain/Subscription/Repository/SubscriberRepository.php`:
- Around line 209-216: getDataById currently calls getArrayResult()[0] which
will trigger an undefined offset/TypeError when no record is found; modify
getDataById to store the result of
createQueryBuilder(...)->getQuery()->getArrayResult() in a variable, check if
the array is empty, and then either return an empty array or throw a specific
not-found exception (e.g., SubscriberNotFoundException) to preserve the declared
return type and avoid notices; update the method (getDataById) to handle the
empty result path before indexing [0].
In
`@tests/Unit/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolverTest.php`:
- Around line 65-80: The test's expected string uses htmlspecialchars() with
default flags while PreferencesUrlValueResolver uses ENT_QUOTES | ENT_SUBSTITUTE
and 'UTF-8'; update the test in testHtmlEscapesUrlAndAppendsUid to call
htmlspecialchars($raw, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') and
htmlspecialchars('&', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') so the escaping
behavior matches PreferencesUrlValueResolver and the asserted string is produced
correctly.
In
`@tests/Unit/Domain/Configuration/Service/Placeholder/UserDataSupportingResolverTest.php`:
- Around line 24-38: The makeCtx helper has a parameter typed as Subscriber but
defaulted to null which is deprecated; update the function signature of makeCtx
to accept a nullable Subscriber (e.g., ?Subscriber $user = null) and keep the
existing null-coalescing logic that constructs a Subscriber when $user is null;
ensure any related docblock/type hints reflect the nullable type so tools and
PHP 8.1+ no longer warn.
In
`@tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php`:
- Around line 371-375: Replace the deprecated withConsecutive usage on the mock
by switching to a willReturnCallback closure that keeps a local call counter;
inside the callback validate each invocation's arguments against the expected
values for that call (e.g., check arg0==1, arg1==$precached, arg2==$subscriber1
for call 0, then subscriber2 for call 1) and return the corresponding predefined
response (or pull from an array of return values, similar to
willReturnOnConsecutiveCalls). Concretely, change the mock setup that used
withConsecutive(...) ->willReturnOnConsecutiveCalls(...) to a single
->willReturnCallback(function(...$args) use (&$i, $expectedArgs, $returns) { /*
assert args match $expectedArgs[$i], $ret=$returns[$i]; $i++; return $ret; */
}), referencing the same expected values ($precached, $subscriber1,
$subscriber2) and returns to preserve behavior.
♻️ Duplicate comments (6)
resources/translations/messages.en.xlf (1)
741-812: Previous feedback regarding "__" prefix still applies.The new entries correctly follow the existing pattern in this file, but as noted in the previous review, the
__prefix typically marks auto-generated placeholder translations in Symfony. Since this is an English-to-English file, these should probably have the prefix removed before release. Deferring to the prior comment on this.src/Domain/Common/RemotePageFetcher.php (1)
51-56: Caching logic may always miss: querying withlastModified = time().Setting
$lastModified = time()then callingfindByUrlAndLastModified($url, $lastModified)will look for cache entries modified after the current timestamp - essentially future-dated entries that won't exist. This likely causes the cache lookup to always fail.If the intent is to check for existing cached content regardless of timestamp, consider using a simpler
findByUrl($url)query here, or rethink the freshness logic.src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
223-224: Move the size check before sending the email.
Right now the oversized message is already sent before the checker runs.♻️ Suggested fix
- $this->rateLimitedCampaignMailer->send($email); - ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail()); + ($this->mailSizeChecker)($campaign, $email, $subscriber->hasHtmlEmail()); + $this->rateLimitedCampaignMailer->send($email);src/Domain/Messaging/Service/Constructor/SystemMailContentBuilder.php (1)
18-66: Guard against null config values for powered‑by content.
ConfigProvider::getValue()can return null;preg_replace()andHtml2Textexpect strings and will throwTypeErrorin PHP 8+. Coalesce to a safe default.🛠️ Proposed fix
public function __construct( private readonly Html2Text $html2Text, private readonly ConfigProvider $configProvider, private readonly TemplateRepository $templateRepository, private readonly TemplateImageManager $templateImageManager, private readonly bool $poweredByPhplist = false, ) { - $this->poweredByText = $configProvider->getValue(ConfigOption::PoweredByText); + $this->poweredByText = $configProvider->getValue(ConfigOption::PoweredByText) ?? ''; } @@ if (!$this->poweredByPhplist) { + $poweredByImage = $this->configProvider->getValue(ConfigOption::PoweredByImage) ?? ''; $phpListPowered = preg_replace( '/src=".*power-phplist.png"/', 'src="powerphplist.png"', - $this->configProvider->getValue(ConfigOption::PoweredByImage), + $poweredByImage, ); } else { $phpListPowered = $this->poweredByText; }#!/bin/bash # Confirm defaults for PoweredByText/PoweredByImage to see if null is possible. rg -n "PoweredByText|PoweredByImage" -S src/Domain/Configurationtests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php (1)
153-160: Replace deprecatedwithConsecutive.
PHPUnit 10 removes this API; a callback keeps the intent without relying on deprecated behavior.🛠️ Suggested refactor
- $this->html2Text->expects($this->exactly(2)) - ->method('__invoke') - ->withConsecutive( - ['Hello <b>World</b>'], - ['<i>PB</i>'] - ) - ->willReturnOnConsecutiveCalls('Hello World', 'PB'); + $callCount = 0; + $this->html2Text->expects($this->exactly(2)) + ->method('__invoke') + ->willReturnCallback(function ($html) use (&$callCount) { + $callCount++; + if ($callCount === 1) { + $this->assertSame('Hello <b>World</b>', $html); + return 'Hello World'; + } + $this->assertSame('<i>PB</i>', $html); + return 'PB'; + });src/Domain/Messaging/Service/MessagePrecacheService.php (1)
91-95: Null safety issue withparseLogoPlaceholderscalls.
$messagePrecacheDto->templatecan benull(set inapplyTemplateonly when template exists), butparseLogoPlaceholdersexpects astring. This will cause aTypeErrorunder strict types.🔧 Suggested fix with null checks
- $messagePrecacheDto->content = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->content); - $messagePrecacheDto->template = $this->templateImageManager - ->parseLogoPlaceholders($messagePrecacheDto->template); - $messagePrecacheDto->htmlFooter = $this->templateImageManager - ->parseLogoPlaceholders($messagePrecacheDto->htmlFooter); + $messagePrecacheDto->content = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->content); + if ($messagePrecacheDto->template !== null) { + $messagePrecacheDto->template = $this->templateImageManager + ->parseLogoPlaceholders($messagePrecacheDto->template); + } + $messagePrecacheDto->htmlFooter = $this->templateImageManager + ->parseLogoPlaceholders($messagePrecacheDto->htmlFooter);
🧹 Nitpick comments (34)
src/Domain/Subscription/Service/SubscriberCsvImporter.php (1)
192-192: Consider tracking this TODO as a follow-up task.
If you want, I can help draft the refactor plan or implement the grace‑timeisBlacklistedlogic swap.resources/translations/messages.en.xlf (1)
745-752: Inconsistent placeholder style between similar entries.Two entries describe essentially the same notification but use different placeholder conventions:
- Line 746:
%s(printf-style)- Line 750:
%subject%(Symfony-style named placeholder)If both are intentionally used in different contexts (e.g., one for logging, one for templates), this is fine. Otherwise, consider standardizing on the named
%subject%style for consistency with Symfony conventions and better readability.tests/Unit/Domain/Identity/Service/SessionManagerTest.php (1)
35-44: Heads up onwithConsecutive()for future PHPUnit upgrades.The
withConsecutive()method used here was deprecated in PHPUnit 9.6 and removed in PHPUnit 10. Since you're on PHPUnit 9.5.0, this works fine for now. Just something to keep in mind if you upgrade later — the pattern would need to shift to callback-based expectations or separate test cases.No action needed right now, just a friendly FYI. 🙂
config/services/parameters.yml (1)
2-10: Avoid drift between flat and nested parameters.You now have two copies of the same values. Consider YAML anchors/aliases so updates stay in sync.
♻️ Suggested change (YAML anchors)
- app.config.message_from_address: '[email protected]' - app.config.default_message_age: 15768000 + app.config.message_from_address: &message_from_address '[email protected]' + app.config.default_message_age: &default_message_age 15768000 @@ - app.config: - message_from_address: '[email protected]' + app.config: + message_from_address: *message_from_address admin_address: '[email protected]' - default_message_age: 15768000 + default_message_age: *default_message_agesrc/Domain/Common/PdfGenerator.php (1)
11-18: Make compression configurable to avoid bloated PDFs.Compression is always disabled right now. That’s great for tests, but could increase attachment size in production. Consider a flag so callers can opt in.
♻️ Example tweak (optional flag)
- public function createPdfBytes(string $text): string + public function createPdfBytes(string $text, bool $compress = false): string { $pdf = new FPDF(); - // Disable compression to ensure plain text and metadata are visible in output (helps testing) + // Allow callers to toggle compression (useful for tests vs prod) if (method_exists($pdf, 'SetCompression')) { - $pdf->SetCompression(false); + $pdf->SetCompression($compress); }tests/Unit/Domain/Common/PdfGeneratorTest.php (1)
12-30: Heads up on non-ASCII characters in the test.The test string includes Japanese and Cyrillic characters (
'Hello PDF! こんにちは Привет'), but FPDF's default Arial font doesn't support these glyphs - they'll likely render as blanks or cause issues. The test will pass (PDF structure is valid), but it's worth noting this doesn't validate actual multi-language rendering.If multi-language PDF support is needed down the road, you'd want to look at FPDF's Unicode extension or TCPDF.
src/Domain/Common/OnceCacheGuard.php (1)
18-27: Minor: Potential race condition betweenhas()andset().There's a small TOCTOU window where two concurrent processes could both see
has()return false and both returntrue. For the intended use case (throttling attachment logging per the AI summary), this is probably fine - worst case you'd get a couple extra log entries during the race window.If stricter "exactly once" semantics are ever needed, you'd want an atomic operation, but PSR-16 doesn't expose
setIfNotExists. This is more of an FYI than something to change.tests/Unit/Domain/Identity/Service/PasswordManagerTest.php (1)
214-216: Heads up onwithConsecutive()deprecation.This works fine with PHPUnit 9.5, but
withConsecutive()was deprecated in PHPUnit 10 and removed in PHPUnit 11. If you upgrade later, you'll need to refactor to usewillReturnCallback()with invocation tracking.src/Domain/Messaging/MessageHandler/SubscriberConfirmationMessageHandler.php (1)
23-23: Minor formatting nit: missing space after attribute.For consistency with other handlers (e.g.,
EmailService.phpuses#[Autowire('%app.mailer_from%')] private readonly), add a space after the closing bracket.Suggested fix
- #[Autowire('%app.confirmation_url%')]private readonly string $confirmationUrl + #[Autowire('%app.confirmation_url%')] private readonly string $confirmationUrlsrc/Domain/Common/RemotePageFetcher.php (1)
81-93: Silent exception swallowing makes debugging tricky.The catch block returns an empty string without logging. Consider at least logging at debug level so failures don't vanish into the void.
💡 Suggested improvement
} catch (Throwable $e) { + // Consider injecting LoggerInterface and logging: + // $this->logger->debug('Failed to fetch URL', ['url' => $url, 'error' => $e->getMessage()]); return ''; }src/Domain/Configuration/Model/ConfigOption.php (1)
18-18: Resolve the TODO or link it to a tracked task.
Leaving TODOs in config enums tends to age poorly—can we either remove it or reference a tracking issue?If you want, I can help track down where
domainis defined and suggest the right fix.src/Domain/Configuration/Service/Placeholder/SignatureValueResolver.php (1)
31-35: Consider non-greedy regex forsrcreplacement.The
.*in the pattern is greedy and could over-match if the HTML contains multiple attributes before the target filename. A non-greedy.*?would be safer:♻️ Suggested tweak
return preg_replace( - '/src=".*power-phplist.png"/', + '/src=".*?power-phplist.png"/', 'src="powerphplist.png"', $this->config->getValue(ConfigOption::PoweredByImage) ?? '' );src/Domain/Configuration/Model/Dto/PlaceholderContext.php (1)
43-46: Minor:getUser()is redundant with public$userproperty.Since
$userispublic readonlyon line 14, consumers can access it directly as$ctx->user. ThegetUser()method works fine but adds API surface without benefit. Could remove it or make$userprivate if you want to enforce getter usage.src/Domain/Configuration/Service/Placeholder/ConfirmationUrlValueResolver.php (2)
30-32: Inconsistent HTML escaping compared to sibling resolvers.This resolver only escapes the separator (
&→&), leaving the base URL and uid unescaped. Other resolvers likeBlacklistUrlValueResolverandForwardUrlValueResolverescape the entire URL withhtmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8').If the configured
ConfirmationUrlcontains characters like<,>, or", they'll pass through unescaped in HTML context.♻️ Suggested fix for consistent escaping
if ($ctx->isHtml()) { - return sprintf('%s%suid=%s', $url, htmlspecialchars($sep), $ctx->getUser()->getUniqueId()); + $fullUrl = sprintf('%s%suid=%s', $url, $sep, $ctx->getUser()->getUniqueId()); + return htmlspecialchars($fullUrl, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); }
22-35: Consider using LegacyUrlBuilder for consistency.Other resolvers like
UnsubscribeValueResolverandBlacklistUrlValueResolveruseLegacyUrlBuilder->withUid()to append the uid parameter. This resolver manually builds the query string. UsingLegacyUrlBuilderwould handle edge cases (existing query params, URL encoding) consistently across all resolvers.src/Domain/Configuration/Service/Placeholder/ForwardMessageIdValueResolver.php (1)
52-66: Same escaping inconsistency:uidis not HTML-escaped.The URL and separator are escaped individually (lines 55-56), but
$uidon line 57 is passed raw. For consistency and safety, consider building the full URL first, then escaping it all at once (same pattern suggested forForwardUrlValueResolver).♻️ Suggested fix
if ($ctx->isHtml()) { - $forwardUrl = sprintf( - '%s%suid=%s&mid=%d', - htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'), - htmlspecialchars($sep, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'), - $uid, - $messageId - ); + $rawUrl = sprintf('%s%suid=%s&mid=%d', $url, $sep, $uid, $messageId); + $forwardUrl = htmlspecialchars($rawUrl, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); return sprintf( '<a href="%s">%s</a>',src/Domain/Configuration/Service/Placeholder/PreferencesValueResolver.php (1)
38-45: UnescapeduniqueIdin HTML attribute context.The
uniqueIdvalue from$ctx->getUser()->getUniqueId()is concatenated directly into thehrefattribute without escaping. While UIDs are typically alphanumeric, if one ever contains special characters, this could break the HTML or introduce XSS.Suggested fix
return '<a href="' . $safeUrl - . htmlspecialchars($sep) + . $sep . 'uid=' - . $ctx->getUser()->getUniqueId() + . htmlspecialchars($ctx->getUser()->getUniqueId(), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . '">' . $safeLabel . '</a> ';src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php (2)
33-39: Potential nullmessageIdand unescapeduniqueId.Two things to consider:
$ctx->messageId()can returnnull(per PlaceholderContext), which would producem=with no value in the URL.- The
uniqueIdis directly concatenated without HTML escaping.Suggested hardening
return '<img src="' . $base . '/ut.php?u=' - . $ctx->getUser()->getUniqueId() + . htmlspecialchars($ctx->getUser()->getUniqueId(), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . '&m=' - . $ctx->messageId() + . ($ctx->messageId() ?? 0) . '" width="1" height="1" border="0" alt="" />';
27-31: Base URL isn't escaped before embedding in HTML.If the configured domain or
restApiDomaincontains special characters, this could break the HTML. Consider escaping$basebefore concatenation.src/Domain/Configuration/Service/Placeholder/ForwardValueResolver.php (1)
36-46: UnescapeduniqueIdin HTML href attribute.The
uniqueIdis concatenated directly without escaping on line 40. For consistency with how the URL and label are escaped, this should also be escaped.Also,
$ctx->messageId()can returnnull. The text path (line 48) uses%dwhich converts null to 0, but the HTML path concatenates it directly.Suggested fix
return '<a href="' . htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') - . htmlspecialchars($sep) + . $sep . 'uid=' - . $ctx->getUser()->getUniqueId() + . htmlspecialchars($ctx->getUser()->getUniqueId(), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . '&mid=' - . $ctx->messageId() + . ($ctx->messageId() ?? 0) . '">' . htmlspecialchars($label, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . '</a> ';src/Domain/Configuration/Service/Placeholder/UserDataSupportingResolver.php (1)
10-51: Consider memoizing subscriber data per resolver instance.
resolve()hits the repository on every placeholder; if several user‑data placeholders are used, this can repeat the same query. A small cache keyed by user id would avoid N+1 behavior.♻️ Suggested memoization
class UserDataSupportingResolver implements SupportingPlaceholderResolverInterface { private array $supportedKeys = [ 'CONFIRMED', 'BLACKLISTED', 'OPTEDIN', 'BOUNCECOUNT', 'ENTERED', 'MODIFIED', 'UNIQID', 'UUID', 'HTMLEMAIL', 'SUBSCRIBEPAGE', 'RSSFREQUENCY', 'DISABLED', 'FOREIGNKEY', ]; + private array $userDataCache = []; @@ public function resolve(string $key, PlaceholderContext $ctx): ?string { $canon = strtoupper($key); - $data = $this->subscriberRepository->getDataById($ctx->getUser()->getId()); + $userId = $ctx->getUser()->getId(); + if (!isset($this->userDataCache[$userId])) { + $this->userDataCache[$userId] = $this->subscriberRepository->getDataById($userId); + } + $data = $this->userDataCache[$userId];src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php (3)
97-129: Heads up: DTO mutation as a side effect.The
replaceUserSpecificRemoteContentmethod modifies$messagePrecacheDto->contentand->htmlFormatteddirectly. While this works, it's a hidden side effect that could surprise callers who don't expect their input DTO to be mutated.Consider either:
- Documenting this behavior clearly
- Returning a modified copy instead
Not a blocker, just something to be aware of.
131-131: Minor typo: "Formating" → "Formatting".Just a small spelling fix for the method name.
✏️ Suggested fix
- private function ensureHtmlFormating(string $content, bool $addDefaultStyle): string + private function ensureHtmlFormatting(string $content, bool $addDefaultStyle): stringAnd update the call site on line 91:
- $htmlMessage = $this->ensureHtmlFormating(content: $htmlMessage, addDefaultStyle: $addDefaultStyle); + $htmlMessage = $this->ensureHtmlFormatting(content: $htmlMessage, addDefaultStyle: $addDefaultStyle);
75-94: Placeholder processing flow looks solid.Good separation of text vs HTML processing paths, with format-specific placeholder resolution. The TODO on line 92 about CLICKTRACK is noted - might be worth creating an issue to track.
Want me to open an issue to track the CLICKTRACK TODO?
src/Domain/Configuration/Service/PlaceholderResolver.php (3)
43-49: Handle potentialpreg_replace_callbacknull return.
preg_replace_callbackcan returnnullon error (e.g., malformed pattern). Currently this would silently set$valueto null, which could cause issues downstream.🔧 Suggested defensive fix
foreach ($this->patternResolvers as $resolver) { - $value = preg_replace_callback( + $result = preg_replace_callback( $resolver['pattern'], fn(array $match) => (string) ($resolver['resolver'])($context, $match), $value ); + if ($result !== null) { + $value = $result; + } }
51-56: Same null-safety concern here.The main placeholder resolution regex could also return null on error.
🔧 Suggested fix
- return preg_replace_callback( + $result = preg_replace_callback( '/\[([^\]%%]+)(?:%%([^\]]+))?\]/i', fn(array $matches) => $this->resolveSinglePlaceholder($matches, $context), $value ); + return $result ?? $value;
73-74: Minor: Double normalization.
normalizePlaceholderKeyis called twice here - once for$keyNormalizedand once inside thestrtoupperfor$canon. Could cache the result, but it's probably not a hot path so NBD.$keyNormalized = $this->normalizePlaceholderKey($rawKey); $canon = strtoupper($keyNormalized); // reuse instead of re-normalizingtests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php (1)
229-244: Using Reflection to access private properties is a bit fragile.Accessing
campaignEmailBuildervia Reflection works but couples the test to internal implementation details. If the property name changes, these tests break silently.Consider making the builder injectable or using a test-specific factory. Not urgent since it works, just something to keep in mind for maintainability.
src/Domain/Configuration/Service/MessagePlaceholderProcessor.php (2)
99-107: Case-sensitive placeholder detection.The
str_containschecks for[FOOTER]and[SIGNATURE]are case-sensitive. If templates might use variations like[Footer]or[footer], this could lead to duplicate placeholders being appended. If case variations are expected, consider usingstriposinstead.
50-50: Consider clarifying theFORWARDFORMplaceholder's purpose.
FORWARDFORMis registered to always return an empty string (line 50). UnlikeFORWARDEDBYwhich has an actual implementation, this placeholder appears incomplete. Either add a comment explaining why it returns empty (e.g., "deprecated", "reserved for future use"), or implement its intended functionality.src/Domain/Messaging/Service/AttachmentAdder.php (1)
198-219: Inconsistent behavior: exception first time, silent pass after.The first copy failure throws
AttachmentCopyException, but subsequent failures for the same key silently continue. This asymmetry might be unexpected for callers. Consider documenting this behavior or returning a consistent signal (e.g., always return false, log only once).tests/Unit/Domain/Messaging/Service/AttachmentAdderTest.php (1)
160-233: Consider adding tests for edge cases.Good exception testing! Consider adding coverage for:
- Memory limit exceeded scenario (the
hasMemoryForAttachmentreturns false path)- Subsequent failures for the same key (should not throw per current implementation)
src/Domain/Messaging/Service/MessagePrecacheService.php (1)
21-41: Large constructor acknowledged with suppression.The parameter count is high but the suppression annotation shows awareness. If this grows further, consider introducing a configuration DTO or smaller collaborator services.
src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
219-266: PDF format methods have similar structure.
applyPdfFormatandapplyTextAndPdfFormatshare a similar pattern. If more PDF variants are added, consider extracting common logic. For now, the duplication is acceptable.
| 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) { | ||
| return null; | ||
| } | ||
|
|
||
| if ($ext !== '') { | ||
| $newFile .= '.' . $ext; | ||
| } | ||
| $relativeName = basename($newFile); | ||
|
|
||
| $fullPath = $directory . '/' . $relativeName; | ||
|
|
||
| $fileHandle = fopen($fullPath, 'w'); | ||
|
|
||
| fwrite($fileHandle, $contents); | ||
| fclose($fileHandle); | ||
|
|
||
| return $fullPath; |
There was a problem hiding this comment.
Handle temp-file rename and write failures.
Appending an extension leaves the original temp file orphaned, and fopen/fwrite failures aren’t checked (so the method can return a path that wasn’t actually written).
🛠️ Suggested fix
- $newFile = tempnam($directory, $name);
- if ($newFile === false) {
- return null;
- }
-
- if ($ext !== '') {
- $newFile .= '.' . $ext;
- }
- $relativeName = basename($newFile);
-
- $fullPath = $directory . '/' . $relativeName;
-
- $fileHandle = fopen($fullPath, 'w');
-
- fwrite($fileHandle, $contents);
- fclose($fileHandle);
-
- return $fullPath;
+ $newFile = tempnam($directory, $name);
+ if ($newFile === false) {
+ return null;
+ }
+
+ if ($ext !== '') {
+ $newWithExt = $newFile . '.' . $ext;
+ if (!rename($newFile, $newWithExt)) {
+ `@unlink`($newFile);
+ return null;
+ }
+ $newFile = $newWithExt;
+ }
+
+ $fileHandle = fopen($newFile, 'wb');
+ if ($fileHandle === false) {
+ `@unlink`($newFile);
+ return null;
+ }
+
+ if (fwrite($fileHandle, $contents) === false) {
+ fclose($fileHandle);
+ `@unlink`($newFile);
+ return null;
+ }
+ fclose($fileHandle);
+
+ return $newFile;🤖 Prompt for AI Agents
In `@src/Domain/Common/FileHelper.php` around lines 36 - 59, The
writeFileToDirectory method can leave the original temp file orphaned when you
append an extension and currently does not check fopen/fwrite results; update
writeFileToDirectory to write to the temp file returned by tempnam (use
file_put_contents($tempPath, $contents, LOCK_EX) or check fwrite/fflush returns)
and verify the write succeeded, then if an extension is required perform a
rename($tempPath, $targetPath) and check its return value; on any failure (write
or rename) unlink the temp file and return null, and only return $fullPath when
write+rename (or write alone when no rename needed) have succeeded. Target
symbols: writeFileToDirectory, tempnam, rename, unlink,
file_put_contents/fopen/fwrite/fflush/fclose.
| $caches = $this->urlCacheRepository->getByUrl($url); | ||
| foreach ($caches as $cache) { | ||
| $this->entityManager->remove($cache); | ||
| } | ||
| $urlCache = (new UrlCache())->setUrl($url)->setContent($content)->setLastModified($lastModified); | ||
| $this->urlCacheRepository->persist($urlCache); |
There was a problem hiding this comment.
Domain purity concern: direct EntityManager usage in Domain layer.
Line 67 uses $this->entityManager->remove($cache) directly in src/Domain/**. Per the coding guidelines, domain services shouldn't perform persistence side effects (this isn't DynamicListAttr-related).
Consider moving the cache purging and persistence logic to an Application/Infrastructure layer component, or have the repository handle the remove/persist internally without exposing the EntityManager here.
Based on coding guidelines, Domain services should not perform persistence operations.
🤖 Prompt for AI Agents
In `@src/Domain/Common/RemotePageFetcher.php` around lines 65 - 70,
RemotePageFetcher is performing persistence side effects by calling
$this->entityManager->remove(...) and then
$this->urlCacheRepository->persist(...); remove direct EntityManager usage from
the Domain class and push the logic into the repository/infrastructure layer:
add a single repository method (e.g., UrlCacheRepository::replaceCacheForUrl or
purgeAndSave) that encapsulates getByUrl, removal of existing UrlCache entities,
and persisting the new UrlCache, then update RemotePageFetcher to call that
repository method (passing $url, $content, $lastModified) and delete any
EntityManager references from RemotePageFetcher.
| private function prepareUrl(string $url, array $userData): string | ||
| { | ||
| //# fix the Editor replacing & with & | ||
| $url = str_ireplace('&', '&', $url); | ||
|
|
||
| foreach ($userData as $key => $val) { | ||
| if ($key !== 'password') { | ||
| $url = str_replace('[' . $key . ']', rawurlencode($val), $url); | ||
| } | ||
| } | ||
| $url = mb_convert_encoding($url, 'ISO-8859-1', 'UTF-8'); | ||
|
|
||
| return $this->expandUrl($url); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ISO-8859-1 encoding URLs PHP non-Latin characters issues
💡 Result:
Short answer: modern URLs/IRIs should be encoded as UTF-8 then percent-encoded; ISO‑8859‑1 percent-encoding will misinterpret non‑Latin characters in clients/servers expecting UTF‑8. Use UTF‑8 throughout (or convert on input) and use rawurlencode() (RFC‑3986) or idn_to_ascii() for hostnames. [1][2][3]. (rfc-editor.org)
Common causes
- You percent‑encode ISO‑8859‑1 byte values (e.g. %E9) but consumers expect UTF‑8 (%C3%A9), so characters become garbled. [1][4]. (rfc-editor.org)
Fixes (concise)
- Prefer UTF‑8 everywhere: store strings as UTF‑8 and percent‑encode them. Use rawurlencode() to follow RFC‑3986 (spaces → %20). [2]. (php.net)
- If input is ISO‑8859‑1, convert to UTF‑8 first, then rawurlencode():
- PHP example:
$utf8 = mb_convert_encoding($iso_string, 'UTF-8', 'ISO-8859-1');
$url_safe = rawurlencode($utf8);
- PHP example:
- If you only have single‑byte ISO data and want a quick conversion: utf8_encode($iso_string) then rawurlencode(), but prefer mb_convert_encoding for clarity.
- For query strings from forms, ensure pages use charset=UTF-8 and send so browsers submit UTF‑8. [3]. (stackoverflow.com)
- For internationalized domain names (non‑ASCII host), use IDNA: idn_to_ascii() in PHP. [2]. (php.net)
If you want, tell me how you currently build/receive the URL (PHP version, example string) and I’ll give the exact code to fix it.
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc3986?utm_source=openai
- 2: https://www.rfc-editor.org/rfc/rfc3986?utm_source=openai
- 3: https://www.php.net/manual/en/function.urlencode.php?utm_source=openai
- 4: https://stackoverflow.com/questions/4683081/is-it-advisable-to-have-non-ascii-characters-in-the-url?utm_source=openai
- 5: https://www.php.net/manual/en/function.urlencode.php?utm_source=openai
🌐 Web query:
modern web standards UTF-8 vs ISO-8859-1 URL encoding
💡 Result:
Short answer: use UTF‑8 everywhere. Modern web standards and browser/URL specs expect UTF‑8 for document encoding, form submission and percent-encoding of non‑ASCII characters; ISO‑8859‑1 (or Windows‑1252) is legacy and causes interoperability/garbling issues.
Key points and sources
- HTML/Forms: HTML5/WHATWG encourage/assume UTF‑8; form submission (application/x‑www-form-urlencoded) and URL-related APIs use the document’s encoding (and in practice UTF‑8). Use or UTF‑8 on the HTTP Content-Type. (html.spec.whatwg.org)
- URL percent‑encoding: URIs are sequences of octets; modern URL standards and implementations percent‑encode the UTF‑8 octets for non‑ASCII characters (WHATWG URL / browser APIs follow UTF‑8). (rfc-editor.org)
- Legacy (ISO‑8859‑1): Historically some HTTP text/* defaults and legacy servers used ISO‑8859‑1 (or locale defaults). Relying on ISO‑8859‑1 risks mis-decoding and security/usability problems; browsers do encoding sniffing, so explicitly declaring UTF‑8 is required for correctness. (stackoverflow.com)
- Practical guidance: always serve HTML with Content-Type charset=utf-8, include early in the head, and percent-encode URL parts using UTF‑8 (use encodeURIComponent / URL / URLSearchParams in JS). (html.spec.whatwg.org)
If you want, I can list exact spec quotes and links for implementation detail (percent‑encode sets, form encoding algorithm, HTTP charset rules).
Citations:
- 1: https://html.spec.whatwg.org/document-metadata.html?utm_source=openai
- 2: https://www.rfc-editor.org/rfc/rfc3986?utm_source=openai
- 3: https://stackoverflow.com/questions/52351400/why-its-necessary-to-specify-the-character-encoding-in-an-html5-document-if-the?utm_source=openai
- 4: https://html.spec.whatwg.org/document-metadata.html?utm_source=openai
🌐 Web query:
RFC 3986 URL encoding character set requirements
💡 Result:
Quick summary of RFC 3986 requirements for characters and percent-encoding in URIs:
-
Character classes:
- unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~". These need not be percent-encoded; percent-encoded octets corresponding to unreserved characters should be decoded by normalizers. [1]
- reserved = gen-delims / sub-delims. These may be used as delimiters and therefore normally must be percent-encoded when used as data. [1]
- gen-delims = ":" / "/" / "?" / "#" / "[" / "]" / "@". [1]
- sub-delims = "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "=". [1]
-
Percent-encoding syntax and rules:
- pct-encoded = "%" HEXDIG HEXDIG (two hexadecimal digits). Uppercase hex is recommended for consistency; uppercase and lowercase hex are equivalent. The "%" character itself must be encoded as "%25" when used as data. Implementations must not percent-encode or decode the same string more than once. Decode percent-encoded octets corresponding to unreserved characters when normalizing. [2]
Sources: RFC 3986 (Sections 2.1–2.4, 2.2–2.3). [1][2]
References:
[1] RFC 3986 (rfc-editor).
[2] RFC 3986 (IETF datatracker).
🏁 Script executed:
cat -n src/Domain/Common/RemotePageFetcher.php | head -150Repository: phpList/core
Length of output: 5213
Remove the ISO-8859-1 conversion; it breaks percent-encoded URLs.
Line 121 converts the entire URL to ISO-8859-1, but this conflicts with RFC 3986 and modern standards. The rawurlencode() on line 118 correctly percent-encodes userData values as UTF-8 octets (e.g., é → %C3%A9), but then the ISO-8859-1 conversion causes HTTP clients expecting UTF-8 to misinterpret these bytes, mangling non-Latin characters. Delete the mb_convert_encoding() call on line 121 and keep everything UTF-8 end-to-end.
🤖 Prompt for AI Agents
In `@src/Domain/Common/RemotePageFetcher.php` around lines 111 - 124, The
prepareUrl method is converting the final URL to ISO-8859-1 which breaks
percent-encoded UTF-8 sequences; remove the mb_convert_encoding($url,
'ISO-8859-1', 'UTF-8') call in prepareUrl so the URL remains UTF-8 after
rawurlencode(), and return the result via expandUrl($url) unchanged; ensure no
other code in prepareUrl replaces or re-encodes the URL.
| 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.
🧩 Analysis chain
🌐 Web query:
PHP htmlspecialchars default flags ENT_QUOTES single quotes escaping behavior
💡 Result:
Short answer: Since PHP 8.1 the default flags for htmlspecialchars are ENT_QUOTES | ENT_SUBSTITUTE | ENT_HTML401, so single quotes are converted by default. With ENT_QUOTES a single quote becomes ' for HTML4 (or ' for XML/XHTML/HTML5). [1][2]
References:
- PHP manual: htmlspecialchars (shows default flags and replacements). [1]
- PHP.watch explanation of the PHP 8.1 change (ENT_COMPAT → ENT_QUOTES|ENT_SUBSTITUTE|ENT_HTML401). [2]
🏁 Script executed:
find . -maxdepth 2 -type f \( -name "composer.json" -o -name ".php-version" -o -name ".tool-versions" \) -exec cat {} \;Repository: phpList/core
Length of output: 5758
Avoid redundant config fetch.
The config value is fetched twice; store it in a variable once and reuse it.
🔧 Suggested fix
+ $url = $this->config->getValue(ConfigOption::VCardUrl) ?? '';
+ if ($url === '') {
+ return '';
+ }
if ($ctx->isText()) {
- return $this->config->getValue(ConfigOption::VCardUrl) ?? '';
+ return $url;
}
- return htmlspecialchars($this->config->getValue(ConfigOption::VCardUrl) ?? '');
+ return htmlspecialchars($url);🤖 Prompt for AI Agents
In `@src/Domain/Configuration/Service/Placeholder/ContactUrlValueResolver.php`
around lines 22 - 28, In ContactUrlValueResolver::__invoke, avoid calling
$this->config->getValue(ConfigOption::VCardUrl) twice; fetch it once into a
local variable (e.g., $url = $this->config->getValue(ConfigOption::VCardUrl) ??
''), then return $url for text contexts and htmlspecialchars($url) for non-text
contexts, preserving the existing logic with a single config lookup.
| $remoteContent = ($this->remotePageFetcher)($regs[1], []); | ||
| if (!$remoteContent) { | ||
| $this->eventLogManager->log( | ||
| page: 'unknown page', | ||
| entry: 'Error fetching URL: ' . $loadedMessageData['sendurl'] . ' cannot proceed', | ||
| ); | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Error message references wrong URL variable.
Line 192 logs $loadedMessageData['sendurl'] but the actual URL being fetched is $regs[1] (extracted from the content). This could be misleading in logs.
🔧 Suggested fix
$remoteContent = ($this->remotePageFetcher)($regs[1], []);
if (!$remoteContent) {
$this->eventLogManager->log(
page: 'unknown page',
- entry: 'Error fetching URL: ' . $loadedMessageData['sendurl'] . ' cannot proceed',
+ entry: 'Error fetching URL: ' . $regs[1] . ' cannot proceed',
);
return false;
}🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/MessagePrecacheService.php` around lines 188 -
196, The log message is referencing the wrong URL variable which is misleading:
when ($this->remotePageFetcher) is called with $regs[1] and returns falsy (the
$remoteContent check in MessagePrecacheService), update the
$this->eventLogManager->log call to reference the actual fetched URL ($regs[1])
instead of $loadedMessageData['sendurl'] — optionally include both values for
context (fetched URL $regs[1] and original sendurl
$loadedMessageData['sendurl']) so the log clearly shows which URL failed; adjust
the log entry in the remoteContent falsy branch accordingly.
| public function getDataById(int $subscriberId): array | ||
| { | ||
| return $this->createQueryBuilder('s') | ||
| ->select('s') | ||
| ->where('s.id = :subscriberId') | ||
| ->setParameter('subscriberId', $subscriberId) | ||
| ->getQuery() | ||
| ->getArrayResult()[0]; |
There was a problem hiding this comment.
Handle “not found” to avoid undefined offset/TypeError.
getArrayResult()[0] will throw a notice and return null when no record exists, violating the array return type.
🐛 Safer return handling
- public function getDataById(int $subscriberId): array
+ 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];
+ ->getArrayResult();
+
+ return $result[0] ?? null;
}📝 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.
| public function getDataById(int $subscriberId): array | |
| { | |
| return $this->createQueryBuilder('s') | |
| ->select('s') | |
| ->where('s.id = :subscriberId') | |
| ->setParameter('subscriberId', $subscriberId) | |
| ->getQuery() | |
| ->getArrayResult()[0]; | |
| public function getDataById(int $subscriberId): ?array | |
| { | |
| $result = $this->createQueryBuilder('s') | |
| ->select('s') | |
| ->where('s.id = :subscriberId') | |
| ->setParameter('subscriberId', $subscriberId) | |
| ->getQuery() | |
| ->getArrayResult(); | |
| return $result[0] ?? null; | |
| } |
🤖 Prompt for AI Agents
In `@src/Domain/Subscription/Repository/SubscriberRepository.php` around lines 209
- 216, getDataById currently calls getArrayResult()[0] which will trigger an
undefined offset/TypeError when no record is found; modify getDataById to store
the result of createQueryBuilder(...)->getQuery()->getArrayResult() in a
variable, check if the array is empty, and then either return an empty array or
throw a specific not-found exception (e.g., SubscriberNotFoundException) to
preserve the declared return type and avoid notices; update the method
(getDataById) to handle the empty result path before indexing [0].
| public function testHtmlEscapesUrlAndAppendsUid(): void | ||
| { | ||
| $raw = 'https://e.com/prefs.php?a=1&x=<tag>"\''; | ||
| $this->config->method('getValue') | ||
| ->with(ConfigOption::PreferencesUrl) | ||
| ->willReturn($raw); | ||
|
|
||
| $ctx = new PlaceholderContext(user: $this->makeUser(), format: OutputFormat::Html); | ||
|
|
||
| $resolver = new PreferencesUrlValueResolver($this->config); | ||
| $result = $resolver($ctx); | ||
|
|
||
| $this->assertSame( | ||
| sprintf('%s%suid=%s', htmlspecialchars($raw), htmlspecialchars('&'), 'UID-PREF'), | ||
| $result | ||
| ); |
There was a problem hiding this comment.
Align HTML escaping flags with production.
Line 77–78 uses htmlspecialchars() defaults, but the resolver uses ENT_QUOTES | ENT_SUBSTITUTE + UTF-8. This can break the expected string (single quotes won’t be escaped the same).
🛠️ 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('&', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
+ 'UID-PREF'
+ ),
+ $result
+ );🤖 Prompt for AI Agents
In
`@tests/Unit/Domain/Configuration/Service/Placeholder/PreferencesUrlValueResolverTest.php`
around lines 65 - 80, The test's expected string uses htmlspecialchars() with
default flags while PreferencesUrlValueResolver uses ENT_QUOTES | ENT_SUBSTITUTE
and 'UTF-8'; update the test in testHtmlEscapesUrlAndAppendsUid to call
htmlspecialchars($raw, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') and
htmlspecialchars('&', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') so the escaping
behavior matches PreferencesUrlValueResolver and the asserted string is produced
correctly.
| private function makeCtx(Subscriber $user = null): PlaceholderContext | ||
| { | ||
| $u = $user ?? (function () { | ||
| $s = new Subscriber(); | ||
| $s->setEmail('[email protected]'); | ||
| $s->setUniqueId('UID-X'); | ||
| // Ensure the entity has a non-null ID for repository lookup | ||
| $rp = new \ReflectionProperty(Subscriber::class, 'id'); | ||
| $rp->setAccessible(true); | ||
| $rp->setValue($s, 42); | ||
| return $s; | ||
| })(); | ||
|
|
||
| return new PlaceholderContext($u, OutputFormat::Text); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "composer.json" -o -name ".php-version" -o -name "phpversion.txt" 2>/dev/null | head -5Repository: phpList/core
Length of output: 38
🏁 Script executed:
cd /tmp && git ls-files | grep -E "(composer\.json|\.php-version)" | head -5Repository: phpList/core
Length of output: 124
🏁 Script executed:
cat /tmp/composer.json 2>/dev/null | grep -A 5 '"php"'Repository: phpList/core
Length of output: 38
🏁 Script executed:
cat /tmp/tests/Unit/Domain/Configuration/Service/Placeholder/UserDataSupportingResolverTest.php | sed -n '24,38p'Repository: phpList/core
Length of output: 179
🏁 Script executed:
pwd && ls -laRepository: phpList/core
Length of output: 1598
🏁 Script executed:
git ls-files | grep "composer.json" | head -1Repository: phpList/core
Length of output: 69
🏁 Script executed:
git ls-files | grep "UserDataSupportingResolverTest.php"Repository: phpList/core
Length of output: 142
🏁 Script executed:
cat composer.json | grep -A 3 '"php"'Repository: phpList/core
Length of output: 192
🏁 Script executed:
sed -n '20,40p' tests/Unit/Domain/Configuration/Service/Placeholder/UserDataSupportingResolverTest.phpRepository: phpList/core
Length of output: 766
🌐 Web query:
PHP 8.1 deprecated implicitly nullable parameters null default
💡 Result:
Short answer: PHP 8.1 deprecates using an implicit nullable type by writing "Type $param = null". You should change such signatures to an explicit nullable union or ?-type (e.g. Type|null $param = null or ?Type $param = null). [1][2][3]
Details and examples:
- Deprecated pattern:
function foo(SomeClass $x = null) {} // implicitly nullable — deprecated in 8.1. [1][2] - Correct replacements:
function foo(?SomeClass $x = null) {} // short nullable syntax. [1]
function foo(SomeClass|null $x = null) {} // union nullable syntax. [1] - Rationale: implicit-null defaults are confusing and conflict with other rules (and caused special-casing around optional-before-required parameters), so RFCs and the migration guide require explicit nullability. [2][1]
- Also note: PHP 8.1 added deprecations for passing null to non-nullable internal function parameters; internal functions now warn when null is passed where null wasn't explicitly allowed. [3]
If you want, I can scan your codebase for occurrences and provide an automated fix (sed/php-cs-fixer rules / Rector suggestions).
Make Subscriber explicitly nullable.
Line 24 uses a default null with a non-nullable type. PHP 8.1 deprecated this pattern—you need to explicitly mark the parameter as nullable.
Suggested fix
- private function makeCtx(Subscriber $user = null): PlaceholderContext
+ private function makeCtx(?Subscriber $user = null): PlaceholderContext📝 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.
| private function makeCtx(Subscriber $user = null): PlaceholderContext | |
| { | |
| $u = $user ?? (function () { | |
| $s = new Subscriber(); | |
| $s->setEmail('[email protected]'); | |
| $s->setUniqueId('UID-X'); | |
| // Ensure the entity has a non-null ID for repository lookup | |
| $rp = new \ReflectionProperty(Subscriber::class, 'id'); | |
| $rp->setAccessible(true); | |
| $rp->setValue($s, 42); | |
| return $s; | |
| })(); | |
| return new PlaceholderContext($u, OutputFormat::Text); | |
| } | |
| private function makeCtx(?Subscriber $user = null): PlaceholderContext | |
| { | |
| $u = $user ?? (function () { | |
| $s = new Subscriber(); | |
| $s->setEmail('[email protected]'); | |
| $s->setUniqueId('UID-X'); | |
| // Ensure the entity has a non-null ID for repository lookup | |
| $rp = new \ReflectionProperty(Subscriber::class, 'id'); | |
| $rp->setAccessible(true); | |
| $rp->setValue($s, 42); | |
| return $s; | |
| })(); | |
| return new PlaceholderContext($u, OutputFormat::Text); | |
| } |
🤖 Prompt for AI Agents
In
`@tests/Unit/Domain/Configuration/Service/Placeholder/UserDataSupportingResolverTest.php`
around lines 24 - 38, The makeCtx helper has a parameter typed as Subscriber but
defaulted to null which is deprecated; update the function signature of makeCtx
to accept a nullable Subscriber (e.g., ?Subscriber $user = null) and keep the
existing null-coalescing logic that constructs a Subscriber when $user is null;
ensure any related docblock/type hints reflect the nullable type so tools and
PHP 8.1+ no longer warn.
| ->withConsecutive( | ||
| [1, $precached, $subscriber1], | ||
| [1, $precached, $subscriber2] | ||
| ) | ||
| ->willReturnOnConsecutiveCalls($precached, $precached); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHPUnit 10 withConsecutive deprecated removed migration
💡 Result:
Short summary: PHPUnit 10 removed InvocationMocker::withConsecutive(). Replace it by (a) using argument constraints/callbacks to validate per-call arguments, (b) use willReturnOnConsecutiveCalls / willReturnCallback for returned values, or (c) adopt a framework/test-case helper that reintroduces a static withConsecutive helper (many projects provide one). See examples below.
Why: withConsecutive was deprecated in 9.6 and removed in PHPUnit 10. [1][2]
Common migration patterns
- Validate per-call arguments with a callback constraint (recommended when you asserted arguments)
- Old (PHPUnit ≤9):
$mock->expects($this->exactly(2))
->method('foo')
->withConsecutive(['a'], ['b']); - New:
$mock->expects($this->exactly(2))
->method('foo')
->with($this->callback(function ($arg) {
static $i = 0;
$expected = ['a', 'b'][$i++] ?? null;
return $arg === $expected;
}));
This pattern (use callback constraint) is documented in the PHPUnit test-doubles docs. [3]
- If you only need different return values, use willReturnOnConsecutiveCalls (no withConsecutive needed)
- $mock->method('foo')->willReturnOnConsecutiveCalls('r1', 'r2');
(Use willReturnCallback when logic is required.) [4]
- Project-level helper approach (ad hoc compatibility)
- Some frameworks/teams add a helper/trait that provides a static withConsecutive() wrapper you can call from tests (CakePHP provides an example trait/approach and docs showing a static-compatible helper). Example conversion shown in CakePHP docs: convert ->withConsecutive(...) to ->with(...self::withConsecutive([...], [...])) via a trait. [5][6]
Concrete resources to read and copy examples
- PHPUnit 9.6 test-doubles note (withConsecutive deprecated/removed). [3]
- PHPUnit 10 release/upgrade notes (removals). [1][2]
- CakePHP and Drupal migration examples showing callback/trait replacements and practical diffs. [5][6][4]
If you want, tell me which specific withConsecutive usage (code snippet) you have and I’ll provide an exact migrated replacement.
Sources
[1] PHPUnit 10 release/upgrade notes.
[2] GitHub issue / PHPUnit removal discussion (withConsecutive deprecation).
[3] PHPUnit 9.6 Test Doubles docs (callback constraint example).
[4] Drupal migration issue and examples (use of callbacks/willReturnCallback).
[5] CakePHP PHPUnit 10 upgrade notes (shows converting withConsecutive via helper/trait).
[6] CakePHP docs example conversion.
Heads up: withConsecutive was deprecated in PHPUnit 9.6 and removed in PHPUnit 10.
For PHPUnit 10 migration, the recommended pattern is using a callback constraint with a counter to validate per-call arguments, or a project-level helper/trait if you want to keep the syntax similar. willReturnOnConsecutiveCalls alone won't validate arguments—you'd need to combine it with the callback approach.
Not blocking since you're on PHPUnit 9.5.0, but worth tackling before upgrading.
🤖 Prompt for AI Agents
In
`@tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php`
around lines 371 - 375, Replace the deprecated withConsecutive usage on the mock
by switching to a willReturnCallback closure that keeps a local call counter;
inside the callback validate each invocation's arguments against the expected
values for that call (e.g., check arg0==1, arg1==$precached, arg2==$subscriber1
for call 0, then subscriber2 for call 1) and return the corresponding predefined
response (or pull from an array of return values, similar to
willReturnOnConsecutiveCalls). Concretely, change the mock setup that used
withConsecutive(...) ->willReturnOnConsecutiveCalls(...) to a single
->willReturnCallback(function(...$args) use (&$i, $expectedArgs, $returns) { /*
assert args match $expectedArgs[$i], $ret=$returns[$i]; $i++; return $ret; */
}), referencing the same expected values ($precached, $subscriber1,
$subscriber2) and returns to preserve behavior.
- Message forwarding: send campaigns to friends (optional personal note), per-user limits, admin notifications on success/failure, and forwarding statistics; forwarded messages prefixed "Fwd". - Admin-copy emails: configurable toggle to send admin copies and select recipients. --------- Co-authored-by: Tatevik <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In `@config/services/messenger.yml`:
- Around line 14-17: The CampaignProcessorMessageHandler has an
#[Autowire('%imap_bounce.email%')] on its constructor $bounceEmail parameter but
the service definition's arguments: block is empty; update the service entry for
PhpList\Core\Domain\Messaging\MessageHandler\CampaignProcessorMessageHandler to
explicitly wire the parameter by adding an arguments mapping for $bounceEmail to
the imap_bounce.email parameter (i.e., set $bounceEmail ->
'%imap_bounce.email%') so DI can resolve the constructor argument.
In `@src/Domain/Common/ExternalImageService.php`:
- Around line 168-173: In isCacheableUrl, guard the str_contains call against a
null website value from $this->configProvider->getValue(ConfigOption::Website):
fetch the website into a local variable (e.g. $website), verify it is a
non-empty string (or coalesce to an empty string) before calling
str_contains(..., '://' . $website . '/'), and only perform that str_contains
check when $website is not null/empty so you avoid a TypeError from str_contains
when getValue returns null.
In `@src/Domain/Configuration/Service/MessagePlaceholderProcessor.php`:
- Around line 95-112: The current ensureStandardPlaceholders method uses
str_contains which misses different-cased placeholders and placeholders with
defaults (e.g. [FOOTER%%default]), causing duplicates; update the presence
checks to use case-insensitive regex that matches the placeholder name plus an
optional "%%..." default segment (for example use a pattern like
/\[(PLACEHOLDER)(?:%%[^\]]*)?\]/i) for each placeholder checked in
ensureStandardPlaceholders so appendContent is only called when no matching
placeholder (FOOTER, SIGNATURE, USERTRACK) exists, keeping the existing
separator logic and respecting OutputFormat::Html and the alwaysAddUserTrack
branch.
In
`@src/Domain/Configuration/Service/Placeholder/ConfirmationUrlValueResolver.php`:
- Around line 22-36: The HTML branch of ConfirmationUrlValueResolver::__invoke
currently only escapes the separator; update it to escape the URL consistently
with sibling resolvers by passing htmlspecialchars($url, ENT_QUOTES |
ENT_SUBSTITUTE, 'UTF-8') (and keep escaping the separator as before) when
building the returned string; leave the non-HTML branch unchanged so only the
HTML output is sanitized.
In `@src/Domain/Configuration/Service/Placeholder/ForwardValueResolver.php`:
- Around line 25-49: The __invoke method in ForwardValueResolver builds a query
parameter mid using $ctx->messageId() which is nullable; when null it becomes
mid=0 or an empty param — guard against null by checking $ctx->messageId()
(and/or $ctx->forwardedBy()) and omit the mid parameter entirely when it is
null: update the HTML branch (where the anchor is built) and the plaintext
return (sprintf call) in __invoke to only append "&mid=..." / "mid=..." when
$ctx->messageId() is not null, keeping existing htmlspecialchars usage and uid
logic (methods: __invoke, messageId(), isHtml(), forwardedBy(),
getUser()->getUniqueId()).
In `@src/Domain/Configuration/Service/Placeholder/PreferencesValueResolver.php`:
- Around line 25-50: In PreferencesValueResolver::__invoke, the UID ($uid) is
inserted raw into the href when $ctx->isHtml() — escape or build the final URL
before escaping to prevent broken/unsafe HTML; either run htmlspecialchars($uid,
ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') before concatenation or (preferable) use
the same pattern as UnsubscribeValueResolver by composing the full URL (e.g. via
urlBuilder->withUid(...) or equivalent) and then apply htmlspecialchars once to
the complete href ($safeUrl or $fullUrl) so the uid is properly encoded in the
anchor href.
In `@src/Domain/Identity/Service/AdminCopyEmailSender.php`:
- Around line 29-52: The __invoke method currently calls
$this->systemEmailBuilder->buildSystemEmail(data: $data) and passes its result
directly to MailerInterface::send, but buildSystemEmail can return null causing
a TypeError; modify the loop in AdminCopyEmailSender::__invoke to check if
$email is null after calling buildSystemEmail (use the same $data/$email
variables), and if null skip sending (and optionally log via
configProvider/logger) instead of constructing the Envelope and calling
$this->mailer->send with a null message; ensure resolveRecipientEmails and
Envelope creation remain unchanged for non-null $email.
In `@src/Domain/Messaging/Service/AttachmentAdder.php`:
- Around line 177-196: copyAttachmentToRepository is persisting the full path
returned by FileHelper::writeFileToDirectory into Attachment::setFilename, which
later causes attachFromRepository to build duplicate-prefixed paths; change the
code so that after writeFileToDirectory returns, you extract and store only the
basename (e.g. via PHP basename/pathinfo) before calling
Attachment::setFilename, or alternatively make writeFileToDirectory return a
relative name — but do not save the absolute path into Attachment::filename
(refer to copyAttachmentToRepository, FileHelper::writeFileToDirectory,
Attachment::setFilename, and attachFromRepository when making the change).
In `@src/Domain/Messaging/Service/Builder/EmailBuilder.php`:
- Around line 236-250: When htmlPref is true the Pdf branch only attaches a PDF
and never sets an email body, resulting in an empty message; update the Pdf
branch in EmailBuilder.php (the block using htmlPref, sentAs =
OutputFormat::Pdf, $this->pdfGenerator->createPdfBytes and $email->attach) to
also set a visible body before calling $this->attachmentAdder->add — e.g. call
$email->text("Please see attached PDF") and/or $email->html("<p>Please see
attached PDF</p>") so recipients have a readable fallback; keep throwing
AttachmentException on add failure as-is.
- Line 238: $textMessage is nullable but is being passed to
PdfGenerator::createPdfBytes(string) which requires a non-null string; add a
null-check before calling createPdfBytes (and the similar call around the other
occurrence) in EmailBuilder so you only call PdfGenerator::createPdfBytes when
$textMessage is !== null — otherwise provide a safe fallback (e.g. skip PDF
generation, pass an empty string, or throw a clear InvalidArgumentException).
Update the logic around the occurrences of $textMessage (the call at the line
with $this->pdfGenerator->createPdfBytes($textMessage) and the similar call at
the other occurrence) to guard against null values and handle the null case
explicitly.
In `@src/Domain/Messaging/Service/Constructor/SystemMailContentBuilder.php`:
- Around line 55-57: The strpos check for '</body>' in
SystemMailContentBuilder.php is using a truthy test that fails when the tag is
at position 0; change the condition in the elseif that currently calls
strpos($htmlContent, '</body>') to a strict comparison strpos($htmlContent,
'</body>') !== false so the block (which calls str_replace('</body>',
$phpListPowered.'</body>', $htmlContent)) runs correctly whenever the tag
exists; update that single conditional and leave the replacement using
$phpListPowered and $htmlContent untouched.
In `@src/Domain/Messaging/Service/MessageForwardService.php`:
- Around line 70-81: Wrap the call to
$this->forwardDeliveryService->send($email) and the subsequent success-path
calls (adminNotifier->notifyForwardSucceeded, forwardDeliveryService->markSent,
campaign->incrementSentCount, forwardingStatsService->incrementFriendsCount) in
a try-catch; on exception catch (\Throwable $e) call
$this->adminNotifier->notifyForwardFailed(...) and
$this->forwardDeliveryService->markFailed($campaign, $forwardingSubscriber,
$friendEmail) (passing the exception or its message as the notify/markFailed
APIs expect), then continue the loop so processing of remaining forwards
proceeds unchanged. Ensure the successful path still executes the same methods
(notifyForwardSucceeded, markSent, incrementSentCount, incrementFriendsCount)
when no exception occurs.
In `@src/Domain/Messaging/Service/MessagePrecacheService.php`:
- Around line 49-53: The cached value ($cached) retrieved via
$this->cache->get($cacheKey) is returned directly but the method signature
declares a boolean return (: bool), causing a TypeError on cache hit; change the
cache-hit branch to return a boolean (e.g., return (bool) $cached or return
true/false based on the cached content) instead of returning the DTO, and ensure
any code that saves into the cache for this key ($this->cache->set(...)) stores
a value that can be meaningfully cast to bool for the method's contract.
In `@tests/Unit/Domain/Messaging/Service/ForwardingStatsServiceTest.php`:
- Around line 69-84: The test configures the attrManager mock twice for
createOrUpdateByName which is unreliable; remove the second expectation
(expects(self::never())) and instead assert behavior by either changing the
initial expectation to expects(self::atMostOnce()) or simply deleting the
reconfiguration so the first expects(self::once()) defined on
ForwardingStatsServiceTest remains authoritative; update the test around the
methods incrementFriendsCount and updateFriendsCount to rely on that single
expectation on attrManager->createOrUpdateByName.
🧹 Nitpick comments (9)
src/Domain/Analytics/Service/LinkTrackService.php (2)
43-43: TODO comment flagged for forwarded message handling.Heads up on this reminder you left yourself. When forwarding, it sounds like you'll want to distinguish forwarded link tracking from regular user tracking. Makes sense to defer this if the forwarding flow isn't fully wired up yet.
Want me to help sketch out how the forwarded case could be handled here, or open an issue to track this work?
52-52: Minor: Redundant null coalescing.The
contentproperty onMessagePrecacheDtois typed asstringwith a default of'', so it can't be null. The?? ''is technically unnecessary here, though it's harmless as defensive coding.💡 Optional cleanup
- $links = $this->extractLinksFromHtml($content->content ?? ''); + $links = $this->extractLinksFromHtml($content->content);src/Domain/Configuration/Model/Dto/PlaceholderContext.php (1)
43-46: Minor: Redundant getter for public property.
getUser()duplicates access to$this->userwhich is alreadypublic readonly. Either make the property private (like$forwardedBy) or remove the getter for consistency.Option A: Remove redundant getter
- public function getUser(): Subscriber - { - return $this->user; - }Option B: Make property private like others
public function __construct( - public readonly Subscriber $user, + private readonly Subscriber $user, public readonly OutputFormat $format,src/Domain/Messaging/Service/ForwardDeliveryService.php (1)
39-57: Consider extracting status strings to constants.Both
'sent'and'failed'are used here and in the repository query. A shared constant (e.g.,UserMessageForward::STATUS_SENT) would reduce magic strings and ensure consistency.src/Domain/Messaging/Service/Builder/HttpReceivedStampBuilder.php (1)
31-33: Consider avoiding reverse-DNS on the request path.
gethostbyaddr()can block and add latency; consider making it optional (config flag) or caching the result, and rely onREMOTE_HOSTwhen already present.Also applies to: 46-52
tests/Unit/Domain/Messaging/Service/Builder/EmailBuilderTest.php (2)
208-246: Redundant configProvider mock setup.Lines 210-220 duplicate the exact same
willReturnMapconfiguration that's already set insetUp()at lines 63-73. This isn't harmful but adds noise.♻️ Consider removing the duplicate mock setup
public function testPrefersTextWhenNoHtmlContent(): void { - $this->configProvider - ->method('getValue') - ->willReturnMap([ - [ConfigOption::MessageFromAddress, '[email protected]'], - [ConfigOption::MessageFromName, 'From Name'], - [ConfigOption::UnsubscribeUrl, 'https://example.com/unsubscribe'], - [ConfigOption::PreferencesUrl, 'https://example.com/prefs'], - [ConfigOption::SubscribeUrl, 'https://example.com/subscribe'], - [ConfigOption::AdminAddress, '[email protected]'], - [ConfigOption::AlwaysSendTextDomains, ''], - ]); - $this->blacklistRepository->method('isEmailBlacklisted')->willReturn(false);
358-390: Test documents a potential bug in production code.Line 388-389 notes that
List-Ownerheader usesUnsubscribeUrlinstead ofAdminAddress, which the comment flags as "likely a bug". The test asserts current behavior, which is the right approach for regression tests.Consider opening an issue to track fixing this in the production code if it's indeed unintended.
src/Domain/Configuration/Model/ConfigOption.php (2)
36-37: Inconsistent string value casing.
PoweredByImageandPoweredByTextuse PascalCase while all other config values use lowercase or snake_case. If these must match existing legacy config keys, that's fine—just worth noting for future additions to follow a consistent pattern.
18-18: TODO comment should be resolved or documented.This TODO has been here a while. Either track it in an issue, add context about what needs checking, or remove it if no longer relevant.
Want me to open an issue to track investigating where
Domainis defined?
| PhpList\Core\Domain\Messaging\MessageHandler\CampaignProcessorMessageHandler: | ||
| autowire: true | ||
| arguments: | ||
| $maxMailSize: '%messaging.max_mail_size%' | ||
|
|
||
| PhpList\Core\Domain\Subscription\MessageHandler\DynamicTableMessageHandler: | ||
| autowire: true | ||
| autoconfigure: true | ||
| tags: [ 'messenger.message_handler' ] | ||
| arguments: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
ast-grep --pattern $'class CampaignProcessorMessageHandler {
$$$
public function __construct($$$) {
$$$
}
$$$
}'Repository: phpList/core
Length of output: 38
🏁 Script executed:
fd -type f -name "*.php" | xargs grep -l "class CampaignProcessorMessageHandler"Repository: phpList/core
Length of output: 228
🏁 Script executed:
rg "class CampaignProcessorMessageHandler" --type php -lRepository: phpList/core
Length of output: 210
🏁 Script executed:
cat src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpRepository: phpList/core
Length of output: 15405
The arguments: block needs to include the bounceEmail configuration.
The CampaignProcessorMessageHandler constructor has an #[Autowire('%imap_bounce.email%')] attribute on the $bounceEmail parameter, which requires explicit wiring. The empty arguments: block should define this value. Something like:
arguments:
$bounceEmail: '%imap_bounce.email%'Without it, dependency injection will fail for this handler.
🤖 Prompt for AI Agents
In `@config/services/messenger.yml` around lines 14 - 17, The
CampaignProcessorMessageHandler has an #[Autowire('%imap_bounce.email%')] on its
constructor $bounceEmail parameter but the service definition's arguments: block
is empty; update the service entry for
PhpList\Core\Domain\Messaging\MessageHandler\CampaignProcessorMessageHandler to
explicitly wire the parameter by adding an arguments mapping for $bounceEmail to
the imap_bounce.email parameter (i.e., set $bounceEmail ->
'%imap_bounce.email%') so DI can resolve the constructor argument.
| private function isCacheableUrl($filename): bool | ||
| { | ||
| if (!(str_starts_with($filename, 'http')) | ||
| || str_contains($filename, '://' . $this->configProvider->getValue(ConfigOption::Website) . '/') | ||
| ) { | ||
| return false; |
There was a problem hiding this comment.
Guard str_contains against null website config.
getValue() can return null; str_contains() will TypeError.
🛠️ Fix
- if (!(str_starts_with($filename, 'http'))
- || str_contains($filename, '://' . $this->configProvider->getValue(ConfigOption::Website) . '/')
- ) {
+ $website = $this->configProvider->getValue(ConfigOption::Website) ?? '';
+ if (!(str_starts_with($filename, 'http'))
+ || ($website !== '' && str_contains($filename, '://' . $website . '/'))
+ ) {
return false;
}🤖 Prompt for AI Agents
In `@src/Domain/Common/ExternalImageService.php` around lines 168 - 173, In
isCacheableUrl, guard the str_contains call against a null website value from
$this->configProvider->getValue(ConfigOption::Website): fetch the website into a
local variable (e.g. $website), verify it is a non-empty string (or coalesce to
an empty string) before calling str_contains(..., '://' . $website . '/'), and
only perform that str_contains check when $website is not null/empty so you
avoid a TypeError from str_contains when getValue returns null.
| private function ensureStandardPlaceholders(string $value, OutputFormat $format): string | ||
| { | ||
| if (!str_contains($value, '[FOOTER]')) { | ||
| $sep = $format === OutputFormat::Html ? '<br />' : "\n\n"; | ||
| $value = $this->appendContent($value, $sep . '[FOOTER]'); | ||
| } | ||
|
|
||
| if (!str_contains($value, '[SIGNATURE]')) { | ||
| $sep = $format === OutputFormat::Html ? ' ' : "\n"; | ||
| $value = $this->appendContent($value, $sep . '[SIGNATURE]'); | ||
| } | ||
|
|
||
| if ($this->alwaysAddUserTrack && $format === OutputFormat::Html && !str_contains($value, '[USERTRACK]')) { | ||
| $value = $this->appendContent($value, ' ' . '[USERTRACK]'); | ||
| } | ||
|
|
||
| return $value; | ||
| } |
There was a problem hiding this comment.
Detect existing placeholders case‑insensitively (and with defaults).
str_contains('[FOOTER]') misses [footer] and [FOOTER%%default], so standard placeholders can be appended twice. A regex check prevents duplicates.
🛠️ Suggested fix
- if (!str_contains($value, '[FOOTER]')) {
+ if (!preg_match('/\[FOOTER(?:%%[^\]]+)?\]/i', $value)) {
$sep = $format === OutputFormat::Html ? '<br />' : "\n\n";
$value = $this->appendContent($value, $sep . '[FOOTER]');
}
- if (!str_contains($value, '[SIGNATURE]')) {
+ if (!preg_match('/\[SIGNATURE(?:%%[^\]]+)?\]/i', $value)) {
$sep = $format === OutputFormat::Html ? ' ' : "\n";
$value = $this->appendContent($value, $sep . '[SIGNATURE]');
}
- if ($this->alwaysAddUserTrack && $format === OutputFormat::Html && !str_contains($value, '[USERTRACK]')) {
+ if (
+ $this->alwaysAddUserTrack
+ && $format === OutputFormat::Html
+ && !preg_match('/\[USERTRACK(?:%%[^\]]+)?\]/i', $value)
+ ) {
$value = $this->appendContent($value, ' ' . '[USERTRACK]');
}🤖 Prompt for AI Agents
In `@src/Domain/Configuration/Service/MessagePlaceholderProcessor.php` around
lines 95 - 112, The current ensureStandardPlaceholders method uses str_contains
which misses different-cased placeholders and placeholders with defaults (e.g.
[FOOTER%%default]), causing duplicates; update the presence checks to use
case-insensitive regex that matches the placeholder name plus an optional
"%%..." default segment (for example use a pattern like
/\[(PLACEHOLDER)(?:%%[^\]]*)?\]/i) for each placeholder checked in
ensureStandardPlaceholders so appendContent is only called when no matching
placeholder (FOOTER, SIGNATURE, USERTRACK) exists, keeping the existing
separator logic and respecting OutputFormat::Html and the alwaysAddUserTrack
branch.
| public function __invoke(PlaceholderContext $ctx): string | ||
| { | ||
| $url = $this->config->getValue(ConfigOption::ConfirmationUrl); | ||
| if (empty($url)) { | ||
| return ''; | ||
| } | ||
| $sep = !str_contains($url, '?') ? '?' : '&'; | ||
| $uid = $ctx->forwardedBy() ? 'forwarded' : $ctx->getUser()->getUniqueId(); | ||
|
|
||
| if ($ctx->isHtml()) { | ||
| return sprintf('%s%suid=%s', $url, htmlspecialchars($sep), $uid); | ||
| } | ||
|
|
||
| return sprintf('%s%suid=%s', $url, $sep, $uid); | ||
| } |
There was a problem hiding this comment.
Inconsistent HTML escaping compared to sibling resolvers.
In the HTML branch (Line 32), only the separator is escaped, but the URL itself is output raw. Compare with ForwardUrlValueResolver and PreferencesUrlValueResolver from the relevant snippets, which both escape the URL using htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8').
If the URL contains characters like & or ", the current implementation could produce invalid HTML or XSS vectors.
🔒 Suggested fix for consistent HTML escaping
if ($ctx->isHtml()) {
- return sprintf('%s%suid=%s', $url, htmlspecialchars($sep), $uid);
+ return sprintf(
+ '%s%suid=%s',
+ htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
+ htmlspecialchars($sep),
+ $uid
+ );
}🤖 Prompt for AI Agents
In
`@src/Domain/Configuration/Service/Placeholder/ConfirmationUrlValueResolver.php`
around lines 22 - 36, The HTML branch of ConfirmationUrlValueResolver::__invoke
currently only escapes the separator; update it to escape the URL consistently
with sibling resolvers by passing htmlspecialchars($url, ENT_QUOTES |
ENT_SUBSTITUTE, 'UTF-8') (and keep escaping the separator as before) when
building the returned string; leave the non-HTML branch unchanged so only the
HTML output is sanitized.
| public function __invoke(PlaceholderContext $ctx): string | ||
| { | ||
| $url = $this->config->getValue(ConfigOption::ForwardUrl); | ||
| if (empty($url)) { | ||
| return ''; | ||
| } | ||
| $sep = !str_contains($url, '?') ? '?' : '&'; | ||
| $uid = $ctx->forwardedBy() ? 'forwarded' : $ctx->getUser()->getUniqueId(); | ||
|
|
||
| if ($ctx->isHtml()) { | ||
| $label = $this->translator->trans('This link'); | ||
|
|
||
| return '<a href="' | ||
| . htmlspecialchars($url, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') | ||
| . htmlspecialchars($sep) | ||
| . 'uid=' | ||
| . $uid | ||
| . '&mid=' | ||
| . $ctx->messageId() | ||
| . '">' | ||
| . htmlspecialchars($label, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') | ||
| . '</a> '; | ||
| } | ||
|
|
||
| return sprintf('%s%suid=%s&mid=%d ', $url, $sep, $uid, $ctx->messageId()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find PlaceholderContext definition
find . -type f -name "*.php" | xargs grep -l "class PlaceholderContext"Repository: phpList/core
Length of output: 115
🏁 Script executed:
# Also search for the specific method signature
rg "messageId\s*\(" --type php -A 2 -B 2 | head -50Repository: phpList/core
Length of output: 3544
🏁 Script executed:
# Check if ForwardValueResolver has any domain purity violations
cat -n src/Domain/Configuration/Service/Placeholder/ForwardValueResolver.phpRepository: phpList/core
Length of output: 1993
🏁 Script executed:
# Verify sprintf behavior with null and %d format in PHP
php -r "echo sprintf('mid=%d', null) . PHP_EOL; echo 'Concatenation: ' . 'mid=' . null . PHP_EOL;"Repository: phpList/core
Length of output: 81
🏁 Script executed:
# Double-check if there are any other callers of messageId() that might have similar issues
rg "messageId\(\)" --type php -B 2 -A 2Repository: phpList/core
Length of output: 3544
Guard against null messageId to prevent malformed query parameters.
messageId() is nullable (?int). When null, sprintf with %d converts it to 0, and string concatenation produces an empty value, resulting in mid=0 (text) or &mid= (HTML)—both invalid. Early-return or omit the mid parameter when missing.
🛠️ Suggested fix
public function __invoke(PlaceholderContext $ctx): string
{
$url = $this->config->getValue(ConfigOption::ForwardUrl);
if (empty($url)) {
return '';
}
+ $mid = $ctx->messageId();
+ if ($mid === null) {
+ return '';
+ }
$sep = !str_contains($url, '?') ? '?' : '&';
$uid = $ctx->forwardedBy() ? 'forwarded' : $ctx->getUser()->getUniqueId();
@@
- . $ctx->messageId()
+ . $mid
@@
- return sprintf('%s%suid=%s&mid=%d ', $url, $sep, $uid, $ctx->messageId());
+ return sprintf('%s%suid=%s&mid=%d ', $url, $sep, $uid, $mid);
}🤖 Prompt for AI Agents
In `@src/Domain/Configuration/Service/Placeholder/ForwardValueResolver.php` around
lines 25 - 49, The __invoke method in ForwardValueResolver builds a query
parameter mid using $ctx->messageId() which is nullable; when null it becomes
mid=0 or an empty param — guard against null by checking $ctx->messageId()
(and/or $ctx->forwardedBy()) and omit the mid parameter entirely when it is
null: update the HTML branch (where the anchor is built) and the plaintext
return (sprintf call) in __invoke to only append "&mid=..." / "mid=..." when
$ctx->messageId() is not null, keeping existing htmlspecialchars usage and uid
logic (methods: __invoke, messageId(), isHtml(), forwardedBy(),
getUser()->getUniqueId()).
| // send a PDF file to users who want html and text to everyone else | ||
| if ($htmlPref) { | ||
| $sentAs = OutputFormat::Pdf; | ||
| $pdfBytes = $this->pdfGenerator->createPdfBytes($textMessage); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the EmailBuilder.php file around lines 238 and 265
cat -n src/Domain/Messaging/Service/Builder/EmailBuilder.php | sed -n '230,270p'Repository: phpList/core
Length of output: 1904
🏁 Script executed:
# Find the PdfGenerator class and check the createPdfBytes method
find . -type f -name "*.php" | xargs grep -l "createPdfBytes" | head -5Repository: phpList/core
Length of output: 263
🏁 Script executed:
# Search for the PdfGenerator interface/class definition
rg -n "class PdfGenerator|interface PdfGenerator" -A 5Repository: phpList/core
Length of output: 1007
🏁 Script executed:
cat -n src/Domain/Common/PdfGenerator.phpRepository: phpList/core
Length of output: 811
🏁 Script executed:
# Also check if there's any null checking or guards in EmailBuilder before line 238 and 265
cat -n src/Domain/Messaging/Service/Builder/EmailBuilder.php | sed -n '220,240p'Repository: phpList/core
Length of output: 959
Type safety issue: nullable $textMessage passed to non-nullable parameter.
$textMessage is declared as ?string but PdfGenerator::createPdfBytes() expects a non-nullable string. With strict_types=1 enabled, passing null will cause a TypeError at runtime. Add a null check before these calls, or ensure the parameter is guaranteed non-null.
Also applies to: 265-265
🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/Builder/EmailBuilder.php` at line 238,
$textMessage is nullable but is being passed to
PdfGenerator::createPdfBytes(string) which requires a non-null string; add a
null-check before calling createPdfBytes (and the similar call around the other
occurrence) in EmailBuilder so you only call PdfGenerator::createPdfBytes when
$textMessage is !== null — otherwise provide a safe fallback (e.g. skip PDF
generation, pass an empty string, or throw a clear InvalidArgumentException).
Update the logic around the occurrences of $textMessage (the call at the line
with $this->pdfGenerator->createPdfBytes($textMessage) and the similar call at
the other occurrence) to guard against null values and handle the null case
explicitly.
| } elseif (strpos($htmlContent, '</body>')) { | ||
| $htmlContent = str_replace('</body>', $phpListPowered.'</body>', $htmlContent); | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/Domain/Messaging/Service/Constructor/SystemMailContentBuilder.php | sed -n '50,65p'Repository: phpList/core
Length of output: 1191
Use strict strpos !== false for </body> detection.
If </body> is at position 0, strpos() returns 0 which is falsy in PHP, so the condition silently fails and the code falls through instead of replacing the tag.
🔧 Suggested fix
- } elseif (strpos($htmlContent, '</body>')) {
+ } elseif (strpos($htmlContent, '</body>') !== false) {📝 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.
| } elseif (strpos($htmlContent, '</body>')) { | |
| $htmlContent = str_replace('</body>', $phpListPowered.'</body>', $htmlContent); | |
| } else { | |
| } elseif (strpos($htmlContent, '</body>') !== false) { | |
| $htmlContent = str_replace('</body>', $phpListPowered.'</body>', $htmlContent); | |
| } else { |
🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/Constructor/SystemMailContentBuilder.php` around
lines 55 - 57, The strpos check for '</body>' in SystemMailContentBuilder.php is
using a truthy test that fails when the tag is at position 0; change the
condition in the elseif that currently calls strpos($htmlContent, '</body>') to
a strict comparison strpos($htmlContent, '</body>') !== false so the block
(which calls str_replace('</body>', $phpListPowered.'</body>', $htmlContent))
runs correctly whenever the tag exists; update that single conditional and leave
the replacement using $phpListPowered and $htmlContent untouched.
| $cacheKey = sprintf('messaging.message.base.%d.%d', $campaign->getId(), (int) $forwardContent); | ||
| $cached = $this->cache->get($cacheKey); | ||
| if ($cached !== null) { | ||
| return $cached; | ||
| } |
There was a problem hiding this comment.
Return type mismatch on cache hit causes a TypeError.
Line 49-53 returns the cached DTO where the method declares : bool, so the first cache hit will fatal. Return a boolean instead (or change the return type).
🛠️ Suggested fix (keep bool return)
- if ($cached !== null) {
- return $cached;
- }
+ if ($cached !== null) {
+ return true;
+ }🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/MessagePrecacheService.php` around lines 49 -
53, The cached value ($cached) retrieved via $this->cache->get($cacheKey) is
returned directly but the method signature declares a boolean return (: bool),
causing a TypeError on cache hit; change the cache-hit branch to return a
boolean (e.g., return (bool) $cached or return true/false based on the cached
content) instead of returning the DTO, and ensure any code that saves into the
cache for this key ($this->cache->set(...)) stores a value that can be
meaningfully cast to bool for the method's contract.
| // After two increments (3 -> 4 -> 5), update should persist '5' | ||
| $this->attrManager->expects(self::once()) | ||
| ->method('createOrUpdateByName') | ||
| ->with( | ||
| subscriber: self::identicalTo($subscriber), | ||
| attributeName: 'FriendsForwarded', | ||
| value: '5' | ||
| ); | ||
|
|
||
| $service->incrementFriendsCount($subscriber); | ||
| $service->incrementFriendsCount($subscriber); | ||
| $service->updateFriendsCount($subscriber); | ||
|
|
||
| // Second update attempt should be a no-op due to cache reset | ||
| $this->attrManager->expects(self::never())->method('createOrUpdateByName'); | ||
| $service->updateFriendsCount($subscriber); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the test file
find . -name "ForwardingStatsServiceTest.php" -type fRepository: phpList/core
Length of output: 124
🏁 Script executed:
# Also check what version of PHPUnit is being used
if [ -f composer.json ]; then
cat composer.json | grep -A 5 "phpunit"
fi
if [ -f composer.lock ]; then
grep -A 10 '"name": "phpunit/phpunit"' composer.lock | head -15
fiRepository: phpList/core
Length of output: 286
🌐 Web query:
PHPUnit mock multiple expectations same method behavior
💡 Result:
In PHPUnit you generally don’t set multiple expects() blocks for the same method on the same mock; instead configure one expectation and then vary behavior by call order or arguments.
1) Same method, different return values per call (call order)
$mock->expects($this->exactly(2))
->method('foo')
->willReturn('first', 'second');This returns different values on consecutive invocations and (with exactly(2)) also asserts the call count. [1]
2) Same method, different return values based on arguments
$mock->expects($this->any())
->method('foo')
->willReturnMap([
['a', 'b', 'ret1'],
['x', 'y', 'ret2'],
]);willReturnMap() selects the return value by matching the argument list. [1]
3) Same method, complex “multiple expectations” logic (mix order + args)
Use a callback (you can track invocation number and inspect args):
$i = 0;
$mock->method('foo')->willReturnCallback(function($arg) use (&$i) {
return (++$i === 1 && $arg === 'a') ? 'ret1' : 'ret2';
});This is the most flexible approach when neither pure “consecutive calls” nor pure “argument map” is enough. [1]
Sources: PHPUnit manual on willReturn() (consecutive values), willReturnMap(), and call-count expectations (exactly(), etc.). [1]
[1] (docs.phpunit.de)
Citations:
🏁 Script executed:
# Let's examine the test file
cat -n ./tests/Unit/Domain/Messaging/Service/ForwardingStatsServiceTest.php | head -100Repository: phpList/core
Length of output: 4710
🏁 Script executed:
# Let's see the specific lines 69-84 and more context around it
cat -n ./tests/Unit/Domain/Messaging/Service/ForwardingStatsServiceTest.php | sed -n '60,95p'Repository: phpList/core
Length of output: 1718
Remove the redundant expectation reconfiguration.
PHPUnit doesn't support multiple expects() calls on the same method reliably—the second expects(self::never()) replaces rather than extends the first expects(self::once()). Once line 80 satisfies the once() expectation, reconfiguring with never() is both unreliable and unnecessary.
If you want to verify the second updateFriendsCount() call is truly a no-op, use expects(self::atMostOnce()) instead of expects(self::once()) on the initial setup. Otherwise, removing the line 83 reconfiguration is fine—the first expectation already enforces the persistence behavior.
Suggested fix
- // Second update attempt should be a no-op due to cache reset
- $this->attrManager->expects(self::never())->method('createOrUpdateByName');
$service->updateFriendsCount($subscriber);
self::assertTrue(true);🤖 Prompt for AI Agents
In `@tests/Unit/Domain/Messaging/Service/ForwardingStatsServiceTest.php` around
lines 69 - 84, The test configures the attrManager mock twice for
createOrUpdateByName which is unreliable; remove the second expectation
(expects(self::never())) and instead assert behavior by either changing the
initial expectation to expects(self::atMostOnce()) or simply deleting the
reconfiguration so the first expects(self::once()) defined on
ForwardingStatsServiceTest remains authoritative; update the test around the
methods incrementFriendsCount and updateFriendsCount to rely on that single
expectation on attrManager->createOrUpdateByName.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/Unit/Domain/Messaging/Service/ForwardContentServiceTest.php`:
- Around line 59-65: The MessageForwardDto instantiation has the fromName and
fromEmail arguments swapped; the constructor is (array $emails, string $uid,
string $fromName, string $fromEmail, ?string $note). Update the test to pass the
display name as the third argument and the email as the fourth (i.e., swap
'[email protected]' and 'From' in the new MessageForwardDto call) so
MessageForwardDto receives correct fromName and fromEmail values.
🧹 Nitpick comments (3)
src/Domain/Messaging/Service/ForwardingGuard.php (1)
51-56: Consider extracting the status string to a constant.The
'sent'status appears both here and inUserMessageForwardRepository::getCountByUserSince. A shared constant (or enum) would prevent typos and make the status values discoverable.tests/Unit/Domain/Messaging/Service/ForwardingGuardTest.php (1)
7-7: Unused import.
DateTimeImmutableis imported but not used in any test.Proposed fix
-use DateTimeImmutable;tests/Unit/Domain/Messaging/Service/MessageForwardServiceTest.php (1)
7-7: Unused import.
DateTimeImmutableis imported but never used.Proposed fix
-use DateTimeImmutable;
| forwardDto: new MessageForwardDto( | ||
| [], | ||
| 'uuid', | ||
| '[email protected]', | ||
| 'From', | ||
| null | ||
| ) |
There was a problem hiding this comment.
Arguments are swapped in MessageForwardDto constructor.
The constructor signature is (array $emails, string $uid, string $fromName, string $fromEmail, ?string $note), but here you're passing '[email protected]' as fromName and 'From' as fromEmail. That's backwards.
While the test still exercises the exception path (cache miss), it's worth fixing to keep test data realistic.
Proposed fix
$service->getContents(
campaign: $campaign,
forwardingSubscriber: $subscriber,
friendEmail: '[email protected]',
forwardDto: new MessageForwardDto(
[],
'uuid',
- '[email protected]',
- 'From',
+ 'From',
+ '[email protected]',
null
)
);📝 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.
| forwardDto: new MessageForwardDto( | |
| [], | |
| 'uuid', | |
| '[email protected]', | |
| 'From', | |
| null | |
| ) | |
| forwardDto: new MessageForwardDto( | |
| [], | |
| 'uuid', | |
| 'From', | |
| '[email protected]', | |
| null | |
| ) |
🤖 Prompt for AI Agents
In `@tests/Unit/Domain/Messaging/Service/ForwardContentServiceTest.php` around
lines 59 - 65, The MessageForwardDto instantiation has the fromName and
fromEmail arguments swapped; the constructor is (array $emails, string $uid,
string $fromName, string $fromEmail, ?string $note). Update the test to pass the
display name as the third argument and the email as the fourth (i.e., swap
'[email protected]' and 'From' in the new MessageForwardDto call) so
MessageForwardDto receives correct fromName and fromEmail values.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Domain/Messaging/Service/Builder/ForwardEmailBuilder.php`:
- Around line 89-98: The subject is being normalized with stripslashes() after
validation, so move normalization before validation: create a normalized subject
(e.g., $normalizedSubject = stripslashes($data->subject)) and use that when
calling validateRecipientAndSubject(to: $friendEmail, subject:
$normalizedSubject) and for any subsequent logic (including building $subject
with $this->translator->trans('Fwd') . ': ' . $normalizedSubject); leave
passesBlacklistCheck and exception types (InvalidRecipientOrSubjectException,
EmailBlacklistedException) unchanged.
- Around line 130-137: The Received header value from
HttpReceivedStampBuilder::buildStamp() must be sanitized before calling
Email::getHeaders()->addTextHeader('Received', ...); guard against CR/LF and
other control characters to prevent header injection: validate or normalize the
string returned by HttpReceivedStampBuilder (e.g., strip \r and \n, remove
control chars, and collapse whitespace or replace with a safe token) and only
call addTextHeader when the cleaned value is non-empty; keep this logic inside
ForwardEmailBuilder::applyCampaignHeaders so addTextHeader never receives
unsanitized REMOTE_HOST-derived content.
🧹 Nitpick comments (5)
config/PHPMD/rules.xml (1)
10-10: Disabling BooleanArgumentFlag loosens clean code checks.Commenting out this rule means methods with boolean parameters won't be flagged anymore. Boolean flags often indicate a method is doing multiple things and could benefit from being split. If specific methods legitimately need boolean args, consider using
@SuppressWarningsannotations on those methods instead of globally disabling the rule.Any specific pain points that led to this? Would be good to know if this is temporary or intentional long-term.
src/Domain/Messaging/Service/ForwardContentService.php (2)
34-38: Magic number1in cache key needs clarification.The hardcoded
1in the cache key is unclear - is this a format ID, version, or template identifier? Would be good to extract this to a named constant or add a brief comment explaining its purpose.Also, a heads up:
CacheInterface::get()returnsmixed, so if something unexpected ends up in the cache, you'll get cryptic errors downstream when it's used as aMessagePrecacheDto. Consider adding type validation.💡 Optional: Add type safety and clarify magic number
+ private const BASE_MESSAGE_FORMAT_ID = 1; // TODO: document what this represents + /** `@return` array{Email, OutputFormat} * `@throws` MessageCacheMissingException */ public function getContents( Message $campaign, Subscriber $forwardingSubscriber, string $friendEmail, MessageForwardDto $forwardDto ): array { - $messagePrecacheDto = $this->cache->get(sprintf('messaging.message.base.%d.%d', $campaign->getId(), 1)); + $messagePrecacheDto = $this->cache->get( + sprintf('messaging.message.base.%d.%d', $campaign->getId(), self::BASE_MESSAGE_FORMAT_ID) + ); - if ($messagePrecacheDto === null) { + if (!$messagePrecacheDto instanceof MessagePrecacheDto) { throw new MessageCacheMissingException(); }
40-45: TODO tracked - link handling in forwarding context.Good call leaving this note. The behavior of tracked links when forwarded to someone outside the subscriber list could have interesting implications for analytics.
Want me to open an issue to track this, or do you have a plan for addressing it later?
src/Domain/Messaging/Model/Dto/ForwardingRecipientResult.php (1)
9-14: Consider makingreasonreadonly and passing it at construction time.The
reasonproperty is mutable whilestatusare readonly. InMessageForwardService, the pattern is to create the result, then mutatereason(lines 64, 85). This works, but constructing with all values upfront would be more consistent and enable full immutability.♻️ Optional refactor for immutability
public function __construct( public readonly string $email, public readonly string $status, - public ?string $reason = null, + public readonly ?string $reason = null, ) { }Then in
MessageForwardService, passreasondirectly:return new ForwardingRecipientResult( email: $friendEmail, status: 'failed', reason: 'precache_failed', );tests/Unit/Domain/Messaging/Service/MessageForwardServiceTest.php (1)
141-178: Consider adding a test forsend()throwing an exception.Current tests cover
getContents()throwing but notsend(). Ifsend()throws, the current implementation would break the batch loop (as flagged in the service review). Adding a test would document the expected behavior and catch regressions once the fix is applied.
| if (!$this->validateRecipientAndSubject(to: $friendEmail, subject: $data->subject)) { | ||
| throw new InvalidRecipientOrSubjectException(); | ||
| } | ||
|
|
||
| if (!$this->passesBlacklistCheck(to: $friendEmail, skipBlacklistCheck: false)) { | ||
| throw new EmailBlacklistedException(); | ||
| } | ||
|
|
||
| $subject = $this->translator->trans('Fwd') . ': ' . stripslashes($data->subject); | ||
|
|
There was a problem hiding this comment.
Normalize subject before validating.
stripslashes() happens after validation, so validation can drift from the actual subject. Consider normalizing first (or re‑validating after normalization).
💡 Suggested tweak
- if (!$this->validateRecipientAndSubject(to: $friendEmail, subject: $data->subject)) {
+ $rawSubject = stripslashes($data->subject);
+ if (!$this->validateRecipientAndSubject(to: $friendEmail, subject: $rawSubject)) {
throw new InvalidRecipientOrSubjectException();
}
- $subject = $this->translator->trans('Fwd') . ': ' . stripslashes($data->subject);
+ $subject = $this->translator->trans('Fwd') . ': ' . $rawSubject;📝 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.
| if (!$this->validateRecipientAndSubject(to: $friendEmail, subject: $data->subject)) { | |
| throw new InvalidRecipientOrSubjectException(); | |
| } | |
| if (!$this->passesBlacklistCheck(to: $friendEmail, skipBlacklistCheck: false)) { | |
| throw new EmailBlacklistedException(); | |
| } | |
| $subject = $this->translator->trans('Fwd') . ': ' . stripslashes($data->subject); | |
| $rawSubject = stripslashes($data->subject); | |
| if (!$this->validateRecipientAndSubject(to: $friendEmail, subject: $rawSubject)) { | |
| throw new InvalidRecipientOrSubjectException(); | |
| } | |
| if (!$this->passesBlacklistCheck(to: $friendEmail, skipBlacklistCheck: false)) { | |
| throw new EmailBlacklistedException(); | |
| } | |
| $subject = $this->translator->trans('Fwd') . ': ' . $rawSubject; |
🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/Builder/ForwardEmailBuilder.php` around lines 89
- 98, The subject is being normalized with stripslashes() after validation, so
move normalization before validation: create a normalized subject (e.g.,
$normalizedSubject = stripslashes($data->subject)) and use that when calling
validateRecipientAndSubject(to: $friendEmail, subject: $normalizedSubject) and
for any subsequent logic (including building $subject with
$this->translator->trans('Fwd') . ': ' . $normalizedSubject); leave
passesBlacklistCheck and exception types (InvalidRecipientOrSubjectException,
EmailBlacklistedException) unchanged.
| public function applyCampaignHeaders(Email $email, Subscriber $subscriber): Email | ||
| { | ||
| $email = parent::applyCampaignHeaders($email, $subscriber); | ||
|
|
||
| $receivedLine = $this->httpReceivedStampBuilder->buildStamp(); | ||
| if ($receivedLine !== null) { | ||
| $email->getHeaders()->addTextHeader('Received', $receivedLine); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find src -name "HttpReceivedStampBuilder.php" -type fRepository: phpList/core
Length of output: 121
🏁 Script executed:
cat -n src/Domain/Messaging/Service/Builder/HttpReceivedStampBuilder.phpRepository: phpList/core
Length of output: 2341
🏁 Script executed:
rg -n "buildStamp|getServerParams|_SERVER|Request|request" src/Domain/Messaging/Service/Builder/HttpReceivedStampBuilder.php -A 3Repository: phpList/core
Length of output: 1092
🏁 Script executed:
rg -n "addTextHeader|Headers|Symfony.*Mail" src/Domain/Messaging/Service/Builder/ForwardEmailBuilder.phpRepository: phpList/core
Length of output: 360
🏁 Script executed:
rg -n "use.*Email|use.*Headers" src/Domain/Messaging/Service/Builder/ForwardEmailBuilder.php -B 2Repository: phpList/core
Length of output: 388
🏁 Script executed:
grep -r "addTextHeader" --include="*.php" src/ | head -10Repository: phpList/core
Length of output: 1324
🌐 Web query:
Symfony Component Mime Email addTextHeader CR LF sanitization header injection
💡 Result:
Email::getHeaders()->addTextHeader($name, $value) creates an unstructured (text) header value (e.g., Subject, X-*). Per RFC 5322, an unstructured header field body must not contain raw CR or LF (except for folding which is CRLF + whitespace and must be generated by the library, not user input) and allowing raw CR/LF enables classic header injection (“header splitting”). [3]
What Symfony Mime does / what you should assume:
- Treat any user-controlled
$name/$valuepassed toaddTextHeader()as unsafe unless you first ensure it contains no\ror\n. - If you need long headers, pass a normal long string and let the component perform RFC-compliant encoding/folding; don’t insert CRLF yourself. (Symfony’s Mailer docs show
addTextHeader()for custom headers likeX-Auto-Response-Suppress.) [1]
Recommended sanitization policy (safe default):
- Reject (preferred) if
$valuecontains\ror\n(and also reject/normalize other ASCII control chars if the value is user-controlled). - If rejection is not possible, replace
[\r\n]+with a single space before callingaddTextHeader()(but rejection is safer because it avoids ambiguous behavior).
This matches the general security guidance for email header construction in PHP: if outside data is used to compose headers, it should be sanitized to prevent injecting unwanted headers. [2]
Sources
- Symfony Mailer docs – custom headers via
addTextHeader()[1] - PHP manual (
mail()): sanitize header data to avoid header injection [2] - RFC 5322 rule (quoted): header field bodies MUST NOT include CR/LF except folding [3]
🏁 Script executed:
rg -n "class Headers|function addTextHeader|sanitiz" src/ --type php -i | head -20Repository: phpList/core
Length of output: 512
Guard the Received stamp against header injection via REMOTE_HOST.
Symfony Mime's addTextHeader() does not automatically sanitize CR/LF characters. The HttpReceivedStampBuilder includes unsanitized REMOTE_HOST from $_SERVER in the returned stamp, which can enable header injection if a reverse proxy or upstream service allows control of this value. Sanitize the header value before passing it to addTextHeader().
Sanitization
if ($receivedLine !== null) {
+ $receivedLine = preg_replace('/[\r\n]+/', ' ', $receivedLine);
$email->getHeaders()->addTextHeader('Received', $receivedLine);
}📝 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.
| public function applyCampaignHeaders(Email $email, Subscriber $subscriber): Email | |
| { | |
| $email = parent::applyCampaignHeaders($email, $subscriber); | |
| $receivedLine = $this->httpReceivedStampBuilder->buildStamp(); | |
| if ($receivedLine !== null) { | |
| $email->getHeaders()->addTextHeader('Received', $receivedLine); | |
| } | |
| public function applyCampaignHeaders(Email $email, Subscriber $subscriber): Email | |
| { | |
| $email = parent::applyCampaignHeaders($email, $subscriber); | |
| $receivedLine = $this->httpReceivedStampBuilder->buildStamp(); | |
| if ($receivedLine !== null) { | |
| $receivedLine = preg_replace('/[\r\n]+/', ' ', $receivedLine); | |
| $email->getHeaders()->addTextHeader('Received', $receivedLine); | |
| } |
🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/Builder/ForwardEmailBuilder.php` around lines
130 - 137, The Received header value from HttpReceivedStampBuilder::buildStamp()
must be sanitized before calling Email::getHeaders()->addTextHeader('Received',
...); guard against CR/LF and other control characters to prevent header
injection: validate or normalize the string returned by HttpReceivedStampBuilder
(e.g., strip \r and \n, remove control chars, and collapse whitespace or replace
with a safe token) and only call addTextHeader when the cleaned value is
non-empty; keep this logic inside ForwardEmailBuilder::applyCampaignHeaders so
addTextHeader never receives unsanitized REMOTE_HOST-derived content.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Domain/Messaging/Model/Message/MessageFormat.php`:
- Around line 19-32: The PostgreSQL init migration creates the MessageFormat
boolean columns that are mapped in the MessageFormat class as integers ($asText,
$asHtml, $asPdf, $asTextAndHtml, $asTextAndPdf), causing Doctrine hydration
errors; update the Postgres migration to define those columns as INT NOT NULL
DEFAULT 0 (matching MySQL and the ORM mapping) and optionally add explicit ORM
column options in the MessageFormat entity (ORM\Column on properties $asText,
$asHtml, $asPdf, $asTextAndHtml, $asTextAndPdf) to ensure type and default are
clear to Doctrine.
🧹 Nitpick comments (2)
config/services/services.yml (2)
7-23: Redundantautowire/autoconfigurecan be removed from services covered by_defaults.With the new
_defaultsblock, services only need to declare properties that differ from the defaults. Most services here can drop explicitautowire: trueandautoconfigure: true, keeping only what's necessary (e.g.,public: trueor customarguments).This applies throughout the file - not urgent, but would reduce noise.
♻️ Example cleanup for a few services
PhpList\Core\Domain\Subscription\Service\SubscriberCsvExporter: - autowire: true - autoconfigure: true public: true PhpList\Core\Domain\Subscription\Service\SubscriberCsvImporter: - autowire: true - autoconfigure: true public: true PhpList\Core\Domain\Messaging\Service\EmailService: - autowire: true - autoconfigure: true + # Just remove these two lines entirely, or add 'public: true' if it needs to be public PhpList\Core\Domain\Messaging\Service\MessageForwardService: - autowire: true - autoconfigure: true + # Same here - can be reduced to just the service key with ~ if no special config
86-91: Hardcoded values won't act as conditional fallbacks.The comment says "Use literal defaults if parameters are not defined" but Symfony YAML doesn't conditionally use parameters - these hardcoded values (
0,204800,30) will always override any parameters you might define elsewhere with the same names.If you want these to be configurable via parameters with fallback defaults, define the parameters in your
parameters.ymlor similar with default values, then reference them here:♻️ Suggested approach for configurable defaults
PhpList\Core\Domain\Common\ExternalImageService: autowire: true autoconfigure: true arguments: $tempDir: '%kernel.cache_dir%' - # Use literal defaults if parameters are not defined in this environment - $externalImageMaxAge: 0 - $externalImageMaxSize: 204800 - $externalImageTimeout: 30 + $externalImageMaxAge: '%external_image.max_age%' + $externalImageMaxSize: '%external_image.max_size%' + $externalImageTimeout: '%external_image.timeout%'Then define the parameters with defaults in your parameters file:
parameters: external_image.max_age: 0 external_image.max_size: 204800 external_image.timeout: 30
| #[ORM\Column(name: 'astext', type: 'integer')] | ||
| private int $asText = 0; | ||
|
|
||
| #[ORM\Column(name: 'ashtml', type: 'boolean')] | ||
| private bool $asHtml = false; | ||
| #[ORM\Column(name: 'ashtml', type: 'integer')] | ||
| private int $asHtml = 0; | ||
|
|
||
| #[ORM\Column(name: 'aspdf', type: 'boolean')] | ||
| private bool $asPdf = false; | ||
| #[ORM\Column(name: 'aspdf', type: 'integer')] | ||
| private int $asPdf = 0; | ||
|
|
||
| #[ORM\Column(name: 'astextandhtml', type: 'boolean')] | ||
| private bool $asTextAndHtml = false; | ||
| #[ORM\Column(name: 'astextandhtml', type: 'integer')] | ||
| private int $asTextAndHtml = 0; | ||
|
|
||
| #[ORM\Column(name: 'astextandpdf', type: 'boolean')] | ||
| private bool $asTextAndPdf = false; | ||
|
|
||
| public const FORMAT_TEXT = 'text'; | ||
| public const FORMAT_HTML = 'html'; | ||
| public const FORMAT_PDF = 'pdf'; | ||
| #[ORM\Column(name: 'astextandpdf', type: 'integer')] | ||
| private int $asTextAndPdf = 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine its full content
cd /tmp && git -C /tmp/repo_verification_workspace ls-files | grep -i messageformat || echo "File list approach"Repository: phpList/core
Length of output: 177
🏁 Script executed:
# Find the file
find . -name "MessageFormat.php" -type f 2>/dev/null | head -20Repository: phpList/core
Length of output: 110
🏁 Script executed:
# Search for migration files related to MessageFormat
find . -path "*/migrations/*" -name "*.php" -type f | head -20Repository: phpList/core
Length of output: 38
🏁 Script executed:
# Search for schema changes or migration comments related to "astext" or message format
rg "astext|ashtml|aspdf" --type php -B 2 -A 2Repository: phpList/core
Length of output: 7780
🏁 Script executed:
# Check for any doctrine schema generation or migration handling
rg "MessageFormat|message.*format" --type php --type yaml -l | head -20Repository: phpList/core
Length of output: 1237
🏁 Script executed:
# Read the full MessageFormat.php file to see current state
cat -n ./src/Domain/Messaging/Model/Message/MessageFormat.phpRepository: phpList/core
Length of output: 3283
🏁 Script executed:
# Check the exact migration that handles the schema change
cat -n ./src/Migrations/Version20251028092902MySqlUpdate.php | sed -n '1,100p'Repository: phpList/core
Length of output: 11143
🏁 Script executed:
# Look for PostgreSQL migration handling
cat -n ./src/Migrations/Version20251031072945PostGreInit.php | sed -n '1,100p'Repository: phpList/core
Length of output: 11442
🏁 Script executed:
# Check if there are any other migrations after these that might affect the schema
find ./src/Migrations -name "*.php" -type f | sort | tail -10Repository: phpList/core
Length of output: 215
🏁 Script executed:
# Check if there's a PostgreSQL update migration after the init
find ./src/Migrations -name "*PostGre*" -o -name "*postgres*" | sortRepository: phpList/core
Length of output: 109
🏁 Script executed:
# Look at the full MySQL migration to see the complete picture
sed -n '94,94p' ./src/Migrations/Version20251028092902MySqlUpdate.phpRepository: phpList/core
Length of output: 702
🏁 Script executed:
# Search for any recent migrations that might update PostgreSQL schema for message format
rg "astext|ashtml|aspdf" ./src/Migrations -A 2 -B 2Repository: phpList/core
Length of output: 5304
🏁 Script executed:
# Check if Doctrine's schema sync would properly handle the change
# Look for configuration related to migration generation
find . -name "doctrine.yaml" -o -name "doctrine.yml" | xargs cat 2>/dev/null | head -30Repository: phpList/core
Length of output: 904
🏁 Script executed:
# Check if there are any other migration files for PostgreSQL after the init
find ./src/Migrations -type f -name "*.php" | xargs grep -l "PostgreSQL\|postgre" | sortRepository: phpList/core
Length of output: 163
🏁 Script executed:
# Look at the down() migration for MySQL to see what it was before
sed -n '1,200p' ./src/Migrations/Version20251028092902MySqlUpdate.php | grep -A 50 "public function down"Repository: phpList/core
Length of output: 3442
🏁 Script executed:
# Check the full line in PostgreSQL init that creates the message table
sed -n '115,115p' ./src/Migrations/Version20251031072945PostGreInit.phpRepository: phpList/core
Length of output: 150
🏁 Script executed:
# Verify the exact schema in the PostgreSQL migration for those fields
rg "astext.*BOOLEAN|ashtml.*BOOLEAN|aspdf.*BOOLEAN" ./src/Migrations/Version20251031072945PostGreInit.php -oRepository: phpList/core
Length of output: 473
🏁 Script executed:
# Check if there are any PostgreSQL migrations AFTER the init that might alter the schema
find ./src/Migrations -type f -name "*PostGre*.php" | sortRepository: phpList/core
Length of output: 109
🏁 Script executed:
# Look for any ALTER TABLE statements for message format fields in all migrations
rg "ALTER TABLE.*message.*astext|ALTER TABLE.*message.*ashtml" ./src/Migrations -B 2 -A 2Repository: phpList/core
Length of output: 2753
🏁 Script executed:
# Double-check the exact PostgreSQL create statement
sed -n '115,115p' ./src/Migrations/Version20251031072945PostGreInit.php | grep -o "astext[^,]*" | head -1Repository: phpList/core
Length of output: 38
🏁 Script executed:
# Let's verify what happens when the ORM tries to hydrate these fields from PostgreSQL
# Check if there's any type casting or handling in the MessageFormat class
cat -n ./src/Domain/Messaging/Model/Message/MessageFormat.php | sed -n '60,90p'Repository: phpList/core
Length of output: 816
🏁 Script executed:
# Check if there's any conversion logic in repositories or services
rg "getAsText|incrementAsText" --type php -B 3 -A 3 | head -50Repository: phpList/core
Length of output: 3317
🏁 Script executed:
# Verify the exact problem: PostgreSQL init has BOOLEAN, MySQL has INT DEFAULT 0
echo "=== PostgreSQL schema ===" && \
sed -n '115,115p' ./src/Migrations/Version20251031072945PostGreInit.php | grep -o "astext[^,]*\|ashtml[^,]*\|aspdf[^,]*" | head -5 && \
echo "=== MySQL schema ===" && \
grep "astext\|ashtml\|aspdf" ./src/Migrations/Version20251028092902MySqlUpdate.php | grep "CHANGE" | grep "down" -A 100 | head -1Repository: phpList/core
Length of output: 102
🏁 Script executed:
# Look at exact line from PostgreSQL migration to see the CREATE TABLE statement
sed -n '115,115p' ./src/Migrations/Version20251031072945PostGreInit.phpRepository: phpList/core
Length of output: 150
🏁 Script executed:
# Check if there are any versions after PostGreInit
ls -la ./src/Migrations/ | grep "Version202510"Repository: phpList/core
Length of output: 317
🏁 Script executed:
# Look for any migration that might handle PostgreSQL updates for message format
find ./src/Migrations -name "*.php" -type f -exec grep -l "astext.*INT\|ashtml.*INT" {} \;Repository: phpList/core
Length of output: 163
🏁 Script executed:
# Let me check the full down() method for MySQL to see what the before-state was
sed -n '110,200p' ./src/Migrations/Version20251028092902MySqlUpdate.php | grep -A 5 "astext"Repository: phpList/core
Length of output: 38
🏁 Script executed:
# Get the exact line number and content of the CREATE TABLE phplist_message in PostgreSQL migration
grep -n "CREATE TABLE phplist_message" ./src/Migrations/Version20251031072945PostGreInit.phpRepository: phpList/core
Length of output: 1676
🏁 Script executed:
# Extract and display the CREATE TABLE statement more cleanly
sed -n '115,115p' ./src/Migrations/Version20251031072945PostGreInit.php | sed 's/, /\n/g' | grep -i "astext\|ashtml\|aspdf\|astextandpdf\|astextandhtml"Repository: phpList/core
Length of output: 38
🏁 Script executed:
# Check what the old state was by looking at the down() method in MySQL migration
sed -n '1,400p' ./src/Migrations/Version20251028092902MySqlUpdate.php | tail -100 | head -50Repository: phpList/core
Length of output: 6066
🏁 Script executed:
# Confirm the exact line in MySQL migration where it converts the fields
sed -n '170,170p' ./src/Migrations/Version20251028092902MySqlUpdate.php | grep -o "astext[^,]*\|ashtml[^,]*\|aspdf[^,]*" | head -3Repository: phpList/core
Length of output: 38
🏁 Script executed:
# Check if the up() in MySQL migration properly converts these fields
sed -n '94,94p' ./src/Migrations/Version20251028092902MySqlUpdate.php | grep -o "astext[^,]*\|ashtml[^,]*" | head -2Repository: phpList/core
Length of output: 123
🏁 Script executed:
# Summary: check what type these were before the migration
cat ./src/Migrations/initial_schema.sql | grep -A 1 -B 1 "astext\|ashtml\|aspdf"Repository: phpList/core
Length of output: 275
🏁 Script executed:
# Final confirmation: Look at the down() for MySQL to see what state these fields should revert to
sed -n '166,166p' ./src/Migrations/Version20251028092902MySqlUpdate.phpRepository: phpList/core
Length of output: 102
🏁 Script executed:
# Let me get the full down() statement for the message table
sed -n '1,400p' ./src/Migrations/Version20251028092902MySqlUpdate.php | grep -A 1 "CHANGE astext"Repository: phpList/core
Length of output: 1557
Fix PostgreSQL migration schema for message format counters.
The MySQL migration correctly converts these fields to INT DEFAULT 0, but the PostgreSQL init migration creates them as BOOLEAN NOT NULL — this will cause Doctrine hydration failures since the ORM mapping expects type: 'integer'. Update the PostgreSQL init to create these fields as INT NOT NULL DEFAULT 0 to match MySQL and the original schema.
Additionally, consider adding explicit ORM options to the mapping:
🔧 Suggested ORM mapping updates
- #[ORM\Column(name: 'astext', type: 'integer')]
+ #[ORM\Column(name: 'astext', type: 'integer', options: ['default' => 0])]
private int $asText = 0;
- #[ORM\Column(name: 'ashtml', type: 'integer')]
+ #[ORM\Column(name: 'ashtml', type: 'integer', options: ['default' => 0])]
private int $asHtml = 0;
- #[ORM\Column(name: 'aspdf', type: 'integer')]
+ #[ORM\Column(name: 'aspdf', type: 'integer', options: ['default' => 0])]
private int $asPdf = 0;
- #[ORM\Column(name: 'astextandhtml', type: 'integer')]
+ #[ORM\Column(name: 'astextandhtml', type: 'integer', options: ['default' => 0])]
private int $asTextAndHtml = 0;
- #[ORM\Column(name: 'astextandpdf', type: 'integer')]
+ #[ORM\Column(name: 'astextandpdf', type: 'integer', options: ['default' => 0])]
private int $asTextAndPdf = 0;🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Model/Message/MessageFormat.php` around lines 19 - 32,
The PostgreSQL init migration creates the MessageFormat boolean columns that are
mapped in the MessageFormat class as integers ($asText, $asHtml, $asPdf,
$asTextAndHtml, $asTextAndPdf), causing Doctrine hydration errors; update the
Postgres migration to define those columns as INT NOT NULL DEFAULT 0 (matching
MySQL and the ORM mapping) and optionally add explicit ORM column options in the
MessageFormat entity (ORM\Column on properties $asText, $asHtml, $asPdf,
$asTextAndHtml, $asTextAndPdf) to ensure type and default are clear to Doctrine.
9f9c52e to
ef094ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@composer.json`:
- Line 161: composer.json is missing the Composer package for the registered
bundle FOS\RestBundle\FOSRestBundle; add the package
"friendsofsymfony/rest-bundle" to the require section with a Symfony
6.4‑compatible constraint (e.g. a 3.x/4.x range compatible with Symfony 6.4) so
the bundle registration resolves at bootstrap; update composer.json's require
entry accordingly and run composer update to install the package.
In `@src/DependencyInjection/PhpListCoreExtension.php`:
- Around line 18-23: The current hardcoded loop in PhpListCoreExtension.php only
loads three YAML files and skips the rest; replace that array with a glob scan:
use glob(__DIR__ . '/../../config/services/*.yml') to get all service YAML
files, iterate over each $path, check is_file($path) && is_readable($path), and
call $loader->load(basename($path)) so the loader receives the same filename
form as before; update the load logic inside the same method where the foreach
currently iterates over ['services.yml','builders.yml','managers.yml'].
| "PhpList\\Core\\EmptyStartPageBundle\\EmptyStartPageBundle", | ||
| "FOS\\RestBundle\\FOSRestBundle", | ||
| "TatevikGr\\RssFeedBundle\\RssFeedBundle" | ||
| "FOS\\RestBundle\\FOSRestBundle" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if FOSRestBundle or friendsofsymfony/rest-bundle is present in composer.json
echo "=== Searching composer.json ==="
cat composer.json | grep -i "friendsofsymfony\|fosrest"
echo ""
echo "=== Full composer.json (for context) ==="
cat composer.jsonRepository: phpList/core
Length of output: 5875
Add FOSRestBundle package to composer.json require section.
The bundle is registered in the Symfony configuration but the Composer package is missing. Add friendsofsymfony/rest-bundle with a version compatible for Symfony 6.4 to the require section, otherwise Symfony will fail during bootstrap.
🤖 Prompt for AI Agents
In `@composer.json` at line 161, composer.json is missing the Composer package for
the registered bundle FOS\RestBundle\FOSRestBundle; add the package
"friendsofsymfony/rest-bundle" to the require section with a Symfony
6.4‑compatible constraint (e.g. a 3.x/4.x range compatible with Symfony 6.4) so
the bundle registration resolves at bootstrap; update composer.json's require
entry accordingly and run composer update to install the package.
| // Load core service definitions if present (keep optional to avoid breaking consumers) | ||
| foreach (['services.yml', 'builders.yml', 'managers.yml'] as $file) { | ||
| $path = __DIR__ . '/../../config/services/' . $file; | ||
| if (is_file($path) && is_readable($path)) { | ||
| $loader->load($file); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# List all YAML files in config/services directory
find . -path "./config/services/*.yml" -type f 2>/dev/null | sortRepository: phpList/core
Length of output: 438
🏁 Script executed:
# Check for other YAML loaders in the DependencyInjection extension
rg -n "YamlFileLoader|load.*\.yml" src/DependencyInjection/ --type phpRepository: phpList/core
Length of output: 327
🏁 Script executed:
# Check if there are other service loaders or configuration elsewhere
rg -n "loadFromExtension|load.*services" src/ --type php -A 2Repository: phpList/core
Length of output: 411
Switch to glob-based loading—9 of 12 service YAML files are currently skipped.
The hardcoded list only loads services.yml, builders.yml, and managers.yml. But config/services/ contains 12 YAML files total, including commands.yml, parameters.yml, repositories.yml, resolvers.yml, validators.yml, and others that won't be loaded. This will cause missing service definitions.
Use glob-based loading to pick up all files:
Refactor to auto-load all YAML files
- foreach (['services.yml', 'builders.yml', 'managers.yml'] as $file) {
- $path = __DIR__ . '/../../config/services/' . $file;
- if (is_file($path) && is_readable($path)) {
- $loader->load($file);
- }
- }
+ $serviceDir = __DIR__ . '/../../config/services';
+ foreach (glob($serviceDir . '/*.yml') as $path) {
+ if (is_readable($path)) {
+ $loader->load(basename($path));
+ }
+ }📝 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.
| // Load core service definitions if present (keep optional to avoid breaking consumers) | |
| foreach (['services.yml', 'builders.yml', 'managers.yml'] as $file) { | |
| $path = __DIR__ . '/../../config/services/' . $file; | |
| if (is_file($path) && is_readable($path)) { | |
| $loader->load($file); | |
| } | |
| // Load core service definitions if present (keep optional to avoid breaking consumers) | |
| $serviceDir = __DIR__ . '/../../config/services'; | |
| foreach (glob($serviceDir . '/*.yml') as $path) { | |
| if (is_readable($path)) { | |
| $loader->load(basename($path)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/DependencyInjection/PhpListCoreExtension.php` around lines 18 - 23, The
current hardcoded loop in PhpListCoreExtension.php only loads three YAML files
and skips the rest; replace that array with a glob scan: use glob(__DIR__ .
'/../../config/services/*.yml') to get all service YAML files, iterate over each
$path, check is_file($path) && is_readable($path), and call
$loader->load(basename($path)) so the loader receives the same filename form as
before; update the load logic inside the same method where the foreach currently
iterates over ['services.yml','builders.yml','managers.yml'].
6f3d486 to
8697556
Compare
TatevikGr
force-pushed
the
ref/campaign-processing
branch
from
8697556 to
1dd0df0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/Composer/ModuleFinderTest.php (1)
33-42:⚠️ Potential issue | 🟠 MajorUpdate three tests to mock
findAll()instead offindModules().The implementation of
findBundleClasses()calls$this->packageRepository->findAll()(line 55 inModuleFinder.php), but the following tests stubfindModules():
testFindBundleClassesForNoModulesReturnsEmptyArray(line 33)testFindBundleClassesForModulesWithoutBundlesReturnsEmptyArray(line 80)testCreateBundleConfigurationYamlForNoModulesReturnsCommentOnly(line 144)Since the mocked method won't actually be invoked, these tests won't properly validate the behavior and could mask bugs.
🤖 Fix all issues with AI agents
In `@composer.json`:
- Around line 160-161: Update the incorrect bundle FQCN in composer.json:
replace the entry "PhpList\\Core\\EmptyStartPageBundle\\PhpListCoreBundle" with
the correct fully-qualified class name "PhpList\\Core\\PhpListCoreBundle" so
Symfony can locate the PhpListCoreBundle class (fix the wrong namespace
reference to PhpList\Core\EmptyStartPageBundle and use
PhpList\Core\PhpListCoreBundle instead).
In `@src/Domain/Subscription/Repository/UserBlacklistRepository.php`:
- Around line 46-59: The isEmailBlacklisted method accepts a nullable $immediate
which, if null, currently behaves like false and enables grace time; normalize
it at the top of the function by coercing $immediate to a boolean default (e.g.,
$immediate = $immediate ?? true) so callers that accidentally pass null get the
intended immediate behavior; update the isEmailBlacklisted method (use the
$immediate parameter and $grace calculation logic) to apply this normalization
before computing $grace and $cutoff.
🧹 Nitpick comments (1)
config/services/builders.yml (1)
14-37: Consider using YAML anchors to reduce duplication.All three email builders share identical constructor arguments. If these ever need updating, you'd have to change them in three places. YAML anchors can help keep things DRY.
♻️ Optional: Use YAML anchors to reduce repetition
+ .email_builder_defaults: &email_builder_args + $googleSenderId: '%messaging.google_sender_id%' + $useAmazonSes: '%messaging.use_amazon_ses%' + $usePrecedenceHeader: '%messaging.use_precedence_header%' + $devVersion: '%app.dev_version%' + $devEmail: '%app.dev_email%' + PhpList\Core\Domain\Messaging\Service\Builder\SystemEmailBuilder: - arguments: - $googleSenderId: '%messaging.google_sender_id%' - $useAmazonSes: '%messaging.use_amazon_ses%' - $usePrecedenceHeader: '%messaging.use_precedence_header%' - $devVersion: '%app.dev_version%' - $devEmail: '%app.dev_email%' + arguments: *email_builder_args PhpList\Core\Domain\Messaging\Service\Builder\EmailBuilder: - arguments: - $googleSenderId: '%messaging.google_sender_id%' - $useAmazonSes: '%messaging.use_amazon_ses%' - $usePrecedenceHeader: '%messaging.use_precedence_header%' - $devVersion: '%app.dev_version%' - $devEmail: '%app.dev_email%' + arguments: *email_builder_args PhpList\Core\Domain\Messaging\Service\Builder\ForwardEmailBuilder: - arguments: - $googleSenderId: '%messaging.google_sender_id%' - $useAmazonSes: '%messaging.use_amazon_ses%' - $usePrecedenceHeader: '%messaging.use_precedence_header%' - $devVersion: '%app.dev_version%' - $devEmail: '%app.dev_email%' + arguments: *email_builder_argsNote: The abstract service key starting with
.prevents Symfony from trying to instantiate it. Feel free to skip this if you anticipate these builders diverging in their config soon.
| "PhpList\\Core\\EmptyStartPageBundle\\EmptyStartPageBundle", | ||
| "FOS\\RestBundle\\FOSRestBundle", | ||
| "TatevikGr\\RssFeedBundle\\RssFeedBundle" | ||
| "PhpList\\Core\\EmptyStartPageBundle\\PhpListCoreBundle", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify bundle class definitions exist
rg -n --type=php 'class\s+EmptyStartPageBundle\b'
rg -n --type=php 'class\s+PhpListCoreBundle\b'Repository: phpList/core
Length of output: 217
🏁 Script executed:
#!/bin/bash
# Check namespace declarations in both files
echo "=== EmptyStartPageBundle namespace ==="
head -20 src/EmptyStartPageBundle/EmptyStartPageBundle.php
echo -e "\n=== PhpListCoreBundle namespace ==="
head -20 src/PhpListCoreBundle.phpRepository: phpList/core
Length of output: 577
Fix incorrect bundle namespace in composer.json line 161.
The PhpListCoreBundle class is registered with the wrong fully-qualified name. It's actually in the PhpList\Core namespace, not PhpList\Core\EmptyStartPageBundle.
Change line 161 from:
"PhpList\\Core\\EmptyStartPageBundle\\PhpListCoreBundle",
To:
"PhpList\\Core\\PhpListCoreBundle",
Incorrect bundle registration will cause Symfony to fail loading the bundle during kernel boot.
🤖 Prompt for AI Agents
In `@composer.json` around lines 160 - 161, Update the incorrect bundle FQCN in
composer.json: replace the entry
"PhpList\\Core\\EmptyStartPageBundle\\PhpListCoreBundle" with the correct
fully-qualified class name "PhpList\\Core\\PhpListCoreBundle" so Symfony can
locate the PhpListCoreBundle class (fix the wrong namespace reference to
PhpList\Core\EmptyStartPageBundle and use PhpList\Core\PhpListCoreBundle
instead).
| public function isEmailBlacklisted(string $email, ?bool $immediate = true): bool | ||
| { | ||
| // allow 5 minutes to send the last message acknowledging unsubscription | ||
| $grTime = $this->blacklistGraceTime; | ||
| $grace = $immediate ? 0 : (($grTime >= 1 && $grTime <= 15) ? $grTime : 5); | ||
| $cutoff = (new DateTimeImmutable())->modify('-' . $grace .' minutes'); | ||
|
|
||
| return $this->createQueryBuilder('ub') | ||
| ->where('ub.email = :email') | ||
| ->andWhere('ub.added < :cutoff') | ||
| ->setParameter('email', $email) | ||
| ->setParameter('cutoff', $cutoff) | ||
| ->getQuery() | ||
| ->getOneOrNullResult() !== null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all calls to isEmailBlacklisted
rg -n 'isEmailBlacklisted\s*\(' --type=php -A2 -B2Repository: phpList/core
Length of output: 3491
The nullable-flag ambiguity is a valid defense, though not currently exploited. All existing callers pass only the email and rely on the default $immediate = true. However, the signature still allows null to sneak in, which would unintentionally enable grace time. The suggested normalization is sound—add $immediate = $immediate ?? true; to guard against future mistakes.
🤖 Prompt for AI Agents
In `@src/Domain/Subscription/Repository/UserBlacklistRepository.php` around lines
46 - 59, The isEmailBlacklisted method accepts a nullable $immediate which, if
null, currently behaves like false and enables grace time; normalize it at the
top of the function by coercing $immediate to a boolean default (e.g.,
$immediate = $immediate ?? true) so callers that accidentally pass null get the
intended immediate behavior; update the isEmailBlacklisted method (use the
$immediate parameter and $grace calculation logic) to apply this normalization
before computing $grace and $cutoff.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Thanks for contributing to phpList!