[D7] SMSGlobal

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Your git clone is wrong, you must give the public account, not yours:

git clone --branch 7.x-1.x torpy@git.drupal.org:sandbox/torpy/2086401.git sms_smsglobal

Maybe you should create a new .inc file where adding all your non hook functions. Your .module is too big, it should just contain hooks.

In line 41 you have:

'inc' => dirname(__FILE__) . '/sms_smsglobal.parser.inc',

instead you should use

drupal_get_path('module', 'sms_smsglobal') . '/sms_smsglobal.parser.inc' ;

Personally I would include the .inc in a separate folder like /includes.

In function sms_smsglobal_execute_send you have several returns, like line 343, 276 and 350. A function should only contain one return at the end, play assigning the variables instead of having several break points in your function.

This same function looks quite big, it has several functionalities that I'd personally move into another function (I use to prefer to have the functions under 100 lines. For example, command switch could be encapsulated into a function itself which would return the sms object. I would call it get_sms_object, for example, but it is just a suggestion.

In your uninstall file, why are you setting the "sms_bootstrap_routes" variable when you are uninstalling the module?

Lastly, review the coding standards with pareview tool for example. I did not run it in your code but you have lines too big, some of your functions return something but it is not properly documented in the function header (in the parser).

Thank you for your work.

Hi torpy,

thanks for your changes. Please add some manual reviews to your issue summary so other developers can join the discussion about your module.

Thanks again.

Please find my initial review comments:

1. Can you please let me know the need of adding variable_set inside hook_uninstall.
2. Also check string length after triming the string inside sms_smsglobal_admin_form_validate function.

Automated code checkes return clean.

Manual review (unable to test with SMS provider):
- (minor) There is a configuration path (admin/smsframework/gateways) which is not suggested to the user in hook_install message or in the module list
- sms_smsglobal.inc:59 namespace usage throws warning "Unexpected character in input: '\' (ASCII=92)" and error "Parse error: syntax error, unexpected T_STRING" on older PHP
- same for sms_smsglobal.inc:113 , see https://drupal.org/node/1322960 and try to find a way to avoid this on PHP 5.2 or lower (https://drupal.org/requirements) or set this requirement in module.info: "php = 5.3"

Changed the Git command for cloning.

Hi Torpy,

Some finding after manual review.

1 . Unset variable using variable_del function instead setting it empty in install file

function sms_smsglobal_uninstall() {
  // Remove our custom variable from the sms_bootstrap_routes variable.
  // @see sms_smsglobal_menu()
  $routes = variable_get('sms_bootstrap_routes');
  unset($routes['sms/smsglobal/incoming']);
  variable_set('sms_bootstrap_routes', $routes);
}

2. sms_smsglobal_admin_form is missing submit handler. Use form submit handler instead of validate handler to set form values.

Cheers,
Anil Sagar

Hi Chinthaka

In your javascript code under sms_smsglobal.admin.js:

1. sender_id_transformations is a JavaScript object and all the general rules for literals apply and hence you cannot just have numeric keys. So wrap the keys 1,2 with quotes as '1', '2'. This is also because of the fact that properties are referred as obj.prop and you cannot refer obj.0 or obj.1. If you must have numeric keys, please user a native array instead of object.
2. Also, on line 90, where the anonymous function passed as a callback to 'once' ends, there's a missing semi-colon.

Overall, things look fairly good. The only things that are really a git vetted blocker at this point are the variable_get logic. The rest is simply feedback from one person's opinion.

.info file is missing a version.
version = 7.x-1.x

function sms_smsglobal_uninstall() {
  // Remove our custom variable from the sms_bootstrap_routes variable.
  // @see sms_smsglobal_menu()
  $routes = variable_get('sms_bootstrap_routes');
  unset($routes['sms/smsglobal/incoming']);
  variable_set('sms_bootstrap_routes', $routes);
}

This probably should use variable_del().

function sms_smsglobal_menu() {
...
  $routes[$incoming_path] = array(
    'inc' => dirname(__FILE__) . '/includes/sms_smsglobal.parser.inc',
    'parser class' => 'SmsSmsglobalParser',
  );

This should module_load_include rather than dirname().
Actually, i'm not seeing where that variable is ever used. Can it be removed?

I see catch (Exception $e) in several places. Usually this in too broad an approach. Maybe try catching one of the more appropriate exceptions at https://github.com/smsglobal/rest-api-client-php/tree/master/Smsglobal/R....

I also see throwing a generic Exception. Doing that makes it very difficult later on to identify issues as the only way to catch it is by catching Exception. Perhaps create an exception class or reuse from above.

@torpy, I stand corrected on the version info: https://drupal.org/node/542202#version.

Otherwise this is ready for RTBC.

Looks good @torpy! Next step is to wait for a git admin to perform final review and grant access.

Checked for security, use of drupal api, licensing, and individual account.

I think this is something that definitely needs to be fixed, but your understanding of the Drupal API is clearly good, so this is not a blocking issue.

1. sms_smsglobal_menu() should definitely not be setting any variable values, this function is only called when Drupal is building menus. If you need a value initialized the first when this module is installed, it should be in hook_install() or hook_enable(). Don't use hook_init(), that will be called on every page request.

Non-blocking issues:

• Some minor style issues reported by http://pareview.sh/pareview/httpgitdrupalorgsandboxtorpy2086401git
• The project page could use more detail
• Where are those IP addresses coming from in SMS_SMSGLOBAL_DEFAULT_IP?
• You could move sms_smsglobal_admin_form() to a separate admin file, this will improve performance slightly
• Be careful when using is_numeric(), this returns true for floats, hex, octal, &c, as well. ctype_digit() is more restrictive

Thanks for your contribution, torpy!

I updated your account so you can 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 stay 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.

Thanks to the dedicated reviewer(s) as well.