Silverstripe-framework: Email not re-rendering HTML template

[NikxDa]

Affected Version

Latest (4.4.4)

Description

Using setHTMLTemplate and setData on an Email will correctly render the data. However when running a second time, the Email class will not re-render.

Cause

I've narrowed this problem down to this line:
https://github.com/silverstripe/silverstripe-framework/blob/d1c927ff2372a422ca8d5145aee6ba5192355f80/src/Control/Email/Email.php#L803

I believe there is no need for the !$htmlPart check other than maybe performance reasons? I have attached a PR.

Thanks!

PRs

• [x] #9876

[maxime-rainville]

Can you clarify the scenario where this behaviour would be helpful?

Are you trying to send the same email twice with some slight alteration?

[NikxDa]

@maxime-rainville Yep, that is exactly what I was trying to do. Here's some pseudo-code to show the issue:

email = new Email
email.setHTMLTemplate
email.setData
email.send

# This does not work, it will re-send the first email without updated data:
email.setData
email.send

[kinglozzer]

This is a tough one to “solve”, but I’d like to bump it because it very nearly caused us to leak one poor member’s details to everyone else on the site:

$email = Email::create();
$email->setSubject('Important information about your account');
$email->setHTMLTemplate('App\Email\AccountUpdateEmail');

foreach ($recipients as $member) {
    $email->setData(['Member' => $member]);
    $email->setTo($member->Email, $member->getName());
    $email->send();
}

Thankfully it was caught, but I don’t think it’s unreasonable to assume the above code would send different email content for each member.

Perhaps the solution is to make setData()/addData()/removeData() clear the email body & plain part to ensure they’re regenerated?

Workaround we’re using for now is:

$email->setBody(null)->render();
$email->send();

[NikxDa]

@kinglozzer Thanks for bringing this back up. I lost focus on this since it was an edge case at the time and I didn't get around to submitting a new PR. I just took another look.

@maxime-rainville Since you were the person to originally review my PR (which broke caching at the time), would you reckon adding $this->setBody(null) to line 573, 591 and 608 in this file:

https://github.com/silverstripe/silverstripe-framework/blob/4/src/Control/Email/Email.php

would fix this? It would essentially clear the cache when data is changed. I can open a new PR if this is suitable.

[maxime-rainville]

Haven't looked at this in a while, but invalidating the current message body when some new data is set sounds like a sensible thing to do.

I'm escalating this to impact/high since @kinglozzer's example looks like something that pretty much any developer could have done and would lead to a pretty bad data breach.

@NikxDa Do you feel like having another crack at this one? Otherwise, I will pick it up.

[NikxDa]

@maxime-rainville Getting a PR ready for you today.

[NikxDa]

@maxime-rainville https://github.com/silverstripe/silverstripe-framework/pull/9876 PR open here.

[maxime-rainville]

Fixed by #9876