Skip to content

Conversation

datamweb
Copy link
Collaborator

@datamweb datamweb commented Jun 3, 2022

Fixed issue with displaying success message if email was not sent.
The success message should not be displayed when the email is not sent to the user for any reason.

@datamweb
Copy link
Collaborator Author

datamweb commented Jun 3, 2022

As usual, I forgot to run cs-fix.

return $this->displayMessage();
}

return redirect()->route('magic-link')->with('error', lang('Auth.unableSendEmailToUser', [$user->email]));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is different from Email2FA/EmailActivator:

if ($return === false) {
throw new RuntimeException('Cannot send email for user: ' . $user->email);
}

The error message is user friendly, but it seems there is no log of the mail sending failure.

Copy link
Collaborator Author

@datamweb datamweb Jun 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kenjis , I really disagree with that.
I like the error message is user friendly.
Why? Just imagine yourself instead of the user.
If necessary, the site administrator should see the logs of file writable\logs.
e.g logs file:
ERROR - 2022-06-03 00:39:13 --> Email: sendWithMail throwed mail(): Failed to connect to mailserver at "localhost" port 25, verify your "SMTP" and "smtp_port" setting in php.ini or use ini_set()

Friends, what do you think about this?
Do you agree with the following code?

 if ($return === false) { 
     throw new RuntimeException('Cannot send email for user: ' . $user->email); 
 } 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot that the Email class logs properly.

Okay, this implementation has no problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change the code like this (the variable name) as with other classes?

$return = emailer()->setFrom(setting('Email.fromEmail'), setting('Email.fromName') ?? '')

$return = emailer()->setFrom(setting('Email.fromEmail'), setting('Email.fromName') ?? '')

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@kenjis kenjis added the bug Something isn't working label Jun 3, 2022
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lonnieezell lonnieezell merged commit 6bf3ba1 into codeigniter4:develop Jun 6, 2022
@datamweb datamweb deleted the fix-displayMessage-MagicLinkController branch June 6, 2022 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants