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

Migrate to Symfony Mailer #30349

Merged
merged 2 commits into from
Feb 2, 2023
Merged

Migrate to Symfony Mailer #30349

merged 2 commits into from
Feb 2, 2023

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Dec 20, 2021 •
edited by miaulalala
Loading

Signed-off-by: Côme Chilliet come.chilliet@nextcloud.com

Fixes #29731

TODO:

As a follow up we could get rid of any RFC validations we're doing with the egulias package as it's the same validator used by Symfony in the Address::__construct

Done
  • lib/private/Mail/Message.php: * SwiftMailer does currently not work with IDN domains, this function therefore converts the domains
  • lib/private/Mail/Message.php: * FIXME: Remove this once SwiftMailer supports IDN
  • lib/private/Mail/Mailer.php: * SwiftMailer does currently not work with IDN domains, this function therefore converts the domains
  • lib/private/Mail/Mailer.php: * FIXME: Remove this once SwiftMailer supports IDN
  • lib/private/Mail/Mailer.php: return new Attachment(new \Swift_Attachment($data, $filename, $contentType));
  • lib/private/Mail/Mailer.php: return new Attachment(\Swift_Attachment::fromPath($path, $contentType));
  • lib/private/Mail/Mailer.php: $mailLogger = new \Swift_Plugins_Loggers_ArrayLogger();
  • lib/private/Mail/Mailer.php: $mailer->registerPlugin(new \Swift_Plugins_LoggerPlugin($mailLogger));
  • lib/private/Mail/Mailer.php: $mailer->send($message->getSwiftMessage(), $failedRecipients);
  • lib/private/Mail/Message.php: * Class Message provides a wrapper around SwiftMail
  • lib/private/Mail/Message.php: $this->swiftMessage->attach($attachment->getSwiftAttachment());
  • lib/private/Mail/Message.php: * Get's the underlying SwiftMessage
  • lib/private/Mail/Message.php: public function setSwiftMessage(\Symfony\Component\Mime\Email $swiftMessage): void {
  • lib/private/Mail/Message.php: * Get's the underlying SwiftMessage
  • lib/private/Mail/Message.php: public function getSwiftMessage(): \Symfony\Component\Mime\Email {
  • lib/private/Mail/Attachment.php: /** @var \Swift_Mime_Attachment */
  • lib/private/Mail/Attachment.php: public function __construct(\Swift_Mime_Attachment $attachment) {
  • lib/private/Mail/Attachment.php: * @return \Swift_Mime_Attachment
  • lib/private/Mail/Attachment.php: public function getSwiftAttachment(): \Swift_Mime_Attachment {
Nothing to do with SwiftMailer (== Done)
  • lib/private/SystemConfig.php: // Legacy Swift (Remove objectstore credentials #17696 (comment))
  • lib/private/SystemConfig.php: // Swift v2
  • lib/private/SystemConfig.php: // Swift v3
  • lib/private/SystemConfig.php: // Swift v2
  • lib/private/SystemConfig.php: // Swift v3

@come-nc come-nc force-pushed the enhancement/migrate_to_symfony_mailer branch from b6f89f8 to 43836fe Compare December 20, 2021 10:41
@come-nc

This comment was marked as duplicate.

Sign in to view
@come-nc come-nc marked this pull request as draft December 20, 2021 10:43
@come-nc

This comment was marked as outdated.

Sign in to view
@nextcloud-command nextcloud-command force-pushed the enhancement/migrate_to_symfony_mailer branch from 43836fe to bdf3360 Compare September 30, 2022 14:37
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 30, 2022
View reviewed changes
lib/private/Mail/Mailer.php Fixed Show fixed Hide fixed
lib/private/Mail/Mailer.php Fixed Show fixed Hide fixed
lib/private/Mail/Mailer.php Fixed Show fixed Hide fixed
lib/private/Mail/Mailer.php Fixed Show fixed Hide fixed
lib/private/Mail/Mailer.php Fixed Show fixed Hide fixed
lib/private/Mail/Message.php Fixed Show fixed Hide fixed
lib/private/Mail/Message.php Fixed Show fixed Hide fixed
lib/private/Mail/Message.php Fixed Show fixed Hide fixed
lib/private/Mail/Message.php Fixed Show fixed Hide fixed
lib/private/Mail/Mailer.php Fixed Show fixed Hide fixed
@come-nc come-nc mentioned this pull request Sep 30, 2022
Merged
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 30, 2022
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2022

Possible performance regression detected

Show Output 
1 queries added

= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
≠ /remote.php/dav/files/test/new_file.txt with 1 queries added
  - UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3, "size" = CASE WHEN "size" > '-1' THEN MAX("size" + :dcValue4, :dcValue5) ELSE "size" END WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
  + UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3 WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
  + UPDATE "oc_filecache" SET "size" = MAX("size" + :dcValue1, :dcValue2) WHERE ("storage" = :dcValue3) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1')) AND ("size" > '-1')
= /remote.php/dav/files/test/new_file.txt

@come-nc come-nc changed the title Migrate to Symfony Mailer using rector Migrate to Symfony Mailer Oct 4, 2022
@come-nc
Copy link
Contributor Author

come-nc commented Oct 4, 2022 •
edited
Loading

Known missing things:

  • setStreamOptions support. This seems not possible with symfony/mailer. Expose Mailer stream options (setStreamOptions) in configuration #12702 seems to suggest that it is mainly used to accept self-signed certificate, maybe we can look into supporting that one usecase if possible.
  • timeout setting is currently commented out. Probably possible to fix that but I did not find yet where.

@come-nc
Copy link
Contributor Author

come-nc commented Oct 5, 2022

Stream options and timeout support are back. Just have to use getStream on the transport and work directly on the stream.

So, tests are green, but I did not actually test that this is able to send any email. Anyone has a setup to test this? Does the CI test it?

@come-nc come-nc marked this pull request as ready for review October 5, 2022 07:55
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

Possible performance regression detected

Show Output 
1 queries added

= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
≠ /remote.php/dav/files/test/new_file.txt with 1 queries added
  - UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3, "size" = CASE WHEN "size" > '-1' THEN MAX("size" + :dcValue4, :dcValue5) ELSE "size" END WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
  + UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3 WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
  + UPDATE "oc_filecache" SET "size" = MAX("size" + :dcValue1, :dcValue2) WHERE ("storage" = :dcValue3) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1')) AND ("size" > '-1')
= /remote.php/dav/files/test/new_file.txt

@come-nc

This comment was marked as outdated.

Sign in to view
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

Possible performance regression detected

Show Output 
1 queries added

= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
≠ /remote.php/dav/files/test/new_file.txt with 1 queries added
  - UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3, "size" = CASE WHEN "size" > '-1' THEN MAX("size" + :dcValue4, :dcValue5) ELSE "size" END WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
  + UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3 WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
  + UPDATE "oc_filecache" SET "size" = MAX("size" + :dcValue1, :dcValue2) WHERE ("storage" = :dcValue3) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1')) AND ("size" > '-1')
= /remote.php/dav/files/test/new_file.txt

@come-nc

This comment was marked as outdated.

Sign in to view
@come-nc

This comment was marked as outdated.

Sign in to view
@come-nc

This comment was marked as outdated.

Sign in to view
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

Possible performance regression detected

Show Output 
1 queries added

= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
≠ /remote.php/dav/files/test/new_file.txt with 1 queries added
  - UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3, "size" = CASE WHEN "size" > '-1' THEN MAX("size" + :dcValue4, :dcValue5) ELSE "size" END WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
  + UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3 WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
  + UPDATE "oc_filecache" SET "size" = MAX("size" + :dcValue1, :dcValue2) WHERE ("storage" = :dcValue3) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1')) AND ("size" > '-1')
= /remote.php/dav/files/test/new_file.txt

2 similar comments
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

Possible performance regression detected

Show Output 
1 queries added

= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
≠ /remote.php/dav/files/test/new_file.txt with 1 queries added
  - UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3, "size" = CASE WHEN "size" > '-1' THEN MAX("size" + :dcValue4, :dcValue5) ELSE "size" END WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
  + UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3 WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
  + UPDATE "oc_filecache" SET "size" = MAX("size" + :dcValue1, :dcValue2) WHERE ("storage" = :dcValue3) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1')) AND ("size" > '-1')
= /remote.php/dav/files/test/new_file.txt

@github-actions
Copy link
Contributor

Possible performance regression detected

Show Output 
1 queries added

= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
≠ /remote.php/dav/files/test/new_file.txt with 1 queries added
  - UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3, "size" = CASE WHEN "size" > '-1' THEN MAX("size" + :dcValue4, :dcValue5) ELSE "size" END WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
  + UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3 WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
  + UPDATE "oc_filecache" SET "size" = MAX("size" + :dcValue1, :dcValue2) WHERE ("storage" = :dcValue3) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1')) AND ("size" > '-1')
= /remote.php/dav/files/test/new_file.txt

@come-nc come-nc mentioned this pull request Nov 15, 2022
Closed
19 tasks
come-nc
come-nc commented Nov 21, 2022
View reviewed changes
lib/private/Mail/Mailer.php Outdated Show resolved Hide resolved
@come-nc

This comment was marked as off-topic.

Sign in to view
@szaimen szaimen added the 2. developing Work in progress label Nov 21, 2022
@kesselb kesselb mentioned this pull request Jan 27, 2023
Merged
9 tasks
@miaulalala miaulalala force-pushed the enhancement/migrate_to_symfony_mailer branch from eae8353 to f8ee45c Compare January 30, 2023 13:19
come-nc
come-nc commented Jan 30, 2023
View reviewed changes
lib/private/Mail/Mailer.php Outdated Show resolved Hide resolved
@come-nc
Copy link
Contributor Author

come-nc commented Jan 30, 2023

(I cannot approve as author of the PR, but I hereby approve what @miaulalala did 👍 )

@come-nc come-nc force-pushed the enhancement/migrate_to_symfony_mailer branch from db8c3f6 to f8ee45c Compare January 30, 2023 15:21
@miaulalala
Copy link
Contributor

documentation ticket here: nextcloud/documentation#9600

@miaulalala miaulalala force-pushed the enhancement/migrate_to_symfony_mailer branch from f8ee45c to 90e9c9b Compare January 30, 2023 18:57
@miaulalala miaulalala added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 30, 2023
@ChristophWurst
Copy link
Member

There were 10 errors:

@miaulalala miaulalala force-pushed the enhancement/migrate_to_symfony_mailer branch 2 times, most recently from 7554b63 to 65709f4 Compare February 1, 2023 15:48
ChristophWurst
ChristophWurst reviewed Feb 1, 2023
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks sane otherwise

apps/settings/lib/Controller/MailSettingsController.php Outdated Show resolved Hide resolved
config/config.sample.php Outdated Show resolved Hide resolved
lib/public/Mail/IMailer.php Outdated Show resolved Hide resolved
lib/public/Mail/IMessage.php Outdated Show resolved Hide resolved
lib/public/Mail/IMessage.php Outdated Show resolved Hide resolved
@blizzz blizzz mentioned this pull request Feb 2, 2023
Merged
ChristophWurst
ChristophWurst requested changes Feb 2, 2023
View reviewed changes
lib/public/Mail/IMailer.php Outdated Show resolved Hide resolved
@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 2, 2023
come-nc and others added 2 commits February 2, 2023 10:30
@come-nc @blizzz
Migrate to Symfony Mailer
dde5c46 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@blizzz
remove depraction of validateEmailAddress
e123d27 
for there are legit use cases to validate an email address without
sending there to

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the enhancement/migrate_to_symfony_mailer branch from 2c0b560 to e123d27 Compare February 2, 2023 09:30
@blizzz blizzz merged commit bbd3e2b into master Feb 2, 2023
@blizzz blizzz deleted the enhancement/migrate_to_symfony_mailer branch February 2, 2023 10:42
@DaphneMuller
Copy link

hello @come-nc ,
Thank you for your work on this pull request! This ticket seems to have the tag 'missing documentation', is there any chance you could clarify what documentation is missing? Is this for admins or for app developers?

@miaulalala
Copy link
Contributor

@DaphneMuller nextcloud/documentation#9600 is the documentation ticket

@kesselb kesselb removed the pending documentation This pull request needs an associated documentation update label Jun 7, 2023
@joshtrichards
Copy link
Member

Indirectly completes #29333

@joshtrichards joshtrichards mentioned this pull request Jul 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miaulalala miaulalala miaulalala approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@tcitworld tcitworld Awaiting requested review from tcitworld tcitworld is a code owner

Assignees

@miaulalala miaulalala

Labels
4. to release Ready to be released and/or waiting for tests to finish technical debt
Projects
None yet
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.

Migrate from Swiftmailer to Symfony Mailer
10 participants
@come-nc @ChristophWurst @juliushaertl @miaulalala @DaphneMuller @joshtrichards @tcitworld @blizzz @kesselb @szaimen