In order to achieve feature parity with 7.x we need to implement incoming messaging functionality. Some work has already been started on this, per this message from @dpi:

See also #2816823: DX: Documentation for incoming message workflow for implementing incoming messages.

@bradjones1 was also working on the incoming component for my Twilio plugin. See his comment.

CommentFileSizeAuthor
#20 interdiff-2829760-17.txt2.24 KBchrisrockwell
#20 implement_incoming-2829760-20.patch7.13 KBchrisrockwell
#17 implement_incoming-2829760-17.patch6.85 KBchrisrockwell
#14 2.patch37.19 KBbradjones1
#11 implement_incoming_messaging-2829760.un-tested.patch5.07 KBchrisrockwell

Comments

chrisrockwell created an issue. See original summary.

dpi’s picture

dpi
Location Perth, Australia
commented

PR originally at: https://github.com/dpi/sms_twilio/pull/2

bradjones1’s picture

bradjones1
Primary language English
commented

From a quick glance it looks like sms_twilio doesn't have a dependency on SMS Framework... Is the idea that is going to happen, or would we need to refactor the incoming logic in that PR to integrate in with this module as it stands? I believe there's been a conversation about that, but I'm late to the party?

dpi’s picture

dpi
Location Perth, Australia
commented

If SMS Framework is not installed then what is going to call the incoming logic? I would guess no-one.

As the code is, there does not have to be a dependency. Since this project is dedicated to SMS Framework then it would make sense. Its ultimately @chrisrockwell choice.

As an aside: I find it easier to develop modules if I do not have to uninstall dependent modules. For example, if a site builder hoses his install of SMS Framework, then he does not have to uninstall sms_twilio. Dependent modules primary configuration can stay intact. (gateway instances will of course be removed by the Configuration API)

I believe there's been a conversation about that, but I'm late to the party?

Only discussion about that is what we discussed in https://github.com/dpi/sms_twilio/pull/2#issue-183274703

chrisrockwell’s picture

chrisrockwell commented

@bradjones1 I don't have the intention of making this stand-alone. I need to catch up on the discussion on github but it's safe to assume the integration will remain.

chrisrockwell’s picture

chrisrockwell commented

Added related issue #2832601: Incoming message handling tweaks

chrisrockwell’s picture

chrisrockwell commented
dpi’s picture

dpi
Location Perth, Australia
commented
Related issues: +#2832601: Incoming message handling tweaks
dpi’s picture

dpi
Location Perth, Australia
commented
Related issues: +#2832862: Automatic incoming route creation
bradjones1’s picture

bradjones1
Primary language English
commented

I have updated the PR on Github to incorporate the new API.

https://github.com/dpi/sms_twilio/pull/2#issuecomment-276535498

chrisrockwell’s picture

chrisrockwell commented
StatusFileSize
new implement_incoming_messaging-2829760.un-tested.patch5.07 KB

I'm uploading my untested work-in-progress, which is an attempt at combining @bradjones1' work and mine (I apologize for not getting it out sooner). @dpi made some comments on the PR linked in #10 that need to be addressed, namely:

  • Lots of doc block cleanups
  • Use addReports instead of setReport

I did not have media handling and I prefer the way @bradjones1 separated out buildIncomingFromRequest as I had it all in processIncoming.

The way I'm handling validation might need to be discussed. I *think* we should be explicitly using the generated route for the signature.

Thanks all!

chrisrockwell’s picture

chrisrockwell commented 
+++ b/modules/contrib/sms_twilio/src/Plugin/Utility/TwilioValidation.php
@@ -0,0 +1,33 @@
+ * @file contains TwilioValidator.php
...
+ * Class TwilioValidator

Should be TwilioValidation.php

dpi’s picture

dpi
Location Perth, Australia
commented

@ #12

generally most @file blocks are no longer required per coding standards. In this case, the file block should be moved to describe the class:

/*
 * Defines methods for dealing with Twilio media
 */
class TwilioMedia {
bradjones1’s picture

bradjones1
Primary language English
commented
StatusFileSize
new 2.patch37.19 KB

Sorry for all the minor commits on the Github PR, but I did get a chance to test this locally against Twilio and it looks like our work merged together, well. Attached is a patch version of the PR at this moment.

dpi’s picture

dpi
Location Perth, Australia
commented

+30 KB?

There should be interdiffs if sticking to patchflow:

chrisrockwell’s picture

chrisrockwell commented

Something got messed up with the patch @bradjones1. For example, none of these should be in Twilio.php (incoming is deprecated, validate is renamed and in a Utility class. I'll take a look at the PR's on GitHub.

+++ b/src/Plugin/SmsGateway/Twilio.php
@@ -145,4 +152,64 @@ public function send(SmsMessageInterface $sms_message) {
+  public function incoming(SmsMessageInterface $sms_message) {
...
+  private function validate(Request $request) {
...
+  private function processMedia(array $params) {
chrisrockwell’s picture

chrisrockwell commented
StatusFileSize
new implement_incoming-2829760-17.patch6.85 KB

I merged in the changes in @bradjones1 fork and am uploading it here.

I'm seeing the benefit of doing the grunt of the work on github and then merging it here as needed so I'll work on transitioning to that.

chrisrockwell’s picture

chrisrockwell commented
Status: Active » Needs review
chrisrockwell’s picture

chrisrockwell commented
chrisrockwell’s picture

chrisrockwell commented
StatusFileSize
new implement_incoming-2829760-20.patch7.13 KB
new interdiff-2829760-17.txt2.24 KB

Fixes/adds error handling. With this I've confirmed incoming using a live Twilio account.

chrisrockwell’s picture

chrisrockwell commented

Somewhere the dependency on SMS Framework was removed.

@dpi in #4:

For example, if a site builder hoses his install of SMS Framework, then he does not have to uninstall sms_twilio

Is this why it's a suggestion instead of a requirement? Without SMS Framework this isn't usable, no?

chrisrockwell’s picture

chrisrockwell commented
Status: Needs review » Fixed

I merged @bradjones1 PR here: http://cgit.drupalcode.org/sms_twilio/commit/?id=9a085f9 so that we could keep the history.

chrisrockwell’s picture

chrisrockwell commented
Status: Fixed » Closed (fixed)