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

Mime messages #30416

Merged
merged 1 commit into from Mar 2, 2019

Conversation

@fabpot
Copy link
Member

commented Mar 1, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets many on Swiftmailer
License MIT
Doc PR upcoming

As announced today at SymfonyLive Lille, here is the new MIME component.

This PR is one step towards the new Symfony Mailer (announced at Symfony London). It started as a fork of Swiftmailer, but soon enough I rewrote almost everything to make it (hopefully) better and more flexible. I've removed all the complexity of Swiftmailer when it comes to multiparts for instance.

Some big differences with Swiftmailer:

  • Way less complexity (no crazy dependency injection when not needed, less interfaces, no cache)
  • Plain data object and no state (out are the observers for charset and encoding, in are POPO and serializable objects)
  • No magic regarding multipart management, but a nice wrapper for the most common use cases
    swiftmailer/swiftmailer#434
    swiftmailer/swiftmailer#775
    swiftmailer/swiftmailer#946
    swiftmailer/swiftmailer#615
    swiftmailer/swiftmailer#184
    swiftmailer/swiftmailer#56
    and probably many others
  • More Symfony-like
  • Messages are built on-demand and we do not mess up with your headers/body (Swiftmailer add headers and change yours, but here, we generate needed headers when converting the message as a string, they are not stored -- it means for instance that generating an Email twice will give you 2 different Date headers)
  • and probably more that I don't remember right now

I've also kept some nice features from Swiftmailer like support for any charset.

More information on the slides:

https://speakerdeck.com/fabpot/2-new-symfony-components-httpclient-and-mime

@fabpot fabpot force-pushed the fabpot:mime-messages branch 3 times, most recently from 36a06d0 to c9c55dc Mar 1, 2019

// % and / are CPU intensive, so, maybe find a better way
$ignored = $strlen % $this->width;
$ignoredChars = $ignored ? substr($string, -$ignored) : '';
$currentMap = $this->width;

This comment has been minimized.

Copy link
@Baptouuuu

Baptouuuu Mar 1, 2019

seems you have a type error here, currentMap must be an array but it is assigned with an int

This comment has been minimized.

Copy link
@fabpot

fabpot Mar 2, 2019

Author Member

removed the type hint for now, the code is quite messy, so it will need more thinking

@OskarStark
Copy link
Contributor

left a comment

Thank you for this great new component ❤️

@sstok
Copy link
Contributor

left a comment

Some minor suggestions, so far.

Show resolved Hide resolved src/Symfony/Bridge/Twig/Tests/Mime/RendererTest.php
Show resolved Hide resolved src/Symfony/Component/Mime/CharacterStream.php
Show resolved Hide resolved src/Symfony/Component/Mime/Email.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Email.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Encoder/Base64Encoder.php
Show resolved Hide resolved src/Symfony/Component/Mime/Header/AbstractHeader.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Header/AbstractHeader.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Header/AbstractHeader.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Header/Headers.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Header/Headers.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Header/ParameterizedHeader.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Message.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/MessageConverter.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Part/AbstractMultipartPart.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Part/AbstractMultipartPart.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Part/Multipart/RelatedPart.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Part/Multipart/RelatedPart.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Part/TextPart.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Part/TextPart.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Tests/EmailTest.php
@Taluu
Copy link
Contributor

left a comment

Had a quick look but overall 👍

@fabpot fabpot force-pushed the fabpot:mime-messages branch 6 times, most recently from a5f485a to 96878a7 Mar 2, 2019

@nicolas-grekas
Copy link
Member

left a comment

That's a lot of work!

Character readers embed lots of logic. Do we want to be a bit more strict and just reject invalid UTF-8? An alternative could be to consider invalid UTF-8 as CP1252 and set a flag telling this was declared as UTF-8 but didn't really match. But that's for later I suppose :)

Show resolved Hide resolved src/Symfony/Component/Mime/composer.json Outdated
$this->twig = $twig;
$this->context = $context;
if (class_exists(HtmlConverter::class)) {
$this->converter = new HtmlConverter([

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 2, 2019

Member

How could we provide autodiscovery for this?
A "suggest" entry in composer.json maybe, but that's barely visible.
I don't have a better idea. (making it a requirement with runtime suggestion for the package?)

This comment has been minimized.

Copy link
@weaverryan

weaverryan Mar 3, 2019

Member

That seems like a good place to start - throw an exception when/if it's used (in convertHtmlToText()). So, if you set the text & html body, you'll never need it and never get an exception about missing it. But if you do, we say:

Cannot automatically convert HTML body to a text body. Run "composer require league/html-to-markdown"
or set the text content explicitly.

This comment has been minimized.

Copy link
@fabpot

fabpot Mar 3, 2019

Author Member

I like your suggestion @weaverryan, can you create a PR for it?

Show resolved Hide resolved src/Symfony/Bridge/Twig/Mime/TemplatedEmail.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Encoder/Base64ContentEncoder.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Encoder/Base64Encoder.php Outdated
Show resolved Hide resolved src/Symfony/Component/Mime/Header/Headers.php Outdated
*
* @experimental in 4.3
*/
final class Renderer

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 2, 2019

Member

Where is the class used? Or where do we plan to use it?

This comment has been minimized.

Copy link
@fabpot

fabpot Mar 2, 2019

Author Member

I thought you were at my keynote, listening carefully :) Everything is explained in the presentation linked in the description of the PR.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 2, 2019

Member

my core point is related to the design of the class :)
if it's final and has no interface, then type-hinting for it means creating strong coupling, while it could be usefull to provide an alternative implementation (eg another html-to-txt - I used to use w3m for that job and it does it nicely)

@fabpot fabpot force-pushed the fabpot:mime-messages branch 4 times, most recently from c741006 to ed0858c Mar 2, 2019

@nicolas-grekas
Copy link
Member

left a comment

(I expect the deps=low failures to disappear after merge)

@fabpot fabpot force-pushed the fabpot:mime-messages branch from ed0858c to 9e92b1d Mar 2, 2019

@fabpot fabpot force-pushed the fabpot:mime-messages branch from 9e92b1d to 9e5be30 Mar 2, 2019

@fabpot fabpot force-pushed the fabpot:mime-messages branch from 9e5be30 to ee787d1 Mar 2, 2019

@fabpot fabpot merged commit ee787d1 into symfony:master Mar 2, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 2, 2019

feature #30416 Mime messages (fabpot)
This PR was merged into the 4.3-dev branch.

Discussion
----------

Mime messages

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | many on Swiftmailer
| License       | MIT
| Doc PR        | upcoming

As announced today at SymfonyLive Lille, here is the new MIME component.

This PR is one step towards the new Symfony Mailer (announced at Symfony London). It started as a fork of Swiftmailer, but soon enough I rewrote almost everything to make it (hopefully) better and more flexible. I've removed all the complexity of Swiftmailer when it comes to multiparts for instance.

Some big differences with Swiftmailer:

* Way less complexity (no crazy dependency injection when not needed, less interfaces, no cache)
* Plain data object and no state (out are the observers for charset and encoding, in are POPO and serializable objects)
* No magic regarding multipart management, but a nice wrapper for the most common use cases
  swiftmailer/swiftmailer#434
  swiftmailer/swiftmailer#775
  swiftmailer/swiftmailer#946
  swiftmailer/swiftmailer#615
  swiftmailer/swiftmailer#184
  swiftmailer/swiftmailer#56
  and probably many others
* More Symfony-like
* Messages are built on-demand and we do not mess up with your headers/body (Swiftmailer add headers and change yours, but here, we generate needed headers when converting the message as a string, they are not stored -- it means for instance that generating an Email twice will give you 2 different Date headers)
* and probably more that I don't remember right now

I've also kept some nice features from Swiftmailer like support for any charset.

More information on the slides:

https://speakerdeck.com/fabpot/2-new-symfony-components-httpclient-and-mime

Commits
-------

ee787d1 [Mime] added classes for generating MIME messages

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.