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.

Sign up for GitHub

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

Jump to bottom

feat: add "null" SMTP transport mode #48977

Merged
merged 4 commits into from
Nov 19, 2024
Merged

feat: add "null" SMTP transport mode #48977

merged 4 commits into from
Nov 19, 2024

Conversation

thlehmann-ionos
Copy link
Contributor

Summary

If mails ought to be send by other means than rendering messages from
templates and sending them via SMTP-like protocols.

Use-case example

Listening to specific Nextcloud events and pass parameters to a centralized (i.e. REST-based) API that sends e-mails.

Background why not ...

  • customize mail templates: mails should not be delivered by SMTP-like protocols and rendering messages that will never be send is unneeded overhead
  • implement template class and set mail_template_class: same as before
  • listen for BeforeMessageSent and act upon that: would be an option, but it would still render messages that will never be sent and the template data might also have to be enriched with custom information (i.e. in our case the sender's user ID, not their e-mail address)

(Short list of internal reasoning we had before making this pull request.)

Checklist

thlehmann-ionos added a commit to IONOS-Productivity/nc-server that referenced this pull request Oct 29, 2024
@thlehmann-ionos
IONOS(feat): add "null" STMP transport mode
fcccc0f 
This is an cherry-picked, early commit of the still pending PR nextcloud#48977.

== Goal

Allow disabling mail delivery altogether.

== Usecase

If mails ought to be send by other means than rendering messages from
templates and sending them via SMTP-like protocols.

Example: listening to specific Nextcloud events and pass parameters to
a centralized (i.e. REST-based) API that sends e-mails.

Signed-off-by: Thomas Lehmann <t.lehmann@strato.de>
@thlehmann-ionos thlehmann-ionos mentioned this pull request Oct 29, 2024
Merged
@thlehmann-ionos
Copy link
Contributor Author

I chose the string null because the NullTransport class in the symfony mailer package inspired me for that, but I realize, that a string null can be confused with a literal null value. I'm happs to change it to none, noop or similar.

@joshtrichards joshtrichards changed the title feat: add "null" STMP transport mode feat: add "null" SMTP transport mode Oct 31, 2024
solracsf
solracsf reviewed Oct 31, 2024
View reviewed changes
config/config.sample.php Outdated Show resolved Hide resolved
artonge
artonge reviewed Nov 5, 2024
View reviewed changes
lib/private/Mail/Mailer.php Outdated Show resolved Hide resolved
@thlehmann-ionos thlehmann-ionos mentioned this pull request Nov 5, 2024
Merged
come-nc
come-nc reviewed Nov 12, 2024
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Current version looks good, it adds support for NullTransport.

How will the SMTP settings UI behave when using null transport though?

@github-actions 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.)

@thlehmann-ionos thlehmann-ionos force-pushed the tl/dev/disable-mailing branch 3 times, most recently from 4d7d2de to 8449979 Compare November 13, 2024 16:57
artonge
artonge approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Looks good :)

@thlehmann-ionos
Copy link
Contributor Author

UI is now also disabled when the transport is configured as null:

image

A new text was added. I think translations are not urgent and don't have to be a blocker.

@artonge artonge requested a review from come-nc November 14, 2024 11:45
@artonge
Copy link
Contributor

artonge commented Nov 14, 2024

@thlehmann-ionos, php linter is not happy, can you fix it? You probably just need to run composer run cs:check

@artonge artonge mentioned this pull request Nov 14, 2024
Merged
@artonge
Copy link
Contributor

artonge commented Nov 19, 2024

@thlehmann-ionos can you rebase? Then it should be good to merge :)

@thlehmann-ionos
feat(Mailer): implement caching
46d8a73 
Currently $this->instance is never set, so the code is no-op. This
brings back caching of the instance.

Caching broke with

   be7db15

   Swift to \Swift_Mailer as abstraction

Signed-off-by: Thomas Lehmann <t.lehmann@strato.de>
@thlehmann-ionos
chore(Mailer): remove no-op local variable initilization
40211f3 
The IDE hinted the value is immediately overwritten.

Signed-off-by: Thomas Lehmann <t.lehmann@strato.de>
@thlehmann-ionos
feat(Mailer): add "null" SMTP transport mode
e4c013d 
== Goal

Allow disabling mail delivery altogether.

== Usecase

If mails ought to be send by other means than rendering messages from
templates and sending them via SMTP-like protocols.

Example: listening to specific Nextcloud events and pass parameters to
a centralized (i.e. REST-based) API that sends e-mails.

Signed-off-by: Thomas Lehmann <t.lehmann@strato.de>
@thlehmann-ionos
feat(Mailer): hide admin settings for "null" transport
3f6c969 
When the mail transport is configured as null transport, the
configuration UI would not work.

== Background

The null transport is meant for situations where operators
implement mail delivery via custom mechanisms like REST APIs.

Signed-off-by: Thomas Lehmann <t.lehmann@strato.de>
@artonge artonge merged commit 73f3b9b into nextcloud:master Nov 19, 2024
359 of 362 checks passed
@welcome Welcome
Copy link

welcome bot commented Nov 19, 2024

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@kesselb kesselb mentioned this pull request Nov 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@solracsf solracsf solracsf left review comments

@printminion-co printminion-co printminion-co approved these changes

@come-nc come-nc come-nc approved these changes

@artonge artonge artonge approved these changes

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement feature: emails feedback-requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@thlehmann-ionos @artonge @solracsf @come-nc @printminion-co @joshtrichards