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

[Mailer] Fix the possibility to set a From header from MessageListener #31774

Merged
merged 1 commit into from Jun 4, 2019

Conversation

Projects
None yet
6 participants
@fabpot
Copy link
Member

commented May 31, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31733
License MIT
Doc PR n/a
@@ -40,14 +37,7 @@ public function __construct(Address $sender, array $recipients)
public static function create(RawMessage $message): self

This comment has been minimized.

Copy link
@fabpot

fabpot May 31, 2019

Author Member

Not sure if we should keep this method.

@weaverryan

This comment has been minimized.

Copy link
Member

commented May 31, 2019

@fabpot I'm getting a new error:

An email must have a "From" header

I think it relates to the difference between "Sender" and "From" that I was mentioning (#31733 (comment)) - though I'm far from an expert on this stuff. Should the EnvelopeListener set both sender and from? Or just the from, since the sender is derived from that?

@fabpot

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

"From" is what you write on the letter, "Sender" is what you write on the envelope. If you don't provide an envelope, we create one for you based on what you wrote on the letter. So, you must always provide a "From" (or register a MessageListener to do it for you -- will be part of the semantic configuration when available of course).

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Jun 1, 2019

@fabpot fabpot force-pushed the fabpot:from-header-from-listener-fix branch from 146f260 to f4254e6 Jun 1, 2019

public function __construct(RawMessage $message)
{
if (!$message instanceof Message) {

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jun 1, 2019

Member

Wouldn't it be better to change the type-hint instead? If we want to support any other message in the future we can still change the type then.

This comment has been minimized.

Copy link
@fabpot

fabpot Jun 1, 2019

Author Member

I don't think so. I've done it that way to get a "better" error message.

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jun 1, 2019

Member

But it means that the user will only get an error at runtime if they didn't discover before that Message instances are required here. With a proper type hint they can be warned before.

This comment has been minimized.

Copy link
@fabpot

fabpot Jun 1, 2019

Author Member

How would they be warned before? I suppose you're thinking about static analysis. Not sure what to do.

This comment has been minimized.

Copy link
@fabpot

fabpot Jun 1, 2019

Author Member

In any case, that's independent from this PR as I'm doing the same at several other places (and this code is copy/pasted from SmtpEnvelope).

This comment has been minimized.

Copy link
@fabpot

fabpot Jun 4, 2019

Author Member

@fabpot fabpot merged commit f4254e6 into symfony:4.3 Jun 4, 2019

3 checks passed

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

fabpot added a commit that referenced this pull request Jun 4, 2019

bug #31774 [Mailer] Fix the possibility to set a From header from Mes…
…sageListener (fabpot)

This PR was merged into the 4.3 branch.

Discussion
----------

[Mailer] Fix the possibility to set a From header from MessageListener

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

<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against branch 4.4.
 - Legacy code removals go to the master branch.
-->

Commits
-------

f4254e6 [Mailer] fixed the possibility to set a From header from MessageListener

@fabpot fabpot referenced this pull request Jun 4, 2019

Merged

Change type hints #31837

fabpot added a commit that referenced this pull request Jun 4, 2019

minor #31837 Change type hints (fabpot)
This PR was merged into the 4.3 branch.

Discussion
----------

Change type hints

| Q             | A
| ------------- | ---
| Branch?       | 4.3 for features / 3.4, 4.2 or 4.3 for bug fixes <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please 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

see #31774 (comment)

<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against branch 4.4.
 - Legacy code removals go to the master branch.
-->

Commits
-------

d56ae06 changed type hints

@fabpot fabpot referenced this pull request Jun 6, 2019

Merged

Release v4.3.1 #31901

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.