Skip to content

Mail attachment reset patch#115

Open
APaikens wants to merge 5 commits intodotkernel:5.2from
APaikens:MailAttachmentReset-patch
Open

Mail attachment reset patch#115
APaikens wants to merge 5 commits intodotkernel:5.2from
APaikens:MailAttachmentReset-patch

Conversation

@APaikens
Copy link
Copy Markdown
Contributor

@APaikens APaikens commented May 7, 2026

There was incomplete processing of mail attachments, when getting errors from SMTP server - like timeout or limit reached.
If emails was sent in cycle, using same mail service, then even after resetting attachment array, next email would contain attachment from previous - because attachments were already incorporated into email body.
Problem appeared only, when there was exceptions during send(), otherwise all works.

Solution is to reset email body - when there are problems with send() as well.

APaikens added 5 commits May 7, 2026 08:26
Signed-off-by: APaikens <shamanis@gmail.com>
Fix comment

Signed-off-by: APaikens <shamanis@gmail.com>
Set mail body to null to discard previous attachments

Signed-off-by: APaikens <shamanis@gmail.com>
Try to fix php-stan error for php 8.2

Signed-off-by: APaikens <shamanis@gmail.com>
Revert previous changes

Signed-off-by: APaikens <shamanis@gmail.com>
@APaikens
Copy link
Copy Markdown
Contributor Author

APaikens commented May 7, 2026

Not sure how to address failing php-stan error, will look into it later.

@alexmerlin
Copy link
Copy Markdown
Member

Not sure how to address failing php-stan error, will look into it later.

If you don't find a better solution:

In src/Service/MailService.php you can call setBody() with new TextPart('') but then you need to adjust your tests to check against an empty string instead of null - at least this way the original body is no longer accessible.

@APaikens
Copy link
Copy Markdown
Contributor Author

APaikens commented May 7, 2026

Checked once more - php stan error is there on branch 5.2 without my changes.
Checked locally, it doesn't have anything with $this->getMessage()->setBody(null);

If I understand correctly, php-stan don't like this line return $part ?? new TextPart($this->text, $this->textCharset);
(src/Email.php, line 300)

And $part at that point is null if all possible email parts ($this->text, $relatedParts and $otherParts) is null.
If all three parts is null, then code tries to recreate new TextPart - but using $this->text, which at that point is null as well.

I have a proposal to throw logical exception - it removes PhpStan error
throw new LogicException('A message must have a text or an HTML part or attachments.');

But yes, it seems like BC break.
Other option would be to ignore this php-stan error - for this PR.

How can we proceed?

@alexmerlin
Copy link
Copy Markdown
Member

Checked once more - php stan error is there on branch 5.2 without my changes. Checked locally, it doesn't have anything with $this->getMessage()->setBody(null);
...
How can we proceed?

You're right, the error is already there even without your latest modifications.
Symfony must have tweaked something recently in mailer/mime because the last time we touched dot-mail everything was green.


The issue is in src/Email.php on line 300 where if $part is null, then we try to create an instance of Symfony\Component\Mime\Part\TextPart (because the generateBody() method must return an instance of Symfony\Component\Mime\Part\AbstractPart).

When we call new TextPart($this->text, $this->textCharset) the problem appears because $this->text is null by default, but the constructor of TextPart does not accept its first parameter to be null.

Solutions:

1. Casting

The quickest and least intrusive solution would be to cast $this->text to a string - since we're initializing a TextPart object, this makes the most sense to me:

return $part ?? new TextPart((string) $this->text, $this->textCharset);

2. Throwing an exception

I have a proposal to throw logical exception - it removes PhpStan error
throw new LogicException('A message must have a text or an HTML part or attachments.');

This would also work, not sure under which circumstances should we throw this.
@SergiuBota1 what do you think?

3. Ignoring

Other option would be to ignore this php-stan error - for this PR.

Ignoring errors is usually the last solution - only for cases when we cannot fix them on our end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants