Skip to content
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

Merged
merged 1 commit into from
May 22, 2020

Conversation

askvortsov1
Copy link
Sponsor Member

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:

  • Inject an instance of translator into notification views
  • Inject an instance of UrlGenerator into notification views

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

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

@askvortsov1 askvortsov1 requested review from clarkwinkelmann, luceos and tankerkiller125 and removed request for clarkwinkelmann May 17, 2020 18:51
@askvortsov1 askvortsov1 merged commit e627616 into master May 22, 2020
@askvortsov1 askvortsov1 deleted the as/provide_more_stuff_to_notification_views branch May 22, 2020 22:10
@askvortsov1 askvortsov1 added this to the 0.1.0-beta.14 milestone May 22, 2020
@franzliedke
Copy link
Contributor

@askvortsov1
Copy link
Sponsor Member Author

Isn't that only for page views / views returned as the result of the frontend?

@franzliedke
Copy link
Contributor

If the mailer component uses the view component as well, it should be likewise affected.

askvortsov1 added a commit that referenced this pull request Jul 24, 2020
…on mailer (#2169)"

This was actually already present and functional, so adding additional code for it
is unnecessary.

This reverts commit e627616.
@askvortsov1
Copy link
Sponsor Member Author

You are 100% correct. Sorry, I should have checked before implementing it. Reverted in 4ee6d6f

@askvortsov1 askvortsov1 removed this from the 0.1.0-beta.14 milestone Jul 24, 2020
@franzliedke
Copy link
Contributor

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. 🤷

@askvortsov1
Copy link
Sponsor Member Author

Although I gotta say, does make me curious why these weren't translated in the first place...

@franzliedke
Copy link
Contributor

Probably because it feels pointless to have a file only consisting of {{ trans() }} calls. There might have been thoughts about providing complete files in language packs. But as soon as HTML gets involved, that doesn't work well. 🤷 So I'm glad we're moving forward here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants