Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Follow up to #2292319-16: Convert SMS Gateways into D8 Plugins, the delivery reports API is not fully completed. This issue will bring that to the finish line and add tests.
| Comment | File | Size | Author |
|---|---|---|---|
| #59 | implement_api_for-2542790-59.patch | 45.33 KB | almaudoh |
| #59 | interdiff.txt | 2.38 KB | almaudoh |
| #49 | implement_api_for-2542790-49.patch | 45.1 KB | almaudoh |
| #44 | implement_api_for-2542790-44.patch | 47.63 KB | almaudoh |
| #22 | implement_api_for-2542790-21.patch | 45.45 KB | almaudoh |
Comments
Comment #1
almaudoh commentedComment #2
almaudoh commentedI propose a simple implementation:
DeliveryReport object
Encapsulates delivery status information per message per recipient.
SmsMessageResultInterface::getReport()should return an array of delivery report objects.SMS Gateways
Delivery reports are created by the SMS gateway plugin when the message is sent and is returned in the
$reportproperty of the SMS Message Result. The default status should beSTATUS_SENT. The SMS gateway also provides the delivery report callback url to the API server by callingSmsGatewayPluginInterface::getDeliveryReportUrl(). There is a default implementation which points to the Delivery reports controller.Delivery Reports Controller
During the life of the message, the delivery reports controller may receive delivery updates from the API server and calls the appropriate gateway to parse it. The controller then calls the
sms_receipt()andhook_delivery_report()hooks so that other modules can respond.Gateway plugins
Each gateway plugin module implements
::getDeliveryReports()and::parseDeliveryReports()which needed by the delivery reports controller.Comment #3
almaudoh commentedA patch implementing #2
Comment #7
almaudoh commentedComment #11
almaudoh commentedFixed remaining test fails. I'm still not happy leaving the machine name in the plugin configuration, but I can't see a better way to do this.
Comment #15
almaudoh commentedOk. Found a better way to pass the config entity machine name to the plugin instance without polluting the plugin schema. A more elegant solution. Also, that should fix the one remaining test fail.
Comment #16
almaudoh commentedSmall docs nits fixes.
Comment #18
almaudoh commentedComment #19
dpiIm not heavily invested in this feature, so I will put my SMS Framework hat back on and do some code reviews as soon as I've finished some RNG tasks. ~a couple of days
Comment #20
almaudoh commentedNeeds reroll after #2678040: Adding more gateway tests
Comment #21
dpiHopefully not too much:
Comment #22
almaudoh commentedRerolled
Comment #23
almaudoh commentedComment #25
dpiI am just doing a basic code review here. With some questions regarding implementation.
Id like global functions, such as found in sms.module, to be removed, and replaced with a service.
Regarding SmsDeliveryReport, SmsMessage:
Can you rationalize why we want this to be an in-memory object only.
Why doesnt this make sense as an entity?
Drupal has transitioned to using chained getters and setters, as opposed to a massive all purpose constructor.
SmsGatewayPluginCollection, SmsGatewayPluginInterface/Base :: setGatewayName
getDeliveryReportPathdoes not belong on a plugin instance. A helper to just contruct a URL is lazy.Overall I'd like to see setGatewayName removed.
In memory temp debug data
For debugging purposes, I have introduced a configuration (in #c5e4a60d) for plugin ID into the memory plugin type. See Memory::send()'s $gateway_id
I've changed the pattern for SmsFrameworkTestTrait (getTestMessages/getLastTestMessage/resetTestMessages) on how messages are stored after $plugin->send() is used. This will improve testing, and ensure we are using the correct plugin. Can you copy this pattern for memory reports.
I have added some nits at https://github.com/dpi/smsframework/pull/4/files if you could address them.
Comment #26
dpiIf you agree and address these, then in the interest of moving forward, I suggest you commit it.
Comment #27
dpiComment #28
almaudoh commentedFor the same reason I stated in the other issue. If it turns out that the use of entities would not significantly impact performance and storage space, then that'day be great. I'll do some profiling on this.
In the default implementation we use the defined route. The method was to allow gateway plugins to override the implementation.
The alternative is to add the gateway configuration entity name as a configuration item in the gateway plugin which would be saved again in the config entity in a different key (which is what I did in the patches before #22). But as you've also stated, the plugin shouldn't know about that.
This is needed just for generating the delivery report URL. So maybe we can figure out a cleaner way to do this cos I don't like it too much myself.
TBH, I don't like this either, but since it is in a test module, it's not a big issue.
Yeah, was planning to...
Will check them out...
Comment #29
dpiMeasuring performance only. Storage does not add to the equation since a memory object cannot compare.
Hmm, I think having a single canonical route is better. And letting others override the view (if required?) is better.
Let me know where you publish your Git repo and branch for this issue, I'd like to build apon your work directly.
Comment #30
dpiComment #37
almaudoh commentedGit repo created.
Comment #38
almaudoh commentedOk. Made some changes to the patch.
Just trying to fix this by copying and pasting in the two or three places I have it used demonstrated to me that it's a good idea to keep this helper method.
I've tried to figure out a better way to incorporate the gateway config name into the delivery report path. The only other option would be to place delivery reports on the SmsMessage, the SmsProviderInterface or create a new DeliveryReportManager to manage the reports. In all these cases, the gateway plugin would have to know about whoever is providing the delivery report path.
Fixed all the other nits.
https://github.com/almaudoh/smsframework/pull/1/files
https://github.com/almaudoh/smsframework/commit/a54fef1cd03c7f9d6fc13675...
Comment #39
dpiInexhaustive review notes based on New delivery reports stuff #4528fc39
getDeliveryReportPath / setGatewayName
getDeliveryReportPathandsetGatewayNameare dependent so I will just argue against the former.The only reason that I can think that SMS framework would need to know what the callback path is: for testing, and for tests we already know which routes are used. Are there other reasons?
DeliveryReportController
Other
pullDeliveryReportshow does the module know whether it has to pull or wait for push? or bothpullDeliveryReportsseems like another terminology mismatch, there is no "push" terms in callback.Comment #40
dpiComment #41
almaudoh commentedMost gateways need you to provide a callback url to which the delivery reports would be pushed (hence push DLRs). This is provided by the gateway plugin when it is making the HTTP request to send the SMS.
So the way this works is that the format of the DLR is different per gateway, so it is the particular gateway plugin that would know how to parse it and normalize it into the standardized format defined by the DeliveryReportInterface.
I will experiment with the approach of using the plugin name (and therefore having to create a plugin instance) instead of the config entity name, but that initially seemed to be potentially more complex.
I think I'll do that, I've not seen any use case myself so far...
In some use cases (I actually have some gateways), they don't provide you a push API, and you have to pull the DLR when you need it. Unfortunately, that introduces the asymmetry you mention, because you don't actually need to push the DLRs as they come from the server if you provide a push callback url.
Will fix the other nits.
Comment #42
almaudoh commentedI didn't know this sort of convention existed. Not documented anywhere...
Comment #43
dpiI'm not sure im getting my point across. The plugin can implement the route, and implement its own controller or use sms frameworks controller. We dont need to know the route that was called, because we are not calling it. Only the third party api is...
You're just taking a string, there is no entity upcasting? Literally change:
public function acknowledgeDelivery($gateway_name) {topublic function acknowledgeDelivery($gateway_id) {ANDpath: '/sms/deliveryreport/{gateway_name}topath: '/sms/deliveryreport/{gateway_id}Its just semantics. We're not using names; we're using ID's. :)
Side note: if you want me to teach you how to upcast entities to a plugin instance, I can do that. But that wasnt my point.
Its the terminology that can cause confusion. I understand there isnt a pushDeliveryReports in plugin instance, but the delivery route could be something like [dont use this] "receivePushedDeliveryReports"
naming things is hard.
re: order of injected
its just an observation of some modules I have made. I think I had this discussion with you before, but its an uncodified convention ;)
re: order of ::create
Seems like it is unofficial law, looking at other classes. (might be symfony or psr?)
Comment #44
almaudoh commentedSo, I have changed the implementation to using the
$optionsarray to pass in thedelivery_report_urlwhich is generated by the SMS provider just before send (if no other module hook does so first). I have moved the meat of delivery report processing into theDefaultSmsProvider()as well. My only concern with the php array is that thedelivery_report_urloption cannot be enforced using an interface or other OOP technique, which might be a loss. Also the gateway name is not available to be put in the SmsMessageResult or SmsDeliveryReport and so is not available anymore. (This of course can be fixed in ahook_sms_delivery_report()Addressed all the other concerns in #39.
Patch changes
Interdiff with #38
Also attached patch for testing...
Comment #49
almaudoh commentedRemoved the stray delivery reports kernel test...
Comment #53
dpiTheres some good improvements, I dont have any major objections to what's here. Hopefully $options['delivery_report_url'] works out. I know you're blocked on this issue, so we can always return later. One thing I'm concerned about, that does not have to necessarily be covered in this patch, is adding access control to the delivery report endpoint. Something along the lines of adding a secret token like Drupal cron does...
In any case I'll continue to think about plugin + path callback.
Whats happening with tests?
Comment #56
almaudoh commentedTests were running against Drupal 8.2.x. I changed it to run against 8.1.x. Don't understand the postgre failure though.
Comment #59
almaudoh commentedI've restored the sms_receipt() method because the removal is not very trivial and is better done in a different issue. The postgresql test fails are not related to this issue and can be fixed separately.
If the other tests come back green, I'll commit this unless there is any further reservations.
https://github.com/almaudoh/smsframework/pull/3/files
Comment #65
almaudoh commentedCommitted / pushed to 8.x-1.x
Comment #66
almaudoh commented