-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
chore: migrate to symfony/mailer #41009
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. |
4d2c394
to
ea838aa
Compare
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? |
026c2ac
to
4d7271e
Compare
@individual-it @phil-davis can I ask you for a quick look - THX a lot: |
The failing case is that |
b7c4f0c
to
4bfd453
Compare
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and the method name changes here - so definitely not backward-compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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 🤷 |
cleaning up - I think this is no longer of anybody's interest |
Description
Related Issue
QA Scenarios
Scenarios
Mail Setups to test
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: