$email =newEmail();$email->addTo($'foo@bar.de');$email->text('text');$email->html('<html></html>');$email->getHeaders()->addTextHeader('X-Template-ID','d-0aac27809ad64ae98d5ebaf896e58b33');$email->getHeaders()->addTextHeader('X-Dynamic-Template-Data',json_encode(['price' =>'42 €']));

I am however not sure about thejson_encode/json_decode for handling an array, any proposal?
I also tried to use sth. like:

$email->getHeader->addArrayHeader('X-Dynamic-Template-Data', ['price' =>'42 €']);

but I am not sure about the spec and if I am allowed to add a new kind of header to the Mime component 🤔

Note that you need to add sth. like:

$email->text('text');$email->html('<html></html>');

otherwise you will receive an exception:

OskarStark added2 commits

June 15, 2021 13:34

[Mailer] Add Sendgrid template support

76c2802

Fix

4a0c4e3

OskarStark self-assigned this

Jun 15, 2021

carsonbot added Status: Needs Review Bug Feature Mailer labels

Jun 15, 2021

OskarStark requested a review fromfabpot

June 15, 2021 11:42

OskarStark removed the Bug label

Jun 15, 2021

OskarStark added this to the5.4 milestone

Jun 15, 2021

OskarStark commented

Jun 15, 2021

View reviewed changes

src/Symfony/Component/Mailer/Bridge/Sendgrid/Tests/Transport/SendgridApiTransportTest.php OutdatedShow resolvedHide resolved

OskarStark added3 commits

June 15, 2021 13:47

Update src/Symfony/Component/Mailer/Bridge/Sendgrid/Tests/Transport/S…

766f66f

…endgridApiTransportTest.php

Fix

9e09240

CS

944552a

Copy link

Member

stof commentedJun 15, 2021

I'm not sure we want to add this to the component. The code you write is strongly tied to the Sendgrid mailer and would break when using another transport.

Copy link

ContributorAuthor

OskarStark commentedJun 15, 2021

I'm not sure we want to add this to the component. The code you write is strongly tied to the Sendgrid mailer and would break when using another transport.

What do you mean, I only set headers on a mime email, so it would not break with other transports. The headers are then only interpreted by the SendgridApiTransport 🤷‍♂️

OskarStark changed the title~~[Mailer] Add Sendgrid template support~~[Mailer] [Sendgrid] Add template support

Jun 15, 2021

fix test

2b74d36

Copy link

Member

@OskarStark but the code you wrote in your PR description would not work if you configure the mailer with another transport (it would send an email with<html></html> as content, which will be consider a bug by any product manager in the world).

Copy link

ContributorAuthor

OskarStark commentedJun 15, 2021•
edited
Loading

In this case documenting it and tell the people to write valid text and html content as fallback?
I mean if I send an html mail and no text fallback the output would be very bad in a text email client, but the mail is send and received.... not sure about your concerns, while I definitely agree with:

it would send an email with as content, which will be consider a bug by any product manager in the world

Another case about headers, if I can setx-sg-eid on transportA, it is not allowed to be overwritten by transportB, in this caseSendgrid, which will end up in different results, too:

symfony/src/Symfony/Component/Mailer/Bridge/Sendgrid/Transport/SendgridApiTransport.php

Lines 106 to 112 in4a0c4e3

	// these headers can't be overwritten according to Sendgrid docs
	// see https://sendgrid.api-docs.io/v3.0/mail-send/mail-send-errors#-Headers-Errors
	$headersToBypass = ['x-sg-id','x-sg-eid','received','dkim-signature','content-transfer-encoding','from','to','cc','bcc','subject','content-type','reply-to'];
	foreach ($email->getHeaders()->all()as$name =>$header) {
	if (\in_array($name,$headersToBypass,true)) {
	continue;
	}

Copy link

ContributorAuthor

OskarStark commentedJun 15, 2021

What about adding

$email->setNoContent(true)

to explicitly mark this email without content?

Copy link

Member

stof commentedJun 15, 2021

In this case documenting it and tell the people to write valid text and html content as fallback?

but if they write the HTML and text body in their app, there is no point using a sendgrid template managing that content on sendgrid (it might even be worse if they get out of sync)

to explicitly mark this email without content?

that would still make your code work properly only with the SendgridTransport, by adding features in that transport that bypass the component abstraction.

Copy link

ContributorAuthor

OskarStark commentedJun 15, 2021

but if they write the HTML and text body in their app, there is no point using a sendgrid template managing that content on sendgrid (it might even be worse if they get out of sync)

I agree

that would still make your code work properly only with the SendgridTransport, by adding features in that transport that bypass the component abstraction.

But it would make clear to the developer that it can be used across transports but without content IF the other transport does not understand that header.

I mean what would be the solution? Our Mailer cannot support such things? Abstractions are nice, but always limit the power to the lowest feature set 🤷 Thats why I wanted to work around this limitation using a common way of dealing with custom things: headers.

Copy link

Member

stof commentedJun 15, 2021

@OskarStark to me, email templates are not something you can abstract easily. Some provider maintain templates in their own system with a parameter in the API call (as done by Sendgrid) or with a separate API endpoint. Others are using the email body as template source, with a different syntax each for the templating language.

And some of the features of the components are also incompatible with that (using the DkimSigner on an email which will have its content replaced on the provider server when applying the template won't work for instance).

Copy link

ContributorAuthor

OskarStark commentedJun 15, 2021

I agree, this is just my first idea on how to implement it.

Any advice how this can be done? I am open for ideas to extend the Symfony mailer ecosystem this way.

Copy link

ContributorAuthor

OskarStark commentedJun 16, 2021•
edited
Loading

Btw. isn't this the same problem we currently have withoptions in Notifier? I can create aChatMessage, but in case I addTelegramOptions to this ChatMessage it cannot be send by another transport anymore.

symfony/src/Symfony/Component/Notifier/Bridge/Telegram/TelegramTransport.php

Lines 61 to 72 inf12a419

	/**
	* @see https://core.telegram.org/bots/api
	*/
	protectedfunctiondoSend(MessageInterface$message):SentMessage
	{
	if (!$messageinstanceof ChatMessage) {
	thrownewUnsupportedMessageTypeException(__CLASS__, ChatMessage::class,$message);
	}

	if ($message->getOptions() && !$message->getOptions()instanceof TelegramOptions) {
	thrownewLogicException(sprintf('The "%s" transport only supports instances of "%s" for options.',__CLASS__, TelegramOptions::class));
	}

And if we would remove the options check we would only send the plain message via another transport, but additional tied information, which are located in the options, would be ignored by another transport.

OskarStark requested a review fromnicolas-grekas

June 16, 2021 07:47

Copy link

Member

fabpot commentedJun 17, 2021

As I mentioned in some other issues/PRs, I'm against supporting specific features of each mailer. The main goal of the component is to be able to switch from one provider to another one or been mix them (round-robin). If we find an abstraction that would work for the vast majority of the current providers, why not, but having specific features is a no-go for me.