Description: ------------ I submitted this a while back, but there was a disagreement over a patch for the Net_SMPP core. So, here's the plain SMPP driver for Net_SMS, split from the core changes. http://atomized.org/PEAR/Net_SMS/generic_smpp.phps Please let me know if this works for you, or if you want anything changed. I could also propose it as a subpackage (Net_SMS_SMPP?), since it will bloat the package dep. chain (it requires Net_SMPP_Client, Net_Socket, and Net_SMPP).

Looks OK for me (I did not enter in details of methods) Just, if you want to take advantage of factory or singleton and keep the PEAR's CS, you cannot use _ in your extension. The factory does only one level ... so , I think Net_SMS_SMPP suits better. By *not* requiring 'Net/SMS.php', do you want to force the user to use Net_SMS:factory() ? What is the intention in added method accept() ?

"...if you want to take advantage of factory or singleton and keep the PEAR's CS, you cannot use _ in your extension. The factory does only one level ... so , I think Net_SMS_SMPP suits better." That's fine by me, but I was just following the style currently used by Net_SMS. Some of the other drivers are: generic_smtp, win_http, clickatell_http and so on. "By *not* requiring 'Net/SMS.php', do you want to force the user to use Net_SMS:factory() ?" Not necessarily, I just wasn't aware that you wanted to support direct instantiation. I can add it if it's an issue. "What is the intention in added method accept() ?" This is passed through to Net_SMPP_Client, and allows you to feed it an instance of Log to see the SMPP message flow.

Looks good. - The naming is fine. - You should fix the class phpdoc description. - Not require'ing Net/SMS.php is fine too, we don't do this in the other drivers either. - The dependencies could be solved with optional dependencies and feature packages with package2.xml. - gettext strings (in getParams()) should be in double quotes, and in this method be more descriptive. - phpdoc for _send() needs to fixed. - Error conditions in _send() should return PEAR::raiseError(). - _send() should not return by reference. - _authenticate() shouldn't echo. - accept() should be documented.

"- The naming is fine. - You should fix the class phpdoc description. - Not require'ing Net/SMS.php is fine too, we don't do this in the other drivers either." Ok. "- gettext strings (in getParams()) should be in double quotes, and in this method be more descriptive." Why? Single- and double-quotes are functionally identical. "- phpdoc for _send() needs to fixed." Fixed. "- Error conditions in _send() should return PEAR::raiseError()." Can you more clearly define "error conditions?" There appear to be error conditions which return an array instead of PEAR_Error the other drivers. "- _send() should not return by reference. - _authenticate() shouldn't echo. - accept() should be documented." Fixed.

> "- gettext strings (in getParams()) should be in double > quotes, and in this method be more descriptive." > > Why? Single- and double-quotes are functionally identical. Consistency and style. > "- Error conditions in _send() should return > PEAR::raiseError()." > > Can you more clearly define "error conditions?" There > appear to be error conditions which return an array > instead of PEAR_Error the other drivers. Oops, you are right. I looked at the drivers that return PEAR_Error's and these need to be fixed. The @return phpdoc for _send() too, btw.

> - The naming is fine. Fine for Net_SMS but not for PEAR CS, the existing drivers names are not CS conforming ... as the SMS class loader is not conform, (easy to bring it CS compat with extra includes ). But, at least, do it right for new add-ons ... I don't mean it's not working, a pure Net_SMS::factory() is fine about it. But, as PEAR intends to automatically translate '_' in '/', something is wrong here ... and historical drivers could not guess it. Add-ons don't have to guess ... Sorry, I will stick on Net_SMS_SMPP ... or Net_SMS_GenericSMPP if you really want "generic", but I feel Net_SMS_SMPP is allready generic enough.

> But, as PEAR intends to automatically translate '_' in '/', something > is wrong here ... and historical drivers could not guess it. Add-ons > don't have to guess ... That's plain wrong. PEAR doesn't translate anything. It's just a convention, nothing more. And we discussed in the Horde project how we want to name the Net_SMS drivers, and this was the best solution, and the naming style we agreed upon.

np for me, the double '_' is consistant here.

I have cleaned up the driver a bit and committed it to Horde's CVS. It's not fully functional yet though, because the bind and submit parameters are not correctly documented in getParams() yet. 'array' is not a valid parameter type, instead all parameters the Net_SMPP_Client has, need to be listed one by one including a proper label. Can you please checkout the current version of the driver from Horde's CVS and provide a patch for getParams()?

Are you going to provide an update?