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

[Twig] Remove TemplatedEmail::template() #30853

Merged
merged 1 commit into from Apr 3, 2019

Conversation

Projects
None yet
6 participants
@fabpot
Copy link
Member

commented Apr 3, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

I propose to remove TemplatedEmail::template() for several reasons:

  • There is no real benefit over using textTemplate and htmlTemplate (ok, you only have one template instead of two... but the text template can only be automatically created based on the HTML one, so...);

  • It means having more than one way to do the same thing (do I set the subject on the object directly or in the template for instance);

  • A major drawback that is not easy to spot: the template is HTML, so the subject and text block must be carefully crafted to avoid avoid HTML escaping.

@javiereguiluz
Copy link
Member

left a comment

I agree.

While documenting the Mime component (pending PR here: symfony/symfony-docs#11280) I also found a bit confusing to have multiple ways of doing the same thing.

Also, using pure Twig code to define all email parts (such as the priority, subject, from, to, etc.) looks appealing at first but it generates complex and not very readable code in real-world applications. Better use PHP to create the message and (optionally) Twig to render its contents.

@stof

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

👍 for me.

My current system in the Incenteev codebase looks like this template way (minus the config block), and usage makes me wish to rewrite that to use separate templates (except I won't do it until I can migrate to symfony/mailer instead, as doing the rewrite now would be a waste of time).

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 3, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

(but see fabbot ;) )

@fabpot fabpot force-pushed the fabpot:template-mime-simplification branch from 1775552 to 5e61b75 Apr 3, 2019

@fabpot fabpot merged commit 5e61b75 into symfony:master Apr 3, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 3, 2019

feature #30853 [Twig] Remove TemplatedEmail::template() (fabpot)
This PR was merged into the 4.3-dev branch.

Discussion
----------

[Twig] Remove TemplatedEmail::template()

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

I propose to remove `TemplatedEmail::template()` for several reasons:

 * There is no real benefit over using `textTemplate` and `htmlTemplate` (ok, you only have one template instead of two... but the text template can only be automatically created based on the HTML one, so...);

 * It means having more than one way to do the same thing (do I set the subject on the object directly or in the template for instance);

 * A major drawback that is not easy to spot: the template is HTML, so the `subject` and `text` block must be carefully crafted to avoid avoid HTML escaping.

Commits
-------

5e61b75 [Twig] removed TemplatedEmail::template()

vincenttouzet added a commit to vincenttouzet/symfony that referenced this pull request Apr 3, 2019

feature symfony#30853 [Twig] Remove TemplatedEmail::template() (fabpot)
This PR was merged into the 4.3-dev branch.

Discussion
----------

[Twig] Remove TemplatedEmail::template()

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

I propose to remove `TemplatedEmail::template()` for several reasons:

 * There is no real benefit over using `textTemplate` and `htmlTemplate` (ok, you only have one template instead of two... but the text template can only be automatically created based on the HTML one, so...);

 * It means having more than one way to do the same thing (do I set the subject on the object directly or in the template for instance);

 * A major drawback that is not easy to spot: the template is HTML, so the `subject` and `text` block must be carefully crafted to avoid avoid HTML escaping.

Commits
-------

5e61b75 [Twig] removed TemplatedEmail::template()
@lyrixx

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

  • There is no real benefit over using textTemplate and htmlTemplate (ok, you only have one template instead of two... but the text template can only be automatically created based on the HTML one, so...);

This is not really true about link :
<a href="XXX"> click here</a> will result in click here without the link :/

@fabpot

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@lyrixx Not if you have Markdown installed :)

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot deleted the fabpot:template-mime-simplification branch May 8, 2019

@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.