Skip to content

[Notifier] Added ability to initialize sms options #39774

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Notifier] Added ability to initialize sms options #39774

wants to merge 1 commit into from

Conversation

krasilnikovs
Copy link
Contributor

Q A
Branch? 5.x for features
Bug fix? no
New feature? yes
Deprecations? no
License MIT

During implementing notifier transport were found SmsMessage::getOptions method always returns null and there is no ability to set sms options.
In the pr added ability to initialize sms options through the constructor.

rheh
rheh approved these changes Jan 10, 2021
View reviewed changes
Copy link

@rheh rheh left a comment

Choose a reason for hiding this comment

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

Would it be worth added a settter too?

@nicolas-grekas nicolas-grekas added this to the 5.x milestone Jan 12, 2021
@nicolas-grekas
Copy link
Member

Can you add a test case please?

@krasilnikovs krasilnikovs force-pushed the add-ability-to-initialize-sms-options branch 2 times, most recently from 56e5dde to e79bf22 Compare January 12, 2021 20:45
@krasilnikovs
Copy link
Contributor Author

Would it be worth added a settter too?

Added

@krasilnikovs
Copy link
Contributor Author

Can you add a test case please?

Sure, added.

rheh
rheh reviewed Jan 13, 2021
View reviewed changes
Copy link

@rheh rheh left a comment

Choose a reason for hiding this comment

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

Would it be worth added a settter too?

@fabpot
Copy link
Member

fabpot commented Jan 14, 2021

Can you share the use case? I did not implemented options for SMS, as there is no need. The only method in the interface is getRecipientId() which we don't need for SMSes.

@krasilnikovs
Copy link
Contributor Author

Can you share the use case? I did not implemented options for SMS, as there is no need. The only method in the interface is getRecipientId() which we don't need for SMSes.

I implemented telnyx transport for sending sms messages(#38909), telnyx provides the ability to set extra fields for example webhook url, media links and etc, and without the chages not possible to use the options in transport because getOptions method always will return null.

symfony/src/Symfony/Component/Notifier/Bridge/Telnyx/TelnyxTransport.php

Lines 60 to 66 in a526e46

if ($message->getOptions() && !$message->getOptions() instanceof TelnyxOptions) {
throw new LogicException(sprintf('The "%s" transport only supports instances of "%s" for options.', __CLASS__, TelnyxOptions::class));
}
$endpoint = sprintf('https://%s/v2/messages', $this->getEndpoint());

OskarStark
OskarStark reviewed Jan 14, 2021
View reviewed changes
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Some comments, afterwards good to go

@krasilnikovs krasilnikovs force-pushed the add-ability-to-initialize-sms-options branch from e79bf22 to fbb901f Compare January 14, 2021 14:27
@OskarStark OskarStark requested a review from fabpot January 14, 2021 14:33
@fabpot
Copy link
Member

fabpot commented Jan 15, 2021

👎

We don't want to code specific features for each provider. The goal of the component is to implement the common features of all providers.

@OskarStark
Copy link
Contributor

But isn't this the base to have a common way to implement specific options for specific Texters? I mean otherwise we need to blow up the telnyx transport and others and find another way which is completely different to the chat messages? 🧐

@OskarStark
Copy link
Contributor

OskarStark commented Jan 30, 2021 •
edited
Loading

@fabpot can you please answer my question? Thanks 😊

@fabpot
Copy link
Member

fabpot commented Jan 31, 2021

Again, the whole idea is to be able to switch one implementation with another one. That's very true with SMS and email, less with chat as there is no real interoperability (yet?). But SMS and emails are pretty standard.

@OskarStark
Copy link
Contributor

Ok let's close here. @krasilnikovm can you please update your implementation And just use the standard way without additional options ? Thanks

@OskarStark OskarStark closed this Jan 31, 2021
@OskarStark OskarStark mentioned this pull request Apr 6, 2021
Closed
@OskarStark OskarStark mentioned this pull request Jul 26, 2021
Closed
@OskarStark OskarStark mentioned this pull request Dec 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rheh rheh rheh approved these changes

@OskarStark OskarStark OskarStark approved these changes

@fabpot fabpot Awaiting requested review from fabpot

Assignees
No one assigned
Labels
Feature Notifier Status: Needs Review
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
6 participants
@krasilnikovs @nicolas-grekas @fabpot @OskarStark @rheh @carsonbot