[D7] Nicereply

Welcome!

Please take a moment to create a README.txt that follows the guidelines for in-project documentation.
Please take a moment to make your project page follow tips for a great project page.

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.
(At least this has to be done before switching back to needs review)

* Implements hook_permission should be * Implements hook_permission().

Only files containing classes or interfaces should be added to "files[]="

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxpetiar1510552git

We do really need more hands in the application queue and highly recommend to get a review bonus so we will(/can) come back to your application sooner.

regards

You already pushed the 7.x-1.x branch as you see here: http://drupalcode.org/sandbox/petiar/1510552.git

You have not listed any reviews of other project applications in your issue summary as strongly recommended here: http://drupal.org/node/1011698

Please resolve the coding issues. See this for more info. http://ventral.org/pareview/httpgitdrupalorgsandboxpetiar1510552git

Manual review :-

• nicereply.module :- Add a proper name to the argument, instead of just $u, line 138
• nicereply.module :- Add a check, like if (arg(0) == 'node' && is_numeric(arg(1))) {, line 123, before loading the node
• nicereply.module :- Add proper function comments, params and return types. Refer Doxygen and comment formatting conventions
• nicereply.pages.inc :- You are not validating anything here before the submit handler gets executed. So, no need to use validate handler, line 54
• nicereply.pages.inc :- Add a check before accessing the argument, line 59
• nicereply.pages.inc :- Add a comment for function call at the top, line 3
• nicereply.install :- Where are you installing the nicereply schema?, it is commented out, line 8

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

This application is not fixed? See http://drupal.org/node/532400

Hey,
Here are a few things to consider.

nicereply.module

1. Line 140 - The drupal message should be enclosed in a t() function so that it is translatable
2. The following lines have drupal messages with variables that should use check_plain() and filter_xss() to be safe and fully sanitized.

• Line 210
• Line 214
• Line 221

nicereply.pages.inc
Line 53: When labelling buttons, make it clear what the button does, "Submit" is too generic.

Thanks for your contributions!

That's right, check_plain() is "safe enough" as it escapes any HTML contained in the variable,
while filter_xss() only filters out certain dangerous HTML parts.

But note that you don't have to use check_plain() or any of these with t(), because t() has these build in using its placeholder functionality.
Read the documentation about t() and the similar but non-translating function format_string().

.module -
At hook_node_view(), you used "global $user", but never used it.

Good luck!

You should definitely improve the text on the project page - instead of explaining your thought process, imagine someone is coming to your page wondering what the module does and why they should use it.

Use php's http_build_query for creating query strings - it handles all the formatting for you instead of carefully building up a string (in nicereply_api_call()). You should check that curl is installed before calling it, some servers/environments do not have it. Or use drupal_http_request() instead.

In D7 variable_get('nicereply_public_key', NULL) can be simplified to just variable_get('nicereply_public_key') since we finally added a default return value.

These are minor issues, except the project description could be considered major, but no critical problems. RTBC from me.

Code still looks good, and it's been over a month since RTBC. This issue has been waiting a long time, thanks for sticking with it!

Thanks for your contribution, petiar!

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.

----
Top Shelf Modules - Enterprise modules from the community for the community.