Contact Plus

Looks like a pretty nice little utility module to allow some custom alterations to the Drupal Core's Contact.

Hello,

In contact_plus.install file, on line 11,
do not use t() or st() in installation phase hooks, use $t =
or get_t() to retrieve the appropriate localization function name.

Thanks.

Looks like you want to resurrect contact_redirect. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please follow the abandoned module process to take over maintainership: http://drupal.org/node/251466

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

BTW: The Git commits are not connected to your user account. You need to specify an email address. See http://drupal.org/node/1022156 and http://drupal.org/node/1051722

>Looks like you want to resurrect contact_redirect

Well contact redirect's name suggests it does just one thing.

> Contact Plus supercedes http://drupal.org/project/contact_redirect (I know, I know... trying to contact maintainer). This module is different because it provides lots of new features, namely an entire extra settings page

The applicant is saying this does more stuff. @NikLP: could you give a list of features?

> Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition

If this module has a wider range of features, and contact redirect is abandoned then the best thing might be to deprecate that and have this as a new project.

NikLP,

Get more information through this link:
http://api.drupal.org/api/drupal/modules!system!system.api.php/function/...

Drupal does not have access to a database in the early stages of the installation process, so any interface strings which might be generated before the database is available cannot be translated via the usual database-based Localization API.As soon as the run time is running on the database (that is after the database tables were created in the first batch process), t() can be used to translate single strings

There are often places in your installation code when you need to write logic which would run both in the installer and run time. A hook_requirements() implementation checking for your installer and run time requirements is a good example. There, depending on whether Drupal runs the installer or the database back end, you need to choose from the two translation functions. To make this easy, get_t() is provided, so for code which might run in both the installer and run time, you should use $t = get_t() and then wrap strings in $t(). Do use the $t name, since the Translation template extractor will only find such strings with this marker.

Here's an eyeball review of the code:

> description = Set a path to redirect to after submitting a message via the core Contact form.

Needs expanding, if this does more than that now. Also, these should usually be in the present tense rather than the imperative, so something like 'Provides cake and tea' or 'Allows users to eat cake'.

Regarding t()/st(), http://api.drupal.org/api/drupal/modules!system!system.api.php/function/... is woefully short on details of this. Compare with core modules perhaps? http://api.drupal.org/api/drupal/includes!install.inc/function/st/7 does say when and how to use it, but that only gets us so far.

    available and an extra tab for <a href="admin/structure/contact/settings">Contact form settings.</a>'));

This link will be broken on the modules page. Use a placeholder like !url and then url() to generate it from that path.

  // can't loop here so just make a best guess attempt than categories < 9
  variable_del('contact_plus_path_1');

I've a vague feeling I've seen modules delete variables with a wildcard. Probably done with a "SELECT FROM {variable} LIKE foo%" and then delete everything you found.

// We use this several times, so let's make a constant for default destination.
define('DEFAULT_DEST', '<front>');

Constants like that are a pet hate of mine. Do you really need this constant? Does it improve DX in any way at all? I would argue it worsens it: you can't tell by seeing DEFAULT_DEST in the code what that is, and you're surely never going to need to change it in the future.

> // Make a new menu item to serve as default tab.
> // This inherits the parent menu item properties (not otherwise set).

Do you maybe also need hook_menu_alter() to change the core contact admin page into a tab too?

> 'leave' => t('Leave the name field as per default'),

Be nice and remind the user what that is :)

> 'vset' => t('Make the name field visible but not editable, but only if the field already has a value'),
> 'hset' => t('Hide the name field completely, but only if the field already has a value'),

What happens with either of those options if the field doesn't have a value? The behaviour seems undefined (at least as far as the UI tells me)

> '#description' => t('Remove the "Subject" field from the contact form. Not generally recommended,
but the email & subject will still be valid as subject will contain [the category name].'),

Needs a proofread -- word missing maybe? Also, write & out in full.

> if ('' != $form['name']['#default_value']) {

Yoda condition!

  switch (variable_get('contact_plus_name', 'leave')) {
    case 'visible':
      // Disable the name field, make it not required (cosmetic & doesn't confuse).
      $form['name']['#disabled'] = TRUE;
      $form['name']['#required'] = FALSE;
      break;
// ... and so on

This isn't a problem, but I wonder if there's a way to refactor this at all. Maybe if your variables corresponded to the form element they relate to you could do them both in one go?

- contact_plus_name - name
- contact_plus_your - mail !!!

as it has received an unanswered detailed review this should have needs work status

• Where's that README file?
• You could probably do a more accurate deletion of the variable_del('contact_plus_path_x') by perhaps? Could you expand on why you can't loop in the hook_uinstall function? Joachim's solution of SELECT FROM {variable} LIKE foo% sounds like a good idea.
• Also, the comment from Joachim: This link will be broken on the modules page. Use a placeholder like !url and then url() to generate it from that path. - handed to you on a plate!

I'm just a bit confused because you'd said you'd fixed everything outlined above, but I've already seen that some of the recommendations weren't touched at all.

I'll come back to this later today.

Many apologies then NikLP. I need to get with the program!

Perhaps you might want to delete the branch called master? See steps 5 and 6 here.

As a follow up to comment #16, I've confirmed that the README.txt file is now there.

The second suggestion from Joaquim for a more accurate deletion has been implemented.

The third suggestion to use a placeholder for the settings link was attempted, but the link is still broken. I would suggest using the $link variable for this situation.

I installed and tried out the module and other than the broken link in the install status message, it works well. This is a great successor to Contact Redirect and look forward to it becoming a full project! :)

Yes, that was me. I was going to let you know I had written up a post about it, but it looks like the interwebs have me beat, Sorry about the name mistake, I fixed that.

Yes, I think that removing the status message altogether would be fine. It's a great module so let's get that thing approved!

this looks like it has similarities with http://drupal.org/project/contact_forms

By the install link, do we mean this bit:

available and an extra tab for Contact form settings.'));

Go look at the documentation for t(). It has an example of how to do this correctly.

> There's much contention about how to do it and with what function, but it all seems ambiguous so let's just skip it altogether.

What? It's simplicity itself, and there is one correct way to do it and one only.

How to show an admin or other local drupal link in a user message:

drupal_set_message(t('Hey go look at this <a href="!url">link</a>.', array
  '!url' =>  url('drupal/path/to/link'),
);

ARGH. t() USED TO have good docs; they've gone. #1862198: It's not clear that you have to look at format_string() to find details of t() arg handling.

In reply to #30, some things you can do:

• Consider changing the status. It is in "needs work" status since November.
• Getting a Review Bonus is recommended.
• Change the priority as follows (see http://drupal.org/node/539608):
  • Major: Applications with a status of needs review that have been awaiting response from a reviewer for 2+ weeks.
  • Critical: Applications with a status of needs review that have been awaiting response from a reviewer for 4+ weeks.

README.txt
Any Line should not exceeds 80 characters

contact_plus.module
Line #106, Inline comments must end in full-stops, exclamation marks, or question marks
Line #36, Line exceeds 80 characters

Minor coding standard errors are surely no application blockers. Please do a real manual review.

hi NikLP,

good job it will be a helpfull module.

Suggection :

1. If u are providing individual settings for individual contact category it will be more helpfull so that we can set seperate form styles for diffrent forms like contact,feedback etc..
2. Please provide a dropdown options for contact category and don't forget to include all catogory option

Ventral Review: http://ventral.org/pareview/httpgitdrupalorgsandboxniklp1817490git

FILE: /var/www/drupal-7-pareview/pareview_temp/contact_plus.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 1 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
36 | WARNING | Line exceeds 80 characters; contains 94 characters
106 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
--------------------------------------------------------------------------------

So... I have a local Drupal 7 install, and I turned on contact, then enabled your module and per your instructions... it works!

So, that's good to note. Your readme successfully guided some one to complete the task at hand!

Line #18, it is good to include the administrative functions in module-name.admin.inc file. So the corresponding change in contact_plus.module would be

/**
 * Implements hook_menu().
 */
function contact_plus_menu() {
  // Make a new menu item to serve as default tab.
  // This inherits the parent menu item properties (not otherwise set).
  $items['admin/structure/contact/categories'] = array(
    'title' => 'Categories',
    'type' => MENU_DEFAULT_LOCAL_TASK,
  );

  // Create a new second tab for the new contact form settings.
  $items['admin/structure/contact/settings'] = array(
    'title' => 'Settings',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('contact_plus_form'),
    'access arguments' => array('administer contact forms'),
    'type' => MENU_LOCAL_TASK,
    'weight' => 5,
    'file' => 'contact_plus.admin.inc',
  );

  return $items;
}

And the function contact_plus_form should go into the file contact_plus.admin.inc.

The Redirect Path text field takes any path even if the path is not a valid path. You should include the validation to check whether the entered path is valid or not. For this you could refer to how core menu module does (line# 368 menu.admin.inc). Pasting the code of the menu module below for your reference.

/**
 * Validate form values for a menu link being added or edited.
 */
function menu_edit_item_validate($form, &$form_state) {
  $item = &$form_state['values'];
  $normal_path = drupal_get_normal_path($item['link_path']);
  if ($item['link_path'] != $normal_path) {
    drupal_set_message(t('The menu system stores system paths only, but will use the URL alias for display. %link_path has been stored as %normal_path', array('%link_path' => $item['link_path'], '%normal_path' => $normal_path)));
    $item['link_path'] = $normal_path;
  }
  if (!url_is_external($item['link_path'])) {
    $parsed_link = parse_url($item['link_path']);
    if (isset($parsed_link['query'])) {
      $item['options']['query'] = drupal_get_query_array($parsed_link['query']);
    }
    else {
      // Use unset() rather than setting to empty string
      // to avoid redundant serialized data being stored.
      unset($item['options']['query']);
    }
    if (isset($parsed_link['fragment'])) {
      $item['options']['fragment'] = $parsed_link['fragment'];
    }
    else {
      unset($item['options']['fragment']);
    }
    if ($item['link_path'] != $parsed_link['path']) {
      $item['link_path'] = $parsed_link['path'];
    }
  }
  if (!trim($item['link_path']) || !drupal_valid_path($item['link_path'], TRUE)) {
    form_set_error('link_path', t("The path '@link_path' is either invalid or you do not have access to it.", array('@link_path' => $item['link_path'])));
  }
}

Tangent:

Is it really necessary to validate that the administrator isn't an idiot?

I understand protecting against intrusion and providing security... but is there a standard for protecting against stupid mistakes?

This is to avoid mistakes, not to validate that admin isn't an idiot.

Stupid... mistakes... whatever we may call it, it's user error. Can you or anyone clearly demonstrate to me (a fellow reviewer) where checking a users input for purposes other than security is a requirement?

I do not see this as a blocker, it is a feature request.
Would it improve the module? Easily arguable that yes... yes it would!
Is it critical for it's release? Not even remotely. It works, and any person with half a brain can make it work, and troubleshoot their errors on their own.

Automated review still showing errors:

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 16 WARNING(S) AFFECTING 16 LINE(S)
--------------------------------------------------------------------------------
8 | WARNING | Line exceeds 80 characters; contains 87 characters
16 | WARNING | Line exceeds 80 characters; contains 86 characters
20 | WARNING | Line exceeds 80 characters; contains 82 characters
27 | WARNING | Line exceeds 80 characters; contains 87 characters
28 | WARNING | Line exceeds 80 characters; contains 86 characters
29 | WARNING | Line exceeds 80 characters; contains 88 characters
30 | WARNING | Line exceeds 80 characters; contains 89 characters
31 | WARNING | Line exceeds 80 characters; contains 88 characters
34 | WARNING | Line exceeds 80 characters; contains 82 characters
38 | WARNING | Line exceeds 80 characters; contains 84 characters
39 | WARNING | Line exceeds 80 characters; contains 85 characters
40 | WARNING | Line exceeds 80 characters; contains 86 characters
41 | WARNING | Line exceeds 80 characters; contains 84 characters
42 | WARNING | Line exceeds 80 characters; contains 84 characters
43 | WARNING | Line exceeds 80 characters; contains 83 characters
46 | WARNING | Line exceeds 80 characters; contains 85 characters
--------------------------------------------------------------------------------

Fix all that, I'll do a final review and then mark as reviewed and tested by the community if it all checks out. The previous issue about path verification isn't a bad idea to consider, but is definitely a feature request and NOT mandatory for release in my strong opinion.

Minor coding standard issues in the README.txt are surely no application blockers. Anything else?

Issues in comment number #40 have not been resolved.
@klausi: Do you feel this is blocker?

Klausi: Good point, I just thought everything had to at least pass through coding standards before acceptance. I'll keep that in mind for future reviews :)

I vote for #40 to not be a blocker! It's a feature request!

Can't call it a feature request. A wrong link added by mistake can redirect to a page which won't even exist, so I think this is required. Imagine a small typo by the admin and the user would be redirected to a "Page not found" page, which is not a great thing!

..and validation of user input is an essential skill for a Drupal developer.

It works, it has no security issues any of us have found and posted about... and yes it doesn't coddle the user and hold their hand all the way to typing in a link... but it's not necessary to do so. Until you can provide documentation of such a requirement it's just your opinion and mine as well in the opposite direction. Opinions however shouldn't hold back projects, unlike requirements like security and coding standards.

I'm marking as reviewed and tested.

I think comment #40 is needed, as not including this validation can lead to unwanted circumstances as mentioned in comment #48.

Okay, since you're going to be a twerp about it, I created a discussion in the module review group since that is where this disagreement belongs, not on this persons module. Continuing to do so is disrespectful and egocentric.

Discussion here: http://groups.drupal.org/node/280973

So until that discussion comes to a conclusion, let's get over ourselves and get out of this persons way with our opinions.

It takes a week of being reviewed and tested by the community anyways to pass. So we've got a week to make our case there where it belongs, not on this poor persons effort to contribute a module. If within a week the community decides that this is to be a requirement, then come back and change it and reference the post.

Thanks for your professionalism on this and taking this discussion where it is appropriate.

FYI: If you go look at our discussion, the final result was it is unnecessary.

This probably needs review again since you changed things up. I'm not available right now to get into this, but I'll check it out Thursday or so.

...or you can roll back to the accepted version, and put this in as functionality for a newer version and have it a "feature request".

Okay, automated Parreview went fine.

Manual review, code all seems good. However, I see a few things that I'm surprised didn't trip up the automated review?

On lines 49,58,60,68,77,78,86,87... the list goes on but they all go way past the 80 character mark... but this isn't really critical I don't believe?

Anyways, Reviewed and tested. Still works for me on my test drupal 7 site. Good jorb :)

Surprised nobody caught this. Rules clearly state that "major" priority is for applications which haven't received any review for at least 2 weeks. Changed accordingly.

Thanks for your contribution, NikLP!

I updated your account to let you 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 get involved!

Thanks to the dedicated reviewers as well.