Email PDFs

Review of the 6.x branch:
1. I attached automated report generated with PAReview.sh.
2. There are files in master branch. You should replace them as described in http://drupal.org/node/1127732 at step 5.
3. Errors:
Error while installation:
warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'content_copy_import_form' was given in \includes\form.inc on line 377.

Node view and edit pages errors:
user warning: Table 'content_field_email_pdf_add_link' doesn't exist query: SELECT n.nid FROM content_field_email_pdf_add_link cfe JOIN node n ON cfe.nid=n.nid WHERE cfe.field_email_pdf_add_link_nid=1 AND n.status=1 in \sites\all\modules\email_pdfs\email_pdfs.module on line 157.
4. I wasn't able to use that module. I didn't find a way to add 'Email this page as PDF' links to any page. Also there are no items in "Default template" field on Email PDFs settings page. I can see only "none".

You should replace the git URI from first post with the public one (git clone http://git.drupal.org/sandbox/ClinicalTools/1444242.git email_pdfs)

On function email_pdfs_link_template($nid)

drupal_add_js(array('ajax' => array('submit_text' => 'Sending...')), 'setting');

if submit_text will be presented to user, it should be passed throught t(), so it could be translated.

The same for function email_pdfs_uninstall() line:

drupal_set_message('The email_pdf_template node type was not removed. You must remove it manually.');

And same again for email_template.cck.inc file.

Greets!

You should actually use st() function instead of t() function in hook_install and hook_uninstall functions.
drupal_set_message(t('The email_pdf_template node type was not removed. You must remove it manually.'));

Your git url in this issue does not work, maybe you can change it to

git clone --branch 6.x-1.x http://git.drupal.org/sandbox/ClinicalTools/1444242.git email_pdfs

The README.txt is pretty hard to read. Line breaks a about 80 charactes would make it easier to read. Some separator for the different blocks would be great.

The function email_pdfs_link_path is empty, there is also some code that is disabled by comments. If it is not needed, you should remove it.

Automated review:

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

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

FILE: ...n/www/drupal7/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 12 WARNING(S) AFFECTING 13 LINE(S)
--------------------------------------------------------------------------------
  1 | WARNING | Line exceeds 80 characters; contains 428 characters
  3 | WARNING | Line exceeds 80 characters; contains 290 characters
 17 | WARNING | Line exceeds 80 characters; contains 92 characters
 20 | WARNING | Line exceeds 80 characters; contains 94 characters
 21 | WARNING | Line exceeds 80 characters; contains 128 characters
 22 | WARNING | Line exceeds 80 characters; contains 119 characters
 23 | WARNING | Line exceeds 80 characters; contains 136 characters
 24 | WARNING | Line exceeds 80 characters; contains 134 characters
 27 | WARNING | Line exceeds 80 characters; contains 181 characters
 33 | WARNING | Line exceeds 80 characters; contains 193 characters
 34 | WARNING | Line exceeds 80 characters; contains 91 characters
 35 | WARNING | Line exceeds 80 characters; contains 125 characters
 36 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...upal7/sites/all/modules/pareview_temp/test_candidate/email_pdfs.install
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 1 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 25 | WARNING | Line exceeds 80 characters; contains 87 characters
 52 | ERROR   | Inline control structures are not allowed
--------------------------------------------------------------------------------


FILE: ...rupal7/sites/all/modules/pareview_temp/test_candidate/email_pdfs.module
--------------------------------------------------------------------------------
FOUND 21 ERROR(S) AND 10 WARNING(S) AFFECTING 21 LINE(S)
--------------------------------------------------------------------------------
  20 | WARNING | Format should be "* Implements hook_foo()." or "Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."
  81 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
  81 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
 114 | WARNING | Line exceeds 80 characters; contains 95 characters
 123 | ERROR   | Inline comments must start with a capital letter
 126 | ERROR   | No space before comment text; expected "//
     |         | $node->content['email_pdf'] = array('#value' => $output,
     |         | '#weight' => 100);" but found "//$node->content['email_pdf'] =
     |         | array('#value' => $output, '#weight' => 100);"
 158 | WARNING | Line exceeds 80 characters; contains 85 characters
 170 | ERROR   | Concat operator must be surrounded by spaces
 174 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
 177 | ERROR   | No space before comment text; expected "// 'file' =>
     |         | 'email_pdfs.theme.inc'," but found "//'file' =>
     |         | 'email_pdfs.theme.inc',"
 179 | WARNING | A comma should follow the last multiline array item. Found: )
 210 | ERROR   | Concat operator must be surrounded by spaces
 230 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 246 | ERROR   | Expected 1 space after comma in function call; 2 found
 246 | ERROR   | Expected 1 space after comma in function call; 16 found
 247 | WARNING | Line exceeds 80 characters; contains 81 characters
 247 | ERROR   | Comments may not appear after statements.
 248 | WARNING | Line exceeds 80 characters; contains 84 characters
 248 | ERROR   | Comments may not appear after statements.
 249 | WARNING | Line exceeds 80 characters; contains 96 characters
 249 | ERROR   | Comments may not appear after statements.
 250 | ERROR   | Expected 1 space after comma in function call; 2 found
 250 | ERROR   | Expected 1 space after comma in function call; 36 found
 251 | WARNING | Line exceeds 80 characters; contains 84 characters
 251 | ERROR   | Comments may not appear after statements.
 252 | WARNING | Line exceeds 80 characters; contains 81 characters
 252 | ERROR   | Comments may not appear after statements.
 253 | WARNING | Line exceeds 80 characters; contains 82 characters
 253 | ERROR   | There must be no blank line following an inline comment
 253 | ERROR   | Comments may not appear after statements.
 263 | ERROR   | Concat operator must be surrounded by spaces
--------------------------------------------------------------------------------


FILE: .../all/modules/pareview_temp/test_candidate/includes/email_pdfs.pages.inc
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AND 2 WARNING(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
  15 | ERROR   | No space before comment text; expected "// email-pdf-link is
     |         | added to the end of the url by javascript." but found
     |         | "//email-pdf-link is added to the end of the url by
     |         | javascript."
  27 | ERROR   | You must use "/**" style comments for a function comment
  96 | WARNING | A comma should follow the last multiline array item. Found:
     |         | TRUE
 101 | WARNING | A comma should follow the last multiline array item. Found:
     |         | TRUE
 113 | ERROR   | You must use "/**" style comments for a function comment
 128 | ERROR   | Missing function doc comment
--------------------------------------------------------------------------------


FILE: ...review_temp/test_candidate/includes/mimemail-message--email_pdf.tpl.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 2 | ERROR | Missing file doc comment
--------------------------------------------------------------------------------

1. I suppose you can use relatives links for including php, so remove: drupal_get_path('module', 'email_pdfs')
email_pdfs_install_import(drupal_get_path('module', 'email_pdfs') . '/includes/email_template.cck.inc', 'email_pdf_template');
I would also separate the include into another row, and probably put it's code into a function.

2. I hate reading long conditions on one line:

    if (!empty($_SESSION['messages']) && (!empty($_SESSION['messages']['error']) || (!empty($_SESSION['messages']['status']) && (drupal_substr(end($_SESSION['messages']['status']), 0, 8) == t('An error'))))) {
      return FALSE;
    }

I recommand putting them on multi-lines as follow (a personal opinion):

    if (!empty($_SESSION['messages'])  
     && (!empty($_SESSION['messages']['error']) 
  || (!empty($_SESSION['messages']['status']) 
     && (drupal_substr(end($_SESSION['messages']['status']), 0, 8) == t('An error'))))) {
      return FALSE;
    }

3. Add Doc-Comments for all functions

4. I disrecommand mixing return types:

  if (!empty($node) && $node->type == EMAIL_PDFS_NODE_TYPE && node_access('view', $node)) {
    return $node;
  }
  return FALSE;

probably return NULL instead of FALSE

5. What shall this do?

/**
 * Get a link for the given internal url path.
 */
function email_pdfs_link_path($path, $use_default = FALSE) {
}

6. Debugging code (code commented out) should be removed.

---
The response time for a review is now approaching 4 weeks.
Get a review bonus and we will come back to your application sooner.
See: http://drupal.org/node/1410826

You could for example start by evaluating my own project:
http://drupal.org/node/1302786

Fixing clone URI

In addition to the findings of PAReview.sh available at http://ventral.org/pareview/httpgitdrupalorgsandboxclinicaltools1444242git
here are my observations:

email_pdfs.module
-------------------------
line 100:

 case 'edit':
      return email_pdfs_node_edit($node, $teaser, $page);

function email_pdfs_node_edit doesn't seem to exist.

line 109: global $user;
variable is not used.

line 118 & 121: $output = email_pdfs_send_template($node);
variable is not used.

line 128:

/**
 * Implements hook_node_view().
 */
function email_pdfs_form_alter(&$form, $form_state, $form_id) {

comment does not match the hook being implemented.

line 139:

/**
 * Implements hook_node_view().
 */
function email_pdfs_preprocess_page(&$vars) {

comment does not match the hook being implemented.

line 212: $sitestyle = variable_get('mimemail_sitestyle', 1);
variable is not used.

email_template.cck.inc
------------------------------
this file does not have a <?php. is it used ?

drupal_set_message(st('The email_pdf_template node type was not removed. You must remove it manually.'));
if will be helpful if gave a link to delete the content type

1. email_pdfs_pages.inc - line 31 - global parameter names should start with underline

Dear ClinicalTools,
You are absolutely right. Sorry for misinformation.

I recommand you to participate in the "review bonus" program: http://drupal.org/node/1410826

Automatic review:
1. There are still some minor code standard issues. See: http://ventral.org/pareview/httpgitdrupalorgsandboxclinicaltools1444242git

Manual review:
2. I think there is a problem with the name of the dependencies:

No release history was found for the requested project (content).    [warning]
No release history was found for the requested project  (nodereference).    [warning]
No release history was found for the requested project (text).                      [warning]
No release history was found for the requested project  (content_copy).      [warning]

Module email_pdfs cannot be enabled because it depends on the        [error]
following modules which could not be found: content,nodereference,text,content_copy

I'm searching on drupal.org modules but I cannot find a module called like those mentioned before so I cannot install the module and review it properly.

I passed over this problem commenting the unknown dependencies.

3. During install I have an incorrect file path reference that raises a warning:

file_get_contents(./includes/email_template.cck.txt): failed to open [warning]
stream: No such file or directory in /var/www/d6/sites/all/modules/email_pdfs/email_pdfs.install on line 36.

This problem is related to the owner or the rights of this file (chmod - chown). If this is not a false positive, it should at least been mentioned at the README.txt or at best be repaired.

4. When I create a page and visit that page (after having enabling the module), I've got the following error:
user warning: Table 'd6.content_field_email_pdf_add_link' doesn't exist query: SELECT n.nid FROM content_field_email_pdf_add_link cfe JOIN node n ON cfe.nid=n.nid WHERE cfe.field_email_pdf_add_link_nid=1 AND n.status=1 in /var/www/d6/sites/all/modules/email_pdfs/email_pdfs.module on line 157.

The table content_field_email_pdf_add_link doesn't exists. Is this a consequance of the issue 2 ? If not then it has to be solved.

5. You are creating a content type from a .txt file ? Why don't use the .install file execution to create and delete the content type ?

Fixing git clone command.

I installed this module using drush, usually drush download and install all dependencies so I guess there is something out of norms either on the .info file, it could also be a drush/cck naming issue, not related to your module. It can help to check other d6 modules that require CCK to see how they set the dependencies.
In any case your module cannot be enabled with drush without manually installing dependencies. (I don't understand this as a blocker issue)

The other issues are, as you said, consequences of n°3.

Regarding n°5, I understand that you are importing a cck export, I just think that it would be much more clean to create the content type inside the hook_install() function instead of doing an import. The code doesn't change so much from what you got on the import, is just executed on a different way.
(here they give some examples: http://drupal.org/node/231019)

Also keeping a .txt file that is executed and that has to be chmoded (to what ? 777 ? ) could be dangerous. Someone with an FTP access (without any right to edit the modules) could edit the .txt file (that remember we were forced to give more rights! what kind of rights? ) and hack/break the system if the administrator uninstall and reinstall the email_pdfs module.
I know its a quite limit situation, but I think it's important to have a refexion on those practices as one of our responsibilities as contributors is to offer and maintain a secured code.

manual review:

1. "module_load_include('inc', 'email_pdfs', 'includes/email_template.cck');": no need to use module_load_include as you are including files from your own module which you already know where they are. Use something like require_once 'includes/mymodule.inc';. Also elsewhere.
2. You provide your own content type in this module, so shouldn't you use hook_form() and hook_view() and other node module hooks?
3. Please note: All user accounts are for individuals. Accounts created for more than one user will be blocked when discovered. Your account looks like it is used by a group of people, so you might want to clarify that.

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Now actually removing tag.

Thanks for your contribution, Bradley!

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.

Thanks to all the reviewers as well. I am delighted that we can contribute to the Drupal community. My small organization has benefited so much from Drupal - we're looking forward to many more contributions.