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

[5.6] Make ResetPassword Notification translateable #24534

Conversation

dimitri-koenig
Copy link
Contributor

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 :-)

@dimitri-koenig dimitri-koenig force-pushed the make-reset-password-mail-translateable branch from 8a40975 to 13a8931 Compare June 9, 2018 18:03
@dimitri-koenig dimitri-koenig changed the title Make ResetPassword Notification translateable [5.6] Make ResetPassword Notification translateable Jun 9, 2018
@GrahamCampbell
Copy link
Member

This introduces cross-dependency on the foundation namespaces, which breaks installing the auth package stand-alone.

@dimitri-koenig
Copy link
Contributor Author

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?

@devcircus
Copy link
Contributor

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:

Reset Email Customization
You may easily modify the notification class used to send the password reset link to the user. To get started, override the sendPasswordResetNotification method on your User model. Within this method, you may send the notification using any notification class you choose. The password reset $token is the first argument received by the method.

Otherwise, the '__' helper method depends on Illuminate\Foundation. So to drop that requirement, you'd need to handle the translation without the helper method.

@dimitri-koenig
Copy link
Contributor Author

dimitri-koenig commented Jun 9, 2018

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.

@devcircus
Copy link
Contributor

10-4 just do it without the foundation helper method.

@dimitri-koenig
Copy link
Contributor Author

@devcircus Any idea how without writing code to make translations?

On a quick glance Auth namespace is dependent on Notifications namespace.
Support namespace is dependent on Notifications namespace.
Foundation namespace is dependent on Notifications namespace.
Auth namespace is already dependent on Support namespace.
Session namespace is also dependent on Support namespace.

So we have a bunch of dependencies. Why then is it so important to not add another dependency to the Foundation namespace?

@dimitri-koenig
Copy link
Contributor Author

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?

@devcircus
Copy link
Contributor

devcircus commented Jun 9, 2018

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.

@dimitri-koenig dimitri-koenig force-pushed the make-reset-password-mail-translateable branch from 13a8931 to dfa271d Compare June 9, 2018 20:37
@dimitri-koenig
Copy link
Contributor Author

dimitri-koenig commented Jun 9, 2018

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.

@taylorotwell taylorotwell merged commit 23fc825 into laravel:5.6 Jun 12, 2018
@dimitri-koenig dimitri-koenig deleted the make-reset-password-mail-translateable branch June 12, 2018 14:16
@dimitri-koenig
Copy link
Contributor Author

Perfect, thx

@dakira
Copy link
Contributor

dakira commented Aug 31, 2018

@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?

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.

None yet

5 participants