What it does

This module provides integration between the SMS Framework module and the SMSGlobal SMS gateway. It has support for sending SMSes and also implements a callback for receiving them.

Project Page

https://drupal.org/sandbox/torpy/2086401

Git

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

Comments

PA robot’s picture

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.

urwen’s picture

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.

torpy’s picture

Thanks for the review! I've fixed up the Git command, that was silly of me.

Good point on the include files, I've moved all non-hooks into includes/sms_smsglobal.inc and put the parser file in there too.

The reason I'm using 'inc' => dirname(__FILE__) . '/sms_smsglobal.parser.inc', is because the SMS framework module explicitly asks for it to be that way. The main reason is because it has an incoming callback bootstrap file that may be loaded prior to Drupal bootstrap (and hence won't have access to drupal_get_path). More in its README: http://drupalcode.org/project/smsframework.git/blob/refs/heads/7.x-1.x:/....

I've also split sms_smsglobal_execute_send() into a few sub-functions to make it more legible.

As for multiple returns, I'm a big proponent of them :) I personally feel having massive if statements and blocks wrapping code makes it harder to read. Happy to change this if it's breaking any Drupal coding standards though.

Also, the reason for setting sms_bootstrap_routes in my uninstall hook is because I add a variable to it in hook_menu. Again, this is a requirement of the SMS Framework and there's very little I can do about it, as unwieldy as it is (there's more about this in the README I linked to above).

Finally, I did run it against pareview and had no issues returned. The parser return specifically is documented in the base class, hence the {@inheritdoc} in the function docblock. As for line length, it looks like it's fine according to the Drupal coding standards, as most of them are due to variable assignments.

torpy’s picture

urwen’s picture

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.

gaurav.pahuja’s picture

Status:Needs review» Needs work

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.
torpy’s picture

Thanks for the review gaurav.pahuja! I've fixed issue #2, thanks for picking that up!

Also, as far as #1 is concerned, I briefly touched upon in my earlier reply to urwen. The gist of it is that the SMS framework has a the (optional) ability to hit the incoming SMS callback prior to the Drupal bootstrap by (in it's words) abusing the cache router.

In order for my gateway module to support this, I need to add something to the sms_bootstrap_enabled variable. Once I've added it, I also need to make sure that I remove it when the module is uninstalled. Now sms_bootstrap_enabled is an array that is shared by all gateway modules unfortunately. So in order to remove my array element from it, I need to load it, unset my key and save it again.

Hope that answers your question! :)

torpy’s picture

Status:Needs work» Needs review

Forgot state!

torpy’s picture

Issue summary:View changes

Updated git command to show non-maintainer version.

softescu’s picture

Status:Needs review» Needs work

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"

softescu’s picture

Issue summary:View changes

Changed the Git command for cloning.

torpy’s picture

Issue summary:View changes
Status:Needs work» Needs review

Sorry, I've been away and was swamped at work.

Thanks for the review! I've just fixed those issues (added the 'configure' and 'php' params to the info file).

torpy’s picture

Issue summary:View changes
anil614sagar’s picture

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

anil614sagar’s picture

Status:Needs review» Needs work
torpy’s picture

Hi Anil, thanks for the review!

1. I can't do a variable_del for that particular variable as it's shared by all SMS framework gateway module, see my reply in #7 for more.

2. sms_smsglobal_admin_form() does have a submit handler, just not one defined in this module. The configure form (and its submit) are handled by the SMS framework module, see sms_smsglobal_gateway_info().

torpy’s picture

Status:Needs work» Needs review
rmn’s picture

Status:Needs review» Needs work

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.
torpy’s picture

Status:Needs work» Needs review

Brilliant, thanks for the review Raman! Those were some really good points, can't believe I missed that semicolon in #2 :)

I've made and pushed those changes through!

heddn’s picture

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.

heddn’s picture

Status:Needs review» Needs work
torpy’s picture

Hi Lucas,

Thanks for the review, I've added the version number in.

As for that variable, as I've mentioned in #7, it's a shared variable set by the smsframework module (which this one is implementing a gateway for). As such, I can't delete the entire variable, it may be used by other gateway modules. I can only remove the element I added, hence the variable_set.

The reason I'm using dirname is also related to this, the set route is used within a very minimal Drupal bootstrap. The use of dirname is explicitly recommended in the smsframework module: http://drupalcode.org/project/smsframework.git/blob/refs/heads/7.x-1.x:/... However, I've changed this to drupal_get_path, which essentially does the same thing anyway :)

As for the exceptions, good catch! I've changed those to be more granular and used one of the SMSGlobal ones rather than throwing a generic one. I did have to leave one generic catch call in, as the SMS framework module expects errors to be returned not thrown (in sms_smsglobal_execute_send()).

torpy’s picture

Status:Needs work» Needs review
heddn’s picture

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

Otherwise this is ready for RTBC.

torpy’s picture

Status:Needs review» Reviewed & tested by the community

Great, just removed that and setting this to RTBC. Thanks! :)

heddn’s picture

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

kscheirer’s picture

Status:Reviewed & tested by the community» Fixed

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:

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.

torpy’s picture

Brilliant, thank you!

Thanks for the suggestions too, I'll make those changes as soon as I can

Status:Fixed» Closed (fixed)

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