[D7] SN Quick Multisite

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

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,

Manual review :

1. Remove $host_string = ''; sn_quick_multiple.inc line 65
2. Undefined variable $default_data sn_quick_multiple.inc line 154
3. Why do you set $output = ''; before adding string in it ?
4. $site_values, $cloning_values, $hostfile_values are not used sn_quick_multisite_form.inc

Hi,

I have created a module and looking someone who can review my modules.
https://drupal.org/node/2063215

Thanks,
Ajesh

Hi

Please find some of my quick review comments.

1) Please give one space gap between each loop and condition and each function declaration.
2) Remove $sitename parameter use $domain_name instead in function sn_quick_multisite_prepare_baseurl($sitename) as it is multiple assignment in line 194.
3) Remove commented code from function sn_quick_multisite_clone_table_populate($from_db, $to_db) in sn_quick_multisite.inc
4) Remove two returns from function sn_quick_multisite_get_dbdata_by_folder($folder) on line 586, 591 in sn_quick_multisite.inc
5) Remove condition if (!strstr($url, 'http://')) use parse_url instead on line 672 in sn_quick_multisite.inc
6) Unwanted variable $tld on line 645, 686 in sn_quick_multisite.inc
7) Error scenario handling missing in public function makeWorldReadable(&$filetransfer, $path, $recursive = TRUE) for sn_quick_multisite_copy_dir.inc

Thanks,
Paritosh Gautam

sn_quick_multisite.module
Line # 33: Incorrect 'access arguments'. This feature will not work for user other than superadmin.

1) Spelling of Domain in link (Create quick multisite with sub domians).
2) Usually in Production enviroment, IT Infrastructure team does not share the path of the Host file,
instead, you should ask for confirmation if Valid Subdomain is configured in your infrastructure or not. If it is available , no need for the host file path.
3) Space (package[]) to be removed from .info file.
4) if condition in sn_quick_multisite_form_hostfilevalidate() – sn_quick_multisite_form.inc
should be re written to check if file exists and inside the if block if check for Write permission. At present these are two separate condition ( if and else if)
5) Directory separator to be used to take care multiple platforms/os issues in file paths.

At present you are asking for the Host file path which May or may not be available to admin due to Infrastructure securities on production/staging environment. But for Development environment We might need to add host entries to make multisite work.

you can ask
1) Confirmation from user if subdomains are configured if it is prod/staging kind of environment.
2) If subdomain are not configured ( probably a dev enviroment) ask for the file path.

HI,

I have review modules and find one observation on files sn_quick_multisite.inc

at line # 670 to 674
// URL from which domain info to be fetched.
  $url = $_SERVER['HTTP_HOST'];

  if (!strstr($url, 'http://')) {
    $url = 'http://' . $url;
  }

you are comparing only for http:// if the website have ssl enable then this condition fails.
Overall it seem to good modules and working as expected.

Thanks,
Ajesh

manual review:

1. The project page should have a big fat warning that this module should never be used on a production site, as it is a security risk to have a writable sites folder.
2. sn_quick_multisite_menu(): why the permission "administer users"? Shouldn't it be "administer site configuration"?
3. If I test this on http://simplytest.me/project/2054335 and I submit the config form nothing happens, no error message, no success message.
4. What is the host file? /etc/hosts on linux systems? Please add a description.
5. An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /batch?render=overlay&id=2&op=do StatusText: Service unavailable (with message) ResponseText: PDOException: SQLSTATE[42000]: Syntax error or access violation: 1044 Access denied for user 'drupal-7'@'localhost' to database 'foo': DROP DATABASE IF EXISTS foo; Array ( ) in sn_quick_multisite_create_db() (line 261 of sn_quick_multisite.inc). Of course my Drupal site has only access to its own database on the database server, this is another security best practice and another risk if arbitrary database can be dropped.

Honestly I don't see the point of such a module on site, you should use more mature and secure solutions such as Aegir.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Hello madhusudanmca.

I fixed a couple of spelling and grammar issues for your project's readme file. I have attached a patch. Please take a look to make sure I corrected the wording in a way that kept your intended meaning.

I hope this helps. Good luck with your module.

Hi

Review of the 7.x-1.x branch

1. Please, fix it https://drupal.org/node/2066777#comment-7762313
2. sn_quick_multisite.inc (functions: sn_quick_multisite_create_db, sn_quick_multisite_clone_table_populate, ) - need use placeholders.
3. sn_quick_multisite.inc (function sn_quick_multisite_copy_dir_batch) - contains duplicate code. Recommend create and use separate function for set arguments of theme or module.
4. sn_quick_multisite.inc (function sn_quick_multisite_get_host_details) - unnecessary use the str_replace function (recomment to write comments in the function).
5. sn_quick_multisite.module (function sn_quick_multisite_help) - need use placeholders and wrap to one t() function.
6. file sn_quick_multisite_copy_dir.inc - need to be included via .info file.
7. file sn_quick_multisite_form.inc (line 168) - missed placeholder.
8. Please, define settings page in the .info file.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

How does SN Quick Multisite compare with the OpenScholar project?
https://drupal.org/project/openscholar

• Instead of using ajax callbackx in sn_quick_multisite_action_form() to modify elements on the fly, use the State API, see https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen... and https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_....
• In sn_quick_multisite_form_validate_sitename() should you allow underscores and dashes in a site name? Those are valid as far as I know.
• The function sn_quick_multisite_make_host_enrty() should be sn_quick_multisite_make_host_entry().
• In sn_quick_multisite_clone_table_populate(), don't use backticks (`) around the db and table names, not all mysql server are configured that way.
• How does sn_quick_multisite_generate_dbname() know it has generated a unique name? I think you need a while loop instead of an if statement. You're just hoping the same random number doesn't get generated twice.

Seems like a useful module, obviously lots of security concerns. Could you add something to the project page warning that installing this module allows all users with "administer site configuration" quite a lot of power.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Thanks for those updates. The whitespace on the do/while loop look off, but that's the only problem I see. You've documented that the db user will need very high level of access, so I can't think of anything more that's needed.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Sorry for the delay, but you forgot to add the "PAReview: review bonus" tag.

manual review:

• "form_set_error('sitename', t('"!sitename" subdomain site already exists.'": the site name is user provided text, so you should use the "@" or "%" placeholder with t() which will sanitize the variable with check_plain() for you. See https://drupal.org/node/28984 . This is not an XSS security blocker now because you need the high level "administer site configuration" permission to exploit this, which is listed as exception on https://drupal.org/security-advisory-policy

Otherwise this looks good, so ...

Thanks for your contribution, madhusudanmca!

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.