Mail Signature

Hi arrubiu,

Thanks very much for submitting this very interesting module.

Please find an initial Automated Review below to get your validation started:

Automatic Code Review
An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.

You can find the results of the automated report at http://ventral.org/pareview/httpgitdrupalorgsandboxsergiodurzu1898098git.

Master Branch
It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

Code too short
This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project for you.

That's all I could find so far, but I will surely let you know if I find anything else.
Hope that helps improving your module.

Cheers!

FILE: /var/www/drupal-7-pareview/pareview_temp/mail_signature.admin.inc
--------------------------------------------------------------------------------
FOUND 9 ERROR(S) AFFECTING 9 LINE(S)
--------------------------------------------------------------------------------
15 | ERROR | Array indentation error, expected 4 spaces but found 6
16 | ERROR | Array indentation error, expected 4 spaces but found 6
17 | ERROR | Array indentation error, expected 4 spaces but found 6
18 | ERROR | Array indentation error, expected 4 spaces but found 6
22 | ERROR | Array indentation error, expected 4 spaces but found 6
23 | ERROR | Array indentation error, expected 4 spaces but found 6
24 | ERROR | Array indentation error, expected 4 spaces but found 6
25 | ERROR | Array indentation error, expected 4 spaces but found 6
31 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mail_signature.info
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
6 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
7 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mail_signature.module
--------------------------------------------------------------------------------
FOUND 12 ERROR(S) AFFECTING 10 LINE(S)
--------------------------------------------------------------------------------
11 | ERROR | Missing function doc comment
15 | ERROR | Array indentation error, expected 4 spaces but found 6
15 | ERROR | Do not use t() in hook_menu()
16 | ERROR | Array indentation error, expected 4 spaces but found 6
16 | ERROR | Do not use t() in hook_menu()
17 | ERROR | Array indentation error, expected 4 spaces but found 6
18 | ERROR | Array indentation error, expected 4 spaces but found 6
19 | ERROR | Array indentation error, expected 4 spaces but found 6
20 | ERROR | Array indentation error, expected 4 spaces but found 6
21 | ERROR | Array indentation error, expected 4 spaces but found 6
30 | ERROR | Missing function doc comment
35 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

Manual review:
----------------------------------------
1. you have to give default value when using variable_get(). for example. line 50, 57 in mail_signature.drush.inc. and also in the .module file.

2. Add a .install file and implements the hook_uninstall() for removing the variable using variable_del().

3. wrap the title and description with t() in mail_signature_menu().

4. as other reviewers said, the code is too short. would you consider adding a image file on the form such that user could add image on the signature?

Sorry, you are correct, we don't need to wrap the title and description with t() since it is called by default.

The menu title. Do NOT use t() which is called by default. You can override the use of t() by defining a 'title callback'. This is explained in the 'menu_example/title_callbacks' example below.

Description (hover flyover for menu link). Does NOT use t(), which is called automatically.

So ignore my point 3.

For point 4, as long as you could write more than 5 functions and 120 lines of code. that should be fine. But i do believe a signature with image would be a great feature.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

* Since you moved the code to a new branch, the git URL in the summary should be updated.

git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/sergio.durzu/1898098.git mail_signature

* PHP_CodeSniffer did not find any problems.

* 7 functions, and 219 lines (including comments). Code size looks good.

* It was mentioned above that a default value needs to be given to variable_get(). According to the API documentation, this is not true for Drupal 7, and that NULL is returned if a default is not specified. No need to change your code back if you don't want, but something to note.

* I tested the functionality, and it looks good.

* A quick review of the code didn't raise any flags.

As far as I can see, this looks good for RTBC.

The module still needs works and can not be RTBCed yet.

# The mail_signature.install file is missing. In mail_signature.admin.inc file in the function

function mail_signature_settings_form($form) {

}

You have used variable_get() to assign #default_value to various form elements. variable_get() would save these variables in the database. So once a module is uninstalled these variables have to be deleted, so mail_signature.install is needed where you would implement hook_uninstall() and delete these variables.

/**
 * Implements hook_uninstall().
 */
function mail_signature_uninstall() {
  variable_del('mail_signature_enabled');
  variable_del('mail_signature_text');
  variable_del('ail_signature_mail');
}

# The file mail_signature.drush.inc line #30

/**
 * Implements of hook_drush_help().
 */

Change this to

/**
 * Implements  hook_drush_help().
 */

# mail_signature.admin.inc file line#46

/**
 * Implements callback declared in form.
 */
function mail_signature_settings_form_validate($form, &$form_state) {

}

Since this is not a hook, so use some other word, other than implements here, intially, I confused this with a hook().

Get review bonus as said by klausi in comment no #9 which would help your application to be processed faster.

Automated review:
PAreview.sh on ventral.org: http://ventral.org/pareview/httpgitdrupalorgsandboxsergiodurzu1898098git Found some problems

FILE: /var/www/drupal-7-pareview/pareview_temp/mail_signature.install
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
9 | ERROR | Expected 1 space(s) before asterisk; 0 found
10 | ERROR | Expected 1 space(s) before asterisk; 0 found
16 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

Coder module: OK, nothing found.

Manual review:

- Install a fresh drupal 7.19
- Move the module package to folder to sites/all/modules
- Install module
- Go to admin/config/system/mail-signature
- Entered text, activate the signature via checkbox, enter test mail.
- Save the settings.
- Result as expected: testmail and mails got the signature appended.

No issues found in manual review and issues from comment #11 are fixed.

Only the whitespace-issues in the new .install file, now i don't know what to
set as issues-status... all works fine so it should be "reviewed by the community"
and the missing 3 whitespaces should not change to "needs work" (please correct me, if its wrong or to smooth).

PLEASE fix the 3 ->minor<- coding-convention-issues thrown by http://ventral.org/pareview/httpgitdrupalorgsandboxsergiodurzu1898098git.

Minor spelling typo on line 52 of mail_signature_admin.inc ("Attention").

Thanks for your contribution, arrubiu!

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, 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.