chore: migrate to symfony/mailer #41009
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
DeepDiver1975
force-pushed
the
chore/replace-swiftmailer-with-symfony-mailer
branch
2 times, most recently
from
4d2c394
to
ea838aa
Compare
DeepDiver1975
force-pushed
the
chore/replace-swiftmailer-with-symfony-mailer
branch
2 times, most recently
from
bc46a27
to
07a47dd
Compare
|
@mmattel we have dropped one config setting - also the admin ui looks a bit different now ..... |
Same as written in the code comment above, this is a release notes thing and needs mentioning there --> @pako81 |
Can one drop me a screenshot we can use for docs into: owncloud/docs-server#1137 ? and clarify for which version this change is targeted? |
DeepDiver1975
force-pushed
the
chore/replace-swiftmailer-with-symfony-mailer
branch
2 times, most recently
from
026c2ac
to
4d7271e
Compare
|
@individual-it @phil-davis can I ask you for a quick look - THX a lot: |
|
The failing case is that |
DeepDiver1975
force-pushed
the
chore/replace-swiftmailer-with-symfony-mailer
branch
from
b7c4f0c
to
4bfd453
Compare
|
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Looks good, and tests all pass.
I made a few notes mainly about backward-compatibility. The way the ownCloud Mailer etc classes are currently structured, they expose parts of SwiftMailer, so to get rid of SwiftMailer there have to be changes to public method interfaces.
What do we do about that for oC core major version 10 with PHP 7.4?
(for PHP 8.2 it is easier - we would bump the major version of oC core to 11)
| @@ -96,7 +95,6 @@ public function setMailSettings( | |||
| $mail_smtpmode, | |||
| $mail_smtpsecure, | |||
| $mail_smtphost, | |||
| $mail_smtpauthtype, | |||
There was a problem hiding this comment.
what do we do about things like this?
In apps that we manage, and that happen to call setMailSettings, we can adjust the app and coordinate the release.
For "unknown" apps, this would be a BC break.
Or am I thinking wrong?
| ]); | ||
|
|
||
| return $failedRecipients; | ||
| return []; |
There was a problem hiding this comment.
Are we keeping the "return array" behavior for backward compatibility?
This method now always either:
- returns an empty array; or
- throws an exception
So it could just "return void", indicating success. But maybe there are other callers that are expecting the old behavior, and think that success is indicated by an empty array returned.
| */ | ||
| public function getSwiftMessage() { | ||
| return $this->swiftMessage; | ||
| public function getMessage(): Email { |
There was a problem hiding this comment.
and the method name changes here - so definitely not backward-compatible.
There was a problem hiding this comment.
Due to this the original design did not have an iMessage.
Never the less: yes - this breaks API.
|
This can wait for merge until release-10.13.2 is out of the way, so we have time to discuss what happens for releases after doing this change for the PHP 7.4 code base - will it be "the usual" 10.14.0 minor version bump, and we work out how to address backward-compatibility issues (for example, that any real backward-compatibility issues can be handled OK for any apps that are affected). Or would this cause a major version bump, forcing an 11.0.0 release that supports PHP 7.4... |
|
From my understanding oc11 is php8.2 |
DeepDiver1975
force-pushed
the
chore/replace-swiftmailer-with-symfony-mailer
branch
3 times, most recently
from
12e9080
to
ad2bb6d
Compare
|
Kudos, SonarCloud Quality Gate passed! |
|
I suppose this is blocked/stalled for now. Can't easily be part of 10.14 because of backward-compatibility issues that are not easily avoided. |
|
I planned to do this as part of #40981 🤷 |
Description
Related Issue
QA Scenarios
Scenarios
Mail Setups to test
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: