[D7] Optify

Code sniffer found a couple problems that should be addressed before review.

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 3 WARNING(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | Line exceeds 80 characters; contains 217 characters
6 | WARNING | Line exceeds 80 characters; contains 322 characters
8 | WARNING | Line exceeds 80 characters; contains 374 characters
8 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/optify_drupal.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
5 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/optify_drupal.install
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
29 | ERROR | No space before comment text; expected "// Warn if Optify Drupal
| | token hasn't been set." but found "//Warn if Optify Drupal token
| | hasn't been set."
41 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/optify_drupal.module
--------------------------------------------------------------------------------
FOUND 21 ERROR(S) AND 2 WARNING(S) AFFECTING 22 LINE(S)
--------------------------------------------------------------------------------
6 | WARNING | Line exceeds 80 characters; contains 93 characters
11 | ERROR | Whitespace found at end of line
13 | ERROR | Missing function doc comment
23 | ERROR | Missing function doc comment
35 | ERROR | Missing function doc comment
38 | ERROR | Expected 1 space after "=>"; 2 found
42 | ERROR | Whitespace found at end of line
49 | ERROR | Missing function doc comment
51 | ERROR | Whitespace found at end of line
52 | ERROR | No space before comment text; expected "// Users enter the
| | Optify 8 character token." but found "//Users enter the Optify
| | 8 character token."
56 | ERROR | Whitespace found at end of line
62 | ERROR | Whitespace found at end of line
63 | ERROR | No space before comment text; expected "// Tell Drupal which
| | pages to not track with Optify." but found "//Tell Drupal
| | which pages to not track with Optify."
69 | ERROR | Whitespace found at end of line
70 | ERROR | If the line declaring an array spans longer than 80
| | characters, each element should be broken into its own line
76 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
81 | ERROR | Concat operator must be surrounded by spaces
94 | ERROR | Function comment short description must be on a single line
97 | ERROR | Invalid @return data type, expected bool but found boolean
97 | ERROR | Missing comment for @return statement
101 | ERROR | Whitespace found at end of line
105 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
109 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

I had used the following repo for reviewing.
http://git.drupal.org/sandbox/chason/1961532.git
There are still some styling issues left as you can check out the automated review here

Manual Code review.

Uninstall function should delete all the variables declared/set by the function. So you should delete 'optify_drupal_no_display' variable in hook_uninstall.

I was able to install the module without any errors. But i must confess i had not tested the actual working.

NB: I also found that you have given a package name as 'Optify Drupal' in info file. So was wondering if you were planning to release more optify related modules in the future. If not consider moving the package to 'Others' or 'seo' or something more appropriate. (This is my personal opinion ).

good to know chason. Lets see what others have to say about your module. Also i suggest you do manual review for other projects too. That way you can call in attention of klausi

*double post*

Yes @chason you may read about it in [META] Review bonus. Please note that it is not mandatory.

But in case you review, it is important that you include the list of links to comments ( your manual reviews ) on the top of this page ( edit your Issue directly ) and if you have 3 reviews at least you can add the 'PAReview: review bonus' tag through comment. Its all mentioned over there in the META. happy Drupalling.

manual review:

1. optify_drupal_uninstall(): not all variables are deleted?
2. optify_drupal_page_alter(): this is vulnerable to XSS exploits. If I enter 1"]);alert('XSS');([" as token I get a nasty javascript popup. You need to sanitize all user provided text before printing. And in this case I would recommend to pass down variables from PHP to JS with Drupal.settings, see http://drupal.org/node/756722 . And the JS should be in a dedicated JS file. Also make sure to read http://drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
3. I would suggest to rename the module simply to "optify", the "drupal" in the short name is totally redundant.

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.

We're still working on this, chason brought myself and another coworker on to help work on it.

manual review:

From #12 I can confirm the variables do not uninstall, but optify_drupal_install() doesn't run either setting the default exclusion paths. The module name is "optify" everywhere else in the code, however in the .install file "optify_drupal" is being used and so those hooks aren't being called. So to echo the third point in #12, standardizing the name to "optify" should be done.

features:
According to Optify's documentation (http://help.optify.net/entries/124186-adding-the-optify-tracking-code-to...) the tracking code should be added in the footer of the site. I think this should either be the default or a configurable option.

Also an ability to exclude users based on role would be useful to not skew the data coming in.

Lastly, my company has started using Optify and is really interested in the work being done with this module. I'd be very willing to submit some patches to help move this project along.

FYI all of Klausi's recommendations were taken care of, and the settings were changed to be loaded via Drupal.settings.

So the module was mostly written by chason, right? Should we leave this open to make him a git vetted user?

Damien: I assume you reviewed the module, could you set this to RTBC if you think chason's work was good?

klausi: Reviewing the revision history for the module it was chason who made the improvements, not anybody else, so it seems he can stand on his own two feet now.

Disclaimer: I work for chason and have maintainer access on the project, as do others, but up until I added the CHANGELOG.txt file this morning nobody else made any changes to the codebase, it was all chason.

We think Optify is ready for a dev release. Currently I cannot add releases to https://drupal.org/project/optify. Maybe it's bc it hasn't been approved? Is there anything that you need addressed in the module before we can get approval to add releases?

Chason is not currently a git vetted user, that could be part of the problem.

To create a new release, go to https://drupal.org/node/add/project-release/1961532. It can take a few hours before a new release shows up on the project page.

Not sure what else is needed here?

Hi @kscheirer we have a 7.x-1.x branch which should allow us to add a dev release but nothing shows up. We have had that branch for days. Is there anything preventing us from getting Chason added as a git vetted user?

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/optify.module
  --------------------------------------------------------------------------------
  FOUND 10 ERROR(S) AFFECTING 9 LINE(S)
  --------------------------------------------------------------------------------
   38 | ERROR | Expected 1 space after "=>"; 2 found
   42 | ERROR | Whitespace found at end of line
   54 | ERROR | Whitespace found at end of line
   58 | ERROR | Whitespace found at end of line
   65 | ERROR | If the line declaring an array spans longer than 80 characters,
      |       | each element should be broken into its own line
   76 | ERROR | Whitespace found at end of line
   84 | ERROR | Function comment short description must be on a single line
   87 | ERROR | Invalid @return data type, expected bool but found boolean
   87 | ERROR | Missing comment for @return statement
   91 | ERROR | Whitespace found at end of line
  --------------------------------------------------------------------------------
  

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:
optify.js: Shouldn't you use Drupal.behaviors? If you can't please add a comment. See also https://drupal.org/node/756722

But that are not a blockers, so ...

Thanks for your contribution, chason!

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.

@drupalninja99: I have enabled "show snapshot release" at https://drupal.org/node/1961532/edit/releases

Hi @klausi, thanks so much! I went ahead and made the fixes you recommended here: https://drupal.org/node/2048257#comment-7689025

And I created a new issue for javascript/behavior conversion: https://drupal.org/node/2051217. I imagine converting that will not be a problem. The release is working now, thanks again.

You shouldn't set a default value in optify_install(). Instead, when you retrieve the variable provide the default there, like $no_display = variable_get('optify_no_display', "admin\nadmin/*\nbatch\nnode/add*\nnode/*/*\nuser/*/*");.

Thanks @kscheirer, I will have added to https://drupal.org/node/2048257#comment-7694667