Incoming functionality of SMS Framework has some quirks. This issue aims to tweak things a little.

Determining if plugin supports incoming

Framework uses instanceof to determine whether a gateway plugin “supports incoming”. This requires an instance of the gateway to be created.

Change “supports incoming” determination to a annotation property. This property is informational only.

This annotation should not be used to automatically create an incoming route in the future.

Plugin incoming method

When you feed a message to SmsProvider::incoming, there isn’t really a pre- and post- process of the message. Officially its post-.
Introduce meaning to the SmsGateway::incoming method wherein it is the middle event. Thusly there is a reason to have a pre- event.

Rename the existing SmsGateway::incoming to SmsGateway::incomingEvent. The functionality of this method will be changed to be that it represents the event that always happens in between pre- and process- incoming events.

This method is optional.

Parameter for incomingEvent is an event object, not a SMS. There is no return value.

Note: logic for handling the incoming request does not happen in this event.

Mark SmsGateway::incoming as deprecated, remove in next release immediately since annotation is a breaking requirement.

Enforce result and reports

(this should also be done for SmsGateway::send

Ensure when SmsGateway::incoming (or SmsGateway::queue with incoming) receives a message:

  • A result is set on the message (via SmsMessage::setResult).
  • A report within above result object exists for each recipient on the message.

Throw an exception if any of the above conditions are not met.

Incoming enforcement should happen in a first class event subscriber responding to pre-incoming.

Outgoing enforcement should happen in a first class event subscriber responding to post-outgoing.

Effects

To implement a plugin with incoming functionality, set supports_incoming annotation to TRUE. There are no longer required interfaces/methods.

CommentFileSizeAuthor
#14 incoming-tweaks-2832601-14.patch37.19 KBdpi
#9 incoming-tweaks-2832601.patch36.2 KBdpi

Comments

dpi created an issue. See original summary.

dpi’s picture

dpi
Location Perth, Australia
commented
Issue summary: View changes
dpi’s picture

dpi
Location Perth, Australia
commented
Issue tags: +release-8.x-1.0-beta2
dpi’s picture

dpi
Location Perth, Australia
commented
Issue summary: View changes
almaudoh’s picture

almaudoh commented

The whole "incoming" thing is something I'm still trying to understand really. It is an artifact from the old D6/D7 versions.

Ideally, if we want to use SMS Framework to provide processing for incoming messages, the likely way that would happen is in a callback route linked to the gateway just like in the delivery reports.

Have you actually made use of incoming in any of your projects?

When you feed a message to SmsProvider::incoming, there isn’t really a pre- and post- process of the message. Officially its post-.
Introduce meaning to the SmsGateway::incoming method wherein it is the middle event. Thusly there is a reason to have a pre- event.

Can you clarify this? ::incoming() actually has the same pre- and post-process events dispatched as ::send() and ::queue()

dpi’s picture

dpi
Location Perth, Australia
commented

the likely way that would happen is in a callback route linked to the gateway just like in the delivery reports.

I think so too. But for now it is easy enough to DIY: I outlined steps in docs.

Have you actually made use of incoming in any of your projects?

Not yet.

I am investigating the process of implementing incoming and how it passes through SMS Framework for SMS Twilio (#2829760: Implement incoming messaging functionality) and Clickatell.

Can you clarify this? ::incoming() actually has the same pre- and post-process events dispatched as ::send() and ::queue()

The existing events don't change at all. Its only re-defining what pre- and post- surrounds: the middle process. For outgoing its gateway::send(), but when you are receiving a message it is _____. ie. no pre-.

dpi’s picture

dpi
Location Perth, Australia
commented

re-defining what pre- and post- surrounds

the middle process. For outgoing its gateway::send(),

An alternative is removing the pre- events for incoming.

almaudoh’s picture

almaudoh commented

An alternative is removing the pre- events for incoming.

There might still be use cases for a pre- process event. It depends.

dpi’s picture

dpi
Location Perth, Australia
commented
Status: Active » Needs review
StatusFileSize
new incoming-tweaks-2832601.patch36.2 KB

Ready for review, PR: https://github.com/dpi/smsframework/pull/60

Travis is green, patch should pass here.

Commit log

  • Enforce result and reports and added plugin incoming event
  • Fixed failing tests
  • Added results and reports for incoming messages created in tests.
  • Added createMessageResult test helper to create reports and result for a message.
  • Fixed failing tests due to missing reports
  • Test coverage for new result/report exceptions
  • Compacted sms_test_gateway gateway config schema (Derivative types)
  • Added plugin incoming method to execution order tests.
  • New test file covering \Drupal\sms\EventSubscriber\SmsMessageProcessor (some \Drupal\Tests\sms\Kernel\SmsFrameworkProviderTest tests should be moved there)
  • Added supports incoming messages gateway annotation property.
  • Added SmsGateway::supportsIncoming()
  • Add preprocess check to incoming messages ensuring they have a gateway set, and the gateway supports incoming messages.
  • Added tests for above.
  • Added missing gateway entity to incoming message tests.
dpi’s picture

dpi
Location Perth, Australia
commented

Change record

  • Gateways supporting incoming must now define an incoming annotation. See \Drupal\sms_test_gateway\Plugin\SmsGateway\Memory for an example.
  • Gateways must cease extending SmsGatewayPluginIncomingInterface — it no longer exists
  • Incoming gateway plugin docs updated: Adding incoming SMS features to Gateway plugins:
    • annotation section.
    • reports and results section.
almaudoh’s picture

almaudoh commented

Made some comments on the PR. This is really more of moving code around than anything else really.

dpi’s picture

dpi
Location Perth, Australia
commented

Addressed some nits in PR review.

What do you think about the interface naming still? (PR comment)

almaudoh’s picture

almaudoh commented

Made a new comment on the PR. The new name still misses 'Processor' or 'Subscriber' which I think is key to understanding at a glance what that interface should represent.

dpi’s picture

dpi
Location Perth, Australia
commented
StatusFileSize
new incoming-tweaks-2832601-14.patch37.19 KB

Addressed remaining review nits.

For PR commit 957cf521e24a2ad35d4132768b2c1f5348a6dcb9

  • dpi committed a677ac9 on 8.x-1.x 
    Issue #2832601 by dpi: Incoming messages handling tweaks and stricter...
dpi’s picture

dpi
Location Perth, Australia
commented
Status: Needs review » Fixed

Committed.

CR at https://www.drupal.org/node/2836073

almaudoh’s picture

almaudoh commented

Great work!!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.