[D7] Global Logout

Hi Anishnirmal,

Please make your code more standard by following this documentation https://www.drupal.org/coding-standards

Fix the issue from
Automated Review:

Review your code on pareview.sh it shows exact standard for the drupal.

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Anishnirmal/2645348.git global_logout

Hello Anishnirmal,

Few Suggstions :

global_logout.module file
1) At hook_user_login() you have defined two variable.
Its a standard practice to prefix all such variable with the module name as all such variables go into 'variable' table in DB.
2) In global_logout_all_devices()
$sql = "DELETE from {sessions} WHERE uid = " . $uid . "";
here $sql variable is not used any where, hence this should be removed.

3) where ever we use drupal_goto(), its a good practice to add exit(); statement in the following line.

4) Define the class in some separate file.

etc...

Automated Review

I ran your code against auto code sniffer at 'http://pareview.sh/pareview/httpgitdrupalorgsandboxanishnirmal2645348git'.

Git errors:

Git default branch is not set, see the documentation on setting a default branch.
Review of the 7.x-1.x branch (commit 9ab3c8b):

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

function load_session_info($uid, $sid, $flag) {
Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
DrupalPractice has found some issues with your code, but could be false positives.
FILE: /var/www/drupal-7-pareview/pareview_temp/global_logout.module
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
---------------------------------------------------------------------------
47 | WARNING | Unused variable $sql.
83 | WARNING | Unused variable $db_op.
93 | WARNING | Class name must be prefixed with the project name
| | "GlobalLogout"
302 | WARNING | Unused variable $user_info.
305 | WARNING | Unused variable $key.
---------------------------------------------------------------------------

FILE: ...ar/www/drupal-7-pareview/pareview_temp/global_logout_config.admin.inc
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
18 | WARNING | All variables defined by your module must be prefixed with
| | your module's name to avoid name collisions with others.
| | Expected start with "global_logout" but found
| | "global_menu_name"
---------------------------------------------------------------------------

Time: 108ms; Memory: 8.75Mb

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.

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Few observation :
1. global_logout.module file at line no. 47
   $sql = "DELETE from {sessions} WHERE uid = " . $uid . ""; here sql variable not used anywhere in this fuction.
2. Use more comments.

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.

global_logout.install
Change the doc comment, it's currently the copied version of the admin menu module.
You might also want to add the following code to your globallogout_session_info schema for documentation purposes:

'foreign keys' => array(
  'globallogout_session_info_user' => array(
    'table' => 'users',
    'columns' => array('uid' => 'uid'),
  ),
),

global_logout.module
Move views hook implementations into seperate files. See hook_views_default_views and hook_views_data for more information.
Line 93: Place Global Logout Default inside a t().
Line 94: Place Global Logout Custom database table inside a t().
Line 145: Do not place parameters directly into the query. Please read the db_query documentation for more info.
Line 149: You might want to give the user the option to configure a date format.

Automated Review

Some issues detected by pareview.sh. Please fix these.

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

No: Does not follow 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

No: Does not follow 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

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (*) Readme is hard to read. Id split up the text into sections with clearly labelled section titles.
2. (*) Default git branch is not set properly.
3. Any reason the 'Active Logins' menu is placed with the main menu and not in the users account page? I think it would be cleaner and make much more sense there.

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.

The git branch and readme format are not application blockers, anything else that you found or should this be RTBC instead?

Looking back at it, the 2 variables set in the settings form are never deleted on uninstall. But unless thats a blocker, I think its good to go.

Add validation for time format field on Global logout setting page. Also add description for time format field and link to php.net so that user enter correct date.

Hi Anishnirmal,
Manual review Suggestions:
1. global_logout_user_logout() we can get user object from $account argument so no need load global variable($user).
2. Also in global_logout_user_login() the $account object have user details.
3. In function global_logout_load_session_info() please use return instead of using echo on line 150 and 153.

Thanks & Regards
Arunkumar K

Readme mentions you need to flush cache. That happens automatically when enabling a module so it is an unnecessary step.

What is BR_OS referring to? Ah, now I see, it refers to browser and operating system. I thought at first it was referring to the country of brazil. Maybe just spell out the full name?

h5 nested directly below an h2 seems odd for your print out in the view.

+ Using php in your view is highly frowned upon. And I don't believe it is even necessary. You can use variable replacement.

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 honestly hesitated a lot before marking this Fixed and granting git access on the last point. Please, please don't use php in views. However, the method in which it is being used isn't insecure and therefore not technical a security issue.

Thanks for your contribution, Anishnirmal!

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.