Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
Add Notifier SentMessage#36611
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
Add Notifier SentMessage#36611
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Sounds good to me.@jeremyFreeAgent Can you finish the PR? |
93d50f0 tod10768fCompareI have set |
| @@ -75,5 +76,9 @@ protected function doSend(MessageInterface $message): void | |||
| throw new TransportException(sprintf('Unable to send the SMS: error %d: ', $response->getStatusCode()).($errors[$response->getStatusCode()] ?? ''), $response); | |||
| } | |||
| $message = new SentMessage($message, uniqid()); | |||
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.
I would allow the id to be null instead of generating a random one.
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.
I removed it from __construct then.
| @@ -79,10 +80,18 @@ protected function doSend(MessageInterface $message): void | |||
| if (200 !== $response->getStatusCode()) { | |||
| $errorMessage = $jsonContents ? $jsonContents['results']['error'] : $response->getContent(false); | |||
| throw new TransportException(sprintf('Unable to post the Firebase message:%s.', $errorMessage), $response); | |||
| throw new TransportException(sprintf('Unable to post the Firebase message:"%s".', $errorMessage), $response); | |||
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.
These changes should be reverted
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.
Ok! (That was asked by fabbot.io)
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.
Done!
| $message = new SentMessage($message); | ||
| return $message; |
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.
You can remove the $message object and return it directly.
| $message = new SentMessage($message); | ||
| $message->setMessageId($success['messages'][0]['message-id']); | ||
| $message->setTransportResponseData($success); |
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.
Passing the returned result as is means that it is not standardized.
I would instead make it a bit more useful by defining a format with things that are common (including the transport name so that one can know how to parse the extra information) and add an entry with the original message.
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.
Yes, you are right.
The only real common thing is the message-id. The other data parts depend on :
- the kind of message (chat or SMS)
- the service (some returns cost for SMS, number of SMS sent for the message)
I think we can start with:
original message(in the construct)transport__toString() (in the construct)message-id(with setter if returned)
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.
Yes, let's not add anything "special" at first. For 5.1, let's be very generic. And I suppose that with me message-id and the transport, you have all the information you need to get more if needed.
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.
Yes exactly. Most of the services have webhook or API to get the other information parts.
Thanks.
jeremyFreeAgent commentedMay 6, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Is it still relevant for 5.1? |
Still relevant, but not for 5.1, I think we need more time to validate what we want. |
@jeremyFreeAgent Can you update this PR with the latest discussion from the comments? |
b6bd5bf to048994cComparerebased! |
| 5.2.0 | ||
| ----- | ||
| * [BC BREAK] The `TransportExceptionInterface::send()` and `AbstractTransport::doSend()` methods changed to return a `SentMessage` instance instead of `void`. |
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.
| *[BC BREAK] The`TransportExceptionInterface::send()` and`AbstractTransport::doSend()` methods changed to return a`SentMessage` instance instead of`void`. | |
| *[BC BREAK] The`TransportInterface::send()` and`AbstractTransport::doSend()` methods changed to return a`SentMessage` instance instead of`void`. |
I guess that was a typo?
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.
Thanks@apfelbox ! Sorry I pushed the fix because in the GitHub iOS app I didn't saw that you use the suggestion feature.
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.
haha thx, don't worry 😄
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, minor comments to be fixed before merge. Thank you.
| * | ||
| * @experimental in 5.2 | ||
| */ | ||
| class SentMessage |
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.
Should befinal.
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.
My bad 🤦
Fixed.
| { | ||
| private $original; | ||
| private $transport; | ||
| private $messageId = null; |
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.
= null is the default, so it can be removed
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.
Fixed
| @@ -71,8 +69,8 @@ public function send(MessageInterface $message): void | |||
| if (!$this->transports[$transport]->supports($message)) { | |||
| throw new LogicException(sprintf('The "%s" transport does not support the given message.', $transport)); | |||
| } | |||
| } | |||
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.
Looks like it's a wrong change here.
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.
Yes this is strange. It is a tab (I may have changed the file with another editor). I fix it. Good catch!
@jeremyFreeAgent Next step would be to fix the tests :) |
Sorry I was working on something else at the same time. |
@jeremyFreeAgent Do you have time to have a look for the failing tests? |
The tests are failing because the bridges require the 5.2 version of notifier which is not yet merged. What is the solution to make them pass? |
The deps=low should pass. It looks like you forgot to update the composer.json file for FreeMobile. |
OMG thanks@fabpot |
Thank you@jeremyFreeAgent. |
Can the Messenger handler be made to return the |
…e handler (fabpot)This PR was merged into the 5.2-dev branch.Discussion----------[Notifier] Return SentMessage from the Notifier message handler| Q | A| ------------- | ---| Branch? | master <!-- see below -->| Bug fix? | no| New feature? | yes <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets | see#36611 (comment) <!-- prefix each issue number with "Fix #", if any -->| License | MIT| Doc PR |Commits-------5855d11 [Notifier] Return SentMessage from the Notifier message handler
Thank you! |
… (derrabus)This PR was merged into the 5.1 branch.Discussion----------[Notifier] Notifier 5.2 is incompatible with 5.1 bridges| Q | A| ------------- | ---| Branch? | 5.1| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | N/A| License | MIT| Doc PR | N/A#36611 introduced a breaking change that made the Notifier 5.2 incompatible with all Notifier bridges 5.0-5.1. Because of this, the tests are currently failing on Travis. This PR attempts to fix this.Commits-------1b6bbc2 [Notifier] Notifier 5.2 is incompatible with 5.1 bridges.
This PR was merged into the 5.2-dev branch.Discussion----------[Notifier] Fix SentMessage implementation| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets | n/a <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->| License | MIT| Doc PR | n/a#36611 broke the Notifier when used with Messenger./cc@jeremyFreeAgentCommits-------92c28de [Notifier] Fix SentMessage implementation
Uh oh!
There was an error while loading.Please reload this page.
Like Mailer, Notifier returns now a SentMessage that contains the messageId (returned by the provider in the response). It contains also the body of the response as array to have more info about price, number of sms sent, status and so on.