Skip to content

[Notifier] Add notification property to SmsMessage class #37818

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[Notifier] Add notification property to SmsMessage class #37818

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/Symfony/Component/Notifier/Message/SmsMessage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ final class SmsMessage implements MessageInterface
private $transport;
private $subject;
private $phone;
private $notification;

public function __construct(string $phone, string $subject)
{
Expand All @@ -38,7 +39,15 @@ public function __construct(string $phone, string $subject)

public static function fromNotification(Notification $notification, SmsRecipientInterface $recipient): self
{
return new self($recipient->getPhone(), $notification->getSubject());
$message = new self($recipient->getPhone(), $notification->getSubject());
$message->notification = $notification;

return $message;
}

public function getNotification(): ?Notification
Copy link
Member

Choose a reason for hiding this comment

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

As I said in the other PR, I don't like that you can get a Notification from the SMS message, it does not feel right. Instead, if many providers have a "quality" setting, we might add a quality property on the SMS message. Can you see if "big" providers like Nexmo or Twilio also have such a setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, I put thechangehere to open the discussion.
It appears that nothing equivalent is present in thoses provider.
They offer an sms pricing without paid options. Or I couldn't find it.

Copy link
Member

Choose a reason for hiding this comment

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

That's what I feared unfortunately. Using options on the message is still possible though. So, I think it could be enough for people really wanting to use this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok thanks for yout feedback.
I might need to clean the mobyt bridge to delete code that won't be used (e.g.: fromNotification in MobytOptions)

{
return $this->notification;
}

/**
Expand Down