[D7] Hover Card

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

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 Rishi,

As your module is using a 3rd party library, you should look at using the Libraries module to include the hovercard plugin. https://www.drupal.org/project/libraries

I'm not sure what the best practice is for callback URL structures but you may want to change this to reduce the chances of a conflict with content aliases.

hover_card.js

• Indentation should be 2 spaces, see https://www.drupal.org/coding-standards#indenting
• Line 15 should not have a trailing comma.
• The selector $('.hover-details') is used multiple times and should be assigned to a variable for efficiency.

Including a CSS file rather than inline styles would be preferable.

Lastly, you may want to consider using theme functions to return the contents of the hover card. This will allow developers to alter the display without need to hack the module.

Good luck with your submission,

Louis

Hi Rishi,

Automated Review

You still need to set the default branch of your repository. To do this edit your sandbox project and click on the green "Default branch" tab near the top. Then just select the 7.x-1.x branch and save. Other than that the review is clean :)

Manual Review

Individual user account: Yes.
Master Branch: Follows guidelines. Mentioned setting the default branch above.
Licensing: None included (Follows guidelines).
3rd party code: Doesn't look to contain any. Uses the Libraries API to load an external jQuery library.
README.txt/README.md: You should add a configuration section even if no configuration is needed; See https://www.drupal.org/node/2181737 under "Configuration".
Security: Not any that I see...
Code long/complex enough for review: Yes. Meets minimum requirement of 5 functions and >120 lines.
Coding style & Drupal API usage: Passes the automated reviews. Also, uses the libraries API and various other Drupal hooks.

In reference to the hover_card() callback:
I think the description of this callback isn't as clear as it could be. What you appear to be doing is using "Drupal Magic" to fetch the user data via the Menu System %user argument and then rendering that content into a hover card. There does not appear to be any JSON involved. Also, you use the $user_fields variable when you should use the $user variable to make it clear that this is the $user object loaded via the menu system. The code on lines 98-100 looks incorrect/brittle to me - a short comment could go a long way here. What do you expect to be in the second position of the users roles & how are you sure it will always be there? Also, I think the brackets around $value are unnecessary; is this correct?

Additional Minor Comments

• Your template file could use some additional whitespace to keep the line length closer to 80 characters. Also, why do you feel the need to not include the footer? Could you add a comment in the code explaining the use of this very rarely used function: drupal_exit()?
• You should add a file doc block to your JS file. Furthermore, in-line comments do wonders for making jQuery readable ;-)

@ Rishi Kulshreshtha, thankyou for your contribution!

Your module well worked for me, Good Job!

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Just one minor

FILE: ...w/drupal-7-pareview/pareview_temp/templates/hover-card-template.tpl.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
29 | ERROR | [x] Whitespace found at end of line
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and 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 the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. Do add hook_help() in your module, would be useful.
2. Project page looks good to me including Readme.txt, very well organised.
3. In hover_card function, you are using $array variable to hold user data, better to use some meaningful name like $user_data or vice-versa.
4. I tested this module functionality for logged in and anonymous users. I have also attached screenshot of both to make it more understandable for other reviewers.
   Admin User:
   
   
   
   
   

   Anonymous User:
   
   
   
   
   

   It work fine as intended. Personally one more thing you can enhance here, If you look into source code for anonymous user, would found every js and css of this module loading on every page that are unused. I think it can be improved to load all necessary files only for logged in users :)

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.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :)

As there is no blocker found, so moving to RTBC.

But I would give one suggestion to you, if you implement your JS file logic with Drupal Ajax framework would be nice. Ajax framework commands provides every possible commands that your require to implement your ajax logic.

Thanks Again for contributing this nice module.

Hi Rishi Kulshreshtha,

Thanks for your contribution.

I don't see any blockers for this module, however I see one PII (Personally identifiable information) concern here, as we are not giving any options to show/hide user data on that information overlay and directly exposing user's email address to other users.
I suggest you to edit your project page and clearly mention that "If you have PII compliance then do not use this module" or provide options show/hide user data.

Thanks

Please don't RTBC your own issues, see the workflow: https://www.drupal.org/node/532400.

Please treat these rules with understanding and be patient.

Automated Review

No issues.

Manual Review

Individual user account

Yes.

No duplication

No any duplication.

Master Branch

Yes

Licensing

No Licensing file. ok

3rd party code

No any ard party code. ok

README.txt/README.md

Yes.

Code long/complex enough for review

Yes.

Secure code

Yes.

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

• 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. hover_card_init(): why do you need hook_init(), it does not make sense to load your library on drush invocations for example? Why don't you load the library when you actually need it? You already have hover_card_page_build() anyway, so you can remove hook_init() and load the library there.
2. hover_card_page_build(): why is this only done for logged in users? Please add a comment.
3. hover_card_page_build(): you have a render array with $page available, so you should use #attached in it for your JS and CSS instead of drupal_add_js/css(). See https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...
4. hover_card_page_build(): if I understand correctly then you only want to add your JS/CSS when a user name is printed on the page, correct? Then you should implement a preprocess hook for theme_username instead of hook_page_build(), so that it is only added when actually needed.
5. hover_card_menu(): The path hover-card/%user is not protected at all with a permission and reveals sensitive information such as the user picture or email address. This needs to use at least the "access user profiles" permission. And we consider email addresses private information in Drupal (so that they are not harvested by spammers) which means that the default value to show the email address should never be TRUE. This is an access bypass security issue. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
6. hover-card-template.tpl.php: All user facing text like "Roles", "Email" etc. need to run through t() for translation.
7. hover-card-template.tpl.php: drupal_exit() is wrong in a template, that should be done in the page callback function if you have to.
8. hover_card_theme(): the template does not specify the variables that will be passed to it? See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
9. hover_card(): this is vulnerable to XSS exploits. User name, roles, email is user provided data and you need to sanitize it before printing to HTML. Make sure to read https://www.drupal.org/node/28984 again.
10. "variable_get('hover_card_user_email_display_status', TRUE)": all variables defined by your module need to be removed in hook_uninstall().

Hello @klausi

I found one problem in your manual review.

10. "variable_get('hover_card_user_email_display_status', TRUE)": all variables defined by your module need to be removed in hook_uninstall().

Please check following article.
-----------------------------------------------------------

https://www.drupal.org/node/1187664#issues

Your module doesn't remove the Drupal variables it defines.
Drupal 6 modules can implement hook_uninstall() to remove Drupal variables it defines, and the database tables it installed; in Drupal 7, database tables defined from a module are automatically removed.

-------------------------------------------------------------

As you see in the article, I think Drupal 7 automatically remove all varialbes used in module when uninstall it.
Am I wrong?

@sendinblue

As you see in the article, I think Drupal 7 automatically remove all varialbes used in module when uninstall it.

In Drupal 7, database tables defined from a module are automatically removed, not variables. The variables that the module has set using variable_set() or system_settings_form() should remove on uninstall. Database tables defined by hook_schema() will be removed automatically.

See: https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...

@ er.pushpinderrana

Thanks very much for your info.
Really I'm a newbie for drupal. (my experience is 2 month) :)

I'll correct my sandbox project too as your info.

Regards

> /**
* @file
* Install, update and uninstall functions for the hover_card module.
*/
unable to find install and update functions inside hover_card.install file as you mentioned in comments.

 if (!empty($user_picture)):
  ?>
    <div class="user-image">
    <?php print_r($user_picture); ?>
    </div>
  <?php endif; ?>

Why print_r() is being used to render $user_picture in hover-card-template.tpl.php and it would be nice if you say hover-card.tpl.php

* If i enable user picture and hover user profile name then it doesn't show any thing in username and email but roles as 'anonymous user' though already logged in as admin. Please find attached image for reference.

manual review:

1. hover_card_uninstall(): the variable_get() call is not needed and can be removed.
2. hover_card_menu(): The path hover-card/%user is not protected at all. You need to remove 'access callback' here so that default access callback user_access() is used.
3. hover_card(): doc block is wrong, this is not a hook. See https://www.drupal.org/coding-standards/docs#functions , so the first line should use the description of a page callback.
4. hover_card_ajax_callback(): same here with the docs, this is not a hook but a page callback.

The unprotected menu path is a security blocker right now. 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 2e9c616):

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

  
  FILE: /home/klausi/pareview_temp/hover_card.module
  --------------------------------------------------------------------------------
  FOUND 2 ERRORS AFFECTING 2 LINES
  --------------------------------------------------------------------------------
   109 | ERROR | Type hint "array" missing for $user
   146 | ERROR | Missing parameter type
  --------------------------------------------------------------------------------
  
  Time: 174ms; Memory: 6.25Mb
  

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

But otherwise looks good to me now.

Assigning to stBorchert as he might have time to take a final look at this.

Automated Review

FILE: /var/www/drupal-7-pareview/pareview_temp/hover_card.module
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
109 | ERROR | Type hint "array" missing for $user
146 | ERROR | Missing parameter type
--------------------------------------------------------------------------------

Manual Review

Individual user account

Yes: the guidelines for individual user accounts.

No duplication

Yes: module duplication and/or fragmentation.

Master Branch

Yes: the guidelines for master branch.

Licensing

Yes: the licensing requirements.

3rd party assets/code

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

README.txt/README.md

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

Code long/complex enough for review

Yes: 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. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. Nothing have been found. Only fix those issues found on pareview

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.

Thanks for your contribution, Rishi!

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.

Some small notes:

function hover_card($user = array()) { }

$user is not an array but should always be an object.

Forgot to set status.