Pass a translator instance to getEmailSubject on MailableInterface

[askvortsov1]

As per #2169, the notification views have access to the translator interface. The email subject, however, does not. That makes it the last significant piece where english is hardcoded. This PR revises the MailableInterface to pass a TranslatorInterface instance to getEmailSubject. It is a breaking change, but I think it is one that is worth it. The only way to do this without a breaking change would be to:

1. Create a TranslatedMailableInterface, deprecate MailableInterface
2. Remove MailableInterface
3. Deprecate TranslatedMailableInterface, make a new MailableInterface which is a copy-paste of TranslatedMailableInterface
4. Remove TranslatatedMailableInterface

Alternatively, a translator instance could be passed into every instantiation of a blueprint, but imo that is not nearly as clean (since all email subjects should be translated).

[clarkwinkelmann]

I'm not a fan of this solution.

The same problem applies to extensions that might need to access the SettingsRepositoryInterface from there.

I do agree most extensions will probably need the translator, but they might just as well not need it and this would just clutter the interface.

I think it's a place where using app() to resolve a dependency when needed makes sense.

I do see some potential for multilingual websites where we might want to process the mailable in a queue with the language that was selected in the interface. But then it might make just as much sense to store it via the constructor.

[tankerkiller125]

I'm going to agree with Clark on this one. I think using app() to resolve the dependency makes sense here as not all extensions will need translations.

[askvortsov1]

Sorry @clarkwinkelmann, I kinda completely split up your reply 🙈

I do agree most extensions will probably need the translator, but they might just as well not need it and this would just clutter the interface.

I do see some potential for multilingual websites where we might want to process the mailable in a queue with the language that was selected in the interface.

Flarum is a software that's used internationally. In fact, if I remember correctly, the majority of our users are from non-english speaking countries. In core and bundled extensions, we make translatability a priority so that these users are not disadvantaged. The current system does not allow for email notifications subjects to be translated at all, which directly contradicts our internationalization goals.

By definition, an email subject is text describing the email. It may contain some variables, but at the end of the day, variable-only subjects are quite rare. By always providing the TranslatorInterface, we not only enable extension authors to translate their email notification subject lines, but we also make it much harder to forget to do so. I think that we should be pushing our extensions to be translatable whenever possible, and this is a step in the right direction.

The same problem applies to extensions that might need to access the SettingsRepositoryInterface from there.

SettingsRepositoryInterface might be needed. TranslatorInterface almost always will be needed for reasons explained above. In the (very rare) case that settings repository is needed (which I haven't seen at all), it can still be provided by the code that instantiates the blueprint instance, since DI isn't applicable here.

But then it might make just as much sense to store it via the constructor.

Blueprints are instantiated directly. If the translator interface could be injected that would definitely be preferable, but it's not an option.

I think it's a place where using app() to resolve a dependency when needed makes sense.

I know I've expressed my thoughts on this in the past, but laravel's global helpers, while convenient, are not an architecture choice that I like. Embracing a maximally DI-dependent architecture in core is, imo, more stable and reliable.

[clarkwinkelmann]

I guess we'll have to wait for the opinion of others to decide how we want to go with this.

The current system does not allow for email notifications subjects to be translated at all

Well, that's not really true since we do have app(). It might not look good to everyone, but it's a working solution and a change is not absolutely needed. We can take the time to discuss on the best approach.

I'd actually even advocate to use a trans() global function like Laravel for translations in contexts like these. Having full access to a translator object is rarely needed. But this probably should be best discussed in an issue.

[tankerkiller125]

Given Sasha's explanation and my own personal push for making Flarum more international friendly, I think I could possibly be OK with this, but it might be something to hold off till beta.15 for merging until we can have a discussion around a global trans() helper or how to better approach this.

I'm approving solely based on the fact that the code is good and does work, and if Franz or someone believes that this is the proper approach then this PR will have it's two approvals required.

[franzliedke]

I agree with @askvortsov1, both with this proposal and the comments about translation being a default requirement for email blueprints. Adding this to the signature is a nice way to make our expectations clear. 😉

I have a small suggestion to maybe alleviate your concerns about backwards compatibility:

• If I am not mistaken, PHP does not complain if you pass additional parameters to methods.
• This allows us to pass the translator when calling getEmailSubject().
• Let's not add the parameter to the interface, so old implementations continue to work. Because of PHP's behavior, they can start using it, though.
• We can then add it to the interface in Beta.15.
• Needs to be documented (in the interface and the upgrade guide), obviously.

[franzliedke]

As for a global trans() helper, I don't particularly like those, because of the conceptual problems I outlined in #2307 (comment).

@clarkwinkelmann I would imagine you would mostly use this in views (in PHP code, you can usually inject the translator) - we could streamline the existing solution a bit using this old approach. 😆

[askvortsov1]

If we don't add it to the interface, but add it to the signature of interface implementations, won't that error because we no longer match the interface signature?

[askvortsov1]

Implementation-wise, we could remove the method from the formal interface for now, and re-add it for the next release, with the translator instance required?

[clarkwinkelmann]

Alright let's go with this solution then 👍

I would imagine you would mostly use this in views (in PHP code, you can usually inject the translator) - we could streamline the existing solution a bit using this old approach. laughing

I think the current solution in views with the shared translator instance is fine.