Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Mailer] Fix the possibility to set a From header from MessageListener#31774
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fabpot commentedMay 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 |
| $this->setRecipients($recipients); | ||
| } | ||
| publicstaticfunctioncreate(RawMessage$message):self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure if we should keep this method.
weaverryan commentedMay 31, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@fabpot I'm getting a new error:
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 |
fabpot commentedMay 31, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
"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). |
Uh oh!
There was an error while loading.Please reload this page.
146f260 tof4254e6Compare| publicfunction__construct(RawMessage$message) | ||
| { | ||
| if (!$messageinstanceof Message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think so. I've done it that way to get a "better" error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But it means that the user will only get an error at runtime if they didn't discover before thatMessage instances are required here. With a proper type hint they can be warned before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How would they be warned before? I suppose you're thinking about static analysis. Not sure what to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
…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 <!-- seehttps://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 peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://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
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 <!-- seehttps://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/asee#31774 (comment)<!--Replace this notice by a short README for your feature/bugfix. This will help peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://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