OrchardCMS / Orchard

Orchard is a free, open source, community-focused Content Management System built on the ASP.NET MVC platform.
https://orchardproject.net
BSD 3-Clause "New" or "Revised" License
2.37k stars 1.12k forks source link

Using MailKit instead of SmtpClient (ORCH-267) #8707

Closed dministro closed 1 year ago

dministro commented 1 year ago

fixes #8691

Jira issue

BenedekFarkas commented 1 year ago

@dministro I'll check the build error, I have an idea why is that.

dministro commented 1 year ago

@dministro I'll check the build error, I have an idea why is that.

Thank you

dministro commented 1 year ago

@BenedekFarkas I can't repro this locally. Tried with both the command line and VS.

We could add System.Net.Http as a direct reference to Orchard.Email and make it private just for a test.

 src/Orchard.Web/Modules/Orchard.Email/Orchard.Email.csproj | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/Orchard.Web/Modules/Orchard.Email/Orchard.Email.csproj b/src/Orchard.Web/Modules/Orchard.Email/Orchard.Email.csproj
index 975fba730..28ad95825 100644
--- a/src/Orchard.Web/Modules/Orchard.Email/Orchard.Email.csproj
+++ b/src/Orchard.Web/Modules/Orchard.Email/Orchard.Email.csproj
@@ -83,6 +83,9 @@
     </Reference>
     <Reference Include="System.Data" />
     <Reference Include="System.Data.DataSetExtensions" />
+    <Reference Include="System.Net.Http">
+      <Private>True</Private>
+    </Reference>
     <Reference Include="System.Security" />
     <Reference Include="System.Web.ApplicationServices" />
     <Reference Include="System.Web.DynamicData" />
BenedekFarkas commented 1 year ago

@BenedekFarkas I can't repro this locally. Tried with both the command line and VS.

We could add System.Net.Http as a direct reference to Orchard.Email and make it private just for a test.

 src/Orchard.Web/Modules/Orchard.Email/Orchard.Email.csproj | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/Orchard.Web/Modules/Orchard.Email/Orchard.Email.csproj b/src/Orchard.Web/Modules/Orchard.Email/Orchard.Email.csproj
index 975fba730..28ad95825 100644
--- a/src/Orchard.Web/Modules/Orchard.Email/Orchard.Email.csproj
+++ b/src/Orchard.Web/Modules/Orchard.Email/Orchard.Email.csproj
@@ -83,6 +83,9 @@
     </Reference>
     <Reference Include="System.Data" />
     <Reference Include="System.Data.DataSetExtensions" />
+    <Reference Include="System.Net.Http">
+      <Private>True</Private>
+    </Reference>
     <Reference Include="System.Security" />
     <Reference Include="System.Web.ApplicationServices" />
     <Reference Include="System.Web.DynamicData" />

Yeah, I don't have a repro either, which is really annoying. It probably comes down to some combination of SDKs installed. I'll play with the references a bit more - BTW this type of warning came up before: https://github.com/OrchardCMS/Orchard/pull/8687/commits/d266db61de22a30b440139e185b5b4884912e883#r1207070731

Edit: This is fixed now.

dministro commented 1 year ago

Yeah, I don't have a repro either, which is really annoying. It probably comes down to some combination of SDKs installed. I'll play with the references a bit more - BTW this type of warning came up before: d266db6#r1207070731

Edit: This is fixed now.

Thank you for your support @BenedekFarkas .

BenedekFarkas commented 1 year ago

Here are the test cases (8 in total) I checked (including my latest changes):

  1. Setup on issue branch, configure for localhost SMTP.
  2. Setup on issue branch, configure for SendGrid account.
  3. Setup on 1.10.x, configure for localhost SMTP, upgrade to issue branch.
  4. Setup on 1.10.x, configure for SendGrid account, upgrade to issue branch.

Note that all the test cases work with either value of AutoSelectEncryption:

  1. True: Port-based detection works fine.
  2. False: EncryptionMethod has to be set to None for localhost and SSL for SendGrid.

I think it's ready to be merged! @AndreaPiovanelliLaser?

AndreaPiovanelli commented 1 year ago

Here are the test cases (8 in total) I checked (including my latest changes):

  1. Setup on issue branch, configure for localhost SMTP.
  2. Setup on issue branch, configure for SendGrid account.
  3. Setup on 1.10.x, configure for localhost SMTP, upgrade to issue branch.
  4. Setup on 1.10.x, configure for SendGrid account, upgrade to issue branch.

Note that all the test cases work with either value of AutoSelectEncryption:

  1. True: Port-based detection works fine.
  2. False: EncryptionMethod has to be set to None for localhost and SSL for SendGrid.

I think it's ready to be merged! @AndreaPiovanelliLaser?

Sorry for the late reply. I think using infoset works without breaking compatibility with previously saved mail settings.