Skip to content
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

TASK: Update to use swiftmailer/swiftmailer version 6 #14

Merged
merged 6 commits into from
Dec 26, 2018

Conversation

kdambekalns
Copy link
Member

No description provided.

@kdambekalns kdambekalns self-assigned this Dec 13, 2018
@kdambekalns
Copy link
Member Author

💀 This has not been tested yet…

@kdambekalns
Copy link
Member Author

✅ Works for me with LoggingTransport and MboxTransport now.

Copy link
Member

@daniellienert daniellienert left a comment

Choose a reason for hiding this comment

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

Looks good by reading. Added a comment about the RTD page.

@@ -53,17 +53,17 @@ Here is an example:

.. code-block:: php

$mail = new \Neos\SwiftMailer\Message();
$mail = new \Neos\SwiftMailer\Message();
Copy link
Member

Choose a reason for hiding this comment

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

TIL, we have a separate RTD page for this package (https://swiftmailer-for-flow.readthedocs.io/en/latest/#installation) - is that really necessary? I would recommend moving the .rst as .md to the root and close that RTD page.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, having all documentation for "our packages" in the same format seems like a good idea. It can surely be made more discoverable, by the (upcoming) https://docs.neos.io/cms/references as well as a README (which I just added) that mentions the documentation.

@daniellienert daniellienert merged commit e732f46 into neos:master Dec 26, 2018
@daniellienert
Copy link
Member

daniellienert commented Dec 26, 2018

Uuuh, what have I done :/
I tested it post-merge thoroughly. And the default Swift transport types like Swift_SmtpTransport are not working anymore as they are not implementing the Neos\SwiftMailer\TransportInterface

@kdambekalns why did you add the interface checks: https://github.com/neos/swiftmailer/pull/14/files#diff-e2bee996df2ca8c9e00358deaa5830efR47 and https://github.com/neos/swiftmailer/pull/14/files#diff-e2bee996df2ca8c9e00358deaa5830efR57. Just to have the return type annotation?

If I remove the checks and the return type annotation, it works again.

@kdambekalns kdambekalns deleted the task/swiftmailer-6 branch December 26, 2018 17:18
@kdambekalns
Copy link
Member Author

Yes, added the return type and then added the checks. But obviously that doesn't make sense, since that interface is our own…

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

Successfully merging this pull request may close these issues.

2 participants