Devel Email

Hi Hurley,

I ran your code through Pareview and there are just a few items that need fixing:

FILE: /var/www/drupal-7-pareview/pareview_temp/devel_email.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
6 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/devel_email.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
44 | WARNING | Hook implementations should not duplicate @param documentation
--------------------------------------------------------------------------------

run your code through http://ventral.org/pareview yourself if you like as well.

Not many errors though which is good so we can proceed to the manual code review process.

-- Barnettech

Those minor coding standard issues are surely no application blockers. Anything else? Make sure to read through the source code of the project.

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 :-)

Don't forget to add the "PAReview: review bonus" tag as indicated in #1410826: [META] Review bonus, otherwise you won't show up on my high priority list.

As seen in comment #1 PAReview is still outputting a couple of errors.

Just to clarify on those:

FILE: /var/www/drupal-7-pareview/pareview_temp/devel_email.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
6 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
--------------------------------------------------------------------------------

You don't need to declare your .module file in the .info, Drupal already knows to do this.

FILE: /var/www/drupal-7-pareview/pareview_temp/devel_email.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
44 | WARNING | Hook implementations should not duplicate @param documentation
--------------------------------------------------------------------------------

I believe what it's saying is that you only need to document the "Implements hook_mail_alter().", no need to add the @params.

You're missing a README.txt file. My only other suggestion would be to create a README.txt and add a hook_help() and/or the "Configure" link on the module page by specifying "configure = admin/config/development/devel_email" in your .info file.

Other than that, it looks good to me.

Ok, just a few minor PAReview errors on your README.txt now.

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 4 WARNING(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
16 | WARNING | Line exceeds 80 characters; contains 85 characters
21 | WARNING | Line exceeds 80 characters; contains 86 characters
23 | WARNING | Line exceeds 80 characters; contains 90 characters
24 | WARNING | Line exceeds 80 characters; contains 89 characters
--------------------------------------------------------------------------------

Optional, but recommended: Checkout the README.txt style examples on http://drupal.org/node/447604 for additional information on how to format it.

Other than that this all looks good to me. I'm leaving this in "needs review" since there is nothing functionally wrong with the module and these are all very minor errors.

Ahh also, you need to properly branch your repo.

See here for instructions on how to do this
http://drupal.org/empty-git-master
http://drupal.org/node/1015226
http://drupal.org/node/1066342

Ok, you're almost there. That documentation is pretty unclear(I did the exact same thing)...

All you've got to do is rename your 7.x-1.x-dev branch to 7.x-1.x. The 1.x means that it's a dev branch so there's no need to add that, it only gets appended once it's a full project and you've created a release off of a dev branch.

Also, I'm not sure this is really a requirement, but PAReview will gripe at you for having a master branch at all.

git checkout 7.x-1.x
git branch -D master
git push origin :master

Looks good to me

manual review:

1. why was this lost in the D7 version of Devel? Please add a link to the issue where that was decided.
2. project page is a bit short, see http://drupal.org/node/997024
3. "watchdog('devel_email', '
   

   ' . print_r($devellog, TRUE) . '

   ');": This is vulnerable to XSS exploits, do not concatenate dynamic variables into watchdog messages, use placeholders instead. Please read http://drupal.org/node/28984 again.

4. 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.
5. "variable_get(DEVEL_EMAIL_ENABLE, 0)": all variables that your module defines must be removed in hook_uninstall().

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

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I'm a robot and this is an automated message from Project Applications Scraper.

review other project update.