Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Mime messages#30416

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromfabpot:mime-messages
Mar 2, 2019
Merged

Mime messages#30416

fabpot merged 1 commit intosymfony:masterfromfabpot:mime-messages
Mar 2, 2019

Conversation

@fabpot
Copy link
Member

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsmany on Swiftmailer
LicenseMIT
Doc PRupcoming

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:

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

smoench, zmitic, javiereguiluz, deguif, onEXHovia, iluuu1994, Taluu, lyrixx, yceruto, dbrumann, and 8 more reacted with thumbs up emojisstok, javiereguiluz, deguif, nesk, plozmun, iluuu1994, Taluu, lyrixx, yceruto, andreybolonin, and weaverryan reacted with hooray emojinesk, OskarStark, iluuu1994, lyrixx, yceruto, dsentker, ker0x, andreybolonin, weaverryan, javiereguiluz, and 2 more reacted with heart emoji
// % and / are CPU intensive, so, maybe find a better way
$ignored =$strlen %$this->width;
$ignoredChars =$ignored ?substr($string, -$ignored) :'';
$currentMap =$this->width;

Choose a reason for hiding this comment

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

seems you have a type error here,currentMap must be anarray but it is assigned with anint

sstok and Taluu reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@OskarStarkOskarStark left a comment

Choose a reason for hiding this comment

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

Thank you for this great new component ❤️

Copy link
Contributor

@sstoksstok left a comment

Choose a reason for hiding this comment

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

Some minor suggestions, so far.

Copy link
Contributor

@TaluuTaluu left a comment

Choose a reason for hiding this comment

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

Had a quick look but overall 👍

sstok reacted with thumbs up emoji
@fabpotfabpotforce-pushed themime-messages branch 6 times, most recently froma5f485a to96878a7CompareMarch 2, 2019 09:25
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

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 :)

$this->twig =$twig;
$this->context =$context;
if (class_exists(HtmlConverter::class)) {
$this->converter =newHtmlConverter([

Choose a reason for hiding this comment

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

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?)

Copy link
Member

Choose a reason for hiding this comment

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

That seems like a good place to start - throw an exception when/if it's used (inconvertHtmlToText()). 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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

*
* @experimental in 4.3
*/
finalclass Renderer

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

sstok, lyrixx, HeahDude, na-ji, BitScout, and derrabus reacted with laugh emoji

Choose a reason for hiding this comment

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

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 usew3m for that job and it does it nicely)

sstok reacted with thumbs up emoji
@fabpotfabpotforce-pushed themime-messages branch 4 times, most recently fromc741006 toed0858cCompareMarch 2, 2019 11:49
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

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

@fabpotfabpot merged commitee787d1 intosymfony:masterMar 2, 2019
fabpot added a commit that referenced this pull requestMar 2, 2019
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        | upcomingAs 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 casesswiftmailer/swiftmailer#434swiftmailer/swiftmailer#775swiftmailer/swiftmailer#946swiftmailer/swiftmailer#615swiftmailer/swiftmailer#184swiftmailer/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 nowI'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-mimeCommits-------ee787d1 [Mime] added classes for generating MIME messages
@fabpotfabpot mentioned this pull requestMay 9, 2019
@fabpotfabpot deleted the mime-messages branchSeptember 7, 2019 12:13
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz left review comments

@weaverryanweaverryanweaverryan left review comments

@stofstofstof left review comments

@OskarStarkOskarStarkOskarStark requested changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+6 more reviewers

@TaluuTaluuTaluu left review comments

@yetheeyetheeyethee left review comments

@BaptouuuuBaptouuuuBaptouuuu left review comments

@deguifdeguifdeguif left review comments

@sstoksstoksstok requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

12 participants

@fabpot@javiereguiluz@weaverryan@Taluu@nicolas-grekas@stof@yethee@Baptouuuu@sstok@deguif@OskarStark@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp