[D6] Sms payments Mobio

Hi,

Let me apologize in the name of the Drupal community, the review process is quite long. They are trying to make it better here.

Concerning your module, are you sure it is not possible to do this with Rules or triggers/actions ?

Indeed, sorry about the long delay! I will try to get your application on the fast track!

On a side note, I would think this could certainly be implemented in a Rules module, which would be by far a best option in my opinion.

Please remove LICENSE.txt, that will be added when your project is promoted.

Just to let you know, you are likely committing with a git application on windows, correct? \r is being added to your commits in your repository, and this will make it very difficult for you to create patches for other projects in the future. Please visit http://help.github.com/line-endings/ for information on how to correct this.

You should remove README.html and keep README.txt.

Please run your module through Coder module (http://drupal.org/project/coder) on minor (most). Ignore CVS tag warnings, and remove all CVS tags like // $Id$ and similar.

Be sure to read over coding standards documentation if you haven't already (http://drupal.org/coding-standards). Namely, read about documentation and commenting. You need an @file comment in each module file, and comments should be in sentence format, beginning with a space, capital letter, and ending with a period. For good examples look at core module files.

In hook_uninstall() you need to remove any variables that are set with your module by using variable_del().

By the way, why did you opt to have roles be expirable?

Regardless of whether it is implemented through Rules, I think this module provides a useful and unique feature for Drupal sites. It's certainly not a requirement that modules that can be implemented with Rules or triggers/actions use those methods, and knowledge of those APIs is not part of the application review process.

I'm moving this back to "Needs work" until the issues in #6 are fixed. We don't currently use the priority field in this queue (we are working on it), but consider your application prioritized. Please fix the issues that I mentioned and set this issue back to "Needs review" once completed. Once that is done I'll do a more thorough review and try to get you through the process.

We had a talk about prioritization, and I updated documentation here. As per the new documentation I'm moving this issue to critical, though I'll still be completing the review.

Thanks for the update, I will check it out.

By the way, don't forget to check out th line-endings link I posted above. It will be important to resolve that issue so you can create patches for other modules and so others can patch your module. If this is not done then patches will remove all lines of a file because git sees the \r even though you can't.

All functions need to be documented.

Drupal coding standards (http://drupal.org/coding-standards) dictate that if statements need to have brackets.

Variables should be lower case and underscores only, no camelCase

Makes good use of t() and database placeholders. Looks secure.

Doc blocks must not have a blank line between the documentation and the function. Php only associates a comment with a function if the function is on the next line.

Your first function documentation, properly formatted according to standards, should look like this

/**
 * Implementation of hook_block().
 *
 * @param string $op
 *   One of "list", "view", "save" and "configure".
 * @param integer $delta
 *   Code to identify the block.
 */

... Although common hook implementations do not need parameters, Drupal will associate the comment with the hook as it is.

92 drupal_set_message(t("<p>The code is valid! Your account has access to <strong>$dt_plus_duration</strong>.</p>"), $type = 'accept'); This needs to use placeholders for the variable. Variables should not be placed directly in t()

Line 209: Inline // comments (on the same line as code) are generally discouraged.

Fix these issues and set this back to needs review once complete. I think you'll be pretty close!

Just one issue I see has not been fixed. I referred to this in my last comment

  drupal_set_message(t("<p>The code is valid! Your account already has access to <strong>$new_role_exp</strong>.</p>"), $type = 'accept');

Needs to be

  drupal_set_message(t('<p>The code is valid! Your account already has access to <strong>%expire</strong>.</p>', array('%expire' => $new_role_exp)), $type = 'accept');

Which is what I meant by using placeholders. Even more, you could avoid using tags in t() if possible. So you could do something like

$message = '<p>'. t('The code is valid! Your account has access to <strong>%expire</strong>.', array('%expire' => $new_role_exp)) .'</p>';
drupal_set_message($message);

Implements menu_hook(). should be Implements hook_menu(). must have missed that!

Aside from those small items, all functions are documented, files have an @file docblock, the module uses t() and database queries use placeholders properly. I don't see any security vulnerabilities. Variables are removed in hook_uninstall(). The README.txt file is well written.

You have been in the review process for a long time, and have been responsive to my comments.

Considering these points, I'm marking this RTBC. Just fix the line that I mentioned. A git administrator will be along (usually in a couple days or so) to review these comments and grant you the git vetted user role.

Hi svetlio,

I added a couple issues in your project issue queue, nothing that needs to hold up moving this to a full project.

I went to grant you the git vetted user role, but it looks like someone else already did that. You should now be able to promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks for your contribution and 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. As someone who has recently completed an application, your input would be especially useful in the code review group as we work to improve this process.