eztexting_gateway

welcome

please don't put both modules into the same branch:

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• README.txt is missing, see the guidelines for in-project documentation.
• The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
  

  ./sms_eztexting-6.x-1.0/sms_eztexting.test
  

• ./sms_eztexting-6.x-1.0/sms_eztexting.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function sms_eztexting_help($path, $arg) {
  function sms_eztexting_gateway_info() {
  function sms_eztexting_admin_form($configuration) {
  function sms_eztexting_admin_form_validate($form, &$form_state) {
  function sms_eztexting_send_form() {
  function sms_eztexting_validate_number($number, $options) {
  function sms_eztexting_send($number, $message, $options) {
  function sms_eztexting_command($command, $data = array(), $config = NULL) {
  function sms_eztexting_parse_auth_response($http_response) {
  function sms_eztexting_parse_send_sms_response($http_response) {
  function sms_eztexting_country_codes() {
  function is_successful_response($code) {
  

• ./sms_eztexting-7.x-1.x-dev/sms_eztexting.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function sms_eztexting_help($path, $arg) {
  function sms_eztexting_gateway_info() {
  function sms_eztexting_admin_form($configuration) {
  function sms_eztexting_admin_form_validate($form, &$form_state) {
  function sms_eztexting_send_form() {
  function sms_eztexting_validate_number($number, $options) {
  function sms_eztexting_send($number, $message, $options) {
  function sms_eztexting_command($command, $data = array(), $config = NULL) {
  function sms_eztexting_parse_auth_response($http_response) {
  function sms_eztexting_parse_send_sms_response($http_response) {
  function sms_eztexting_country_codes() {
  function is_successful_response($code) {
  

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./sms_eztexting-6.x-1.0/sms_eztexting.info:       ASCII English text, with CRLF line terminators
  ./sms_eztexting-7.x-1.x-dev/sms_eztexting.info:   ASCII English text, with CRLF line terminators
  sms_eztexting-6.x-1.0/sms_eztexting.info
  sms_eztexting-7.x-1.x-dev/sms_eztexting.info
  

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See http://ventral.org/pareview/httpgitdrupalorgsandboxeztexting1393990git.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.

Manual review of the 7.x-1.x branch:

• There are 2 .info files, where I think the eztexting_gate.info is obsolete
• Remove the Developed by in sms_eztexting.module, you can give credits to the maintainers in the README.txt
• Same for @author in functions
• In the README.txt you have the path admin/build/modules for D7 the correct path is admin/modules
• Is it necessary to drop the module in the smsframework directory (instead of the normal modules directory)?
• In the function sms_eztexting_admin_form() you make use of $configuration, I think variable_get() is a more proper way to save the configuration

I'm taking a look at the D7 branch. First up, note the remaining complaints from the code style checker:

http://ventral.org/pareview/httpgitdrupalorgsandboxeztexting1393990git

The following git branches do not match the release branch pattern, you should remove/rename them. See http://drupal.org/node/1015226

remotes/origin/6.x-1.0

That should be 6.x-1.x.

FILE: ...iew/sites/all/modules/pareview_temp/test_candidate/sms_eztexting.module
--------------------------------------------------------------------------------
FOUND 24 ERROR(S) AFFECTING 24 LINE(S)
--------------------------------------------------------------------------------
26 | ERROR | There must be no space before the colon in a DEFAULT statement
47 | ERROR | Function comment short description must end with a full stop
54 | ERROR | Additional blank line found at the end of doc comment
83 | ERROR | Function comment short description must end with a full stop
88 | ERROR | Last parameter comment requires a blank newline after it
90 | ERROR | Additional blank line found at the end of doc comment
100 | ERROR | Function comment short description must end with a full stop
104 | ERROR | Additional blank line found at the end of doc comment
119 | ERROR | Function comment short description must end with a full stop
129 | ERROR | Additional blank line found at the end of doc comment
160 | ERROR | Additional blank line found at the end of doc comment
168 | ERROR | Function comment short description must end with a full stop
181 | ERROR | Additional blank line found at the end of doc comment
202 | ERROR | BREAK statements must be followed by a single blank line
206 | ERROR | BREAK statements must be followed by a single blank line
207 | ERROR | There must be no space before the colon in a DEFAULT statement
230 | ERROR | BREAK statements must be followed by a single blank line
233 | ERROR | BREAK statements must be followed by a single blank line
234 | ERROR | There must be no space before the colon in a DEFAULT statement
248 | ERROR | Function comment short description must end with a full stop
271 | ERROR | Function comment short description must end with a full stop
313 | ERROR | Function comment short description must end with a full stop
326 | ERROR | Function comment short description must end with a full stop
333 | ERROR | Additional blank line found at the end of doc comment
--------------------------------------------------------------------------------

Comments on the 7.x-1.x branch:

---

"Implements of hook_" should be "Implements hook_"

---

You should use t() for text strings, and consider breaking up the really long HTML strings. In addition to being good practice, it allows you to break out more important portions of strings as parameters, which tends to make things easier to maintain. For example, you have:

return '<p><strong>Create an Ez Texting account:</strong> <a href="http://www.eztexting.com">Ez Texting</a> is free to try and inexpensive to use for your text messaging needs. <a href="http://www.eztexting.com/group-sms-pricing.html">See our pricing</a>, and <a href="http://www.eztexting.com/signup.php">start your free trial now</a>. In order to use Ez Texting within Drupal you will need to activate API on your account. You can do so by <a href="http://www.eztexting.com/ticketing-contact.php">contacting support</a> to let them know you\'re using the Drupal plugin.</p>
                    <p>Please note when using Ez Texting: We do not allow illegal, obscene or sexually oriented messages. Message can be up to a max of 160 characters. We do not recommend using non-standard characters such as but not limited to ~ or { or }.</p>';

The sort of thing might be built as:

$tokens = array(
  '@et' = 'Ex Texting',
  '!et_link' => l(t('Ez Texting'), 'http://www.eztexting.com'),
  '!pricing_link => l(t('See our pricing'), 'http://www.eztexting.com/group-sms-pricing.html'),
  '!trial_link => l(t('start your free trial now'), 'http://www.eztexting.com/signup.php'),
  '!support_link => l(t('contacting support'), 'http://www.eztexting.com/ticketing-contact.php'),
);

$help_html = '<strong>' . t('Create an @et account:', $tokens) . '</strong>';
$help_html .= t('!et_link is free to try and inexpensive to use for your text messaging needs. !pricing_link, and !trial_link. In order to use Ez Texting within Drupal you will need to activate API on your account. You can do so by !support_link to let them know you\'re using the Drupal plugin.', $tokens);
$help_html = '<p>' . $help_html . '</p>';
$help_html .= '<p>' . t('Please note when using @et: We do not allow illegal, obscene or sexually oriented messages. Message can be up to a max of 160 characters. We do not recommend using non-standard characters such as but not limited to ~ or { or }.', $tokens) . '</p>';

return $help_html;
  

This is one of a number of places where t() should be used at a minimum, and maintainability could be improved along the way.

---

You are using trim(), which is only sort of/most of the time multibyte safe.

  $phone_number = trim($number);

A good alternative to trim is this:

  $phone_number = preg_replace('/^\s+|\s+$/', '', $number);

---

Comments on the D6 branch:

---

The same notes on use of t() and trim() as for the D7 branch apply.

---

There are strlen() calls in there that should be drupal_strlen(). Ditto for substr() that should be drupal_substr(). Iis good practice to behave as though all incoming strings might be multibyte for the sake of consistency in code, and not use functions that might break when working with multibyte encodings.

---

Use preg_replace in place of non-multibyte function str_replace - another multibyte string thing.

  $number = str_replace(array(' ', '+'), '', $number);

Should be:

  $number = preg_replace('/\s|\+/'), '', $number);

---

I see that there is test code in the D6 branch - that should be brought up into the D7 branch as well, and altered if needed to work. It seems a shame not to have the tests ready for D7 given that you've done most of the work in building them already.

Switching status to needs work.

We have currently exceeded the proposed project application thresholds, so this is on hold for me for now. Get a review bonus and I will review/approve this right away.

Your project page is not very detailed, please have a look at the tips for a great project page, you may also use HTML-tags for better structure. (What about a picture?)

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch:

git checkout 7.x-1.x
git branch -D master
git push origin :master

/**
 * Validate phone number.
 *
 * @param string $number
 *   phone number
 *
 * @param array  $options
 *   array of options
 *
 * @return string|void
 *   if phone number not valid return error message else return void
 */

should be

/**
 * Validate phone number.
 *
 * @param string $number
 *   Phone number.
 * @param array  $options
 *   Array of options.
 *
 * @return string|void
 *   If phone number not valid return error message else return void.
 */

watchdog(
    'sms-eztexting',
    'Sending to ' . $request_url . '?' . http_build_query($data, '', '&'),
    NULL,
    WATCHDOG_DEBUG
  );

Have a look at the documentation of watchdog! Similar to t() it has placeholders! Don't build the message directly, use them.

Also your printing raw data directly into watchdog, note that messages are NOT filtered for watchdog! if there's any input that can entered by the user in these variables filter them by check_plain() or use watchdogs's placeholders properly!

correct the watchdog things and I'll approve you.

additionally to patrickd's comments, 7.x branch:

1. Remove the PHP version from the info file, Drupal 7 already depends on PHP 5.2.
2. sms_eztexting_command(): $result is third party provided input and needs to be sanitized before printing into the watchdog() log to avoid XSS vulnerabilities. See http://drupal.org/node/28984 . This is a security issue, therefore I have to set this back to "needs work".

Automated review:

Minor stuff, and complains about uppercase html, which is related to test data.

Manual review:

Looks good, above mentioned problems have been adhered and code looks solid.

I see no reason not to approve you

Thanks for your contribution, EzTexting!

I have updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers as well.

Thanks to the dedicated reviewer(s) as well.