If an exception is thrown in any phase after the build phase, neither the theme nor the current account are switched back to their initial state. This obviously also affects SkipMailException.
Given a site with separate backend and frontend (mail) theme, try to send a mail from within a backend form. If any mailer hook throws a SkipMailException, then the resulting page will be rendered in the frontend theme.
Ensure that theme and current user are switched back to their initial values after an exception has been thrown while processing a mail.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | symfony_mailer.switch-back.3281732-10.patch | 1.19 KB | adamps |
| #6 | 3281732-switch-back-current-6.diff | 2.35 KB | znerol |
| #2 | 3281732-switch-back-current-2.diff | 4.55 KB | znerol |
Comments
Comment #2
znerol commentedThis patch introduces three new methods on
Mailer:executeInTheme()executeForAccount()executeWithLanguage()The design follows Renderer::executeInRenderContext.
Initially I wasn't sure about the responsibilities of
Email::customize()and I wondered whether it would have any side effect if this method is executed before switching the theme. I think that the order of execution has no effect on the results, and therefore deferred switching the theme to afterEmail::customize.Comment #3
adamps commentedThanks for the issue and patch.
I agree with the basic principle of how to fix however I think we can simplify. The only valid exception is
SkipMailExceptionand it can only occur in a call toprocess()orrender(). So I would put a try block around lines 203-210 and a move the "switch back" code into a finally block.Comment #4
znerol commentedThis might be true if Symfony Mailer is used in a very small site without any additional custom / contrib modules. The problem is that
process()potentially invokes contrib and custom code which isn't under the control of Symfony Mailer. Especially custom code clobbered together in a hurry can do stupid things (at least mine does).Failing to restore the original context (language, theme and current user) isn't just an annoyance, it might also lead to security issues. As a community member who was working with Drupal security team on several core and contrib fixes, I feel obliged to insist here. Let's better be safe than sorry and apply the rules of defensive programming when running code in the context of a different user.
Comment #5
adamps commentedThe current patch is far too complex.
I don't see how it's a security problem as I described it. The call to
process()would be within a try...finally so it seems that we would definitely switch back no matter what the called code does - that being the whole point of finally??Comment #6
znerol commentedHow about this?
Comment #7
adamps commentedThat seems much better thanks. However I don't see any need to include the switching in the try block as it does not throw exceptions. In core,
EntityContentBase::validateEntity()puts the call toswitchTo()outside the try block.Comment #8
znerol commentedFeel free to modify whatever is necessary to meet the standards of this project when committing the patch.
Comment #9
adamps commentedComment #10
adamps commentedHere is my proposal - basically the same, however the patch is only half the size. Please can you check it still solves the issue for your site?
Comment #11
adamps commented