[D7] Locker module

Git clone command for the sandbox is missing in the issue summary, please add it.

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.

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

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

Git clone command is wrong:

git clone --branch master websolutions.hr@git.drupal.org:sandbox/websolutions.hr/2262991.git locker_module
Cloning into 'locker_module'...
websolutions.hr@git.drupal.org's password:

Also, no need to tag this with 'module review'! It's already in the issue queue for that!

That should not be an application blocker, you can use the version control tab on the sandbox page. Can you review the source code?

Given the ratio of the number of applications to the number of reviewers, I feel it's important to try to keep the process streamlined where possible.

Besides, the PA robot set it to needs work on the same grounds:

> Git clone command for the sandbox is missing in the issue summary, please add it.

Well nearly... that gets me a folder called 2262991.

/**
 * Implements form_print().
 */

?? That's not a hook name! Also this function looks really odd.

description = "Locker - authentication module"

That doesn't say much!

  if (!function_exists('drush_main')) {
    $current_page = $_GET['q'];

Looks weird. Document what on earth is going on here!

unlock.html

It's not customary to use .html in paths.

Also, again, what on earth is all that about??!

 * Implements hook_form().

I really hope this is NOT implementing https://api.drupal.org/api/drupal/modules!node!node.api.php/function/hoo...! Also, how could it, given the function name!

  $active_value = variable_get('site_locked', NULL);

Prefix your variable_get() variables with your module's name.

 * Implements hook_form_validate().

No such hook. Look at core code to see how to document form builders and handlers.

 * Stylesheet for the Locker module.

Make it clear that this stylesheet is only for the single page.

it seems that this module is not passed through the basic "Pareview" check.
Get a look at this: http://pareview.sh/pareview/httpgitdrupalorgsandboxwebsolutionshr2262991git. There are some errors in your code.

You are also working in the "master" branch in git. Instead you should work in a version specifich branch. Get a look at:

• Moving from a master branch to a version branch.
• release naming conventions
• creating a branch in git

to accomplish this task.

Git clone command in issue summary is still wrong.

Not a blocker, and maybe somebody else will be enthusiastic enough about this project to visit the version tab and find the right command to clone. However, given the length of the review queue, I am not. I don't see why I should use some extra effort to review somebody that is too sloppy to even get the issue summary right.

The priority for this application should not be changed to major at this time.

2. Basic repository checks

2.1 Ensure the repository actually contains code.

Repository contains code.

2.2 Ensure you are working in a version specific branch.

Version specific branch used.

3. Security Review

3.1 Ensure the project does not contain any security issues.

I couldn't find any that were obvious other than from PAReview.

4. Licensing checks

4.1 Ensure the repository does not contain a ‘LICENSE.txt’ file.

No LICENSE.txt

4.2 Ensure the repository does not contain any 3rd party (non-GPL) code.

No 3rd-party code.

5. Documentation checks

5.1 Ensure the project page contains detailed information.

Project page seems to indicate what the module does and how it is used.

5.2 Ensure the repository contains a detailed README.txt.

README.txt exists, and has documentation.

5.3 Ensure the code contains a well-balanced amount of inline-comments.

Very little in the way of inline comments. This code definitely needs to be better documented.

6. Coding standards and style

6.1 Run an automated review and ensure there are no major issues.
Automated Review:

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

Remove all .project files from your repository.
Remove all .settings files from your repository.
Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting

FILE: /var/www/drupal-7-pareview/pareview_temp/locker.info
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
5 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/locker.module
--------------------------------------------------------------------------------
FOUND 32 ERRORS AND 5 WARNINGS AFFECTING 20 LINES
--------------------------------------------------------------------------------
1 | WARNING | [ ] File has mixed line endings; this may cause incorrect
| | results
9 | WARNING | [ ] Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().",, "* Implements
| | hook_foo_BAR_ID_bar() for xyz-bar.html.twig.", or "*
| | Implements hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.".
9 | ERROR | [x] Whitespace found at end of line
49 | ERROR | [ ] You must use "/**" style comments for a function comment
105 | ERROR | [x] Whitespace found at end of line
106 | ERROR | [x] Whitespace found at end of line
107 | ERROR | [x] Whitespace found at end of line
109 | ERROR | [x] Whitespace found at end of line
110 | ERROR | [x] Whitespace found at end of line
111 | ERROR | [x] Whitespace found at end of line
112 | ERROR | [x] Whitespace found at end of line
113 | ERROR | [x] Whitespace found at end of line
124 | ERROR | [ ] You must use "/**" style comments for a function comment
139 | ERROR | [x] The open comment tag must be the only content on the line
139 | WARNING | [ ] Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().",, "* Implements
| | hook_foo_BAR_ID_bar() for xyz-bar.html.twig.", or "*
| | Implements hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.".
139 | ERROR | [ ] Doc comment short description must be on the first line
139 | ERROR | [ ] Function comment short description must start with exactly
| | one space
139 | ERROR | [x] Whitespace found at end of line
140 | ERROR | [ ] Doc comment short description must be on a single line,
| | further text should be a separate paragraph
140 | ERROR | [x] Whitespace found at end of line
179 | ERROR | [x] The open comment tag must be the only content on the line
179 | WARNING | [ ] Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().",, "* Implements
| | hook_foo_BAR_ID_bar() for xyz-bar.html.twig.", or "*
| | Implements hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.".
179 | ERROR | [ ] Doc comment short description must be on the first line
179 | ERROR | [ ] Function comment short description must start with exactly
| | one space
179 | ERROR | [x] Whitespace found at end of line
180 | ERROR | [ ] Doc comment short description must be on a single line,
| | further text should be a separate paragraph
180 | ERROR | [x] Whitespace found at end of line
195 | ERROR | [x] Whitespace found at end of line
196 | ERROR | [ ] Use XHTML style tags instead of
196 | ERROR | [x] Whitespace found at end of line
202 | ERROR | [x] The open comment tag must be the only content on the line
202 | WARNING | [ ] Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().",, "* Implements
| | hook_foo_BAR_ID_bar() for xyz-bar.html.twig.", or "*
| | Implements hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.".
202 | ERROR | [ ] Doc comment short description must be on the first line
202 | ERROR | [ ] Function comment short description must start with exactly
| | one space
202 | ERROR | [x] Whitespace found at end of line
203 | ERROR | [ ] Doc comment short description must be on a single line,
| | further text should be a separate paragraph
203 | ERROR | [x] Whitespace found at end of line
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 20 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/locker.inc
--------------------------------------------------------------------------------
FOUND 9 ERRORS AND 1 WARNING AFFECTING 6 LINES
--------------------------------------------------------------------------------
1 | WARNING | [ ] File has mixed line endings; this may cause incorrect
| | results
1 | ERROR | [x] End of line character is invalid; expected "\n" but found
| | "\r\n"
1 | ERROR | [x] Additional whitespace found at start of file
6 | ERROR | [ ] There must be exactly one blank line after the file comment
8 | ERROR | [x] Line indented incorrectly; expected 1 spaces, found 0
8 | ERROR | [ ] You must use "/**" style comments for a function comment
13 | ERROR | [ ] There should be no white space before a closing "}"
13 | ERROR | [ ] Closing brace must be on a line by itself
29 | ERROR | [x] Line indented incorrectly; expected 3 spaces, found 2
30 | ERROR | [ ] Files must end in a single new line character
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

7. API and best practices Review

7.1 Ensure you are using Drupals API correctly.
I've never seen anyone attempt to do this before, but I think deeper adherence to 'The Drupal Way' is warranted.

My Specific Issues:

Use a template for the unlock page, instead of trying to generate html in code.
A menu item of unlock.html is going to be server/?q=unlock.html, which somewhat defeats the purpose.
I can't understand why you're using '#states' the way you are in locker_lock_form
In locker_lock_form_validate, you don't verify there is data to work with before working with it.
form_set_error('name' doesn't seem to correspond to any elements.

Module is not working for me.. i don't know may be i configure it wrong but i follow steps written in readme file

I just do a quick review of your project, please see below points

1. Update the purpose of your module in readme file.

2. There are many places where you direct get the value from form_state, so if form state has no variable set then it may through error, Please use isset function.

3. Please use install file to delete variable from database and remove sites/default/files/locker_logos folder when module is uninstall.

4. Try to use drupal api like you may use user_hash_password instead of md5, drupal_get_query_parameters() instead of $_GET['q'].

5. In this locker_lock_form_submit() function

you wrote

  $file = file_load($form_state['values']['logo']);
  if (!empty($file)) {
    $fid = $file->fid;
  }
.
.
.
  if (!empty($file)) {
    variable_set('locker_logo', $fid);
  }

you may write in this way

  $file = file_load($form_state['values']['logo']);
  if (!empty($file)) {
    $fid = $file->fid;
    variable_set('locker_logo', $fid);
  }

It will optimise your code

6. Please follow the drupal coding standard. Please describe about the function and put some comments in function definition.

Thanks for your contribution. Here are my initial comments:

Getting following error while uploading logo:

Please strip HTML tags from all input fields. These tags are security risk.

Also use drupal_add_css would be a better option to include CSS file to locker page.

/**
 * Using function locker_unlock_form_print() to get form.
 */
function locker_unlock_form_print() {
  $get_form = drupal_get_form('locker_unlock_form');
  $print_form = drupal_render($get_form);
  $display_logo = variable_get('locker_display_logo', NULL);
  if ($display_logo == 'no') {
    $logo_print = '<div style="margin-top:150px;"></div>';
  }
  else {
    $fid = variable_get('locker_logo', NULL);
    if (!empty($fid)) {
      $file_load = file_load($fid);
      $file_uri = $file_load->uri;
      $locker_logo = file_create_url($file_uri);
      $logo_print = '<div class="logo"><img src="' . $locker_logo . '"></div>';
    }
    else {
      $logo_print = '<div class="logo"><img src="' . base_path() . drupal_get_path('module', 'locker') . '/img/logo.png"></div>';
    }
  }
  $print_form = '<div class="form"><h3>Locker</h3>' . $print_form . '</div>';
  $css_file = '<link type="text/css" rel="stylesheet" href="' . base_path() . drupal_get_path('module', 'locker') . '/css/locker.css" media="all" />';
  print $logo_print . $css_file . $print_form;
  drupal_exit();
}

Also I am not getting error if my credentials are incorrect on unlock.html.

Hi info@websolutions.hr

Manual Review

Individual user account

No: Does not follow the guidelines for individual user accounts.

According to https://groups.drupal.org/node/427683#comment-1062368 the account of this application must belong to an individual.

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/ No: Does (not) follow the guidelines for in-project documentation and/or the README Template.

I'm not so sure about this one, there are nearly no inline comments.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

You are using an unsaltet md5 hash to store the password. People tend to recycle their passwords, and when your site gets compromised, the attacker also gets a password.

Coding style & Drupal API usage

1. (*) Once you unlock the site in the frontend, it stays unlocked. In the backend it says, that the site is still locked though.
2. (*) The locking process only works, when you have an active session. Once you clear your cookies or use an other browser, the site is automatically unlocked!

The starred items (*) are fairly big issues and warrant going back to Needs Work.

This review uses the Project Application Review Template.

Tobi

Automated Review

No best practice issues identified by pareview.sh / drupalcs / coder. http://pareview.sh/pareview/httpgitdrupalorgsandboxwebsolutionshr2262991git

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.

Manual Review

Individual user account

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

Your code does not seem very well commented, please make sure it´s clearly understandable by other developers. Please have a look at the module documentation guidelines and the Doxygen and comment formatting conventions

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

NO! Does not meet the security requirements.

Coding style & Drupal API usage

• + Do not trust MD5 as it has fallen out of favor and is considered by many to be no longer secure. See https://www.drupal.org/writing-secure-code/hash-functions for more information.
  

  the md5() and sha1() hash functions should never be used in any code, since they are considered obsolete and potentially insecure for some applications. This is a settled policy for Drupal core. For a normal hash function use sha-256

• When returning to drupal/admin/config/development/locker after enabling it, it is not clear whether a password is set or not. Consider adding a method to indicate whether or not a password is set, perhaps by hiding the password fields and setting a "Change Password" button. This may help reduce confusion.
• * No hook_help exisits. Please consider adding it to drupal/admin/config/development/locker in the very least as it is not clear what needs to be done to use the module.
• * Your account does not appear to belong to an individual.
• * Virtually no inline comments could be found in your code.
• I'm not sure it is waranted to have the module appear in it's own Package (WEBSOLUTIONS.hr modules). Perhaps having it listed in Other or Authentication would be more appropriate and make it easier to find for site administrators.

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.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Automated Review

no issues

Manual review
every thing seems good. functionality working fine.It is acting as a protection layer before accessing site

Hi info@websolutions.hr

Automated Review

everything fine

Manual Review

Individual user account
Yes: Follows follow 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/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. / No: List of security issues identified.]

Hi

I tried to use your module.
But, it seems to lack the basic documention for the project.
So I was unable to understand what it does.
Please visit "https://www.drupal.org/node/2190239".
So, please fix these things and good luck :).

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2576713

Project 2: https://www.drupal.org/node/2264953

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Hi, info@websolutions.hr
I will like to say that it a good module, perfectly as in the description.
I have just one queries that is would list:
How is this module different from user login.(We can display the data to the users when they are logged in, not otherwise)

Hi info@websolutions.hr,

It seems like your drupal.org account is not individual user account. This account is of any organisation account. Can you please clear it.

Otherwise, your module works fine and I think there is no other major blocker for this module.

Hi info@websolutions.hr,

But your profile page has the 'Organisation Member' badge.

@infowebsolutionshr

Part of the Standard Project review process is for reviewers to check if an account belongs to that of an individual or an organization.

PAReview: Individual user account

It seems you are using a non-individual account.
All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).
Please note that organization accounts cannot be approved for git commit access. See https://drupal.org/node/1966218 and https://drupal.org/node/1863498 for details on what is/isn't allowed. Please update your user profile so that we don't have to assume that this is a group account.

This is also echoed in the Drupal.org terms of service Section A. Accounts, sub-section 3.

3. If you are sharing your user account with multiple people (e.g. as your “official” organization account), you are not allowed to do the following using this account:

commit code to Git repositories on the Website
create any nodes except for organization, case study or project nodes
comment on nodes
If you are sharing your user account with multiple people you ARE allowed to:

create project nodes
create organization nodes
create case study nodes
submit translations to localize.drupal.org

The wording of your profile text, the username on the account, and the fact that your account has the organization member Drupal Association badge, lead the reviewers to believe your account does not belong to an individual. That is what is blocking the approval of this project, and your account being given the permission to promote projects from sandbox to full projects. Once the account is updated to reflect that of an individual and not an Organization I'd support the change of this application to "Reviewed & Tested by the Community".

I hope this helps clarify the confusion.

There is an individual name on the user page, so this does not appear to be an organization account.

Hi @klausi,

If this account is individual user account, I think, there is no other major blocker in this module.

Bumping to major.
Has been RTBC for more than 2 weeks.
Was created almost 2 years ago.
Has been through multiple rounds of review.

@info@websolutions.hr - if you could please review other peoples modules, and add links to those reviews in the issue summary, that will also speed up your final approval and permissions. For more info on the PAReview process see https://www.drupal.org/node/1975228

Thanks for your contribution! Congratulations, you are now a vetted Git user. You can promote this to a full project.

When you create new projects (typically as a sandbox to start) you can then promote them to 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 (here you get to ask fans of your project to help you!)

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.