Configurable email

Hi @megakeegman, it's a nice feature to have in this module, I'm looking into it.

Hey, thanks for taking a look. The str_replace method you are using looks like it would work, but it is a little strange, since email configuration generally has good support for tokens. Do you think we should not add the token module → as a dependency?

Hello @megakeegman,

Thank you for reviewing the code changes. Yes, we can use the Token module. I had initially considered implementing it that way. However, since it requires adding an extra module (i.e., the Token module) as a dependency, I opted for the string replacement method instead.

As you suggested using the Token module, I am assigning this issue to myself to update the code accordingly.

Added the support for the token, Please review the changes.

I think it would make more sense for the email settings to be in the mail settings at /admin/config/people/accounts rather than on their own settings page. I had this part done on the original branch of the issue fork you want to use that for reference.

I am actually taking a go at making one more commit on your branch. Shouldn't be long before it is pushed

Okay ready for review again. If possible, would like to get this in, and then update https://www.drupal.org/project/one_time_login_link_admin/issues/3379311 ✨ Allow option to edit valid time Needs review on top

I forgot to change status to needs work, but status is accurately needs review again

Just tested and the login link is not actually making its way into the email via the token. Also, I see that a new token was created for this—one-time-login-link. I wonder if this can be updated to just use the existing one-time-login-url. But need to look into how the login link actually makes it into the token for that one. Because currently our code is somehow breaking the one-time-login-url token, so need to figure out why.

Hi @megakeegman, thanks for reviewing the code.
I'm attaching here the email in which I used both the tokens created in the MR along with the existing one-time-login-url token. I'm also Attaching the screenshot of the mail.

And I have also tried by commenting out the code for one-time-login-link i.e. the new token but still the URL token is not working.

Can confirm that the URL token does work when the patch is removed entirely, so we are breaking it somewhere.

Trying to dig into this, it looks like the uli generation for the url token occurs in user_mail hook, which is module specific. So the URL token currently ONLY works for emails that are provided by the user module. I think the only way around this is to duplicate some of the code from the user module

Okay I see that the major difference is that we are handling token replacement in the controller, rather than in HOOK_mail implementation

I went ahead and updated MR 18 to be consistent with the core user module. This removes the one-time-login-link token, and makes the one-time-login-url token accessible. It also moves all of the token replacement logic into the HOOK_mail implementation. Final note that I removed the inclusion of the login url from the alert. I don't think we need to show it to the admin that clicked the send login link button.

Whoops also going to revert some of the stuff in the generate function, since we don't really need the token functionality there either.

Okay I think it's in good shape now

My only issue with this at this point is what I said above in comment #9 ✨ Configurable email Active

..and related that we have the timeout token categorized as a 'user' token. These things could probably be improved, but I am not sure what would be best practice for these.

I reviewed the MR-18 and it is working fine for me as expected and mail is being sent with tokens accessibility.
The changes looks good to me thus moving it to RTBC+!