[D7] Missing Module

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

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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 Paul,

Great idea for a module. I've had a need for this in the past. PAReview looked good (http://ventral.org/pareview/httpgitdrupalorgsandboxp4ul1947350git)

A few things that jumped out (see http://drupal.org/node/1587704):

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

Lines 34-36 of missing_module.install - These should be formatted for better readability.

hook_requirements() - If I'm reading the API documentation correctly, the return value array should have four parts. You are missing the description. I think you should dump all of your $missing_output information into description instead of value. Value can be empty if not applicable.

Paul,

Code fixes look better.

When on /admin/reports/status and have a missing module, your message provides a link to the module page to disable the module. This is a good idea, but if a module does not reside in code, it doesn't show up on the module page.

A couple of thoughts. Maybe you could put a disable link directly next to each module path, that when clicked, the database entries will be removed. Also, in addition to having the Drush download code, could you provide a link to the d.o project itself for those who don't use Drush?

Paul,

Your new changes look and work great. I guess your next step is to either wait until someone with the proper privileges comes along and reviews your project, or perform the steps outlined to get the review bonus.

Good luck!
Rob

Hi p4ul,

First of all i like this module. I installed your module in local system and verified for the same. It looks promising.
----
No issues found in automated review in ventral.org
----

I just noticed some minor things.

Files:
missing_module.module
missing_module.install
missing_module.module

Line 2 & Line 8: Single blank line is enough instead 2 lines.
Line 6: Unwanted '*' character.

Thanks and Regards,
D. Veera Prasad.
www.drup-all.com

What is the difference between this http://drupal.org/project/bootstrap_optimizer ?

Hey,

I installed and reviewed your module manually and i think it's a pretty neat and usefull idea ... The only thing that i feel is that you should add is some hook to cron or a 'check' button in an admin page, since your only checking during install hook or when 'drush mm' is called, and i think you should try to use system_list('module_enabled') instead of running a db_select to gather the active modules.

Your absolutely right about the runtime, sorry about that.

About the system_list .. not sure what you should do there, using framework functions seems better then a db_select no ? Up to you, and keep up the good work ..

Hello Paul:
It's a very useful module. But I find a file named:'missing_module.drush.inc', which seems unnecessary, is it a test file? :>

drush mm work fine!

I suggest you optimalize the README.TXT
Like:

-- SUMMARY --

This module lists modules that are activated in your database but missing
from your file system.

These can greatly impact the performance of your drupal 7 site.

For example: 1 missing module lead to 900 file_scan_directory() calls
resulting in a wasted 250ms every page load.

-- Usage --
1 To use either view the site status report
/admin/reports/status
2 from drush run
drush mm

-- Module url --

* http://drupal.org/xxx

-- CONTACT --

Current maintainers:
*** - http://drupal.org/user/***

Hi p4ul,

First i found this module interesting, it could be used for my pre-production site.

Just two suggestions :

• In the missing_module_disable_confirm_submit() function, you don't need to use arg() function because arguments are still in $form_state.
  So you can replace : $module_name = arg(4); by $view = $form_state['build_info']['args'][0];
• Actually, you can delete a module present on the filesystem, for example delete a module like system with : admin/reports/status/disable_module/system
  To avoid this action, in the missing_module_disable_confirm() function, you could check if the module is enabled and return an error message in this case.

Hi pierre_cotiniere:
I'm not module creator.T_T.

Hi jeffstric, fixed.

Very straightforward module, short and sweet. Consider renaming the link to the module pages to something along the lines of "Module Home" just to make sure people understand that it links to the module home page instead of maybe Drupal itself. I'll be recommending this to my colleagues because it deserves more attention then it's getting at the moment. Nice Work!

*Doublepost

p4ul,
it's not to inform, it's to block uninstallation if the module exists on the filesystem, to prevent errors.
With your module, i can disable the system module for example.

I guess your next step is to either wait until someone with the proper privileges comes along and reviews your project

There are no special privileges required to review projects. Everyone can do it.

I see two blocker issues:

1) The permission for disabling modules is currently "access administration pages" and should be "administer modules". In general, it's very important to use the most specific permission available, so you're not granting any more access than necessary.
2) As #26 said, you're currently disabling modules with no check to confirm they're actually missing code. There's nothing wrong with disabling modules that have code, but that would need to happen differently, with module_disable(), to run the proper hooks in the module. Probably simpler to only allow your module to disable modules with no code rather than write two different processes for disabling modules.

Manual Review

In .module file at line number 38 add t() function for 'title' => 'Disable Module',
For line 55 to 60, do proper indentation.

Hi,
This is a useful module, Thanks !
Here is the result of my review :
- $result = db_select('system')->fields('system', array('filename'))->condition('status', '1', '=')->range(0, 150)->execute();
I think it would be better to not hard code '150' without giving the user the possibility to alter it.
I would suggest to add a config form to set this value and use variable_get() instead of the hard coded value.

- $missing_modules = missing_module_find_missing();
You are doing this twice : in the form and in its submit handler, i would suggest to store the result you get from the form builder in $form_state['missing_module']['modules'] for example, so that you can access it in the submit handler and avoid an extra query/foreach/file_exists ;)

-Extra suggestion : For better performance the .module should only contain necessary code : the form builder and its submit handler are not necessary if we are not in 'admin/reports/status/disable_module/%'' page. i suggest to add a 'file' => 'missing_module.form.inc'to your $items['admin/reports/status/disable_module/%'] array and move the form definition and submit handler to it.

Hi,
I reviewed it again and tried it locally, it look good for me.
Thanks

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

manual review:

1. maintainers link on project page is broken
2. please add the differences to http://drupal.org/project/bootstrap_optimizer to the project page.
3. "$value = 'No missing modules detected.';": all user facing text must run through t() for translation.
4. missing_module_find_missing(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075
5. missing_module_disable_confirm_submit(): this should have a comment why you cannot use module_disable().

But that are not blockers, so ...

Thanks for your contribution, p4ul!

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.