Skip to content

Conversation

Cybso
Copy link

@Cybso Cybso commented Aug 1, 2024


/** @psalm-suppress InternalMethod */
$stream->setStreamOptions($currentStreamingOptions);
} else if ($mailSmtphost === 'localhost' || $mailSmtphost === '127.0.0.1') {
Copy link
Contributor

@szaimen szaimen Aug 2, 2024

Choose a reason for hiding this comment

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

In theory we could even do this if no valid host was provided, e.g. by using such a filter: https://github.com/nextcloud/all-in-one/blob/c9b97220d0175914c3ee8c7199949376f7bfe3b0/php/src/Data/ConfigurationManager.php#L274-L295

However this sounds like magic and not sure if we want to actually do this automatically.

WDYT @nextcloud/server-backend @miaulalala

@kesselb kesselb requested a review from come-nc August 2, 2024 15:14
@kesselb
Copy link
Collaborator

kesselb commented Aug 2, 2024

1) Test\Mail\MailerTest::testStreamingOptionsWrongType
Failed asserting that 1 matches expected 0.

/home/runner/actions-runner/_work/server/server/tests/lib/Mail/MailerTest.php:291

@Cybso
Copy link
Author

Cybso commented Aug 2, 2024

1) Test\Mail\MailerTest::testStreamingOptionsWrongType
Failed asserting that 1 matches expected 0.

/home/runner/actions-runner/_work/server/server/tests/lib/Mail/MailerTest.php:291

I will adapt the test as soon as it is clear whether this change has a chance of being accepted or not.

Personally I'm totally fine if the PR is being rejected. #41935 was just an idea to reduce confusion for users using a local mail server, resulting in many false bug reports.

Comment on lines +310 to +318
} else if ($mailSmtphost === 'localhost' || $mailSmtphost === '127.0.0.1') {
$currentStreamingOptions = $stream->getStreamOptions();
$currentStreamingOptions = array_merge_recursive($currentStreamingOptions, array(
'ssl' => array(
'allow_self_signed' => true,
'verify_peer' => false,
'verify_peer_name' => false
)
));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm of two minds here - one is that it indeed makes localhosts work without any setup, which, as you stated has confused many users due to how the Symfony Mailer works. OTOH I'm not entirely happy with overwriting config options per default.

I think there's some middle ground here: check if the ssl option is set and only overwrite it if a) the host is localhost || 127.0.0.1 and b) if the option doesn't already exist in the $currentStreamingOptions

I also think it doesn't need to be an elseif but can be an if condition on it's own. Something like:

Suggested change
} else if ($mailSmtphost === 'localhost' || $mailSmtphost === '127.0.0.1') {
$currentStreamingOptions = $stream->getStreamOptions();
$currentStreamingOptions = array_merge_recursive($currentStreamingOptions, array(
'ssl' => array(
'allow_self_signed' => true,
'verify_peer' => false,
'verify_peer_name' => false
)
));
}
$local = ['localhost', '127.0.0.1']; // You could make this a constant
if (in_array($mailSmtphost, $local) && !isset($currentStreamingOptions['ssl'])) {
$currentStreamingOptions = $stream->getStreamOptions();
$currentStreamingOptions = array_merge_recursive($currentStreamingOptions, array(
'ssl' => array(
'allow_self_signed' => true,
'verify_peer' => false,
'verify_peer_name' => false
)
));

@kesselb
Copy link
Collaborator

kesselb commented Aug 5, 2024

symfony/symfony#53621 (but it will take a while until we are on mailer 7.1)

@github-actions
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 29, 2024
@skjnldsv skjnldsv added this to the Nextcloud 32 milestone Jan 30, 2025
This was referenced Aug 22, 2025
This was referenced Sep 2, 2025
This was referenced Sep 25, 2025
@skjnldsv skjnldsv removed this from the Nextcloud 32 milestone Sep 28, 2025
@skjnldsv skjnldsv added this to the Nextcloud 33 milestone Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Automatically set allow_self_signed=true if mail_smtphost = localhost

6 participants