Conversation
📝 WalkthroughWalkthroughRefactors messaging around a precache-driven pipeline, adds modular placeholder resolution, new email builders/constructors, forwarding orchestration (guard/delivery/stats), image/attachment utilities, mail-size enforcement, many repo/DTO additions, DI/config rebinding, and extensive unit 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🎯 5 (Critical) | ⏱️ ~120 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
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
'from@example.com' 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', | ||
| 'from@example.com', | ||
| '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 'from@example.com' 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: 'friend@example.com',
forwardDto: new MessageForwardDto(
[],
'uuid',
- 'from@example.com',
- 'From',
+ 'From',
+ 'from@example.com',
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', | |
| 'from@example.com', | |
| 'From', | |
| null | |
| ) | |
| forwardDto: new MessageForwardDto( | |
| [], | |
| 'uuid', | |
| 'From', | |
| 'from@example.com', | |
| 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
'from@example.com' 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.
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'].
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.
1dd0df0 to
0a9b9a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
55-59:⚠️ Potential issue | 🟡 MinorInconsistent
in_arraystrictness.Line 32 uses
in_array($key, $this->booleanValues, true)(strict), but line 57 usesin_array($key, $this->booleanValues)(non-strict). Should be consistent.🐛 Fix for consistency
- if (in_array($key, $this->booleanValues)) { + if (in_array($key, $this->booleanValues, true)) { throw new InvalidArgumentException('Key is a boolean value, use isEnabled instead'); }
🤖 Fix all issues with AI agents
In `@src/Domain/Common/RemotePageFetcher.php`:
- Around line 65-70: RemotePageFetcher currently performs persistence side
effects by calling $this->entityManager->remove() and
$this->urlCacheRepository->persist() inside src/Domain, which violates the rule
that Domain layer must not perform persistence. Modify RemotePageFetcher to stop
calling entityManager->remove and urlCacheRepository->persist directly: instead
have it create and return a domain value (e.g., an UrlCache instance via new
UrlCache()->setUrl(...)->setContent(...)->setLastModified(...)) and also return
or emit the list of existing UrlCache identifiers (from
$this->urlCacheRepository->getByUrl($url)) as a plain domain DTO/event; then
move the actual deletion and persistence logic into the
Application/Infrastructure layer (implementing repository persistence) where you
can call the repository implementation or entity manager to remove old caches
and persist the new UrlCache. Ensure method signatures (RemotePageFetcher) are
updated to return the UrlCache and items-to-delete or to dispatch a domain event
so infrastructure can handle persistence.
In `@src/Domain/Messaging/Service/Builder/BaseEmailBuilder.php`:
- Around line 87-99: resolveDestinationEmail() is declared to return string but
can return null when $to is null and $this->devVersion is false; update the
contract by either (A) make the parameter non-nullable (change ?string $to to
string $to) and update callers to always pass a string, or (B) keep ?string $to
but explicitly handle null by throwing a clear exception (e.g.,
InvalidArgumentException) when $to is null and $this->devVersion is false so the
method always returns a string or throws; adjust the signature and/or add the
null-check/throw in resolveDestinationEmail() (method name:
resolveDestinationEmail, class: BaseEmailBuilder, fields: $this->devVersion and
$this->devEmail, exception currently thrown: DevEmailNotConfiguredException) and
update any callers/tests to match the chosen approach.
- Around line 101-113: In createBaseEmail ensure that both $fromEmail and
$subject are non-null before calling new Address(...) and $email->subject(...):
add explicit guards at the start of createBaseEmail (after resolving
$destinationEmail) that check $fromEmail !== null and $subject !== null and
throw a clear InvalidArgumentException (or Domain-specific exception) if either
is null; reference resolveDestinationEmail, Address, Email::subject,
ConfigProvider::getValue and MessagePrecacheDto::fromEmail in the error messages
so failures point to missing config/DTO values, and keep existing
validateRecipientAndSubject usage for other checks.
In `@src/Domain/Messaging/Service/MessagePrecacheService.php`:
- Around line 90-94: The calls to TemplateImageManager::parseLogoPlaceholders on
$messagePrecacheDto->template and $messagePrecacheDto->htmlFooter (and any other
nullable DTO properties) are unsafe because parseLogoPlaceholders(string
$content) requires a non-null string; update the block in MessagePrecacheService
(where $messagePrecacheDto->content, ->template, ->htmlFooter are set) to check
nullability before calling parseLogoPlaceholders — e.g. use conditional checks
or null-coalescing so you only call parseLogoPlaceholders when the property is
non-null and otherwise leave it null; ensure applyRemoteContentIfPresent()’s
nullable assignments are respected.
In `@src/Migrations/Version20260204094237.php`:
- Line 34: The migration shrinks phplist_linktrack_forward.url and
phplist_urlcache.url to VARCHAR(255) without checking existing data length; add
a pre-flight validation in the migration (e.g., in the up() method of
Version20260204094237) that runs SELECT COUNT(*) WHERE char_length(url) > 255
for both tables (phplist_linktrack_forward and phplist_urlcache) and
aborts/throws a clear exception (or logs the offending rows) if any rows exceed
255 characters, so the ALTER TABLE statements ($this->addSql('ALTER TABLE
phplist_linktrack_forward ALTER url TYPE VARCHAR(255)') and the analogous change
for phplist_urlcache) only run when it is safe to do so.
🧹 Nitpick comments (4)
src/Domain/Common/FileHelper.php (1)
11-14: Implicit boolean coercion onfilesize()may be unclear.
filesize()returnsint|false, and the loose boolean check treats 0-byte files as invalid. If that's intentional, a comment or explicit comparison would clarify intent.♻️ Explicit comparison for clarity
public function isValidFile(string $path): bool { - return is_file($path) && filesize($path); + // Reject non-files and empty (0-byte) files + return is_file($path) && filesize($path) > 0; }src/Domain/Messaging/Service/MessagePrecacheService.php (2)
47-47: Consider adding type hint for$loadedMessageData.The parameter lacks a type declaration. Adding
array $loadedMessageDatawould improve type safety and IDE support.♻️ Add type hint
- public function precacheMessage(Message $campaign, $loadedMessageData, ?bool $forwardContent = false): bool + public function precacheMessage(Message $campaign, array $loadedMessageData, ?bool $forwardContent = false): bool
208-221: Potential null handling instr_ireplace.Line 217 calls
str_ireplaceon$messagePrecacheDto->textContentwhich may be null. PHP coerces null to empty string, but explicit null-coalescing would be cleaner.♻️ Explicit null handling
- $messagePrecacheDto->textContent = str_ireplace($searchKey, $replace, $messagePrecacheDto->textContent); + $messagePrecacheDto->textContent = str_ireplace( + $searchKey, + $replace, + $messagePrecacheDto->textContent ?? '' + );tests/Unit/Domain/Identity/Service/AdminCopyEmailSenderTest.php (1)
116-122: Deprecated PHPUnit methods in use.
withConsecutive()andwillReturnOnConsecutiveCalls()were deprecated in PHPUnit 9.6 and removed in PHPUnit 10. Consider refactoring to use a callback-based approach or separate expectations.♻️ Refactor suggestion
- $configProvider->expects(self::exactly(2)) - ->method('getValue') - ->withConsecutive([ConfigOption::AdminAddress], [ConfigOption::AdminAddresses]) - ->willReturnOnConsecutiveCalls( - 'single@example.com', - ' admin1@example.com, , admin2@example.com ,admin1@example.com ' - ); + $valueMap = [ + [ConfigOption::AdminAddress, 'single@example.com'], + [ConfigOption::AdminAddresses, ' admin1@example.com, , admin2@example.com ,admin1@example.com '], + ]; + $configProvider->expects(self::exactly(2)) + ->method('getValue') + ->willReturnCallback(function (ConfigOption $key) use (&$valueMap) { + foreach ($valueMap as $entry) { + if ($entry[0] === $key) { + return $entry[1]; + } + } + return null; + });
| $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 layer persistence violation.
Lines 67 and 70 call $this->entityManager->remove() and $this->urlCacheRepository->persist() directly in src/Domain/**. Per coding guidelines, persistence side effects should be in Infrastructure/Application layer, not Domain.
As per coding guidelines: Domain services should not perform persistence operations - $entityManager->remove() and persistence calls are prohibited in src/Domain/**.
🤖 Prompt for AI Agents
In `@src/Domain/Common/RemotePageFetcher.php` around lines 65 - 70,
RemotePageFetcher currently performs persistence side effects by calling
$this->entityManager->remove() and $this->urlCacheRepository->persist() inside
src/Domain, which violates the rule that Domain layer must not perform
persistence. Modify RemotePageFetcher to stop calling entityManager->remove and
urlCacheRepository->persist directly: instead have it create and return a domain
value (e.g., an UrlCache instance via new
UrlCache()->setUrl(...)->setContent(...)->setLastModified(...)) and also return
or emit the list of existing UrlCache identifiers (from
$this->urlCacheRepository->getByUrl($url)) as a plain domain DTO/event; then
move the actual deletion and persistence logic into the
Application/Infrastructure layer (implementing repository persistence) where you
can call the repository implementation or entity manager to remove old caches
and persist the new UrlCache. Ensure method signatures (RemotePageFetcher) are
updated to return the UrlCache and items-to-delete or to dispatch a domain event
so infrastructure can handle persistence.
| protected function resolveDestinationEmail(?string $to): string | ||
| { | ||
| $destinationEmail = $to; | ||
|
|
||
| if ($this->devVersion) { | ||
| if (!$this->devEmail) { | ||
| throw new DevEmailNotConfiguredException(); | ||
| } | ||
| $destinationEmail = $this->devEmail; | ||
| } | ||
|
|
||
| return $destinationEmail; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all call sites of resolveDestinationEmail
rg -n -C3 --type=php 'resolveDestinationEmail\s*\('Repository: phpList/core
Length of output: 1298
🏁 Script executed:
#!/bin/bash
# Get context around the method definition
cd src/Domain/Messaging/Service/Builder
wc -l BaseEmailBuilder.phpRepository: phpList/core
Length of output: 80
🏁 Script executed:
#!/bin/bash
# Read the entire BaseEmailBuilder.php to understand the class context
cat -n src/Domain/Messaging/Service/Builder/BaseEmailBuilder.php | head -150Repository: phpList/core
Length of output: 6394
Fix the type contract: resolveDestinationEmail() cannot safely return null.
The method claims to return string but when $to is null and devVersion is false, it returns null anyway—this violates the return type hint and will throw a TypeError under strict types (which your code declares). Either handle null explicitly or remove the ? from the parameter.
Suggested fix
- protected function resolveDestinationEmail(?string $to): string
+ protected function resolveDestinationEmail(string $to): string
{
$destinationEmail = $to;🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/Builder/BaseEmailBuilder.php` around lines 87 -
99, resolveDestinationEmail() is declared to return string but can return null
when $to is null and $this->devVersion is false; update the contract by either
(A) make the parameter non-nullable (change ?string $to to string $to) and
update callers to always pass a string, or (B) keep ?string $to but explicitly
handle null by throwing a clear exception (e.g., InvalidArgumentException) when
$to is null and $this->devVersion is false so the method always returns a string
or throws; adjust the signature and/or add the null-check/throw in
resolveDestinationEmail() (method name: resolveDestinationEmail, class:
BaseEmailBuilder, fields: $this->devVersion and $this->devEmail, exception
currently thrown: DevEmailNotConfiguredException) and update any callers/tests
to match the chosen approach.
| protected function createBaseEmail( | ||
| string $to, | ||
| ?string $fromEmail, | ||
| ?string $fromName, | ||
| ?string $subject, | ||
| ) : Email { | ||
| $email = (new Email()); | ||
| $destinationEmail = $this->resolveDestinationEmail($to); | ||
|
|
||
| $email->to($destinationEmail); | ||
| $email->from(new Address($fromEmail, $fromName ?? '')); | ||
| $email->subject($subject); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the BaseEmailBuilder file to understand the context better
cat -n src/Domain/Messaging/Service/Builder/BaseEmailBuilder.php | sed -n '90,130p'Repository: phpList/core
Length of output: 1601
🏁 Script executed:
#!/bin/bash
# Find all call sites of createBaseEmail to see how it's invoked
rg -n 'createBaseEmail\s*\(' --type=php -A2Repository: phpList/core
Length of output: 1503
🏁 Script executed:
#!/bin/bash
# Also check the full class structure to understand if there are parent classes or traits
rg -n -A50 'class BaseEmailBuilder' --type=php | head -60Repository: phpList/core
Length of output: 5270
🏁 Script executed:
#!/bin/bash
# Check if Symfony's Address class is imported and look at how it's used
rg -n 'new Address' --type=php -B2 -A2Repository: phpList/core
Length of output: 10991
🏁 Script executed:
#!/bin/bash
# Check SystemEmailBuilder call site 1
cat -n src/Domain/Messaging/Service/Builder/SystemEmailBuilder.php | sed -n '65,80p'Repository: phpList/core
Length of output: 787
🏁 Script executed:
#!/bin/bash
# Check SystemEmailBuilder call site 2
cat -n src/Domain/Messaging/Service/Builder/SystemEmailBuilder.php | sed -n '110,125p'Repository: phpList/core
Length of output: 885
🏁 Script executed:
#!/bin/bash
# Check ForwardEmailBuilder call site
cat -n src/Domain/Messaging/Service/Builder/ForwardEmailBuilder.php | sed -n '100,115p'Repository: phpList/core
Length of output: 658
🏁 Script executed:
#!/bin/bash
# Check EmailBuilder call site
cat -n src/Domain/Messaging/Service/Builder/EmailBuilder.php | sed -n '85,100p'Repository: phpList/core
Length of output: 754
🏁 Script executed:
#!/bin/bash
# Also check what happens to fromEmail & subject before these calls
cat -n src/Domain/Messaging/Service/Builder/SystemEmailBuilder.php | sed -n '50,75p'Repository: phpList/core
Length of output: 1181
🏁 Script executed:
#!/bin/bash
# Check ForwardEmailBuilder more context to see if there's validation
cat -n src/Domain/Messaging/Service/Builder/ForwardEmailBuilder.php | sed -n '50,115p'Repository: phpList/core
Length of output: 2835
🏁 Script executed:
#!/bin/bash
# Check EmailBuilder method signature and validation
cat -n src/Domain/Messaging/Service/Builder/EmailBuilder.php | sed -n '50,100p'Repository: phpList/core
Length of output: 2200
🏁 Script executed:
#!/bin/bash
# Check if ConfigProvider::getValue can return null
rg -n 'getValue.*MessageFromAddress' --type=php -B2 -A2Repository: phpList/core
Length of output: 1699
🏁 Script executed:
#!/bin/bash
# Check Symfony Mime Address constructor behavior with null
rg -n 'class Address' --type=php | head -5Repository: phpList/core
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Look for any tests that might show expected behavior
fd -e php test | xargs grep -l 'fromEmail.*null\|subject.*null' | head -5Repository: phpList/core
Length of output: 278
🏁 Script executed:
#!/bin/bash
# Check the full ConfigProvider to understand getValue return type
rg -n 'function getValue|public function getValue' --type=php -A3 | head -20Repository: phpList/core
Length of output: 1498
🏁 Script executed:
#!/bin/bash
# Check MessagePrecacheDto to see if fromEmail/fromName/subject are nullable
fd -name '*MessagePrecacheDto*' --type=phpRepository: phpList/core
Length of output: 286
🏁 Script executed:
#!/bin/bash
# Examine the DTO structure
cat -n src/Domain/Messaging/Service/MessagePrecacheDto.php 2>/dev/null | head -60Repository: phpList/core
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for createBaseEmail with null values
rg -n 'createBaseEmail' --type=php tests/ -B2 -A5Repository: phpList/core
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for MessagePrecacheDto
find . -name '*.php' -type f | xargs grep -l 'class MessagePrecacheDto'Repository: phpList/core
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Look at the actual file
rg -n 'class MessagePrecacheDto' --type=php -A30Repository: phpList/core
Length of output: 2245
🏁 Script executed:
#!/bin/bash
# Make absolutely sure we got all createBaseEmail call sites
rg 'createBaseEmail' --type=php -B5 | grep -E '(fromEmail|subject|createBaseEmail)'Repository: phpList/core
Length of output: 1330
🏁 Script executed:
#!/bin/bash
# Check if ConfigProvider has any guarantees about MessageFromAddress never being null
rg -n 'MessageFromAddress' --type=php | head -20Repository: phpList/core
Length of output: 1166
🏁 Script executed:
#!/bin/bash
# Verify Symfony Mime Address behavior - search for its usage with null handling
rg -n 'new Address\(' --type=php | grep -E '(if|null|??|?:)' | head -10Repository: phpList/core
Length of output: 1537
🏁 Script executed:
#!/bin/bash
# Check the exact return type and implementation of ConfigProvider::getValue
cat -n src/Domain/Configuration/Service/Provider/ConfigProvider.php | sed -n '55,65p'Repository: phpList/core
Length of output: 629
🏁 Script executed:
#!/bin/bash
# Look for any tests that show Address constructor behavior with null
rg -n 'Address.*null' --type=php -B2 -A2Repository: phpList/core
Length of output: 484
🏁 Script executed:
#!/bin/bash
# Check if there's any actual runtime issue or error handling around this
rg -n 'TypeError|InvalidArgumentException' --type=php src/Domain/Messaging/Service/Builder/ -B3 -A3Repository: phpList/core
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Get the full picture - let's see what the actual Symfony Email and Address classes expect
# Search for any error handling or try-catch around Address instantiation
rg -n 'try|catch|throw' src/Domain/Messaging/Service/Builder/ --type=php -B2 -A2 | head -30Repository: phpList/core
Length of output: 2374
🏁 Script executed:
#!/bin/bash
# Check all three concrete call sites more thoroughly to see validation flow
echo "=== SystemEmailBuilder buildCampaignEmail ===" && \
cat -n src/Domain/Messaging/Service/Builder/SystemEmailBuilder.php | sed -n '53,76p'
echo "" && \
echo "=== EmailBuilder buildCampaignEmail ===" && \
cat -n src/Domain/Messaging/Service/Builder/EmailBuilder.php | sed -n '69,94p'
echo "" && \
echo "=== ForwardEmailBuilder buildForwardEmail ===" && \
cat -n src/Domain/Messaging/Service/Builder/ForwardEmailBuilder.php | sed -n '79,111p'Repository: phpList/core
Length of output: 3721
Guard nullable fromEmail and subject before passing to Symfony Mime APIs.
Address constructor and Email::subject() require non-null strings; ConfigProvider::getValue() can return null, and MessagePrecacheDto::fromEmail is nullable. While validateRecipientAndSubject() validates some fields, it doesn't check fromEmail. Passing null will cause a TypeError at runtime.
Add validation in createBaseEmail() to enforce these as required:
Suggested guard
protected function createBaseEmail(
string $to,
?string $fromEmail,
?string $fromName,
?string $subject,
) : Email {
+ if ($fromEmail === null || trim($fromEmail) === '') {
+ throw new InvalidArgumentException('From email is required.');
+ }
+ if ($subject === null || trim($subject) === '') {
+ throw new InvalidArgumentException('Subject is required.');
+ }
$email = (new Email());
$destinationEmail = $this->resolveDestinationEmail($to);
$email->to($destinationEmail);
$email->from(new Address($fromEmail, $fromName ?? ''));
$email->subject($subject);📝 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.
| protected function createBaseEmail( | |
| string $to, | |
| ?string $fromEmail, | |
| ?string $fromName, | |
| ?string $subject, | |
| ) : Email { | |
| $email = (new Email()); | |
| $destinationEmail = $this->resolveDestinationEmail($to); | |
| $email->to($destinationEmail); | |
| $email->from(new Address($fromEmail, $fromName ?? '')); | |
| $email->subject($subject); | |
| protected function createBaseEmail( | |
| string $to, | |
| ?string $fromEmail, | |
| ?string $fromName, | |
| ?string $subject, | |
| ) : Email { | |
| if ($fromEmail === null || trim($fromEmail) === '') { | |
| throw new InvalidArgumentException('From email is required.'); | |
| } | |
| if ($subject === null || trim($subject) === '') { | |
| throw new InvalidArgumentException('Subject is required.'); | |
| } | |
| $email = (new Email()); | |
| $destinationEmail = $this->resolveDestinationEmail($to); | |
| $email->to($destinationEmail); | |
| $email->from(new Address($fromEmail, $fromName ?? '')); | |
| $email->subject($subject); |
🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/Builder/BaseEmailBuilder.php` around lines 101 -
113, In createBaseEmail ensure that both $fromEmail and $subject are non-null
before calling new Address(...) and $email->subject(...): add explicit guards at
the start of createBaseEmail (after resolving $destinationEmail) that check
$fromEmail !== null and $subject !== null and throw a clear
InvalidArgumentException (or Domain-specific exception) if either is null;
reference resolveDestinationEmail, Address, Email::subject,
ConfigProvider::getValue and MessagePrecacheDto::fromEmail in the error messages
so failures point to missing config/DTO values, and keep existing
validateRecipientAndSubject usage for other checks.
| $messagePrecacheDto->content = $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->content); | ||
| $messagePrecacheDto->template = $this->templateImageManager | ||
| ->parseLogoPlaceholders($messagePrecacheDto->template); | ||
| $messagePrecacheDto->htmlFooter = $this->templateImageManager | ||
| ->parseLogoPlaceholders($messagePrecacheDto->htmlFooter); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine TemplateImageManager class
find . -type f -name "*.php" | xargs grep -l "class TemplateImageManager"Repository: phpList/core
Length of output: 194
🏁 Script executed:
# Look for parseLogoPlaceholders method
rg "function parseLogoPlaceholders" -A 15Repository: phpList/core
Length of output: 1677
🏁 Script executed:
# Check the MessagePrecacheService.php file around lines 90-94 and 201
fd "MessagePrecacheService.php" -x cat -n {} | head -210 | tail -125Repository: phpList/core
Length of output: 6665
🏁 Script executed:
# Get the full method that contains lines 90-94 to see call ordering
fd "MessagePrecacheService.php" -x cat -n {} | grep -E "^\s+(40|41|42|43|44|45|46|47|48|49|50|51|52|53|54|55|56|57|58|59|60|61|62|63|64|65|66|67|68|69|70|71|72|73|74|75|76|77|78|79|80|81|82|83|84|85)" | head -50Repository: phpList/core
Length of output: 2613
🏁 Script executed:
# Look at the method signature and structure - get lines 40-75
fd "MessagePrecacheService.php" -x sed -n '40,99p' {} -nRepository: phpList/core
Length of output: 2847
🏁 Script executed:
# Search for MessagePrecacheDto class to understand default values
find . -type f -name "MessagePrecacheDto.php" -o -name "MessagePrecacheDTO.php" | head -1 | xargs cat -n | head -80Repository: phpList/core
Length of output: 1221
Add null-safety handling before parseLogoPlaceholders calls for nullable properties.
$messagePrecacheDto->template is nullable and can be null (set at line 201 in applyRemoteContentIfPresent()). Since TemplateImageManager::parseLogoPlaceholders(string $content) expects a non-null string, passing null will raise a TypeError under strict types.
Use null-coalescing or conditional checks:
$messagePrecacheDto->template = $messagePrecacheDto->template
? $this->templateImageManager->parseLogoPlaceholders($messagePrecacheDto->template)
: null;🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/MessagePrecacheService.php` around lines 90 -
94, The calls to TemplateImageManager::parseLogoPlaceholders on
$messagePrecacheDto->template and $messagePrecacheDto->htmlFooter (and any other
nullable DTO properties) are unsafe because parseLogoPlaceholders(string
$content) requires a non-null string; update the block in MessagePrecacheService
(where $messagePrecacheDto->content, ->template, ->htmlFooter are set) to check
nullability before calling parseLogoPlaceholders — e.g. use conditional checks
or null-coalescing so you only call parseLogoPlaceholders when the property is
non-null and otherwise leave it null; ensure applyRemoteContentIfPresent()’s
nullable assignments are respected.
| get_class($platform) | ||
| )); | ||
|
|
||
| $this->addSql('ALTER TABLE phplist_linktrack_forward ALTER url TYPE VARCHAR(255)'); |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
cat -n src/Migrations/Version20260204094237.phpRepository: phpList/core
Repository: phpList/core
Exit code: 0
stdout:
1 <?php
2
3 declare(strict_types=1);
4
5 namespace PhpList\Core\Migrations;
6
7 use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
8 use Doctrine\DBAL\Platforms\MySQLPlatform;
9 use Doctrine\Migrations\AbstractMigration;
10 use Doctrine\DBAL\Schema\Schema;
11
12 /**
13 * ⚠️ Wizard warning:
14 * Doctrine will `helpfully` remove url(255) prefixes and add collations 5.7 can’t read.
15 * Review the SQL unless you enjoy debugging key length errors at 2 AM.
16 *
17 * Ex: phplist_linktrack_forward phplist_linktrack_forward_urlindex (but there are more)
18 */
19 final class Version20260204094237 extends AbstractMigration
20 {
21 public function getDescription(): string
22 {
23 return '';
24 }
25
26 public function up(Schema $schema): void
27 {
28 $platform = $this->connection->getDatabasePlatform();
29 $this->skipIf(!$platform instanceof PostgreSQLPlatform, sprintf(
30 'Unsupported platform for this migration: %s',
31 get_class($platform)
32 ));
33
34 $this->addSql('ALTER TABLE phplist_linktrack_forward ALTER url TYPE VARCHAR(255)');
35
36 $this->addSql('ALTER TABLE phplist_message ALTER astext TYPE INT USING astext::integer');
37 $this->addSql('ALTER TABLE phplist_message ALTER ashtml TYPE INT USING ashtml::integer');
38 $this->addSql('ALTER TABLE phplist_message ALTER aspdf TYPE INT USING aspdf::integer');
39 $this->addSql('ALTER TABLE phplist_message ALTER astextandhtml TYPE INT USING astextandhtml::integer');
40 $this->addSql('ALTER TABLE phplist_message ALTER astextandpdf TYPE INT USING astextandpdf::integer');
41
42 $this->addSql('ALTER TABLE phplist_urlcache ALTER url TYPE VARCHAR(255)');
43 }
44
45 public function down(Schema $schema): void
46 {
47 $platform = $this->connection->getDatabasePlatform();
48 $this->skipIf(!$platform instanceof PostgreSQLPlatform, sprintf(
49 'Unsupported platform for this migration: %s',
50 get_class($platform)
51 ));
52
53 $this->addSql('ALTER TABLE phplist_linktrack_forward ALTER url TYPE VARCHAR(2083)');
54
55 $this->addSql('ALTER TABLE phplist_message ALTER astext TYPE BOOLEAN USING (astext::integer <> 0)');
56 $this->addSql('ALTER TABLE phplist_message ALTER ashtml TYPE BOOLEAN USING (ashtml::integer <> 0)');
57 $this->addSql('ALTER TABLE phplist_message ALTER aspdf TYPE BOOLEAN USING (aspdf::integer <> 0)');
58 $this->addSql('ALTER TABLE phplist_message ALTER astextandhtml TYPE BOOLEAN USING (astextandhtml::integer <> 0)');
59 $this->addSql('ALTER TABLE phplist_message ALTER astextandpdf TYPE BOOLEAN USING (astextandpdf::integer <> 0)');
60
61 $this->addSql('ALTER TABLE phplist_urlcache ALTER url TYPE VARCHAR(2083)');
62 }
63 }
Script executed:
fd -t f "schema\|fixture\|database" . -x file {} \; 2>/dev/null | head -20Repository: phpList/core
Repository: phpList/core
Exit code: 0
Script executed:
# Search for existing column type definitions
rg "phplist_linktrack_forward|phplist_urlcache" --type yaml --type json --type php -A 3 | head -40Repository: phpList/core
Repository: phpList/core
Exit code: 0
stdout:
src/Migrations/Version20251028092902MySqlUpdate.php: $this->addSql('ALTER TABLE phplist_linktrack_forward RENAME INDEX urlindex TO phplist_linktrack_forward_urlindex;');
src/Migrations/Version20251028092902MySqlUpdate.php: $this->addSql('ALTER TABLE phplist_linktrack_forward CHANGE urlhash urlhash VARCHAR(32) DEFAULT NULL');
src/Migrations/Version20251028092902MySqlUpdate.php: $this->addSql('ALTER TABLE phplist_linktrack_forward RENAME INDEX uuididx TO phplist_linktrack_forward_uuididx');
src/Migrations/Version20251028092902MySqlUpdate.php: $this->addSql('ALTER TABLE phplist_linktrack_forward RENAME INDEX urlunique TO phplist_linktrack_forward_urlunique');
src/Migrations/Version20251028092902MySqlUpdate.php- $this->addSql('ALTER TABLE phplist_linktrack_ml RENAME INDEX fwdindex TO phplist_linktrack_ml_fwdindex');
src/Migrations/Version20251028092902MySqlUpdate.php- $this->addSql('ALTER TABLE phplist_linktrack_ml RENAME INDEX midindex TO phplist_linktrack_ml_midindex');
src/Migrations/Version20251028092902MySqlUpdate.php- $this->addSql('ALTER TABLE phplist_linktrack_uml_click RENAME INDEX midindex TO phplist_linktrack_uml_click_midindex');
--
src/Migrations/Version20251028092902MySqlUpdate.php: $this->addSql('ALTER TABLE phplist_urlcache RENAME INDEX urlindex TO phplist_urlcache_urlindex');
src/Migrations/Version20251028092902MySqlUpdate.php- $this->addSql('ALTER TABLE phplist_user_attribute RENAME INDEX idnameindex TO phplist_user_attribute_idnameindex');
src/Migrations/Version20251028092902MySqlUpdate.php- $this->addSql('ALTER TABLE phplist_user_attribute RENAME INDEX nameindex TO phplist_user_attribute_nameindex');
src/Migrations/Version20251028092902MySqlUpdate.php- $this->addSql('DROP INDEX email ON phplist_user_blacklist');
--
src/Migrations/Version20251028092902MySqlUpdate.php: $this->addSql('ALTER TABLE phplist_linktrack_forward DROP INDEX phplist_linktrack_forward_urlindex, ADD INDEX urlindex (url(255))');
src/Migrations/Version20251028092902MySqlUpdate.php: $this->addSql('ALTER TABLE phplist_linktrack_forward CHANGE urlhash urlhash CHAR(32) DEFAULT NULL');
src/Migrations/Version20251028092902MySqlUpdate.php: $this->addSql('ALTER TABLE phplist_linktrack_forward RENAME INDEX phplist_linktrack_forward_urlunique TO urlunique');
src/Migrations/Version20251028092902MySqlUpdate.php: $this->addSql('ALTER TABLE phplist_linktrack_forward RENAME INDEX phplist_linktrack_forward_uuididx TO uuididx');
src/Migrations/Version20251028092902MySqlUpdate.php- $this->addSql('ALTER TABLE phplist_linktrack_ml RENAME INDEX phplist_linktrack_ml_midindex TO midindex');
src/Migrations/Version20251028092902MySqlUpdate.php- $this->addSql('ALTER TABLE phplist_linktrack_ml RENAME INDEX phplist_linktrack_ml_fwdindex TO fwdindex');
src/Migrations/Version20251028092902MySqlUpdate.php- $this->addSql('ALTER TABLE phplist_linktrack_uml_click RENAME INDEX phplist_linktrack_uml_click_miduidfwdid TO miduidfwdid');
--
src/Migrations/Version20251028092902MySqlUpdate.php: $this->addSql('ALTER TABLE phplist_urlcache DROP INDEX phplist_urlcache_urlindex, ADD INDEX urlindex (url(255))');
src/Migrations/Version20251028092902MySqlUpdate.php- $this->addSql('ALTER TABLE phplist_user_attribute RENAME INDEX phplist_user_attribute_nameindex TO nameindex');
src/Migrations/Version20251028092902MySqlUpdate.php- $this->addSql('ALTER TABLE phplist_user_attribute RENAME INDEX phplist_user_attribute_idnameindex TO idnameindex');
src/Migrations/Version20251028092902MySqlUpdate.php- $this->addSql('ALTER TABLE phplist_user_blacklist DROP INDEX `primary`, ADD UNIQUE INDEX email (email)');
--
src/Migrations/Version20260204094237.php:* Ex: phplist_linktrack_forward phplist_linktrack_forward_urlindex (but there are more)
src/Migrations/Version20260204094237.php-*/
src/Migrations/Version20260204094237.php-final class Version20260204094237 extends AbstractMigration
src/Migrations/Version20260204094237.php-{
--
src/Migrations/Version20260204094237.php: $this->addSql('ALTER TABLE phplist_linktrack_forward ALTER url TYPE VARCHAR(255)');
src/Migrations/Version20260204094237.php-
src/Migrations/Version20260204094237.php- $this->addSql('ALTER TABLE phplist_message ALTER astext TYPE INT USING astext::integer');
src/Migrations/Version20260204094237.php- $this->addSql('ALTER TABLE phplist_message ALTER ashtml TYPE INT USING ashtml::integer');
--
src/Migrations/Version20260204094237.php: $this->addSql('ALTER TABLE phplist_urlcache ALTER url TYPE VARCHAR(255)');
src/Migrations/Version20260204094237.php- }
src/Migrations/Version20260204094237.php-
src/Migrations/Version20260204094237.php- public function down(Schema $schema): void
URL shrinking to VARCHAR(255) lacks pre-flight check and will fail if existing data exceeds limit.
Lines 34 and 42 reduce phplist_linktrack_forward.url and phplist_urlcache.url from VARCHAR(2083) → VARCHAR(255). If any row exceeds 255 characters, PostgreSQL aborts the migration. Consider adding a pre-check to catch oversized data upfront before attempting the ALTER.
Example: Validate before shrinking
public function up(Schema $schema): void
{
$platform = $this->connection->getDatabasePlatform();
$this->skipIf(!$platform instanceof PostgreSQLPlatform, sprintf(
'Unsupported platform for this migration: %s',
get_class($platform)
));
+ $maxForward = (int) $this->connection->fetchOne(
+ 'SELECT MAX(LENGTH(url)) FROM phplist_linktrack_forward'
+ );
+ if ($maxForward > 255) {
+ throw new \RuntimeException('phplist_linktrack_forward.url has entries > 255 chars');
+ }
+
+ $maxCache = (int) $this->connection->fetchOne(
+ 'SELECT MAX(LENGTH(url)) FROM phplist_urlcache'
+ );
+ if ($maxCache > 255) {
+ throw new \RuntimeException('phplist_urlcache.url has entries > 255 chars');
+ }
+
$this->addSql('ALTER TABLE phplist_linktrack_forward ALTER url TYPE VARCHAR(255)');Also applies to: 42-42
🤖 Prompt for AI Agents
In `@src/Migrations/Version20260204094237.php` at line 34, The migration shrinks
phplist_linktrack_forward.url and phplist_urlcache.url to VARCHAR(255) without
checking existing data length; add a pre-flight validation in the migration
(e.g., in the up() method of Version20260204094237) that runs SELECT COUNT(*)
WHERE char_length(url) > 255 for both tables (phplist_linktrack_forward and
phplist_urlcache) and aborts/throws a clear exception (or logs the offending
rows) if any rows exceed 255 characters, so the ALTER TABLE statements
($this->addSql('ALTER TABLE phplist_linktrack_forward ALTER url TYPE
VARCHAR(255)') and the analogous change for phplist_urlcache) only run when it
is safe to do so.
9a23394 to
289099f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/Domain/Messaging/Service/MessageDataLoader.php`:
- Around line 115-137: In mergeStoredMessageData, guard all stripslashes calls
against non-strings to avoid TypeError: when walking the unserialized payload in
array_walk_recursive (inside the if branch for 'SER:') only call stripslashes if
is_string($val) (leave other scalars as-is), and in the else branch where you
set $data = stripslashes($storedMessageDatum->getData()) change to conditionally
stripslashes only when is_string($storedMessageDatum->getData()); likewise guard
the stripslashes on storedMessageDatum->getName() before using it as the array
key so non-string names are not passed to stripslashes; keep existing logic and
constants (self::AS_FORMAT_FIELDS, mergeStoredMessageData, $storedMessageDatum)
otherwise.
- Around line 171-210: normaliseFromField assigns $defaultFrom into
$messageData['fromemail'] and then calls str_replace on that value, which can be
null and will cause a TypeError under strict_types; ensure $defaultFrom is
coerced to a safe string early (e.g. $defaultFrom ?? '') before assigning into
$messageData['fromemail'] (or apply coalescing when setting
$messageData['fromemail']) so subsequent calls like str_replace and trim operate
on strings; update the assignment sites in normaliseFromField (where
$messageData['fromemail'] = $defaultFrom) to use a non-null fallback.
In `@src/Domain/Messaging/Service/MessagePrecacheService.php`:
- Around line 80-85: The regex uses $this->uploadImageDir directly which can
contain regex metacharacters and break preg_replace; update the code in
MessagePrecacheService (where $messagePrecacheDto->content is set by
preg_replace) to wrap $this->uploadImageDir with
preg_quote($this->uploadImageDir, '/') to escape all regex-significant chars
(use '/' as the preg delimiter), then build the pattern using that quoted value
instead of str_replace; keep the rest of the pattern and replacement unchanged
so preg_replace correctly matches and rewrites img src paths.
🧹 Nitpick comments (1)
src/Domain/Messaging/Service/MessageDataLoader.php (1)
14-14: TODO should be resolved or tracked.Chill reminder: this TODO can easily get forgotten. Either tighten types now or spin up a tracking issue so it doesn’t ship half-baked.
| private function mergeStoredMessageData(array &$messageData, Message $message): void | ||
| { | ||
| $storedMessageData = $this->messageDataRepository->getForMessage($message->getId()); | ||
| foreach ($storedMessageData as $storedMessageDatum) { | ||
| if (str_starts_with($storedMessageDatum->getData(), 'SER:')) { | ||
| $unserialized = unserialize(substr($storedMessageDatum->getData(), 4), ['allowed_classes' => false]); | ||
| if (!is_array($unserialized)) { | ||
| $this->logger->warning('Invalid serialized data for message ID: ' . $message->getId()); | ||
| continue; | ||
| } | ||
| array_walk_recursive($unserialized, function (&$val) { | ||
| $val = stripslashes($val); | ||
| }); | ||
|
|
||
| $data = $unserialized; | ||
| } else { | ||
| $data = stripslashes($storedMessageDatum->getData()); | ||
| } | ||
| if (!in_array($storedMessageDatum->getName(), self::AS_FORMAT_FIELDS)) { | ||
| //# don't overwrite counters in the message table from the data table | ||
| $messageData[stripslashes($storedMessageDatum->getName())] = $data; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHP 8 stripslashes TypeError int bool non-string parameter
💡 Result:
In PHP 8+, stripslashes() is strictly typed as:
stripslashes(string $string): string(php.net)
So if you pass an int, bool, null, etc., PHP throws a TypeError (older PHP often just emitted warnings or coerced types). PHP 8 specifically tightened internal function parameter checks and many now throw TypeError/ValueError instead of warnings. (php.watch)
Fix patterns
1) Only call it for strings (recommended)
$value = is_string($value) ? stripslashes($value) : $value;2) If you expect arrays (e.g., $_POST)
function stripslashes_deep($v) {
return is_array($v) ? array_map('stripslashes_deep', $v)
: (is_string($v) ? stripslashes($v) : $v);
}
$_POST = stripslashes_deep($_POST);(Manual notes stripslashes() isn’t recursive; you must handle arrays yourself.) (php.net)
3) If you truly want coercion
$value = stripslashes((string)$value);Be careful: true becomes "1" and false becomes "" after casting.
Also sanity-check why you’re using it: magic_quotes_gpc is long gone, so many legacy stripslashes($_POST[...]) calls are unnecessary now (and often the root of these upgrade errors). (php.net)
Citations:
- 1: https://www.php.net/manual/en/function.stripslashes.php?utm_source=openai
- 2: https://php.watch/versions/8.0/internal-function-exceptions?utm_source=openai
- 3: https://www.php.net/manual/en/function.stripslashes.php?utm_source=openai
- 4: https://www.php.net/manual/en/function.stripslashes.php?utm_source=openai
Guard stripslashes against non-strings in serialized payloads.
In PHP 8+, stripslashes(string $string): string is strictly typed and throws TypeError if passed an int, bool, or null. Since array_walk_recursive will hit all scalar values in the deserialized payload, this needs a type guard.
✅ Suggested fix
- array_walk_recursive($unserialized, function (&$val) {
- $val = stripslashes($val);
- });
+ array_walk_recursive($unserialized, function (&$val) {
+ if (is_string($val)) {
+ $val = stripslashes($val);
+ }
+ });🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/MessageDataLoader.php` around lines 115 - 137,
In mergeStoredMessageData, guard all stripslashes calls against non-strings to
avoid TypeError: when walking the unserialized payload in array_walk_recursive
(inside the if branch for 'SER:') only call stripslashes if is_string($val)
(leave other scalars as-is), and in the else branch where you set $data =
stripslashes($storedMessageDatum->getData()) change to conditionally
stripslashes only when is_string($storedMessageDatum->getData()); likewise guard
the stripslashes on storedMessageDatum->getName() before using it as the array
key so non-string names are not passed to stripslashes; keep existing logic and
constants (self::AS_FORMAT_FIELDS, mergeStoredMessageData, $storedMessageDatum)
otherwise.
| private function normaliseFromField(array &$messageData, ?string $defaultFrom): void | ||
| { | ||
| //## parse the from field into it's components - email and name | ||
| if (preg_match('/([^ ]+@[^ ]+)/', $messageData['fromfield'], $regs)) { | ||
| // if there is an email in the from, rewrite it as "name <email>" | ||
| $messageData['fromname'] = str_replace($regs[0], '', $messageData['fromfield']); | ||
| $messageData['fromemail'] = $regs[0]; | ||
| // if the email has < and > take them out here | ||
| $messageData['fromemail'] = str_replace('<', '', $messageData['fromemail']); | ||
| $messageData['fromemail'] = str_replace('>', '', $messageData['fromemail']); | ||
| // make sure there are no quotes around the name | ||
| $messageData['fromname'] = str_replace('"', '', ltrim(rtrim($messageData['fromname']))); | ||
| } elseif (str_contains($messageData['fromfield'], ' ')) { | ||
| // if there is a space, we need to add the email | ||
| $messageData['fromname'] = $messageData['fromfield']; | ||
| $messageData['fromemail'] = $defaultFrom; | ||
| } else { | ||
| $messageData['fromemail'] = $defaultFrom; | ||
| $messageData['fromname'] = $messageData['fromfield']; | ||
| } | ||
| // disallow an email address in the name | ||
| if (preg_match('/([^ ]+@[^ ]+)/', $messageData['fromname'], $regs)) { | ||
| $messageData['fromname'] = str_replace($regs[0], '', $messageData['fromname']); | ||
| } | ||
| // clean up | ||
| $messageData['fromemail'] = str_replace(',', '', $messageData['fromemail']); | ||
| $messageData['fromname'] = str_replace(',', '', $messageData['fromname']); | ||
|
|
||
| $messageData['fromname'] = trim($messageData['fromname']); | ||
|
|
||
| // erase double spacing | ||
| while (str_contains($messageData['fromname'], ' ')) { | ||
| $messageData['fromname'] = str_replace(' ', ' ', $messageData['fromname']); | ||
| } | ||
|
|
||
| //# if the name ends up being empty, copy the email | ||
| if (empty($messageData['fromname'])) { | ||
| $messageData['fromname'] = $messageData['fromemail']; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHP 8.x str_replace null parameter TypeError behavior
💡 Result:
In PHP 8.x, what happens when you pass null to str_replace() depends on PHP version and whether you’re in coercive mode (default) or strict_types.
str_replace() signature (PHP 8.x)
str_replace(array|string $search, array|string $replace, string|array $subject, int &$count = null): string|array [1]
Behavior when an argument is null
-
PHP 8.0 (default / coercive mode, no
strict_types=1)
Passingnullto a non-nullable scalar/union parameter of an internal function was historically silently coerced (e.g.,null→"",0, etc.). This includes cases likestr_replace(..., null)for$subject(it will be coerced and the call proceeds). This was done for backward compatibility and consistency wasn’t enforced yet for internal functions. [3][2] -
PHP 8.1+ (default / coercive mode)
Passingnullto a non-nullable internal-function parameter is deprecated (but still coerced and executed). So you’ll typically see:
Deprecated: str_replace(): Passing null to parameter[Meta] Build core component classes #3($subject) ... is deprecated[2][3] -
Any PHP 8.x with
declare(strict_types=1);
In strict mode,nullis not coerced to satisfy scalar types—expect aTypeErrorwhennullis passed where it isn’t allowed. The PHP 8.1 change mainly affects the default coercive mode. [3][4]
What to do
If your code might pass null, normalize before calling:
$subject = $subject ?? '';
$result = str_replace($search, $replace, $subject);This avoids the PHP 8.1+ deprecation and will also keep you safe if this becomes a TypeError in a future major version (as planned by the RFC). [3][4]
Sources: PHP manual for str_replace() [1], PHP 8.1 migration deprecation note [2], RFC describing the change and its intent [3], overview/explanation (PHP.Watch) [4].
🏁 Script executed:
# Find the MessageDataLoader.php file and check the full context
find . -type f -name "MessageDataLoader.php" | head -5Repository: phpList/core
Length of output: 108
🏁 Script executed:
# Check the MessageDataLoader.php file to see the full context
cat -n ./src/Domain/Messaging/Service/MessageDataLoader.php | head -100Repository: phpList/core
Length of output: 4673
🏁 Script executed:
# Check for declare(strict_types
grep -n "strict_types\|public function.*normaliseFromField\|private function.*normaliseFromField" ./src/Domain/Messaging/Service/MessageDataLoader.php | head -20Repository: phpList/core
Length of output: 175
🏁 Script executed:
# Look for where normaliseFromField is called and how $defaultFrom is passed
grep -B5 -A5 "normaliseFromField" ./src/Domain/Messaging/Service/MessageDataLoader.php | head -40Repository: phpList/core
Length of output: 1133
Handle null $defaultFrom before string ops.
The file uses declare(strict_types=1), so PHP 8.x will throw a TypeError if null is passed to str_replace(). Since both config values can be null, $defaultFrom may be null—it gets assigned to $messageData['fromemail'] at lines 186 or 190, then passed to str_replace() at line 196. Add the coalescing operator early to prevent the error.
✅ Suggested fix
private function normaliseFromField(array &$messageData, ?string $defaultFrom): void
{
+ $defaultFrom ??= '';
//## parse the from field into it's components - email and name🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/MessageDataLoader.php` around lines 171 - 210,
normaliseFromField assigns $defaultFrom into $messageData['fromemail'] and then
calls str_replace on that value, which can be null and will cause a TypeError
under strict_types; ensure $defaultFrom is coerced to a safe string early (e.g.
$defaultFrom ?? '') before assigning into $messageData['fromemail'] (or apply
coalescing when setting $messageData['fromemail']) so subsequent calls like
str_replace and trim operate on strings; update the assignment sites in
normaliseFromField (where $messageData['fromemail'] = $defaultFrom) to use a
non-null fallback.
| if ($this->uploadImageDir) { | ||
| //# escape subdirectories, otherwise this renders empty | ||
| $dir = str_replace('/', '\/', $this->uploadImageDir); | ||
| $messagePrecacheDto->content = preg_replace( | ||
| '/<img(.*)src="\/' . $dir . '(.*)>/iU', | ||
| '<img\\1src="' . $this->publicSchema . '://' . $baseurl . '/' . $this->uploadImageDir . '\\2>', |
There was a problem hiding this comment.
Escape uploadImageDir for regex safety.
If uploadImageDir contains regex metacharacters (e.g., . or +), the current pattern can mis-match and skip replacements. Use preg_quote instead of manual / escaping.
💡 Suggested fix
- $dir = str_replace('/', '\/', $this->uploadImageDir);
+ $dir = preg_quote($this->uploadImageDir, '/');🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/MessagePrecacheService.php` around lines 80 -
85, The regex uses $this->uploadImageDir directly which can contain regex
metacharacters and break preg_replace; update the code in MessagePrecacheService
(where $messagePrecacheDto->content is set by preg_replace) to wrap
$this->uploadImageDir with preg_quote($this->uploadImageDir, '/') to escape all
regex-significant chars (use '/' as the preg delimiter), then build the pattern
using that quoted value instead of str_replace; keep the rest of the pattern and
replacement unchanged so preg_replace correctly matches and rewrites img src
paths.
289099f to
ef65472
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Migrations/Version20260204094237.php`:
- Around line 12-18: The PHPDoc block in the migration class
Version20260204094237.php is stale and references URL/index collisions related
to phplist_linktrack_forward, but the migration only modifies phplist_message
columns (astext, ashtml, etc.); update or remove that warning so it matches the
actual changes—either delete the old URL/key-length warning entirely or replace
it with a short, accurate note describing that this migration alters
phplist_message text/html columns and to watch for collation/length issues when
altering string/blob columns.
🧹 Nitpick comments (4)
src/Migrations/Version20260204094237.php (2)
7-8: Unused import:MySQLPlatform
MySQLPlatformis imported but never referenced—the migration only guards against PostgreSQL. Feel free to drop it to keep things tidy.use Doctrine\DBAL\Platforms\PostgreSQLPlatform; -use Doctrine\DBAL\Platforms\MySQLPlatform; use Doctrine\Migrations\AbstractMigration;
21-24: Consider adding a meaningful migration description.Returning an empty string from
getDescription()is fine, but a brief note like "Convert phplist_message format columns from BOOLEAN to INT for PostgreSQL" would make migration history easier to scan.public function getDescription(): string { - return ''; + return 'Convert phplist_message format columns (astext, ashtml, aspdf, astextandhtml, astextandpdf) from BOOLEAN to INT'; }src/Domain/Messaging/Service/MessagePrecacheService.php (1)
21-41: That's quite the constructor.The
@SuppressWarnings("ExcessiveParameterList")annotation acknowledges the large parameter count. While this works, you might consider grouping related dependencies into a config object or splitting responsibilities if this service grows further.src/Domain/Messaging/Service/MessageDataLoader.php (1)
14-15: TODO comment indicates incomplete type coverage.The
// todo: check typescomment suggests the type system isn't fully verified yet. Consider addressing this before merge or creating a follow-up issue.Would you like me to open an issue to track the type verification task?
| /** | ||
| * ⚠️ Wizard warning: | ||
| * Doctrine will `helpfully` remove url(255) prefixes and add collations 5.7 can’t read. | ||
| * Review the SQL unless you enjoy debugging key length errors at 2 AM. | ||
| * | ||
| * Ex: phplist_linktrack_forward phplist_linktrack_forward_urlindex (but there are more) | ||
| */ |
There was a problem hiding this comment.
PHPDoc comment is stale and references removed code.
The warning talks about phplist_linktrack_forward, URL columns, and key length errors—but the migration now only touches phplist_message columns (astext, ashtml, etc.). Looks like the URL-related changes were removed but the comment stayed behind. Worth updating or removing to avoid confusion for the next person who reads this at 2 AM.
,
🤖 Prompt for AI Agents
In `@src/Migrations/Version20260204094237.php` around lines 12 - 18, The PHPDoc
block in the migration class Version20260204094237.php is stale and references
URL/index collisions related to phplist_linktrack_forward, but the migration
only modifies phplist_message columns (astext, ashtml, etc.); update or remove
that warning so it matches the actual changes—either delete the old
URL/key-length warning entirely or replace it with a short, accurate note
describing that this migration alters phplist_message text/html columns and to
watch for collation/length issues when altering string/blob columns.
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Thanks for contributing to phpList!