embedFromPath API addition (for #3284140)

Since the API changes here were already RTBCed over in ✨ Email adjuster to attach/embed by scanning the email body Needs work , moving directly to RTBC.

AdamPS → credited metallized → .

Great thanks @heddn. Definitely good to split this into a separate issue as it's much easier to review one step at a time.

Even with the reduced scope, this area is pretty tricky when you look into it closely. Here are my comments and points for discussion - probably best if we try to agree what to do before spending time changing code.

1) Let leave allowEmbed() out of this issue. Until we have completed the "attachment security" task then we are just guessing about the interface. Probably it would be called allowAttach() as it's equally possible to have security bugs with attaching files.

What we can and should do is add a comment to all the attach/embed functions that the caller is responsible for access checking of attachments to avoid sending a private file or even worse settings.php to someone who shouldn't see it. Code call this function using a $filename taken from unsanitized user input.

2) What about embedNoPath()? This would correspond to Symfony embed(). Implementation will depend on the points below....

3) embed/attach are almost the same except for the $inline flag, and even symfony will swap the inline flag in prepareParts() if you got it wrong. We're keeping an array $this->filenames[$path], probably we should do that too in attachFromPath(). And as in point 1), the security will likely be shared. So maybe we should try to combine the code? even combine the interface, with an $embed flag??

4) There is confusion on names. The $name parameter is not in general a file name. Inspecting into symfony code, the gets written to the 'Content-Type' parameter 'name', so we could call it an attachment name or a part name.

Symfony defaults to the base file name for attachFromPath() - which will break if it clashes with an embed. Our patch currently defaults to a hash of the file name for embedFromPath(), but that wouldn't work for embedNoPath(). I had suggested several times that we just use an incrementing integer but this wasn't taken up.

The name is used in a complex way in Email::prepareParts(). It's strange that we end up writing code to pick a unique name only for symfony to run a lot of code swapping it for the content id which is a different, random unique name. I don't know what we can do as Symfony makes a lot of things private, and difficult/expensive to query the attachments / names / content ids.

5) Let's combine this with ✨ Simpler way to get attachments Needs work , and then write a test that covers all 5 attachment methods.

What I'd like to aim for in an MVP is something that allows:

  public function build(EmailInterface $email): void {
    $email->embedFromPath('themes/custom/my_theme/logo.png', 'logo', 'image/png');
}

then in my email.html.twig:

  <img src="cid:logo" alt="'Logo'|t" title="'Logo'|t">

Right now, there isn't enough of the symfony underlying API available to do this without hacking the Email object through reflection. If we need to add dire warnings that adding random files to emails is a security concern, let's do that. Anything so we can get the API addition into the mix and improve things over time.

I agree we can cover security with a comment - that's my point 1).

Points 2) and 5) are simple - let's complete the attachments API by adding all the remaining gaps at once rather than an issue for each. This would also allow writing better tests.

Points 3) and 4) are questions about what is the right API. Sorry it was a bit unfocused and unclear, as I'm still thinking things through.

I think it's really important to understand how this API will be used. #7 matches the Symfony API well - but is it really what we want in Drupal? The theme-designer needs to tell the coder every file to embed - changing the code every time they want to add a file. It could work in custom code but it's awkward. Clearly it won't work in a contrib module where each site will want different files.

Instead we might prefer any of these:

• <img src="themes/custom/my_theme/logo.png" embed="true" alt="'Logo'|t" title="'Logo'|t">
• symfony syntax: <img src="{{ email.image('@public/logo.png') }}" alt="Logo">
• Field formatter for image field with option to embed
• (moving on to attach rather than embed) Email adjuster to attach a file

So I propose the Symfony API is not good for Drupal and instead we should have something like this:

1. The application sets $name which is a human-readable value that the mail client often displays with the attachment. It defaults to the file basename for the FromPath functions.
2. The framework assigns the CID, which is a machine-oriented ID that must be unique. I propose this has no relationship to $name because they have quite different needs and the application in general can't easily pick a unique value given that also hooks, plug-ins, templates etc. might embed/attach. Instead the embed functions return a CID.
3. All attach and embed functions may fail due to access checking, in which case they return FALSE. Therefore they can't return $this. However this issue isn't about access checking, so we can defer the non-BC change to the existing attach methods to a separate issue.
4. getAttachments() should return attachments not data parts - it should be easy to get back all of the data from the original attach call which is not available on a data part. Also this should be performance efficient. Probably we keep our own record of the attachments instead of just $this->filenames

My other points are coding details:

• We should use a function for common code in the 4 attach functions - this can include tracking the attachments, calculating CID & mime type.
• We can delete allowEmbed() and put the single line of code in embedFromPath(). In a separate issue we can write allowAttach() with general purpose access checking.

Anything so we can get the API addition into the mix and improve things over time.

Yes but we can't change the interface later, so we need to get that right now. This is an important decision that will hopefully even get adopted into Core for use on 100000s sites. So let's not rush, and instead take a little time to get it right😃.

+1

Lack of image embedding is blocking us from migrating to Symfony Mailer.
What are the prospects of moving forward with this?