-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Notifier] Add SentMessage additional info #59481
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
0dff16e
to
ffdf909
Compare
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.
Putting the full info array is a bad idea to me. it contains unwanted things like the "pause" closure for example.
I think it's a good idea to add extra info, but I'd expect the transport to know which items are useful, and where they come from. No generic solution but adhoc ones: one for ovh, another for allmysms, etc.
@nicolas-grekas indeed, I only saw scalar data in WDYT would be the best ? Totally removing the |
|
ffdf909
to
74aa75a
Compare
@nicolas-grekas method for easier PR history, the method was: /**
* As many of the Transports are using HTTP requests,
* this shortcut attaches an HTTP response to the message in a standardised way.
*/
public function setHttpResponseInfo(ResponseInterface $response): void
{
$this->setInfo('http_response', $response->getInfo() + ['body' => $response->toArray(false)]);
} |
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.
would you be able to populate that info for a few bridges you know about? it'd be nice to showcase the benefit of this!
19b6b8a
to
ef126c3
Compare
I've added implementation for both OvhCloud & allmysms (@Vincentig's issue) transports |
909b832
to
70b1c96
Compare
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.
LGTM with minor changes
src/Symfony/Component/Notifier/Bridge/OvhCloud/Tests/OvhCloudTransportTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Notifier/Bridge/OvhCloud/Tests/OvhCloudTransportTest.php
Outdated
Show resolved
Hide resolved
58c0e85
to
068cccf
Compare
068cccf
to
a50f38c
Compare
@@ -48,6 +48,9 @@ public function supports(MessageInterface $message): bool | ||
return $message instanceof SmsMessage && (null === $message->getOptions() || $message->getOptions() instanceof AllMySmsOptions); | ||
} | ||
|
||
/** | ||
* @See https://doc.allmysms.com/api/en/#api-SMS-sendsimple |
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.
* @See https://doc.allmysms.com/api/en/#api-SMS-sendsimple | |
* @see https://doc.allmysms.com/api/en/#api-SMS-sendsimple |
Thank you @mRoca. |
This PR allows adding additional data into Notifier/Message/SentMessage.php, based on (and using) HttpClient Response's info. The initial need behind this feature is to be able to access to some data of the Transport's response (e.g.:
nbSms
andcost
for allmysms, ortotalCreditsRemoved
for OVH Cloud).Original (closed) PR: #51746
Some points to discuss:
ResponseInterface
object into the constructor as a readonly property, but not all Transports are using HTTP