NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[Notifier] support`SmsMessage` with 'from' set#58539

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

Jump to bottom

Open

petski wants to merge1 commit intosymfony:6.4

base:6.4

Choose a base branch

frompetski:fix-for-58535

Open

[Notifier] support`SmsMessage` with 'from' set#58539

petski wants to merge1 commit intosymfony:6.4frompetski:fix-for-58535

Conversation

Copy link

petski commentedOct 11, 2024•
edited by OskarStark
Loading

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix#58535
License	MIT

FixesError Email does not comply with addr-spec of RFC 2822

See

[Notifier][FakeSMS] ErrorEmail does not comply with addr-spec of RFC 2822 #58535

petski requested a review fromOskarStark as acode owner

October 11, 2024 07:50

carsonbot added Status: Needs Review Bug labels

Oct 11, 2024

carsonbot added this to the6.4 milestone

Oct 11, 2024

Copy link

carsonbot commentedOct 11, 2024

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
Features and deprecations must be submitted against the 7.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

OskarStark added the Notifier label

Oct 11, 2024

carsonbot changed the title~~fix: support SmsMessage with 'from' set~~[Notifier] fix: support SmsMessage with 'from' set

Oct 11, 2024

OskarStark changed the title~~[Notifier] fix: support SmsMessage with 'from' set~~[Notifier] supportSmsMessage with 'from' set

Oct 11, 2024

Copy link

Author

petski commentedOct 11, 2024

Can someone give me a pointer on how to solve the psalm error?

Fan2Shrek reviewed

Oct 12, 2024

View reviewed changes

src/Symfony/Component/Notifier/Bridge/FakeSms/FakeSmsEmailTransport.phpShow resolvedHide resolved

petski force-pushed thefix-for-58535 branch from977abdb tocc6c14fCompare

October 13, 2024 10:35

xabbuh reviewed

Oct 13, 2024

View reviewed changes

src/Symfony/Component/Notifier/Bridge/FakeSms/FakeSmsEmailTransport.php OutdatedShow resolvedHide resolved

petski force-pushed thefix-for-58535 branch fromcc6c14f toafc0e4dCompare

October 13, 2024 17:32

fix: support SmsMessage with 'from' set

e44e04e

Fixes `Error Email does not comply with addr-spec of RFC 2822`See:symfony#58535

petski force-pushed thefix-for-58535 branch fromafc0e4d toe44e04eCompare

October 13, 2024 17:39

nicolas-grekas reviewed

Nov 20, 2024

View reviewed changes

src/Symfony/Component/Notifier/Bridge/FakeSms/FakeSmsEmailTransport.php

		throw new UnsupportedMessageTypeException(__CLASS__, SmsMessage::class, $message);
		}

		$email = (new Email())
		->from($message->getFrom() ?: $this->from)
		->from($this->from)

Copy link

Member

nicolas-grekasNov 20, 2024

Choose a reason for hiding this comment

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

can you explain this change? it looks suspicious to me

Copy link

Member

fabpotFeb 10, 2025

Choose a reason for hiding this comment

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

@petski Any insight?

Copy link

Member

xabbuhApr 29, 2025

Choose a reason for hiding this comment

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

The change looks valid to me. At least, there is nothing indicating that the$from property of theSmsMessage class has to be a valid e-mail address and thus we IMO must not use it as such in theFakeSmsEmailTransport.