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

Winter 1.2 not using SMTP settings from database #626

Closed
tintong opened this issue Jul 23, 2022 · 24 comments · Fixed by wintercms/storm#107
Closed

Winter 1.2 not using SMTP settings from database #626

tintong opened this issue Jul 23, 2022 · 24 comments · Fixed by wintercms/storm#107
Assignees
Milestone

Comments

@tintong
Copy link

tintong commented Jul 23, 2022

Winter CMS Build

1.2

PHP Version

8.0

Database engine

MySQL/MariaDB

Plugins installed

Winter.Pages, Winter.Blog, Winter.User

Issue description

Since upgrading to Winter 1.2 from the previous version, emailing via SMTP has not worked.

The error message when emailing has always been:
Expected response code "250/251/252" but got code "550", with message "550 5.7.1 Relaying denied".

I've looked into why and I have found that the email settings entered in the Winter backend, saved to the database, are not being used when actually emailing within Winter.

This can be tested by pressing the "Send test message" button in the "Mail configuration" backend section.

Upon looking at the code, when "createSmtpTransport($config)" in MailManager.php from laravel is called, the values it is looking for are $config['host'], $config['username'], $config['password'], $config['port'].

Ref: https://github.com/laravel/framework/blob/8156743f644fb4597b63261a3fda4e4d6ca49a78/src/Illuminate/Mail/MailManager.php#L163-L183

However, when "applyConfigValues()" in MailSetting.php is called, both the values from config/mail.php and the database are combined.

Ref: https://github.com/wintercms/winter/blob/develop/modules/system/models/MailSetting.php#L86-L118

The config values that get passed to laravel are in this format:
Array ( [driver] => smtp [host] => smtp.mailgun.org [port] => 587 [from] => Array ( [address] => mailaddress [name] => Winter CMS ) [encryption] => tls [username] => [password] => [sendmail] => /usr/sbin/sendmail -bs [default] => smtp [mailers] => Array ( [smtp] => Array ( [host] => smtp.office365.com [port] => 587 [username] => mailaddress [password] => password [encryption] => tls ) ) )

It seems that laravel then only uses the config values from config/mail.php instead of the database to make the Smtp connection.

Steps to replicate

Leave the config/mail.php as default from the Winter installation.
Add Smtp settings into the Mail configuration section in the Winter CMS and click on "Send Test email".
Error message "Expected response code "250/251/252" but got code "550", with message "550 5.7.1 Relaying denied"." is then presented.

Workaround

As a temporary workaround, put your Smtp configuration values into config/mail.php.

@tintong tintong added needs review Issues/PRs that require a review from a maintainer Type: Unconfirmed Bug labels Jul 23, 2022
@tintong
Copy link
Author

tintong commented Jul 23, 2022

I'd be happy to continue helping with this, just need some more pointers to look at please.

@mjauvin
Copy link
Member

mjauvin commented Jul 23, 2022

I tried replicating this with your procedure and I couldn't. The proper config gets passed from what's provided in the form, and mail gets sent properly. I had no config in the config/mail.php (other than default values)

@mjauvin mjauvin self-assigned this Jul 23, 2022
@mjauvin mjauvin added needs response Issues/PRs where a maintainer is awaiting a response from the submitter and removed needs review Issues/PRs that require a review from a maintainer labels Jul 23, 2022
@mjauvin
Copy link
Member

mjauvin commented Jul 23, 2022

I also added trace_log($config) call here and I got the proper config as expected:

https://github.com/laravel/framework/blob/8156743f644fb4597b63261a3fda4e4d6ca49a78/src/Illuminate/Mail/MailManager.php#L171

@tintong
Copy link
Author

tintong commented Jul 23, 2022

Thank you for helping.

The only other plugin I have but not enabled is the Gmail email sending one.

I'll try again today.

@mjauvin
Copy link
Member

mjauvin commented Jul 23, 2022

It's possible the gmail plugin is interfering, try removing/disabling it.

@tintong
Copy link
Author

tintong commented Jul 23, 2022

I've removed the gmail plugin and deleted the files.
Then tested it again and the config that comes in to the "createSmtpTransport" function still points to "smtp.mailgun.org" which is what is in the config/mail.php file.

What else might it be please?

@tintong
Copy link
Author

tintong commented Jul 23, 2022

Does trace_log($config); echo an output automatically or should it be done separately?

@mjauvin
Copy link
Member

mjauvin commented Jul 23, 2022

It should work, please show how you debug this.

Also, show the output of composer show winter*

@mjauvin
Copy link
Member

mjauvin commented Jul 23, 2022

Does trace_log($config); echo an output automatically or should it be done separately?

it sends the output to your /storage/logs/system.log file

@tintong
Copy link
Author

tintong commented Jul 23, 2022

I've added "trace_log($config)" to the "resolve" function in winter\storm\src\Mail\MailManager.php
The trace_log() command is right after $config = $this->getConfig($name);
The output is:

[2022-07-23 17:34:37] development.INFO: Array
(
[host] => smtp.office365.com
[port] => 587
[username] => emailadrress
[password] => password
[encryption] => tls
)

I then added the same function into the "createSmtpTransport(array $config)" function within the laravel MailManager.php file.

The output is:
[2022-07-23 17:42:55] development.INFO: Array
(
[driver] => smtp
[host] => smtp.mailgun.org
[port] => 587
[from] => Array
(
[address] => emailaddress
[name] => Winter CMS
)

[encryption] => tls
[username] => 
[password] => 
[sendmail] => /usr/sbin/sendmail -bs
[default] => smtp
[mailers] => Array
    (
        [smtp] => Array
            (
                [host] => smtp.office365.com
                [port] => 587
                [username] => emailaddress
                [password] => password
                [encryption] => tls
            )

    )

)

the output for "composer show winter*" is:

winter/storm v1.2.0 Winter CMS Storm Library
winter/wn-backend-module v1.2.0 Backend module for Winter CMS
winter/wn-blog-plugin v2.0.2 Blog plugin for Winter CMS
winter/wn-cms-module v1.2.0 CMS module for Winter CMS
winter/wn-pages-plugin v2.0.2 Pages plugin for Winter CMS
winter/wn-system-module v1.2.0 System module for Winter CMS
winter/wn-user-plugin v2.0.0 User plugin for Winter CMS

@mjauvin
Copy link
Member

mjauvin commented Jul 23, 2022

It really doesn't make any sense since createSymfonyTransport() is called from the resolve() method in Winter\Storm\Mail\MailManager. There are no other manipulation of the $config after that.

@tintong
Copy link
Author

tintong commented Jul 23, 2022

Apologise for the confusion.

My first trace_log() output was from a previous test.
The output gave me the info from "$this->app['config']["mail.mailers.{$name}"];" within the same resolve function in Winter Storm MailManager.php

If I am reading the data correct in the config object, it is storing the config values from config/mail.php in the root of the object, but the config values from the database are stored further nested in the object under "mailers" and then "smtp".

When "createSymfonyTransport()" is called, it only looks for the values on the root of the object.

I hope this makes sense, if not, please say and I'll try and explain it differently.

@mjauvin
Copy link
Member

mjauvin commented Jul 23, 2022

I understand what you mean, but it doesn't match the behavior I am seeing.

Did you completely remove the gmail plugin? Something is screwing with your config dynamically

@tintong
Copy link
Author

tintong commented Jul 23, 2022

Yes, deleted from within the backend first and then manually deleted the folder/files.
This is all I have left:

image

And my plugins:

image

@mjauvin
Copy link
Member

mjauvin commented Jul 23, 2022

Please, can you test this with a CLEAN install with NO PLUGINS installed and report results back? This makes absolutely no sense.

@mjauvin mjauvin removed the needs response Issues/PRs where a maintainer is awaiting a response from the submitter label Jul 23, 2022
@mjauvin
Copy link
Member

mjauvin commented Jul 23, 2022

Tested working on multiple clean install, not a bug.

@mjauvin mjauvin closed this as completed Jul 23, 2022
@mjauvin
Copy link
Member

mjauvin commented Jul 23, 2022

Problem caused by the old format used in config/mail.php

@mjauvin mjauvin reopened this Jul 23, 2022
@m-paniez9292
Copy link

Hi,
I have a similar problem : Expected response code "250/251/252" but got code "550", with message "550 5.7.1 Relaying denied".
I'm not using Gmail and I just installed a migration from october v1.x to Winter v1.2.
I put trace_log($config); in winter\storm\src\Mail\MailManager.php and I have :

[2022-07-25 16:36:57] development.INFO: Array
(
    [driver] => smtp
    [host] => smtp.mailgun.org
    [port] => 587
    [from] => Array
        (
            [address] => contact@my_domain_name.com
            [name] => my_name
        )

    [encryption] => tls
    [username] =>
    [password] =>
    [sendmail] => /usr/sbin/sendmail -bs
    [default] => smtp
    [mailers] => Array
        (
            [smtp] => Array
                (
                    [host] => smtp.my_provider.com
                    [port] => 465
                    [username] => contact@my_domain_name.com
                    [password] => my_password
                    [encryption] => ssl
                )

        )

)

Thanks

@mjauvin
Copy link
Member

mjauvin commented Jul 25, 2022

Please use the new config/mail.php from 1.2 branch

@m-paniez9292
Copy link

m-paniez9292 commented Jul 25, 2022

Re,
With the config/mail.php file, it works fine.
Thanks

@LukeTowers
Copy link
Member

@mjauvin is there anything we can do to better support the old config style so no one else runs into this during the upgrade process?

@bennothommo bennothommo added this to the v1.2.1 milestone Aug 25, 2022
@bennothommo
Copy link
Member

Marking as a bug due to more confirmations from the community.

bennothommo added a commit to wintercms/storm that referenced this issue Aug 25, 2022
This reverses the order for checking mail configs by searching for the new format first, then checking for the old format.

Laravel checks old format first, then new format, which works fine for them, but doesn't work for us as we also populate settings via the Backend, which populates the settings in the new format.

Fixes wintercms/winter#626
@mjauvin
Copy link
Member

mjauvin commented Oct 15, 2022

@bennothommo can this be closed, I see you commited a fix for this.

@bennothommo
Copy link
Member

The commit is within the open PR, @mjauvin, so this shouldn't be closed yet :)

LukeTowers pushed a commit to wintercms/storm that referenced this issue Oct 17, 2022
This reverses the order for checking mail configs by searching for the new format first, then checking for the old format.

Laravel checks old format first, then new format, which works fine for them, but doesn't work for us as we also populate settings via the Backend, which populates the settings in the new format.

Fixes wintercms/winter#626
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 a pull request may close this issue.

5 participants