I discovered this issue while trying to get delivery report URL out of the SMS entity.

Problem

$provider->send() and $provider->incoming(), have $options parameter.

SMS entity has innate options, but SMS standard class does not.

Proposal

In any case I thought Id put this up for discussion, Is there any reason why this wouldn't work out?

CommentFileSizeAuthor
#9 message-options-2712521-9.patch20.24 KBdpi
#5 message-options-2712521.patch20.25 KBdpi

Comments

dpi created an issue. See original summary.

almaudoh’s picture

almaudoh commented

Really, the uses of $options in the original sms_send() as for sender and gateway. Since those are now obsolete with the introduction of the SMS Provider and SmsMessage classes, we can remove options from the Provider classes. So +1 from me for the proposal in the IS.

dpi’s picture

dpi
Location Perth, Australia
commented

Fantastic! DX++

dpi’s picture

dpi
Location Perth, Australia
commented
Issue summary: View changes

Added get/setGateway info to OP

dpi’s picture

dpi
Location Perth, Australia
commented
Status: Active » Needs review
StatusFileSize
new message-options-2712521.patch20.25 KB

https://github.com/dpi/smsframework/pull/32

Status: Needs review » Needs work

The last submitted patch, 5: message-options-2712521.patch, failed testing.

The last submitted patch, 5: message-options-2712521.patch, failed testing.

The last submitted patch, 5: message-options-2712521.patch, failed testing.

dpi’s picture

dpi
Location Perth, Australia
commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new message-options-2712521-9.patch20.24 KB

fixed a test setting gateway via setOption

  • dpi committed 1c0a3d2 on 8.x-1.x 
    Issue #2712521 by dpi: Realigned optional SMS message parameters...
dpi’s picture

dpi
Location Perth, Australia
commented
Status: Needs review » Fixed
almaudoh’s picture

almaudoh commented

Just looking at the patch again, I noticed that you changed a test where the message is sent to the default gateway. Do we have coverage for this elsewhere? I don't have time right now to sift through the whole code base.

Also, while this is a good change, it presents a challenge to the Rule-Based SMS Provider module which will have to create multiple new instances of the SmsMessage for each corresponding gateway (similar to the chunking problem) which may not necessarily be optimal.

Interesting to see how that can be solved...

The last submitted patch, 5: message-options-2712521.patch, failed testing.

Status: Fixed » Needs work

The last submitted patch, 9: message-options-2712521-9.patch, failed testing.

dpi’s picture

dpi
Location Perth, Australia
commented
Status: Needs work » Active

I noticed that you changed a test where the message is sent to the default gateway.

Do you mean testSmsSendSpecified? I removed half of that test because it was testing default behaviour (overcoverage).

a challenge to the Rule-Based SMS Provider

Sorry, I'm not very familiar with the module. Im not sure I understand is the issue that needs to be resolved.

dpi’s picture

dpi
Location Perth, Australia
commented

Do we have coverage for this elsewhere?

To answer your question properly, SmsFrameworkProviderTest::testSend has coverage for normal behaviour.

almaudoh’s picture

almaudoh commented

Sorry, I'm not very familiar with the module. Im not sure I understand is the issue that needs to be resolved.

Module is here. But no worries...that will be tackled in that project's issue queue.

dpi’s picture

dpi
Location Perth, Australia
commented
Status: Active » Fixed

Alrighty then

Status: Fixed » Closed (fixed)

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