Add provider Cardboardfish

[kbsali]

this!

[K-Phoen]

Thanks for your PR!

I'll definitely merge it, but before that there are a few things that I'd like you to fix:

• see the comments I left in the code ;
• you made changes not directly related to the new provider, could you explain them to me? (I saw some changes in the Curl adapter) i.e.: the cs-related changes should be in another PR
• how do you handle the statuses? I saw that you created a new STATUS_INFO constant that isn't supported by any other provider, what is its meaning?

[kbsali]

see the comments I left in the code ;

• yep, all fixed.

you made changes not directly related to the new provider, could you explain them to me? (I saw some changes in the Curl adapter) i.e.: the cs-related changes should be in another PR

• Curl Adapter : the only change is to "support" $method = 'GET' as well as POST.

how do you handle the statuses? I saw that you created a new STATUS_INFO constant that isn't supported by any other provider, what is its meaning?

• For this provider the "getStatus" method is not related to any specific message, so I thought that I'm just converting the statuses of X messages in a "status_info" array key together with "status => info" (see the tests).

[K-Phoen]

The CurlHTTPAdapter already supports POST and GET HTTP methods. I see in the code that if the method is GET, you append the data to the url. I don't like that because it's not consistent with behavior described by the HttpAdapterInterface. Moreover, if you look at other providers you'll notice that they don't work this way. This kind of stuff should be handled at the provider level IMHO. Btw, why bothering to change the GET handling if you only use POST?

Concerning the status thing and if I understand well, the STATUS_INFO constant will only be used if you call the getStatus() method. As this method isn't part of any public interface exposed by the library, there is no reason for this constant to be defined in ResultInterface. I think that you should refactor a bit your provider and in particular split the parseResults() method (see the comments I left in your code).It's okay for getStatus() and send() to return different results.

Last thing: could you remove all the cs-related commits from this PR? You can re-submit them in a new PR if you want then to be merged.

Thanks for your work :)

[K-Phoen]

I took the liberty to clean a bit your PR. You can see the result here : https://github.com/Carpe-Hora/SmsSender/commits/pr-11

Do you think it's mergeable?

[kbsali]

please be my guest! :) The changes look good to me. I sort of agree with you with most of the points and appreciate that you want to keep it nice and clean, but to be honest I don't understand the need to undo the CS fixes.

Also, i'm ok with removing the STATUS_INFO constant, even though i don't understand why you would not want this added to the ResultInterface : for some providers (Cardboardfish being one of them) the "status" api returns the status of the queue, no of a specific message, meaning that sent / delivered / failed / queued don't apply.

But that's good by me!

I'm about to create a PR for the Indian provider ValueFirst

[K-Phoen]

I "undid" the CS fixes to keep the PR focused on one thing. Don't worry, I'll hunt the CS right now ;)

Concerning the STATUS_INFO stuff, we need a real and unified support for this kind of feature through all the providers and the ResultInterface isn't the right place for this. I should have time to work on this in the next days.

Thanks again for your PR! Looking forward to the next one.