Rankingalive

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

• Coder Sniffer has found some issues with your code (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. You have to get a review bonus to get a review from me.

FILE: /var/www/drupal-7-pareview/pareview_temp/rankingalive.admin.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 54 | ERROR | If the line declaring an array spans longer than 80 characters,
    |       | each element should be broken into its own line
--------------------------------------------------------------------------------

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

This is fixable by breaking that line into multiple lines, as follows:

    $description = t("Specify pages by using their paths. Enter one path per line. The '*' character is a wildcard. Example paths are %blog for the blog page and %blog-wildcard for every personal blog. %front is the front page.",
        array(
          '%blog' => 'blog',
          '%blog-wildcard' => 'blog/*',
          '%front' => '<front>',
        )
    );

Manual review:

• ✓ README.txt -- Great documentation in the README. Minor nitpicks:
  • You have a superfluous space under the heading at ln. 12 col. 1.
  • ln. 57, col. 51 -- "copies"
• ✓ rankingalive.test -- Nicely done with the tests. Tests are admittedly not my strong suit, but yours seem complete and well-documented.
• ✓ rankingalive.module -- Looks good! I like that you explained how the $visibility var worked -- though maybe move that up into the function declaration block comment?
• ✓ rankingalive.install -- Deletes all module variables on uninstall; displays module requirements (Configuration) on install.
• ✓ rankingalive.admin.inc -- Looks good, apart from code style note above.
• ✓ rankingalive.info -- No problems here.

Looks good to me. Fix those few incredibly minor nitpicks and you'll be on your way.

Hi JulienD!
Thanks for the note! Thinking about it, neither of those two issues really should've been blocking in the first place, and you've fixed them both already. That said, one other thing I've noticed in the meantime is that you print a variable on page and don't really sanitize it anywhere, beyond doing a trim() on ln. 121 in rankingalive.admin.inc. At some point you should do a filter_xss() or check_plain() on that variable (Probably the latter). All user input that gets put on page needs to be sanitized beforehand.

Didn't realize that FAPI text field attribute #default_value gets auto check_plain'd.

In light of my realization in #3, setting to RTBC.

manual review:

1. rankingalive_permission(): at least the PHP execution permission needs to be marked as restrict access => TRUE, because a user can easily p0wn a site with that. See the hook_permission() documentation.
2. rankingalive_page_alter(): all JS code should live in a dedicated JS file. You can use Drupal.settings to pass down variables from PHP to JS. See http://drupal.org/node/756722
3. "protected function testRankingAlivePageVisibilityAllExcepted()": does this even work? I think all test methods need to be public?

The permissions issue is a security blocker. Make sure to review more project applications and complete the review bonus and this will get finished faster.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

rankingalive.js: this should use Drupal.behaviors, see http://drupal.org/node/756722

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

Assigning to Michelle as she might have time to finally approve this.

Thanks for your contribution, JulienD!

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.

Congrats, JulienD!