10000 [TwigBridge][Mime] Use content for images attached to emails by aleho · Pull Request #60098 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[TwigBridge][Mime] Use content for images attached to emails #60098

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

aleho
Copy link
Contributor
@aleho aleho commented Mar 31, 2025
Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
License MIT

I tested this change with local Maildev and a real Postmark API transport.

Instead of referencing a path like <img src=3D"cid:@assets/img/email-bg.png" the content ID of the attachment will be used, e.g., <img src=3D"cid:18ca3e824a76c3d38ac2f1817ad9f6af@symfony".

aleho added 2 commits March 31, 2025 16:44
Relying on the attachment name (i.e. file path) to equal the content ID
of the attachment breaks as soon as the content ID differs and is used
to refer to an inline attachment.
Use Twig this would normally result in attachment names like
'@assets/img/file.png' which might not be a good file name to expose to
email clients.
@carsonbot carsonbot added this to the 6.4 milestone Mar 31, 2025
@aleho aleho changed the title 6.4 [TwigBridge][Mime] Use content for images attached to emails Mar 31, 2025
@aleho
Copy link
Contributor Author
aleho commented Mar 31, 2025

I'm not sure if the change to Postmark mailer was actually a bug or if an image part attached to an email should never have a content ID.

Copy link
Member
@stof stof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a test to prevent regressions on the bug.

@@ -42,14 +42,16 @@ public function toName(): string
* some Twig namespace for email images (e.g. '@email/images/logo.png').
* @param string|null $contentType The media type (i.e. MIME type) of the image file (e.g. 'image/png').
* Some email clients require this to display embedded images.
* @param string|null $name A custom file name that overrides the original name (filepath) of the image
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a new parameter is a new feature, which should go in 7.3 rather than in 6.4. Please separate that from the bugfix.

@aleho
Copy link
Contributor Author
aleho commented Apr 7, 2025

We need a test to prevent regressions on the bug.

I'm not even sure whether #60097 is a bug or not.

If attachments from Twig should never have a CID then all is well and I was using it incorrectly.

If everything should work even if the part gets a CID after attaching the image from Twig then I'd say generating a CID unconditionally might be a good idea.

I really need input on this.

@aleho
Copy link
Contributor Author
aleho commented Apr 7, 2025

Looking at:

foreach ($this->attachments as $part) {
foreach ($names as $name) {
if ($name !== $part->getName() && (!$part->hasContentId() || $name !== $part->getContentId())) {
continue;
}
if (isset($relatedParts[$name])) {
continue 2;
}
if ($name !== $part->getContentId()) {
$html = str_replace('cid:'.$name, 'cid:'.$part->getContentId(), $html, $count);
}
$relatedParts[$name] = $part;
$part->setName($part->getContentId())->asInline();
continue 2;
}
$otherParts[] = $part;
}

This is the reason why attachments always get a CID (as the name is set) and a few lines below that it seems that a CID is always generated unconditionally and then used to replace the name.

So from my point of view using the CID directly in TemplatedEmailWrapper just anticipates that.

Is there a reason why it was implemented like that with replacing of strings in the rendered HTML content?

@aleho aleho closed this Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0