[D7] Multiple role login pages

role_login_page.module

severity: criticalreview: security_16Line 47: The value for the 'access callback' must always be a string which is the the name of the function - never a function call. It may also be assigned the value TRUE or FALSE if the callback is always (or never) accessible. (Drupal Docs) [security_16]

'access callback' => (user_is_anonymous()) ? TRUE : FALSE,

severity: normalreview: style_control_spacingLine 98: Control statements should have one space between the control keyword and opening parenthesis [style_control_spacing]

if($uid = user_authenticate($username, $password))

severity: normalreview: style_curly_bracesLine 99: curly braces { should end a line, not start one [style_curly_braces]

{

severity: normalreview: style_uppercase_constantsLine 104: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE [style_uppercase_constants]

return true;

severity: normalreview: style_curly_bracesLine 111: curly braces { should end a line, not start one [style_curly_braces]

{

severity: normalreview: style_uppercase_constantsLine 113: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE [style_uppercase_constants]

return false;

Please run coder module.

Hi Nisith,

There are some errors reported by automated review tools. See below URL and resolve the issue generated by automated script first.
http://pareview.sh/pareview/httpgitdrupalorgsandboxnisith2696411git

Thanks,
Ajesh

Hi @Nitish,

Please update module REAdME.txt file with more details. You can review README Template format to update the file.

This will help users who will be using this module to understand and configured it easily.

Thanks

@Nitish

Also update GIT clone URL in this post.

The URL is -

git clone --branch master https://git.drupal.org/sandbox/Nisith/2696411.git multiple_role_login_pages

so that community members can clone and review your module.

Thanks

Hi @Nitish,

As a part of module review on drupal.org you need to do some manual reviews of project posted by community members at https://www.drupal.org/project/issues/projectapplications

Once you reviewed some projects, you need to add URLs of reviews in this post as part of review bonus system.

For more information please check How it works and Summary section at below URL -

https://www.drupal.org/node/1975228

Thanks

Hi @Nitish,

Important:- For this post/issue you have selected your own project which is wrong.

Below is the URL where we can view all the projects which are posted for the review. Updating the project for this post to Drupal.org Project applications so that all the reviewers can see your project and review it.

URL - https://www.drupal.org/project/issues/projectapplications

Thanks

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.

Hi @Nisith,

I have manually reviewed your code, found the below issues,

1. _role_login_page_settings_submit(): Before inserting in to db you are passing all the values through filter_xss(). Best practice is always store exactly what the user typed. Conversions should be performed when content is output, not when saved to the database. Found same in _role_login_page_settings_edit_submit() also. Refer Handle text in a secure fashion.
2. _role_login_page_settings_list(): Pass label text through check_plain()/filter_xss_admin() before display it to the user.
3. Use confirm_form() to before delete an item instead of custom intermediate from. See Avoid cross-site request forgeries (CSRF).
4. All user facing text should be passed through Drupal's t() function.

   Found
   $output .= '<a href="' . $base_url . '/admin/config/login/role_login_settings" style="float:right;">Add login page</a>';

5. Also use MENU_LOCAL_ACTION instead of html markup to display action links at top of listing page. Check the implementation in core node module.
6. Not able to edit exiting login page configuration. While saving configuration getting following error message.
   

   The menu URL already exists

   Found

   $menu_exists = db_query_range("SELECT 1 FROM {menu_links} WHERE link_path = :link_path", 0, 1, array(':link_path' => $url))->fetchField();
       $login_page_exists = db_query_range("SELECT 1 FROM {role_login_page_settings} WHERE url = :link_path and rl_id <> :rl_id", 0, 1, array(':link_path' => $url, ':rl_id' => $rl_id))->fetchField();
       if ($menu_exists && !$login_page_exists) {
           form_set_error('loginmenu_url', t('The menu URL already exists'));
       }

7. Explain more details, configuration and instructions in README.txt file. Not following the guidelines for in-project documentation and the README Template.

Thanks,
ARUN AK

Hi Nisith

Automated Review

By PAReview.sh:
http://pareview.sh/pareview/httpsgitdrupalorgsandboxnisith2696411git

DrupalPractice has found some issues with your code, but could be false positives.
FILE: /var/www/drupal-7-pareview/pareview_temp/role_login_page.admin.inc
------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------
290 | WARNING | Unused variable $update_login_url.
------------------------------------------------------------------------

Please fix code review first

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
No: Follows the guidelines for in-project documentation

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

(+) Please add hook_help

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 walk through 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
Nhan

@mrken201016: looks like you forgot to change the status. Is this now RTBC after your review or are there application blockers left and this should be "needs work"?

Hi Nisith

Automated Review

By PAReview.sh:
http://pareview.sh/pareview/httpsgitdrupalorgsandboxnisith2696411git

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

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

It is okie with me. i've Just changed status to RTBC

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 walk through 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
Nhan

Hello @Nisith

Please check following warnings.
http://pareview.sh/pareview/httpgitdrupalorgsandboxnisith2696411git

EDIT: removed long pareview.sh dump.

@Sanjay Chauhan: please don't post long pareview.sh dumps directly to the issue here, a link or attachment is better :)

Thanks for your contribution, Nisith!

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.