Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Compat for Swiftmailer 6 but preserve compatibility with Swiftmailer 5. #240

Merged

Conversation

thirsch
Copy link
Collaborator

@thirsch thirsch commented Sep 9, 2020

This pr is the follow up for #237. Thanks for your review.

Copy link
Contributor

@alquerci alquerci left a comment

Choose a reason for hiding this comment

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

LGTM but let's not forgot to add tests.

For that I suggest to add one job with SwiftMailer version 6 on travis build.

Do you know enough about travis-ci to do that change or you need help?

PS: you can update the same PR by just push another commit to feature/swiftmailer-6-compat-2.

@thirsch
Copy link
Collaborator Author

thirsch commented Sep 10, 2020

LGTM but let's not forgot to add tests.

For that I suggest to add one job with SwiftMailer version 6 on travis build.

Do you know enough about travis-ci to do that change or you need help?

PS: you can update the same PR by just push another commit to feature/swiftmailer-6-compat-2.

Thanks for your re-review. I've not done a project on travis so far and help would be much appreciated. At the moment we are installing swiftmailer 5 if the tests are running on PHP < 7 and if we are running on PHP >= 7, it is already installing swiftmailer 6. Any specific point to test or would that be sufficient?

@alquerci
Copy link
Contributor

alquerci commented Sep 10, 2020

Any specific point to test or would that be sufficient?

That's sufficient.

To execute the current test for each swift version
https://github.com/FriendsOfSymfony1/symfony1/blob/6185ffc7f31bfe68d37690b4d3533a5ab69279dd/test/unit/mailer/sfMailerTest.php

help would be much appreciated

I will try to tackle this this week-end.

A trade-off here is to install swift version 6 on before install then revert the commit when travis build is done.
The next step will be to add a dedicated job.

@thirsch
Copy link
Collaborator Author

thirsch commented Sep 10, 2020

help would be much appreciated

I will try to tackle this this week-end.

Thanks

A trade-off here is to install swift version 6 on before install then revert the commit when travis build is done.
The next step will be to add a dedicated job.

At the moment composer install during the build process already installs swift version 5 on PHP < 7 and swift version 6 on PHP >= 7. Or do you plan do somehow force swift version 5 on PHP > 7 as an additional test for version 6?

@alquerci
Copy link
Contributor

At the moment composer install during the build process already installs swift version 5 on PHP < 7 and swift version 6 on PHP >= 7. Or do you plan do somehow force swift version 5 on PHP > 7 as an additional test for version 6?

Ha, got it now (refreshing my memory => because composer install the latest possible version).

In that case I think that's enough except there is a bug on lowest versions.
Using composer update --prefer-lowest => ✔️

Copy link
Contributor

@mentalstring mentalstring left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@thePanz thePanz merged commit d9a1684 into FriendsOfSymfony1:master Mar 24, 2021
@thePanz
Copy link
Member

thePanz commented Mar 24, 2021

Thank you @thirsch , merged

@thirsch thirsch deleted the feature/swiftmailer-6-compat-2 branch March 24, 2021 11:29
@thePanz
Copy link
Member

thePanz commented Jan 13, 2023

Found out that lib/plugins/sfDoctrinePlugin/lib/mailer/Swift_DoctrineSpool.class.php class still references to Swift_Mime_Message, which is not around if the application installs SwiftMailer v6.

Any hints @thirsch ?

@thirsch
Copy link
Collaborator Author

thirsch commented Jan 13, 2023

Good catch, @thePanz. We could do the same as in this pr by removing the type from the method signature and add both types to the phpdoc. I can create a pr for it, if you like.

@thePanz
Copy link
Member

thePanz commented Jan 13, 2023

Good catch, @thePanz. We could do the same as in this pr by removing the type from the method signature and add both types to the phpdoc. I can create a pr for it, if you like.

Yes, please go ahead 👍
the tricky part: the interface is from the SwiftMailer itself, IIRC :)

@thirsch
Copy link
Collaborator Author

thirsch commented Jan 13, 2023

Yes, please go ahead 👍 the tricky part: the interface is from the SwiftMailer itself, IIRC :)

Ok, got it. We might be able to fix it with the solution I had before just removing the type. The pr is still available, so please have a look at it: https://github.com/FriendsOfSymfony1/symfony1/pull/237/files#diff-ab78635a7285fe1eca9b5c7fad605d21d3f09ccd2737c0af6bbf7022f66f95a7

As I'm on vacation next week, it will take a few days to come up with a proposal.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants