[D7] Mailman Simple

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

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.

@spficklin, thanks for your contribution!

I have manually reviewed your module and below are my assumptions:

1. You have defined all callback functions in mailman_simple.module file, I think you should include .inc file and defined all callback function there.
2. You have called following function on number of places but at some places you forget to check count.

Correct Code:

$lists = mailman_simple_get_lists();
if (count($lists) > 0) {
.....
} 

Incorrect Code:

$lists = mailman_simple_get_lists();
foreach ($lists as $list_id => $info) {
..... 
}

Pointer lines: 548 and 572.

3. On line number 121, you used db_select but before iterating you missed to check count. Check count first to avoid unnecessary notices.

As I analyzed your code, at most of places you have used forloop without checking whether variable contains values or not. Please do this at all places, one more thing add some more comment to following code.

// Iterate through the roles that should be synced and construct
  // a comma-delimited list of role IDs.
  $counter_selected = 0;
  foreach ($sync as $c) {
    if ($c) {
      $counter_selected++;
    }
  }
  $counter_sync = 0;
  foreach ($sync as $s_rid => $s_selected) {
    if ($s_selected) {
      $sync_rids .= $s_rid;
      if ($counter_sync != $counter_selected - 1) {
        $sync_rids .= ",";
        $counter_sync++;
      }
    }
  }

Looks like you are moving loop for same array twice, not sure why it can;t be solved using one loop.

Please fix these.

Again thanks for your contribution!

@er.pushpinderrana

I have inspected his code, and there is no reason to call count within the form submit function. He calls count in the form, because if there are no items, he displays some markup saying there are no items. Within the submit function, he already knows there are items, because you can't submit the form without buttons, which require there were items. But even if he didn't know that, it isn't necessary to check if there are list items, as nothing is sent if there aren't. His code is not 'incorrect'.

On line 121, it is unnecessary to check the count before iterating over the array, since an array with 0 elements is not iterated by foreach.

mailman_simple_subscribe_form_validate does not implement hook_validate(). hook_validate() is from node api, not form api.

likewise, there is no hook_submit

in mailman_simple_callbacks.inc (which would be more appropriately named mailman_simple.callbacks.inc)
on line 418, you shouldn't concatenate onto t(), but instead use a replacement.

is link a form api element type on line 426?

It might be a better idea in mailman_simple_get_lists() to use db_select, and use IN with a list of the user's roles.

In mailman_simple_show_lists()

This

return array(
   '#theme' => 'table',
   '#header' => $header,
   '#rows' => $rows,
   '#attributes' => array(),
    '#sticky' => TRUE,
    '#caption' => '',
    '#colgroups' => array(),
    '#empty' => t('No lists),
);

is preferable to this

$table = array(
    'header' => $header,
    'rows' => $rows,
    'attributes' => array(),
    'sticky' => TRUE,
    'caption' => '',
    'colgroups' => array(),
    'empty' => '',
  );
  return theme('table', $table);

Those issues are fairly minor, I think, so I am RTBCing this.

As far as I can tell, all of the comments above have been about the 7.x-1.x branch, but there is also a 6.x-1.x branch. Has this been reviewed? Is this branch part of the application?

Thanks for the clarification. RBTC is appropriate, as that was the status before I requested some clarification.

Assigning to myself for my next review.

Automated Review

Review of the 7.x-1.x branch (commit aa4f570):

Bleeding-edge PAReview script crashed in PHPSniffer. Online version came up clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation. My call on this is that it is different enough to warrant a separate module.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

(*) I can't get any lists to show up on admin/config/mailman_simple/show after I add them. I can't adequately test this as a result, and it looks like this is
XSS vulnerable, as $list->list_name and $list->description are unfiltered user input. These need to be plained for output. See See https://www.drupal.org/node/28984

Coding style & Drupal API usage

In mailman_simple_schema(), you should have a description for the table and each column. You can also add foreign key definitions (useded by code, but useful for documentation purposes).

Single quoted strings are, in general, preferred. See https://www.drupal.org/coding-standards#quotes

Why do you have two $items['mailing_lists'] in your hook_menu? I think as a result, the 'subscribe lists' permissions ends up being unused?

You have some db_select() in the drush commands that are static queries, and can be converted to db_query(). Also elsewhere in the module.

The drush command should probably check that sync_members command actually exists and error if it doesn't.

In mailman_simple_subscribe_form, is 20 characters really long enough for an email address?

Why doesn't this use the registered email account for the user if they are logged in?

(+) mailman_simple_subscribe_form() needs validation on the email element. See #element_validate and valid_email_address().

(+) mailman_simple_form() also needs validation on the email elements.

(+) mailman_simple_get_lists() is defined in the menu callbacks file, but it is used main module. And to get around this you require the callbacks file?

(+) The forms are in a separate include, but the include is not specified in the menu paths.

mailman_simple_get_lists() gets called a lot. Static caching may help (but I dind't trace out exactly how mahy times it wouldbe called during a page request). See https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/drupal...

(+) Shouldn't mailman_simple_subscribe_form_submit() use the langauge for the current session and not the default language for the site?

(+) In mailman_simple_subscribe_form_submit(), t() will sanitize paramters via format_string(). Explicit filter_xss() are not needed and may lead to double escaping, and filter_xss() is only needed when
you expect the values to have HTML in them that you need to have displayed as HTML. See https://www.drupal.org/node/28984

In mailman_simple_subscribe_form_validate(), can't you just make that form element required, which will eliminate the validation you have?

When you execute db_insert() it should return the id already?

Your hook_mail usage is a little wrong. You normally wouldn't use the $key like that, rather pass in message parts via the $params parameter.

Maybe your add list form should #redirect to the view lists page? That sequence was a little confusing.

mailman_simple_show_lists() is a page callback. You should really just return the render array for the table, and not explicitly call theme().

(+) In mailman_simple_subscribe_form(), I would breakout the title and description into different parts in the checkboxes, #title and #description. See comment above about filter_xss().

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Re, the queries. Use db_query() for simple SELECT queries that are static. Use db_select() for SELECT queries for ones where the conditions and/or fields can change (not talking about parameters here). See https://www.drupal.org/node/310072 and https://www.drupal.org/node/310075. It isn't a rule to use one over the other, but db_query() is faster in general.

Re, mailman_simple_subscribe_form_submit() and the language. The default language for the site may not be the one that that the user is using. Read the second paragraph on https://api.drupal.org/api/drupal/includes!mail.inc/function/drupal_mail/7 I think user_preferred_language() is the better option here, but this may be picking nits. To be honest, I am not sure if Mailman will use the subscribe email's language in any real way.

I think this should go directly into a 7.x-1.x branch of one of the existing mailman modules that seem to be abandoned. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Creating yet another mailman project namespace will just confuse users. Please open an issue in the mailman issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

Once your code is renamed and pushed into one of the existing projects we can continue here with the project application to grant you the git vetted user role, just set the issue back to "needs review" once you are ready. Thanks for your understanding.

As indicated in https://www.drupal.org/node/251466 taking over an abandoned project should only take 2 weeks, after that time you escalate the project ownership issue to the Drupal.org webmasters if you cannot reach the maintainers.

Can you hold on for another 2-3 weeks? You can also open another project application for your other projects and close this one, but that will most likely also take a couple of weeks.

Reviews of the 7.x-1.x branch (commit df87a66):

Automated review came back clean except for the master branch issue mentioned in comment #23.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation. I think this was adequately addressed through take-over of the mlist project.

Master Branch

No: Due to a permissions problem since it's tied to an old release node. Maybe someone with more power in the drupal community can delete the old release node?

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. I didn't see any security issues & it appears the issue mentioned above was fixed (mlist.callbacks.inc:502-503) :)

Coding style & Drupal API usage

(+) I am not sure if hardcoding /tmp is a good idea. file_directory_temp() is probably safer here, and will provide Windows support.

Your mailman sync drush command should output something if is run when no mailing lists are yet registered with the module (ie: if the mailing_lists table is empty).

(+) Add a configuration link to the module page to make for easy finding of your administration pages. See https://www.drupal.org/node/542202. I would link to the view mailing lists page and add an action link (using hook_menu with type MENU_LOCAL_ACTION) to add the "Add Mailing list link to this page.

Not seeing anything to prevent RTBC with this. Verified that code was committed in #2315105: Offering to maintain Mailman Mailing List Management Wrapper. This review was done on http://git.drupal.org/project/mlist.git even though I notice your link in the issue still points to your sandbox. The repo link should be adjusted before final review.

1) In the below code can be improved from

if (in_array($list->role_name, array_values($user->roles)) or
user_access('manage mailing lists')) {
// Don't add the list more than once.
if (!in_array($list->list_id, $mylists)) {
$mylists[$list->list_id] = array(
'name' => $list->list_name,
'description' => $list->description,
'subscribe' => $list->subscribe_email,
'unsubscribe' => $list->unsubscribe_email,
);
}
}

TO

if ((in_array($list->role_name, array_values($user->roles)) || user_access('manage mailing lists')) && !in_array($list->list_id, $mylists)) {
$mylists[$list->list_id] = array(
'name' => $list->list_name,
'description' => $list->description,
'subscribe' => $list->subscribe_email,
'unsubscribe' => $list->unsubscribe_email,
);
}

2) Minor code improvements in mailman_simple_subscribe_form

a) if (count($lists) > 0) { could be changed to if (count($lists)) {
b) if ($user->uid == 0) could be changed to if (!$user->uid)

3) Minor code improvements in the function mailman_simple_form

// Initialize the default values.
$default_sync = array();
$default_roles = array();
$default_name = '';
$default_desc = '';
$default_sub = '';
$default_usub = '';
if (array_key_exists('values', $form_state)) {
$default_name = array_key_exists('list_name', $form_state['values']) ? $form_state['values']['list_name'] : '';
$default_desc = array_key_exists('description', $form_state['values']) ? $form_state['values']['description'] : '';
$default_sub = array_key_exists('subscribe_email', $form_state['values']) ? $form_state['values']['subscribe_email'] : '';
$default_usub = array_key_exists('unsubscribe_email', $form_state['values']) ? $form_state['values']['unsubscribe_email'] : '';
$default_sync = array_key_exists('sync', $form_state['values']) ? $form_state['values']['sync'] : '';
$default_roles = array_key_exists('default_roles', $form_state['values']) ? $form_state['values']['default_roles'] : '';
}

This piece of code could we better written by using
$default_sync = !empty($form_state['values']['list_name']) ? $form_state['values']['list_name'] : '';

So that we could reduce lot of code here

Could we also please change below code to use ternary operator than if statements
if (!$default_name) {
$default_name = $list->list_name;
}
if (!$default_desc) {
$default_desc = $list->description;
}
if (!$default_sub) {
$default_sub = $list->subscribe_email;
}
if (!$default_usub) {
$default_usub = $list->unsubscribe_email;
}

4) Could you also please simply the drush_mailman_simple_mailman_sync , i mean could you modularize it a bit more into couple functions

Please let me know if you need more info.

Regards,
Prajwal

Hi Sir,

Sure. Sorry if you have mistaken me. Just thought of sharing my thoughts. Would be of a great help from you if you could share your review thoughts on one of my modules "https://www.drupal.org/node/2326909"

Regards,
Prajwal

HI

Thanks for your contribution.
Your module coded in nice mannager. I got following to issues in your module.

1 - Please user db_select() instead of simple mysql select statement. while you used db_select in other module functions.
2 - There is not any need to define array or argument type in function defination.
function mlist_validate_email(array $element, array &$form_state, array $form). However it is not an error.

Thanks!!

For simple queries db_query() is perfectly fine, see https://www.drupal.org/node/310075

Type hinting for parameters is recommended, so that is also not a problem.

Looks like you have not found any application blocking issues.

Hi klausi,

Thanks for quick reply on my comments.
db_query() is always used for the query, where small amount of data need to fetch like value for one column or count etc. how ever it is not a good practise.

In your query i can see you are using "join statement" to join 3 database tables. I dont think that it is simple query.
You must use db_select() instead of db_query().

Also i read artcile link which you posted above, there is nothing like this that it is recommanded to use db_query() for any kind of join query.

About second comments -- "Type hinting for parameters is recommended, so that is also not a problem."

I did not see anywhere in drupal.. however if you are recommending like this then you must use type hinting everywhere in all function of you module files, not in one or two function.

Thanks!!

Usage of db_query() is good practice if the query is static, which seems to be the case here. It is a matter of taste if you want to use db_select() or not here, so not an application blocker.

Type hinting is mostly used in D8 for object oriented code, but it is perfectly fine in D7 as well. There is no rule that all code must be type hinted or not, that is up to the developer to decide.

So those two issues are not application blockers.

I concur with @klausi here.

Ignoring tags and extensions, db_query() vs db_select() boils down to whether the query is static or not (eg, joining or not based on PHP logic), and not the complexity of the query. See https://www.drupal.org/node/310075 For static queries, though, using one over the other isn't a mandatory decision.

Reviews are not meant to hold back release so that they can be perfect. They are sanity checks to make sure developers have a decent idea of what they are doing. Blocking issues are security issues, major API problems, duplication, and a few others. The review admins will sometimes send a module back for one round of polishing, but we don't want to hold back promotion for little things.

There is still a master branch, make sure to set the correct default branch: https://www.drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in https://www.drupal.org/node/1127732
Review of the 7.x-1.x branch (commit 22d1b70):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. "t('Your email address: %email', array('%email' => filter_xss($user->mail)))": The "%" placeholder will already run check_plain() for you, so the filter_xss() is not needed here.
2. "drupal_mail('lists', $action, $list_email, language_default(), $params, $from);": the first parameter to drupal_mail() should be the module name, shouldn't that be mlist? Please add a comment.
3. mlist_form(): why do you need to set the breadcrumb? I think you should move the admin path to a section like "system" ==> admin/config/system/mlist, that will give you the correct breadcrumbs automatically.
4. mlist_form(): $action values are user facing text and must run through t() for translation.

But otherwise looks good to me, so ...

Thanks for your contribution, spficklin!

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.