-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[5.6] Make ResetPassword Notification translateable #24534
[5.6] Make ResetPassword Notification translateable #24534
Conversation
8a40975
to
13a8931
Compare
This introduces cross-dependency on the foundation namespaces, which breaks installing the auth package stand-alone. |
Laravel is all about making life for developers easier. The current way of not being able to translate such strings is a perfect target to make lifer easier. Any other idea on how to achieve that? |
One way laravel makes life easy, is making it so the developer can deviate from the convention when they need to. Overriding a method in a trait in order to specify your own notification, is not "funky" or a hack. It is made this way so you can use your own notification. It's even in the documentation:
Otherwise, the '__' helper method depends on Illuminate\Foundation. So to drop that requirement, you'd need to handle the translation without the helper method. |
I get the point about being able to customize the notification. But technically it's not about customizing the notification but about finishing the translation process. Per default laravel ships with resources/lang/en/* files which handle most of the auth translations. Even the auth views are done in such a way that an easy and fast translation of labels is possible without touching those view files. But it's like 80% done. The reset password notification is missing. Now I would like to finish it. So, any other idea's are very welcome. |
10-4 just do it without the foundation helper method. |
@devcircus Any idea how without writing code to make translations? On a quick glance Auth namespace is dependent on Notifications namespace. So we have a bunch of dependencies. Why then is it so important to not add another dependency to the Foundation namespace? |
What about adding a local translate method which does the same as the __ foundation method? Would be dependent on the Translation namespace, but in light of many other namespace cross dependencies all around laravel it would be ok, imho. Any thoughts on this? |
Foundation is not a standalone package that can be pulled into a project that is using Auth. You would have to include the entire framework. Foundation exists simply to glue the framework together with the other components. I haven't looked deep into it, but couldn't you just use the Lang facade. The rest of the framework uses facades occasionally and it is in the Illuminate\Support namespace, which is already required by the Auth component. |
13a8931
to
dfa271d
Compare
Thanks for the tip. I've updated the commit accordingly, now it uses actually the same code as the __ method, just via the Lang facade. |
Perfect, thx |
@dimitri-koenig wow. This is great. I tried to get this into the framework in the past and couldn't move on for the same reasons. I'm confused, though. The facade needs Illuminate/Translation/Translator which is a dependency in neither Support nor Auth. So the way I see it this would just break if Illuminate/Translation is not present, right? OTOH illuminate/notification is also not a required dependency, how is that handled? Maybe @GrahamCampbell could help me understand? |
Instead of doing funky stuff like this (https://laracasts.com/discuss/channels/laravel/how-to-language-for-illuminateauthnotificationsresetpasswordphp) it would be so nice to just be able to translate the messages in the password reset mail like everything else, kinda like a no brainer :-)