[D7] Total Subscription

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxdeepakaryan1988211307...

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.

Here is correct url to results of preview.sh review tool:
http://pareview.sh/pareview/httpgitdrupalorgsandboxdeepakaryan19882113077git-0

When i click on "site_name/?q=verify" link get the error message:

Notice: Undefined index: entity_type in total_subscription_verification_mail() (line 245 of /home/sygnetica/projects/contrib/birthday_inform.dev/sites/all/modules/total_subscription/total_subscription.module).

Notice: Undefined index: 1 in total_subscription_verification_mail() (line 247 of /home/sygnetica/projects/contrib/birthday_inform.dev/sites/all/modules/total_subscription/total_subscription.module).

Hi,

I took the liberty of installing the module locally and reviewing the code. I could not find any errors, which is brilliant. I do however have the following suggestion:

perhaps code such as:

/**
 * Implements hook_block_info().
 */
function total_subscription_block_info() {
  $blocks = array();
  $blocks['susbcription_block'] = array(
    'info' => t('Subscription Block'),
  );

  return $blocks;
}

Could be moved into a .inc file? Perhaps blocks.inc? This just helps in future for maintainability of the code, particularly for third party developers who might want to extend your module.

Other than that, great module!

Cheers,
Adam

@deepakaryan1988 Mails not getting sent if a content is unpublished and then published back. The module is missing the entry to $messages array in hook_mail for the email key being used to send emails in that case.

Thank you for the contribution. Just had a look at the code, it seems to be fine with no errors.
However, the name of the ctools content type in your module could conflict with the ctools content types of other modules.
Just changing the name from "subscription.inc" to "total_subscription.inc" would be enough to get this through. Not sure if it is a blocker, hence not changing the status.

Tested. No serious issues found. Works as expected.

removed copies of automated reviews from the issue summary.

Review of the 7.x-1.x branch:

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/total_subscription.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 7 WARNING(S) AFFECTING 7 LINE(S)
  --------------------------------------------------------------------------------
   659 | WARNING | Variable $taxonomy_names is undefined.
   843 | WARNING | Unused variable $options.
  --------------------------------------------------------------------------------
  

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. You have to get a review bonus to get a review from me.

manual review:

1. what are the differences to the subscriptions module? Please add that to the project page so that users can make an educated decision which module to use.
2. total_subscription_uninstall(): drupal_uninstall_schema() is not necessary in hook_uninstall() in D7.
3. total_subscription_schema(): what is the hash used for? the hash of what? Please improve the description.
4. "'access arguments' => array('Total Subscription Permission'),": permission names are lower case usually. Also elsewhere.
5. total_subscription_permission(): the first description of the permission is bad, what site?
6. total_subscription_menu(): "verify" as path could easily clash with other stuff on the site, maybe prefix it with totalsubscription or similar?
7. "'placeholder' => array('Please Enter your Email.'),": all suer facing text must run through t() for translation. Please check all your strings, most of them are not translated.
8. The "verify" path shows up in my menu tree, maybe this should be of type MENU_CALLBACK?
9. "t('Subscribe to these kind of') . $contexts_values->data->type,": do not concatenate variables to translatable strings, use placeholders with t() instead.
10. total_subscription_subscribe_form(): If a form has a submit function, then hidden form values are not needed. Instead, any values that you need to pass to $form_state['values'] can be declared in the $form array as '#type' => 'value'.
11. When I'm not an a node page the block just shows a subscribe button and then throws notices upon submission: Notice: Undefined index: entity_type in total_subscription_subscribe_form_submit() (line 551 of total_subscription.module). Just leave the block content empty on other pages, then it will not be displayed.
12. total_subscription_node_mail(): you are sending the mail to the site owner? Why? Where do you actually send out all the emails to the users?
13. total_subscription_mail(): do not duplicate hook documentation on your doc block.
14. total_subscription_mail() should only do the templating of the message; all the receivers should be determined in total_subscription_mail_send().
15. total_subscription_mail_send(): this is vulnerable to access bypass exploits. Where do you check that the receiver even has access to the node? I don't see any node_access() calls? Please also note that such an access check might not scale depending on how many subscribers you have, so I think it would be a good idea to postpone the mail sending using the queue API and let it be processed on cron runs. Otherwise your node saves could run into timeouts. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
16. total_subscription_mail(): instead of using foreach after a query to loop over all results you can just use ->fetchCol() to just retrieve the mails as one array directly.
17. Your module file is quite large, have you considered to move your forms and page callbacks into include files?

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

manual review:

1. 'Admin Settings for Mail': that permission name has still upere case letters in it.
2. "'#title' => 'Select Content Types:' ,": all user facing text must run through t() for translation. Please check all your strings.
3. total_subscription_get_tids(): the while loop is not necessary, just use fetchCol() on the result. Same in total_subscription_mailing_queue_callback().
4. total_subscription_bitly_shorten(): why do you need cURL and cannot use drupal_http_request()? cURL might not be available on all servers.
5. total_subscription_mailing_queue_callback(): still misses a node access check. If I create a node that is published but protected by some node access module like content_access, then anyone subscribed to the content type or a taxonomy term used on the node will get an email. The same happens if you some internal private content type that uses the same taxonomy terms from public content - people will get emails about nodes they don't have access to. So the access bypass issue is still present. The minimum you could do is call node_access() with drupal_anonymous_user() to make sure to only send out emails for content that is accessible to any anonymous user. That could even be done in the node insert/update hook before even queuing the node.
6. "variable_del('email_template');": all variables defined by your module need to be prefixed with your module's name. Applies to some other variables as well.

manual review:

1. total_subscription_uninstall(): all variables defined by your module need to be prefixed with your module's name. Please check all your variables.
2. total_subscription_permission(): doc block is wrong - this is a hook implementations, see https://drupal.org/node/1354#hookimpl

But otherwise looks RTBC to me now.

Assigning to ELC as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, deepakaryan1988!

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.