[D7] module_install

Looks good. But I think that you should give ability to users manage the directories to install. Currently you have hardcoded

'#options' => array(
         'modules' => 'modules',
         'modules/contrib' => 'modules/contrib',
         'modules/custom' => 'modules/custom',
         'modules/patched' => 'modules/patched',
       ),

May be settings for edit them, or using alter hook will be good.

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,

The git link on project application page is your private link. Please tick the check box for non-maintainers and refresh the page to get the git commands that others can use.

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/kriboogh/1962208.git module_install
cd module_install

I tried to install a module on my localhost into a contrib folder. But was met with an error. Seems more like some issue with my local setup.

Added project sandbox page link

Seem to be good now, but please note that features module use sites/all/modules/features for itself, so IMO will be good idea to exclude the 'features' subdir (just idea)

Seems to be a flexible, good and useful module now. Yea, you're right about the setting page.

Also I notice that module actually need from update_manager module to get working. I suggest you to add it as dependancie in the .info module_install.info

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

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

@kriboogh You'll need to participate in the Bonus Review program. You can get all the details you need from this post: http://drupal.org/node/1962318#comment-7262290

While the PA Robot suggests that approval of this project application will happen whether or not you participate in the review program, for all practical purposes, this project might not be approved for a very long time if you don't participate. Good luck!

Updating the status to needs work.

@musicalvegan0: please set issues only to "needs work" if you did an actual review and found application blockers.

@klausi Sorry about that. Thanks for letting me know.

Hi,

Nice module.

However, please do following:
1. Seperate not-hook functions from "module_install.module".
Functions such as "_module_install_get_search_directories", "_module_install_get_destination_directories" should be in a different file.
2. There are at least 2 external files that load for each page. You can try to make files load only in dedicated page. e.g.: admin/modules/%

Good luck :)

manual review:

1. module_install.updater.inc: why do you need the module_load_include() call there? Classes are loaded with the registry auto loader in Drupal 7, so I think you can remove that.
2. module_install_module_install_includes(): why is that empty hook implementation needed? Please add a comment. module_invoke_all() returns an array anyway?
3. module_install_form_alter(): use hook_form_FORM_ID_alter(), then you don't need the ugly arg() call and the form id check?
4. "variable_set('module_install_destination', $form_state['values']['destination']);": passing the destination directory around in a single variable could be unreliable and I can imagine race conditions when two admins try to install modules at the same time. I would add the current user ID and maybe project name to the variable, if there is no better way to pass the installation directory. And you should delete the variable after using it.
5. "'#title' => 'Destination folder',": all user facing text must run through t() for translation.
6. _module_install_get_destination_directories(): you could also provide alter hooks by using drupal_alter(), so that module can also change exceptions/includes.

Although you should definitively fix those issues they don't seem to be critical application blockers, so I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to cweagans as he might have time to take a final look at this.

Thanks for your contribution, kriboogh!

I updated your account to let you 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 get 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.

When issues are done, mark them as fixed. If there's no further activity for two weeks, they'll automatically be closed.

Thanks!