Add Notifier SentMessage #36611

jeremyFreeAgent · 2020-04-28T15:27:08Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets
License	MIT
Doc PR	symfony/symfony-docs#13624

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.

apply to bridges

src/Symfony/Component/Notifier/Bridge/Twilio/TwilioTransport.php

src/Symfony/Component/Notifier/Transport/TransportInterface.php

fabpot · 2020-05-03T14:33:20Z

Sounds good to me. @jeremyFreeAgent Can you finish the PR?

jeremyFreeAgent · 2020-05-03T20:12:29Z

I have set uniqid() for transports that do not give back an identifier.

fabpot · 2020-05-04T06:05:06Z

src/Symfony/Component/Notifier/Bridge/FreeMobile/FreeMobileTransport.php

@@ -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());


I would allow the id to be null instead of generating a random one.

I removed it from __construct then.

fabpot · 2020-05-04T07:52:40Z

src/Symfony/Component/Notifier/Bridge/Firebase/FirebaseTransport.php

@@ -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);


These changes should be reverted

Ok! (That was asked by fabbot.io)

fabpot · 2020-05-04T09:01:38Z

src/Symfony/Component/Notifier/Bridge/FreeMobile/FreeMobileTransport.php

+
+        $message = new SentMessage($message);
+
+        return $message;


You can remove the $message object and return it directly.

fabpot · 2020-05-04T09:03:33Z

src/Symfony/Component/Notifier/Bridge/Nexmo/NexmoTransport.php

+
+        $message = new SentMessage($message);
+        $message->setMessageId($success['messages'][0]['message-id']);
+        $message->setTransportResponseData($success);


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.

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)

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.

Yes exactly. Most of the services have webhook or API to get the other information parts.

Thanks.

jeremyFreeAgent · 2020-05-06T09:57:48Z

Is it still relevant for 5.1?

fabpot · 2020-05-06T11:27:17Z

Still relevant, but not for 5.1, I think we need more time to validate what we want.

fabpot · 2020-06-09T06:00:23Z

@jeremyFreeAgent Can you update this PR with the latest discussion from the comments?

jeremyFreeAgent · 2020-06-13T09:10:12Z

rebased!

apfelbox · 2020-06-14T19:13:33Z

src/Symfony/Component/Notifier/CHANGELOG.md

+5.2.0
+-----
+
+ * [BC BREAK] The `TransportExceptionInterface::send()` and `AbstractTransport::doSend()` methods changed to return a `SentMessage` instance instead of `void`.


Suggested change

* [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?

Thanks @apfelbox ! Sorry I pushed the fix because in the GitHub iOS app I didn't saw that you use the suggestion feature.

haha thx, don't worry 😄

fabpot

LGTM, minor comments to be fixed before merge. Thank you.

fabpot · 2020-06-17T08:35:51Z

src/Symfony/Component/Notifier/Message/SentMessage.php

+ *
+ * @experimental in 5.2
+ */
+class SentMessage


Should be final.

My bad 🤦
Fixed.

fabpot · 2020-06-17T08:36:09Z

src/Symfony/Component/Notifier/Message/SentMessage.php

+{
+    private $original;
+    private $transport;
+    private $messageId = null;


= null is the default, so it can be removed

fabpot · 2020-06-17T09:04:22Z

src/Symfony/Component/Notifier/Transport/Transports.php

@@ -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));
-        }
+	}


Looks like it's a wrong change here.

Yes this is strange. It is a tab (I may have changed the file with another editor). I fix it. Good catch!

fabpot · 2020-06-17T09:43:48Z

@jeremyFreeAgent Next step would be to fix the tests :)

jeremyFreeAgent · 2020-06-17T13:08:50Z

Sorry I was working on something else at the same time.

fabpot · 2020-06-23T05:13:26Z

@jeremyFreeAgent Do you have time to have a look for the failing tests?

jeremyFreeAgent · 2020-06-23T06:18:24Z

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?

fabpot · 2020-06-23T06:32:56Z

The deps=low should pass. It looks like you forgot to update the composer.json file for FreeMobile.

jeremyFreeAgent · 2020-06-23T06:57:49Z

OMG thanks @fabpot ♥️

fabpot · 2020-06-23T07:05:14Z

Thank you @jeremyFreeAgent.

mbabker · 2020-06-23T21:54:03Z

Can the Messenger handler be made to return the SentMessage, similar to the Mailer's handler (#36424)?

fabpot · 2020-06-24T06:25:49Z

@mbabker Sure, see #37403

…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  | Bug fix? | no | New feature? | yes  | Deprecations? | no  | Tickets | see #36611 (comment)  | License | MIT | Doc PR | Commits ------- 5855d11 [Notifier] Return SentMessage from the Notifier message handler

mbabker · 2020-06-24T12:04:42Z

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.

@jeremyFreeAgent

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  | Deprecations? | no  | Tickets | n/a  | License | MIT | Doc PR | n/a #36611 broke the Notifier when used with Messenger. /cc @jeremyFreeAgent Commits ------- 92c28de [Notifier] Fix SentMessage implementation

carsonbot added Status: Needs Review Feature labels Apr 28, 2020

nicolas-grekas added this to the next milestone Apr 28, 2020

Koc reviewed Apr 28, 2020

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Twilio/TwilioTransport.php Show resolved Hide resolved

Koc reviewed Apr 28, 2020

View reviewed changes

src/Symfony/Component/Notifier/Transport/TransportInterface.php Show resolved Hide resolved

jeremyFreeAgent force-pushed the notifier-sent-message branch from 93d50f0 to d10768f Compare May 3, 2020 18:22

fabpot requested changes May 4, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels May 4, 2020

fabpot reviewed May 4, 2020

View reviewed changes

fabpot requested changes May 4, 2020

View reviewed changes

jeremyFreeAgent force-pushed the notifier-sent-message branch 2 times, most recently from b6bd5bf to 048994c Compare June 13, 2020 09:00

apfelbox reviewed Jun 14, 2020

View reviewed changes

fabpot approved these changes Jun 17, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Jun 17, 2020

fabpot reviewed Jun 17, 2020

View reviewed changes

Add Notifier SentMessage

5a6f053

fabpot force-pushed the notifier-sent-message branch from 616a60f to 5a6f053 Compare June 23, 2020 07:04

fabpot merged commit ce8f8a5 into symfony:master Jun 23, 2020

fabpot mentioned this pull request Jun 24, 2020

[Notifier] Return SentMessage from the Notifier message handler #37403

Merged

derrabus mentioned this pull request Jun 27, 2020

[Notifier] Notifier 5.2 is incompatible with 5.1 bridges #37438

Merged

fabpot mentioned this pull request Aug 6, 2020

[Notifier] Fix SentMessage implementation #37748

Merged

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

fabpot mentioned this pull request Oct 5, 2020

Release v5.2.0-BETA1 #38417

Merged

	* [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`.

Add Notifier SentMessage #36611

Add Notifier SentMessage #36611

Conversation

jeremyFreeAgent commented Apr 28, 2020 • edited Loading

fabpot commented May 3, 2020

jeremyFreeAgent commented May 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyFreeAgent commented May 6, 2020 • edited Loading

fabpot commented May 6, 2020

fabpot commented Jun 9, 2020

jeremyFreeAgent commented Jun 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jun 17, 2020

jeremyFreeAgent commented Jun 17, 2020

fabpot commented Jun 23, 2020

jeremyFreeAgent commented Jun 23, 2020

fabpot commented Jun 23, 2020

jeremyFreeAgent commented Jun 23, 2020

fabpot commented Jun 23, 2020

mbabker commented Jun 23, 2020

fabpot commented Jun 24, 2020

mbabker commented Jun 24, 2020

jeremyFreeAgent commented Apr 28, 2020 •

edited

Loading

jeremyFreeAgent commented May 6, 2020 •

edited

Loading