[D7] Module Checker

Git clone failed for http://git.drupal.org/sandbox/jabastinameex/2829119.git while invoking http://pareview.sh/pareview/httpgitdrupalorgsandboxjabastinameex2829119git

Git clone failed. Aborting.

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.

Fixed the git clone URL in the issue summary for non-maintainer users.

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

@Jabastin Arul: Module working fine and displaying all the modules correctly.

Hello,
As for me this module is useless and has a bad UI.
Please check https://www.drupal.org/project/module_filter, for now it much better, but don't have few your functions.
Try to integrate your module with module_filter module. It will bring more benefit to the Drupal community.

Also I have found a bug:
Your module show me this table:

Testing Module         66	0	66
Core Module	        47	28	19
Contribute Module	58	26	32
Custom Module	3	3	0
Other Module	        6	3	3

But I don't have 66+47+58+3+6 = 180 modules.
Command drush pml | wc -l show only 102 (101, because 1st line is just a header)

Needs work.

Jabastin,

I would suggest using custom permissions for the reports. Now it can be viewed only by administrators. Since it's a module for report & analysis there maybe cases where a user with different roles(other than admin) may try out this module. In that case adding necessary permissions would be advisable.

Automated Review:

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

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:

1. In "module_checker.admin.inc" file as I can see, you using the same code to render the multiple times, the code is as follows:
$header = array(
t('Module Name'),
t('Machine Name'),
t('Module Type'),
t('Enable/Disable'),
);
// Render table.
$render_array['maintable'] = array(
'#theme' => 'table',
'#header' => $header,
'#rows' => $other,
);
You can create a function for this and reduce your code upto 50%.

2. You are passing the site url in the function module_checker_is_404 and defining the $site_url as $site_url = "https://www.drupal.org/project/" . $machinename; just before calling the function each time. You can just pass the module name to function and remove the code duplication by defining the url inside the function.

3. On lIne 239 in file "module_checker.admin.inc" , you are using $httpCode in camel letters. Camel letters in not considered best practices in drupal.

@vaibhavdev: nice tips, but those alone are surely not application blockers. Anything else that you found or should this be RTBC instead?

@klausi: i think this is not a application blocker and can be moved as RTBC

Looks good so far moving to RTBC

kindly take care of @vaibhavdev suggestions

Hi Jabastin Arul,

Module Looks good and working fine for me.

My Findings are :

1: All user facing text should be passed through t().

Example :

$rows = array(
    "Test" => array(
      "Testing Module",
      count($test),
      $test_enable,
      $test_disable,
    ),
    "Core" => array(
      "Core Module",
      count($core),
      $core_enable,
      $core_disable,
    ),
    "contrib" => array(
      "Contribute Module",
      count($contrib),
      $contrib_enable,
      $contrib_disable,
    ),
    "custom" => array(
      "Custom Module",
      count($custom),
      $custom_enable,
      $custom_disable,
    ),
    "other" => array(
      "Other Module",
      count($other),
      $other_enable,
      $other_disable,
    ),
  );

2: In Read me we have a Type error : "identy" , please fix with "identify"

2. Easily identy the custom, contribute and core module.
So it's really helpful for site auditing.

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 Jabastin Arul,

I found 2 things. First, there are some errors in pareview, need to fix it.
https://pareview.sh/node/138

Second, you used system_rebuild_module_data() function 7 times in your module_checker.admin.inc file. Whenever this function gets called, it again and again hit db,

function system_get_files_database(&$files, $type) {
  // Extract current files from database.
  $result = db_query("SELECT filename, name, type, status, schema_version, weight FROM {system} WHERE type = :type", array(':type' => $type));
  foreach ($result as $file) {
    if (isset($files[$file->name]) && is_object($files[$file->name])) {
      $file->uri = $file->filename;
      foreach ($file as $key => $value) {
        if (!isset($files[$file->name]->$key)) {
          $files[$file->name]->$key = $value;
        }
      }
    }
  }
}

and reset cache. I think redundant use of this function should get minimized from performance perspective.

Hi,

in module_checker_help hook your t() length is greater than 80 character
https://www.drupal.org/docs/develop/standards/coding-standards#linelength

thanks

Hi Jabastin,

Found few errors in pareview

15 | ERROR | [x] Equals sign not aligned correctly; expected 1 space
| | but found 2 spaces
273 | ERROR | [x] Equals sign not aligned correctly; expected 1 space
| | but found 2 spaces
308 | ERROR | [x] Equals sign not aligned correctly; expected 1 space
| | but found 2 spaces
341 | ERROR | [x] Equals sign not aligned correctly; expected 1 space
| | but found 2 spaces
374 | ERROR | [x] Equals sign not aligned correctly; expected 1 space
| | but found 2 spaces
408 | ERROR | [x] Equals sign not aligned correctly; expected 1 space
| | but found 2 spaces
------------------------

Please fix this .... https://pareview.sh/node/2533

Module works fine .!! No errors found

Thanks,
Aaron

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These 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 more contributors chatting on the IRC #drupal-contribute channel. 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.

I thank all the dedicated reviewers as well.