[D7] Doubtfire

Hi, thanks for your contribution here are my observations.
Review of the 7.x-1.x branch (commit 0f1005b):

Automated Review

Found errors in automated review: http://pareview.sh/pareview/httpgitdrupalorgsandboxwelsby2570705git

Manual Review

Individual user account

[Yes] Follows the guidelines for individual user accounts.

No duplication

[No] Same core concept as the existing Masquerade module.

Master Branch

[Yes] Follows the guidelines for master branch.

Licensing

[No] Does not follow 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] But, automated review found some security errors

Coding style & Drupal API usage

1. (*) Implement hook_help() to help the users to get the basic understanding about your module.
2. (*) Fix errors found by automated review.
3. Just a recommendation: add .editorconfig to .gitignore

Hello,

Thank you for reviewing the code, I'm a main contributor to the Doubtfire project.

A few questions:

The Project application checklist states to not include a LICENSE.txt, I was wondering what it is about the module that doesn't follow the licensing requirements?

I have pushed some code that fixes some of the automated errors, but there are a few issues which require some clarification:

• "js/doubtfire.js: line 9, col 10, Error - closesearch is defined but never used (no-unused-vars)" this function is called from within the template file in an OnClick attribute.
• "Input to unserialize is unsanitized from variable_get" - what would you advise to solve this? check_plain() makes the string unable to be unserialized, I have added filter_xss to the call chain however now it is telling me that the input to filter_xss is unsanitized.

I have implemented hook_help().

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,

I reviewed your module. Please see my observations below:

1) Instead of calling module_load_include() function in hook_init() you can use it inside specific functions. So that we can avoid un-nessary load of files.

2) Add hooks in .module file instead of in inc files.

3) Why are you created custom function doubtfire_error_message() to set message? You can directly use drupal_set_message() to set message. Is there any particular reason?

function doubtfire_error_message($message) {
  drupal_set_message(check_plain(t('!message', array('!message' => $message))), 'error');
  return FALSE;
}

also you can avoid check_plain() function in above code by using @message instead of !message.

4) I installed this module and admin config url admin/config/doubtfire/settings is not loading any form elements. I saw in doubtfire-admin.tpl.php

<?php
/**
 * @file
 * Template file for admin page.
 */
?>
<?php print render($form); ?>

If there is no other template customization then you no need to use a template file for admin form. Also you can mention related file in hook_menu iteself like below:

$items['admin/config/doubtfire/settings'] = array(
    'title' => 'Configure',
    'description' => 'Configure the Doubtfire Module',
    'page callback' => 'doubtfire_admin_page',
    'access arguments' => array('administer doubtfire'),
    'type' => MENU_NORMAL_ITEM,
    'file' => 'doubtfire.admin.inc',
    'file path' => drupal_get_path('module', 'doubtfire') . '/includes',
  );

5) Instead of using menu callback you can add a checkbox/radio in settings page to enable/disable configuration.

$items['admin/config/doubtfire/toggle'] = array(
    'title' => 'Toggle Doubtfire',
    'description' => 'Enables/Disables Doubtfire',
    'page callback' => 'doubtfire_admin_toggle',
    'access arguments' => array('doubtfire go undercover'),
    'type' => MENU_NORMAL_ITEM,
  );

  $items['admin/doubtfire/toggle'] = array(
    'title' => 'Toggle Doubtfire',
    'description' => 'Enables Doubtfire',
    'page callback' => 'doubtfire_admin_toggle',
    'position' => 'left',
    'weight' => 100,
    'access arguments' => array('doubtfire go undercover'),
    'type' => MENU_NORMAL_ITEM,
  );

6) Instead of adding custom html in hook_page_build(&$page), provide it as a custom block. So that it will be more flexible. Also found attaching external css file with page.

$page['content']['#attached'] = array(
    'js' => array(drupal_get_path('module', 'doubtfire') . '/js/doubtfire.js'),
    'css' => array(
      drupal_get_path('module', 'doubtfire') . '/css/doubtfire.css' => array(
        'group' => CSS_DEFAULT,
        'weight' => CSS_THEME,
        'every_page' => TRUE,
      ),
      'https://maxcdn.bootstrapcdn.com/font-awesome/4.4.0/css/font-awesome.min.css' => array(
        'type' => 'external',
        'every_page' => TRUE,
      ),
    ),
  );

Thanks,
ARUN AK

Hi Arun,

Thanks for reviewing the code! I've pushed some changes to the repository, here are my notes on your points:

1 & 2: I have removed doubtfire.admin.inc altogether as it was mostly hook_ functions, the favourites include file is now only loaded on page builds when doubtfire is toggled "on".

3: Originally this function was created for DRY reasons, as I was often repeating drupal_set_message followed by a return FALSE for validation, I have refactored this out to use the more verbose method now. Thank you for the check_plain tip I wasn't aware of that!

4: This template file has been removed and it is now a drupal_get_form, this template file was created early on in case we needed to do anything custom with the admin page.

5: I believe this goes against one of the core strengths of the module, which is the ability to toggle the action bar on and off from anywhere provided you have the administration bar showing.

6: I have added block functionality under our future functionality list.

I'd suggest updating $items['doubtfire/login-as/%'] = array( (http://cgit.drupalcode.org/sandbox-Welsby-2570705/tree/doubtfire.module#n55) to $items['doubtfire/favourite/%user'] = array(.

By using %user, the account will be automatically loaded via user_load() if it exists, so there's no need to check for this within the callback function. function doubtfire_login_as_page($uid) { would then become function doubtfire_login_as_page($account) {. See "Auto-Loader Wildcards" on the hook_menu() documentation.

This could also be changed for any other menu callback that require a user account as an argument.

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.

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

Good submission and module. The code seems pretty clean now and I couldn't really spot any issues. The only ones you might want to look for are the security issues mentioned in the automated review.

Automated Review

Only a few errors in the automated review, mostly formatting issues, however there was a few issues with the use of i : http://pareview.sh/pareview/httpgitdrupalorgsandboxwelsby2570705git-7x-1x

Manual Review

Individual user account

[Yes] Follows the guidelines for individual user accounts.

No duplication

[No] Does not cause module duplication and/or fragmentation.

Master Branch

[Yes] Follows the guidelines for master branch.

Licensing

[Yes] Follow 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] Uses db_select properly although automated review show a security errors.

Coding style & Drupal API usage

(*) Fix errors found by automated review.

Multiple Applications

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

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

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

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.

I installed this on my local development machine, and I am rather impressed with it. I have been using masquerade for years and was always upset with how it handled switching users and the need to constantly clear the cache. Good work on this project!

@welsby - You can now promote this to a full project yourself.

Thank you for your contribution!

I updated your account so you can opt into security advisory coverage now.

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!

Thank you, 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 go the dedicated reviewer(s) as well.

Assigning credits.