[D7] SideComments Now

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxmelvix2633590git

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.

Hi @melvix,

This looks really interesting. I'm a bit snowed under right now, but if I get a chance to install this later tonight I will.

It's the kind of module I could imagine being used on a lot of media sites.

The <script> tag in sidecomments-now-wrapper.tpl.php appears to be a just a placeholder for some JS vars. Is this all the template is supposed to do? (i.e. is there a reason why a themer would expect to edit this template?)

If the template is just a way to inject some JS vars, it is probably better to do it using the Drupal.settings JS API (see https://www.drupal.org/node/304258#drupal-settings). Inside your Drupal.behaviors JS you'll be able to access Drupal.settings.sidecomments_now.sideCommentNid, instead of window.sideCommentNid. The advantage of doing this is that you'll be able to dispense with the sidecomments_now_wrapper entirely.

The other sidecomments module doesn't appear to have had much work done on it, just a stub module really. However it does appear that the author intends it to use the core comment module; it's listed as a dependency.

Since this module lets an admin user add executable JS code, you probably want to use the 'restrict access' => TRUE option in hook_permission().

maybe i have done something incorrectly while installing, but i got that error in console(i have installed side-comments from git):

side-comments.min.js?nzqero:1 Uncaught TypeError: this.$body.on is not a function
Drupal.behaviors.sidecomments_now.attach @ sidecomments_now.js?nzqero:16
(anonymous function) @ drupal.js?nzqero:76
b.extend.each @ jquery.js?v=1.4.4:33
Drupal.attachBehaviors @ drupal.js?nzqero:74
(anonymous function) @ drupal.js?nzqero:481
b.extend.ready @ jquery.js?v=1.4.4:30u @ jquery.js?v=1.4.4:38

Automated Review

No automated tests, but this is not a requirement.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

As melvix mentioned above and has been discussed in the comments to this issue, there seems to be a person working on a Drupal 8 version, although it has not progressed very far and hasn't seen any commits recently.

So this would be a No: Causes module duplication and/or fragmentation.

On the other hand, I'd rather not block on this and leave it for the maintainers of the project applications to decide, what to do here.

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/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No: Doesn't meet the security requirements. See below for more information on the issue(s) identified.

Coding style & Drupal API usage

1. (*) In hook_menu() you implement the sidecomments_now/delete/%, where you check for permission to edit own comments. The argument is a cid and in the callback you just call comment_delete(), so a user might actually delete any comment from the system. This is a security issue! Please check, that the comment was acutally posted by the current user, before you delete the comment.
2. (*) As andrewmacpherson already said above in #9, you should set 'restrict access' => TRUE for your permission in hook_permission(), since you allow admins to add executable JS code to the page and this might be a security issue, if you don't trust those users.
3. Since libraries module is listed as a dependency, why did you implement the fallback code in _sidecomments_now_get_library_path()?
4. Line 106 of your module file: You probably want to call drupal_exit() instead of exit here.
5. Typo on line 396 of your module file.
6. Line 37 of admin.inc: As a best practice and to facilitate l10n, don't use !link to insert the link markup into t(). Use t('<a href="@link_url">Link title</a>'). See https://www.drupal.org/node/322774 for more info.
7. The doc blocks of several functions are missing proper documentation of parameters and return values.
8. Upon saving a node, which has sidecomments enabled, I get a notice: Notice: Undefined index: en in sidecomments_now_node_presave() (line 150 of sidecomments_now.module). This is, because $node->language is en, but the body field uses LANGUAGE_NONE. I was able to fix this by replacing $lang with LANGUAGE_NONE in lines 150 and 229 of your module.
9. Next, I got $ is not defined with the latest stable release of sidecomments.js. It worked with the master version. You might want to document this requirement somewhere.
10. Then I got section.data() is undefined, because the text format I was using was filtering the data attributes, so I had to switch to Full HTML to make it work. You might want to document somewhere, that in order for your module to work, you need to configure your text format to allow class and data attributes.
11. Since I have disabled "Enable user pictures." at Configuration > People > Account settings > Personalization, I got an empty img path in the comment form. as I think this is the default, you might want to check for that setting and act accordingly. The same might happen, if the user did not upload a picture. I didn't test that, though.
12. If I posted a comment and closed the comment box and later opened it again, the text of the comment was missing.

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.

Hi melvix,

thank you for addressing all the issues and also for giving credit in the commit messages. That's very nice.

I didn't do a full review this time, but instead reviewed the git commits made since the last review. I verify, that the issues I identified – especially the blocker/security issues – have been fixed. I also checked your project again using preview.sh and it found a few minor issues, but this doesn't block the application.

I really appreciate your effort trying to work with the maintainer of the sidecomments module. I said before, that I'd rather not block on this, but leave it for a maintainer of the project applications to decide, what to do here. So...

... in my view, this module is RTBC :).

Regarding your questions:

I don't see a typo anywhere in this area. Can you elaborate?

I was probably referring to this comment // in a jsn object in the way that will make sidecomments.js happy., where jsn should read JSON, I guess. It's now on line 476.

I added more documentation, but I did not see any that were missing info about return values specifically. Please let me know if you are referring to a specific function.

That's definitely an improvement! However, here are a few functions I was referring to:

• sidecomments_now_permalink(): Missing @params and @return
• sidecomments_now_comment_reply(): Missing @params and @return
• theme_sidecomments_now_user_picture(): Missing @return
• sidecomments_now_save(): Missing @return
• sidecomments_now_callback_js_default(): Missing @return
• All functions in mollom.inc: Missing @param and @return
• All functions in admin.inc: Missing at least @param

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

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

  FILE: /home/klausi/pareview_temp/sidecomments_now.module
  -----------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  -----------------------------------------------------------------------
   120 | ERROR | Return type missing for @return tag in function comment
  -----------------------------------------------------------------------
  

• DrupalPractice has found some issues with your code, but could be false positives.
  

  FILE: /home/klausi/pareview_temp/sidecomments_now.module
  ---------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  ---------------------------------------------------------------------------
   168 | WARNING | Doc comment indicates hook_form_alter() but function
       |         | signature is "sidecomments_now_form_node_type_form_alter"
       |         | instead of "sidecomments_now_form_alter". Did you mean
       |         | hook_form_FORM_ID_alter()?
  ---------------------------------------------------------------------------
  

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

1. sidecomments_now_install(): do not juggle with module weights since that is unreliable. Use hook_module_implements_alter() if you need to run before/after a specific hook. And the comment does not tell me why you need to adjust the weight. Always document why you are doing something - what you are doing I can read from the code itself :-)
2. Does this module only work on the default body fields or on other fields as well? Please add that to the project page.
3. sidecomments_now_save() and sidecomments_now_delete() are vulnerable to CSRF attacks. You need to confirm user intent either with the Form API (which has built in CSRF tokens) or you need to validate CSRF tokens yourself. See http://epiqo.com/de/all-your-pants-are-danger-csrf-explained and https://docs.acquia.com/articles/protecting-your-drupal-module-against-c... . And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

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

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

• ESLint has found some issues with your code (please check the JavaScript coding standards). See attachment.
• 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:

1. sidecomments_now.js: why do you need escapeHtml()? Should you use Drupal.checkPlain() instead?
2. template_preprocess_sidecomments_now_js(): "$sidecomment->authorName = $comment->name;": could you add a comment here explaining why this does not have to be sanitized? I assume that sidecomments.js takes care of sanitizing it when it displays it?
3. _sidecomments_now_get_library_path(): the module_exists() call is not needed here since the libraries module will always be there (it is a dependency).
4. JS error: TypeError: this.$body.on is not a function. Does this module work in the default bartik theme on a standard Drupal install?

But otherwise looks good to me. Assigning to mpdonadio as he might have time to take a final look at this.

The correct ESLint config version ships with Drupal core, I fixed the project page at https://www.drupal.org/node/1955232

So your module should depend on jquery_update module if you need a higher jquery version?

no objections for more than a week, so ...

Thanks for your contribution, Dagwood!

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.