[Notifier] Add options to SmsMessage

[gnito-org]

Q	A
Branch?	6.3
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets
License	MIT
Doc PR

Add the ability to attach optional options to an SMS message, which can then be processed by the notifier bridge.

Some SMS APIs allow dynamic options that can be set on a per message basis, which cannot be satisfied by options in the DSN.

[OskarStark]

There was a declined PR in the past

• [Notifier] Added ability to initialize sms options #39774

But lets bring this topic back on the table, as it looks like there is a need for being more specific.

Maybe its more important then we think @fabpot.

The follow up policy could be, that you can easily switch from one to another SMS Provider if the current and the new transport does not support specific options. And if your SmsMessage was instantiated without given options... 🤔

[gnito-org]

For my purposes, the ability to add options to the SMS could be limited to only when you use the notifier bridge as a stand-alone component.

My app does not use the Symfony SMS channel to automatically choose the bridge from the DSN. It 'manually' instantiates the bridge using the transport factory of the bridge.

In other words, for my purposes, you do not need to complicate the Symfony texter with the ability to handle options.

If the SMSMessage can carry options, and the bridge can handle those options, then it works for me.

If you're really adamant that SMSMessage should not have options, then it's not a train-smash. I'll implement Plan B.

[gnito-org]

Looking at the existing SMS DSNs, there are already DSNs that have options that others don't have.

How is that different from adding an options property to the SMSMessage object?

[gnito-org]

Let me throw this personal opinion in the mix as well.

MessageInterface promises that the object will have non-mandatory options. In its current state, SmsMessage does not properly implement MessageInterface because it does not allow options at all. It uses a dummy stub getOptions() method to pretend that it impements the interface.

[gnito-org]

SmsMessage will have get an options property because of the new Webhook & Remote Event components.

@OskarStark @fabpot Should I close this PR or leave it open?

[OskarStark]

Leave it open please 👍

[gnito-org]

I will add the XXXOptions unit tests to all the SMS notifiers that I contributed once this PR has been merged.

[OskarStark]

Thank you 👍

[leblanc-simon]

Shouldn't we remove the $from from the constructor to go through via the options?

We will have an identical structure like ChatMessage and PushMessage.

A lot of texter bridges let's to customize the from globally at the DSN level and going through an option would seem consistent with the ways of doing things for the other briges.

Moreover with the new webhook component, it would force to pass an empty parameter in the constructor to pass the options (or use a named parameter)

Could I propose to deprecate the $from parameter for 6.3 and remove at 6.4 :

public function __construct(string $phone, string $subject, string | MessageOptionsInterface $options = null)
{
    If (is_string($options)) {
        trigger_error('using from inside constructor is deprecated',E_USER_DEPRECATED);
        $this->from = $from;
    }

    $this->subject = $subject;
    $this->phone = $phone;
    $this->options = $options;
}

[fabpot]

Thank you @gnito-org.

[alamirault]

Thanks @gnito-org for adding a way to add optional options.

However how can it works with multiple providers (roundrobin or failover https://symfony.com/doc/current/notifier.html#configure-to-use-failover-or-round-robin-transports) with this implementation ?

I saw you start created dedicated Option object (MessageBirdOptions, ContactEveryoneOptions, ...) for each bridge. So maybe we can add multiple options object to SmsMessage and transport can use only supported object option ?

WDYT ?

[gnito-org]

I presume that one cannot use transport-specific options if you want to use round-robin failover.

[OskarStark]

We are discussing this internally right now.

Stay tuned

[gnito-org]

I'm pausing all further work on the notifiers until you folks have finalized your internal deliberations and communicated your decisions.

[alamirault]

@nicolas-grekas I saw you merged bridge options. What is the internal decision for this question ?