[D7] Answers to comment

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

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

1. If you use a hook you need to have a comment like this: Implements hook_name(). Example: answers_to_comment_permission() should have comment Implements hook_permission(). Some of your functions are missing this.
2. Remove the space after the word comment drupal_set_message(t('Answer has been transformed to a comment !'));
3. Perhaps look into using the README template.

Automated Review

No issues identified by the automated review.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

List of identified issues in no particular order.

1. Line 164 of the .module file has a comment referencing "group_mind_q_a_messages" module.
   The comment should be updated and the function doc comment should let people know that the "transform_to_answer" property exists in case they want to use it in the provided answers_to_comment_done.
2. Same thing line 264 with the "transform_to_comment" property
3. Functions answers_to_comment_to_comment and answers_to_comment_to_answer are lacking parameter description in the doc comment (even if it's self explanatory).

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Nothing blocking for me, changing to RTBC.

Functionality Review

I installed the module on a fresh D7 setup with the Answers 7.x-4.x dev branch, and when I click the "transform to comment" on an answer, I got WSoD'd. Here's the output in the PHP error log:

[05-Nov-2014 11:44:59 America/Denver] PHP Fatal error:  Call to undefined function comment_save() in ~/Sites/test/a2c/site/sites/all/modules/custom/answers_to_comment/answers_to_comment.module on line 267
[05-Nov-2014 11:44:59 America/Denver] PHP Stack trace:
[05-Nov-2014 11:44:59 America/Denver] PHP   1. {main}() ~/Sites/test/a2c/site/index.php:0
[05-Nov-2014 11:44:59 America/Denver] PHP   2. menu_execute_active_handler() ~/Sites/test/a2c/site/index.php:21
[05-Nov-2014 11:44:59 America/Denver] PHP   3. call_user_func_array:{~/Sites/test/a2c/site/includes/menu.inc:517}() /Users/Home/Sites/test/requirejs/site/includes/menu.inc:517
[05-Nov-2014 11:44:59 America/Denver] PHP   4. answers_to_comment_to_comment() ~/Sites/test/a2c/site/includes/menu.inc:517
[05-Nov-2014 11:44:59 America/Denver] PHP   5. __answers_to_comment_to_comment() ~/Sites/test/a2c/site/sites/all/modules/custom/answers_to_comment/answers_to_comment.module:115

Being that this is part of core, I tried a registry rebuild at first just to make sure it wasn't something odd happening on my end. This is happening during the answers_to_comment_to_comment page callback when you're calling that helper function in your .module @ line 115.

It's possible since this is a fresh install that there's a dependency that I don't know about, in which case that ought to be added to your .info.

Code Review

answers_to_comment.module

• You could simplify your db query @ line 103 quite a bit by removing all of the unnecessary $query declarations. See the example here for a demonstration: https://api.drupal.org/comment/11699#comment-11699 The good news is that your implementation already follows security best practices.
• Similarly, you could be using arrays to define all of your class object values instead @ 152-162 & 252-263. It seems like these blocks both have very similar functionality, so it may be worth abstracting these into a second helper function.
• With respect to your hook, you might consider having a second hook (or moving the existing one) further up in your helper function @ 249 in case developers need to process comment data before your query.

Everything else I saw looked fairly standard, you're not using any weird hooks or doing anything unnecessarily strange.

This may be ignorant but I have to ask, why are you requiring Answers 4.x? Since it hasn't been released yet, you should think about supporting 3.x+ instead if possible.

@rgkimball I also tried on a fresh 7.32 install and I can't seem to get the same error. I used a "standard" install, did you use minimal ?

What action were you doing exactly ?

Here's my module list:

Answers
Answers to comment
ctools
entity
entityreference
views

Everything is running with the latests STABLE, except Answers of course.

@growiel

I'm not sure exactly what the conflict was, but I was using a custom install profile with a few extra contrib modules enabled. Fresh Standard install with the dependencies you mentioned and everything runs smoothly now. Working now to try to figure out where the conflict is occurring.

Update: I've narrowed it down to text formats, though I don't know exactly what sorts of format settings are breaking the transition to comments yet.

I reviewed the latest commit, the added hook seems to work fine, the comment were modified as I requested.

I think this can go live, I change to RTBC.

Assigning to myself for next review.

Automated Review

Review of the 7.x-1.x branch (commit ba7f045):

• ./answers_to_comment.module: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming
  

  function __answers_to_comment_to_comment($source_node, $target_nid) {
  

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

  
  FILE: /home/matt/PAR/pareview_temp/answers_to_comment.api.php
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   30 | ERROR | Files must end in a single new line character
  --------------------------------------------------------------------------------
  
  
  FILE: /home/matt/PAR/pareview_temp/answers_to_comment.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   164 | ERROR | Inline comments must end in full-stops, exclamation marks, or
       |       | question marks
  --------------------------------------------------------------------------------
  
  Time: 280ms; Memory: 8.5Mb
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template. Short, but looks sufficient for the module.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

(*) answers_to_comment_choose_node_form(), the $target['name'] appears to go straight from database (which is user-input) to output unescaped via the !placeholder. This will be XSS prone. Use the @placeholder here. You may also want to use format_username(), but this still needs to be plained.

(*) answers_to_comment_to_comment() has an access bypass problem. In general all queries against {node} should use db_select() with ->addtag('node_access'). You may be revealing
nodes that the current user doesn't have access to (even revealing just the title is a problem). You be need to reword this query so that {node} is the base table, and then join the rest.

(*) answers_to_comment_to_comment_access() should also check node_access('delete') to make sure the current user can delete the node.

(*) The menu $item for 'comment/%/to_answer' needs a custom callback which also does a _node_add_access() node_access('create', 'answers_answer') check to make sure a user can create nodes of that type.

Read https://api.drupal.org/api/drupal/modules!node!node.module/group/node_ac... for more info

Coding style & Drupal API usage

Project page and application above should make it more apparent that answers in this context means the Answers module. I didn't get that at first.

The additional comments on 'Implements ...' comments are superfluous, especially for the really common hooks, unless you need to explain why you are implementing it (eg, .

(+) The $role argument to answers_to_comment_to_comment_access() is really the $permission to check. This function should also have a proper docblock.

Does 'html' really have to be TRUE in answers_to_comment_comment_view() and answers_to_comment_node_view()?

answers_to_comment_to_comment() should use NODE_PUBLISHED instead of the value.

(+) answers_to_comment_to_comment(), avoid directly poking into a $node like that. Use the Field API; see field_get_items(). Also elsewhere.

(+) All of your custom functions need proper docblocks. See https://www.drupal.org/node/1354

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

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

This review uses the Project Application Review Template.

Assigning to myself for next review, which should be tonight.

Automated Review

Review of the 7.x-1.x branch (commit 69762db):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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

Did a quick read of `git diff ba7f045..` and didn't see any problems.

My four blockers are addressed, as are the strong recomendations.

Not seeing anything to hold up RTBC. Assigning to @er.pushpinderrana for a second look, if he has time.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None

Review of the 7.x-1.x branch (commit 69762db):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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

Coding style & Drupal API usage

List of identified issues.

1. (*) answers_to_comment_node_view(): 'title' => 'transform to comment' that is user facing text and should be wrapped with t(), same applies with answers_to_comment_comment_view().
2. (+) answers_to_comment_choose_node_form_submit(): Using drupal_goto() in a form submission handler() is hacky, and it should be avoided, since it stops the other form submission handlers from being called.. See http://drupal.stackexchange.com/questions/94578/drupal-goto-usage
3. answers_to_comment_to_comment(): You should move your callback function to a separate admin.inc or .inc file that is useful to keep all administration stuff like menu callbacks and forms etc. It's also useful for performance as well because all the functions from .module files get loaded at every initialization.
4. A hook_help() would be nice.
5. (*) answers_to_comment_choose_node_form_submit(): Implements hook_FORMID_form_submit, doc block is wrong, this is not a hook. See https://www.drupal.org/coding-standards/docs#forms.
6. Project Page: Extend this little bit more, better to go through Tips for a great project page again.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

I am not able to install this module using Drush.
Answer module 4.x has not a stable release and just a alpha version is available.

Also, 4.x version has not all the features available specially search functionality is missing.

[root@bangvmplccpc01 modules]# git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Fabulous/2367101.git
Initialized empty Git repository in /var/www/html/review/sites/all/modules/2367101/.git/
remote: Counting objects: 60, done.
remote: Compressing objects: 100% (60/60), done.
remote: Total 60 (delta 35), reused 0 (delta 0)
Unpacking objects: 100% (60/60), done.
[root@bangvmplccpc01 modules]# drush en answers_to_comment
The following projects have unmet dependencies:                                                                                                              [ok]
answers_to_comment requires answers, entity
Would you like to download them? (y/n): y
Project answers (7.x-3.1) downloaded to sites/all/modules/answers.                                                                                           [success]
Project entity (7.x-1.5) downloaded to sites/all/modules/entity.                                                                                             [success]
Project entity contains 2 modules: entity, entity_token.
No release history was found for the requested project (node_reference).                                                                                     [warning]
The following projects have unmet dependencies:                                                                                                              [ok]
answers_to_comment requires views, strongarm, references, nodereference_url, nodereference_count, features, eva
Would you like to download them? (y/n): y
Project views (7.x-3.8) downloaded to sites/all/modules/views.                                                                                               [success]
Project views contains 2 modules: views, views_ui.
Project strongarm (7.x-2.0) downloaded to sites/all/modules/strongarm.                                                                                       [success]
Project references (7.x-2.1) downloaded to sites/all/modules/references.                                                                                     [success]
Project references contains 3 modules: user_reference, node_reference, references.
Project nodereference_url (7.x-1.12) downloaded to sites/all/modules/nodereference_url.                                                                      [success]
Project nodereference_count (7.x-1.0) downloaded to sites/all/modules/nodereference_count.                                                                   [success]
Project features (7.x-2.2) downloaded to sites/all/modules/features.                                                                                         [success]
Project eva (7.x-1.2) downloaded to sites/all/modules/eva.                                                                                                   [success]
The following projects have unmet dependencies:                                                                                                              [ok]
answers_to_comment requires ctools
Would you like to download them? (y/n): y
Project ctools (7.x-1.5) downloaded to sites/all/modules/ctools.                                                                                             [success]
Project ctools contains 10 modules: ctools_access_ruleset, page_manager, stylizer, ctools_plugin_example, ctools_ajax_sample, bulk_export, term_depth, views_content, ctools_custom_content, ctools.
Module answers_to_comment cannot be enabled because it depends on answers  (4.x) but 3.1 is available                                                        [error]
[root@bangvmplccpc01 modules]#

You already provided some reason for using 4.x branch so I am not changing status of this issue.

I am not getting any way to download dependency's dev version through drush.
Rest everything looks good.

Moving this module to RTBC.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None.

Review of the 7.x-1.x branch (commit 5e72852):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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

(+) answers_to_comment_help(): Wrong path case 'node/%/to_comment' , instead it should be case 'admin/help#answers_to_comment':

Project Page: Extend this little bit more, better to go through Tips for a great project page again.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

My blocking issues from #21 have been addressed and nothing major jumped out at me when I read `git diff 5e72852..` so...

Thanks for your contribution, Fabulous!

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.