-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
Inject Url Generator and Translator Interface into notification mailer #2169
Inject Url Generator and Translator Interface into notification mailer #2169
Conversation
@askvortsov1 Aren't these already available in views thanks to this? |
Isn't that only for page views / views returned as the result of the frontend? |
If the mailer component uses the view component as well, it should be likewise affected. |
You are 100% correct. Sorry, I should have checked before implementing it. Reverted in 4ee6d6f |
It was very easy to miss. Stumbled across those lines while changing something completely unrelated, just after having seen this PR a couple of hours earlier. 🤷 |
Although I gotta say, does make me curious why these weren't translated in the first place... |
Probably because it feels pointless to have a file only consisting of |
Allows Fix For #1934
Allows us to get rid of https://github.com/flarum/mentions/blob/e054158ae5917d4edadb3c996e4c9ae992ab78e0/views/emails/postMentioned.blade.php#L5
moving one step closer to deprecating helpers
Changes proposed in this pull request:
Reviewers should focus on:
What else should we inject? SettingsRepository came to mind, as well as possibly an optional associative array of arguments provided when calling
$mailer->send
One important thing to note is that we want to avoid an anti-pattern of business logic in the email views, so I'd stick to the essentials for now.Confirmed
composer test
).