Feature/swiftmailer 6 compat #237
Conversation
|
FWIW, I'd welcome support for Swiftmailer 6, specially if this also maintains support for v5. I would be fine with not using the submodule approach, if that's the only roadblock. |
|
I came to another, way easier approach without breaking any backward compatibility within the code. Maybe we can also keep the submodule and if the user is using the submodule, it will simply install ~5.2 and using the composer it will install ~5.2 or ~6.0 depending on the available php version. Let me do some tests for the changes and the submodule idea. |
…on via composer will not install submodules. Yeay!
|
Yes, it's working! With the current change we are backward compatible and we even can keep the submodule approach! To have the tests working again, we would need #236 first. After that, we can check the status of the mailer tests. |
There was a problem hiding this comment.
Hello @thirsch ,
Thank you to propose this compatibility.
Look at line comments where I expose the minimum change to add SwiftMailer version 6 compatibility.
No need to introduce sfMailerBase.
Also the submodule should be left untouched.
| * | ||
| * @return int|false The number of sent emails | ||
| */ | ||
| public function send(Swift_Mime_Message $message, &$failedRecipients = null) |
There was a problem hiding this comment.
Thanks, that was way to simple... ;-) I wasn't aware, that you can remove the typehint and be less specific while extending from another class.
There was a problem hiding this comment.
You rise me a doubt. But for sure the test have always right, and the result may depend on the PHP version.
There was a problem hiding this comment.
Yes it's working. On a new codebase I would not do prefer it that way, but on the legacy stuff it's sufficient to get the compat for swiftmailer 6 :)
There was a problem hiding this comment.
Yes, however, even on new codebase as the type constraint is respected with the call on the parent and the transport.
There is a goal to avoid using language version specific code as much as possible to avoid useless complexity.
But that's another story depending of the codebase. I give you me point of view.
| @@ -5,7 +5,7 @@ | |||
| "license": "MIT", | |||
| "require": { | |||
| "php" : ">=5.3.0", | |||
| "swiftmailer/swiftmailer": "~5.2" | |||
| "swiftmailer/swiftmailer": "~5.2|~6.0" | |||
There was a problem hiding this comment.
The dependency constraint should be more strict like that => ^6.0.
There was a problem hiding this comment.
Thanks, could you please explain the difference to me? From what I understood on https://getcomposer.org/doc/articles/versions.md#next-significant-release-operators, it will be the same and only differ, if I would introduce the third digit.
There was a problem hiding this comment.
The ~ include 7.0@dev but not with ^.
There was a problem hiding this comment.
The
~include7.0@devbut not with^.
Hm... Really?
I think they explicitly mention that it will not install versions having major number different from 6 @alquerci
Note: Although
2.0-beta.1is strictly before2.0, a version constraint like~1.2would not install it. As said above~1.2only means the.2can change but the1.part is fixed.
https://getcomposer.org/doc/articles/versions.md#tilde-version-range-
There was a problem hiding this comment.
Hey, indeed I wasn't understand in details the difference.
Now yes.
The ^ operator behaves very similarly, but it sticks closer to semantic versioning, and will always allow non-breaking updates.
https://getcomposer.org/doc/articles/versions.md#caret-version-range-
#learningAllDays
Thanks @alquerci, I'll close this pr and open a new one with only the relevant changes and without removing the submodule. |
We have added support for swiftmailer 6 by providing a second version of sfMailer that is compatible with swiftmailer 6. As we are installing symfony via composer, it's no issue for us to not using the submodule version. But is that ok for the upstream too?
Updating a previous symfony1 installation might require to change any static access from
sfMailer::tosfMailerBase::. But before adjusting the doc etc. I wanted to check with you if this branch is of interest for you.Solves #161