[D7] Subfolders Domain

Multiple Applications

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

Project 1: http://drupal.org/node/1993640

Project 2: http://drupal.org/node/1992940

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.

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.

This is something I was looking for awhile ago! Really interested in this one. I've seen something similar(also a sandbox project) by agentrickard(the maintainer of domain access): http://drupal.org/sandbox/agentrickard/1175752 have you checked it out? If you haven't you should!

Branch
It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

General
I miss a lot of commenting. You added only one line describing the function. In the function a lot of things happen without barely any comment about how and what. This makes it hard for other people to jump in and help. Might be handy to add more comments.

subfolders_domain.module
Line 417 - I think subfolders_domain_valid is a better name. As your comments suggests, it check if its valid.

/**
 * Is the domain choice valid.
 */
function subfolders_domain_valide($subdomain, $error_list = array(), $form_id = '') {

Line 463 - The module file is quite big, maybe seperate the admin functions to a seperate admin include. e.g:
function subfolders_domain_admin($form, &$form_state, $domain = array()) {
function subfolders_domain_admin_validate($form, &$form_state) {
function subfolders_domain_admin_submit($form, &$form_state) {

Line 568 - Shouldn't this happen in the install file?

// Create table subfolders_domain if not exist yet.
    if (!db_table_exists('subfolders_domain')) {
      $schema_subfolders_domain_sites = array(
        'description' => 'Table subfolders_domain',
        'fields' => array(
          'site' => array(
            'type' => 'varchar',
            'length' => 255,
            'not null' => TRUE,
          ),
        ),
      );
      db_create_table('subfolders_domain', $schema_subfolders_domain_sites);
    }

Line 463 - Plus i think this check is not even needed because in the form function there is already a check. And i don't think this should be needed if its moved to the install file(in hook_update or hook_install)

if (!db_table_exists('subfolders_domain')) {
    return FALSE;
  }

Line 584 - Same with the dynamicly adding of fields to the table. Is this really needed because the fields don't seem dynamic. Can't this be also in the install file? It would clean up a whole lot of code.

foreach ($form_state['values']['subfolders_domain'] as $field => $value) {
        if (!db_field_exists('subfolders_domain', trim($field))) {
          db_add_field('subfolders_domain', trim($field), array(
            'type' => intval($value) > 0 ? 'int' : 'varchar',
            intval($value) > 0 ? 'default' : 'length' => intval($value) > 0 ? 0 : 255,
            'not null' => FALSE,
          ));
        }
      }

If I'm missing something and there's a really good reason for this, please comment it.

I'm scanning through the rest of the file but I see too many db_table_exists and db_field_exists and even a sleep(1) in it incase the table is not created yet? It seems like the database logic might need an overhaul.

Line 707 - You are assuming role id 3 is an administrator role. This is an id that can be different for other websites. It might be better to check for a permission with user_access. Maybe an existing permission or one of your own.

Line 722 - You added your own domain roles functionality. There is a domain roles module for this. It is recently upgraded to a full project: http://drupal.org/project/domain_roles
function subfolders_domain_roles_form($form, &$form_state, $account) {

Line 1108 - It might be better to use the function domain_get_domain() instead of global $_domain;

Line 1173 - Again you create a table in a function. Why not in the install file? It will make your module file so much easier to read without all these checks and db_create_tables

function subfolders_domain_create_alias() {
  // Create table subfolders_domain_sites if not exist yet.
  if (!db_table_exists('subfolders_domain_sites')) {
    $schema_subfolders_domain_sites = array(
      'description' => 'Table subfolders_domain_sites',
      'fields' => array(
        'site' => array(
          'type' => 'varchar',
          'length' => '255',
          'not null' => TRUE,
          'default' => '',
        ),
        'domain_id' => array(
          'type' => 'int',
          'unsigned' => TRUE,
          'not null' => TRUE,
          'default' => 0,
          'description' => 'Domain numeric id.',
        ),
      ),
    );
    db_create_table('subfolders_domain_sites', $schema_subfolders_domain_sites);
  }

Line 1234 - Typo in comments
// ALIASIES.

It's a lot clearer now! But there are still some things that might be unnecessary. Plus I still miss a lot of commenting. There is sooo much code but soo little comments. It is much better though, just some functions.

subfolders_domain.module
Line 23 - Is this needed? Domain module is a dependency so I assume this function will always be there.

if (function_exists('domain_set_domain')) {
    domain_set_domain(subfolders_domain_resolve_host('', TRUE), TRUE);
  }

Line 88 - This function seems useless, the table is created on the install file so it will always be there.

/**
 * Is domain admin ready to work.
 */
function subfolders_domain_admin_ready() {
  return db_table_exists('subfolders_domain');
}

Line 437 - Is the sleep function really needed? It looks so dirty.

function subfolders_domain_form_submit($form, &$form_state) {
  sleep(1);

Line 598 - This function does not seem to be called anywhere? But I guess it might be usefull for other modules. The only thing wrong with it is that it looks like it got the wrong comments for the function.

/**
 * Subfolders domain admin form.
 */
function subfolders_domain_subdomains($domain_id = NULL) {
  static $sites = array();
  if ($domain_id != NULL && isset($sites[$domain_id])) {
    return $sites[$domain_id];
  }

  $rs = db_select('subfolders_domain_sites', 's')->fields('s', array('domain_id', 'site'))->execute();
  $sites[1] = '';
  if (!empty($rs)) {
    foreach ($rs as $domain) {
      $sites[$domain->domain_id] = $domain->site;
    }
  }
  return $sites;
}

Line 912 - Seems like old code? Just remove it
// $user_domains = domain_gesubfolders_domain_domains($user);

Multiple Applications

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

Project 1: http://drupal.org/node/2002640

Project 2: http://drupal.org/node/1993640

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.

Multiple Applications

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

Project 1: http://drupal.org/node/1993640

Project 2: http://drupal.org/node/2002640

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.

manual review:

1. "exec("rm " . DRUPAL_ROOT . "/" . trim($domain->site));": do not use exec to delete a folder! Use file_unmanaged_delete_recursive() or drupal_rmdir() or something similar. Also you are not escaping the site property here, which might pose a security vulnerability (depending what characters are allowed in site).
2. subfolders_domain_enable(): why do you need to clear the cache? Please add a comment.
3. why do you need subfolders_domain_cron.php and cannot use hook_cron()? Please add a comment and documentation about that.
4. subfolders_domain_boot(): this should be documented as hook, see http://drupal.org/node/1354#hookimpl
5. "(int) 1": why do you have to cast an integer to an integer?
6. "$cache[$subdomain] = isset($domain->domain_id) && (int) $domain->valid > 0 && (int) $domain->domain_id > 0 ? (int) $domain->domain_id : (int) 1;": That line is impossible to read, please split it up into proper if statements. Also elsewhere.
7. subfolders_domain_infos(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075
8. $_SERVER['HTTP_HOST']: I'm a bit worried about the security implications of using that because it can be spoofed by an attacker. It is hard for me to follow what that would mean for your module, can you look into that?

So the exec() call is a blocker right now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Demo site

exec call still present in subfolders_domain_disable().

Ventral review is still green. Good job!

Manual Review, Part 1, (lines 1-238)

subfolders_domain.install

Lines 13, 16, 22, 28, 34, 53, 57, 63

It would be nice (but not necessary) if you provided something a little more descriptive here. Otherwise, I would just drop these lines.

Line 78

I would just say something like "Configure the role for each user on the 'Subfolders Domain Roles' tab on the user page" instead of using a URL.

Line 88

I think this is redundant as core already shows a success message.

subfolders_domain.module

Line 12

First, I don't see why you prefix the name of this variable. It is only in scope within this file. Second, is there a reason why you are suppressing errors? Generally it would be better to check for an error condition beforehand.

Lines 28-50

Some more explanation of what this code does would be nice, either in the docblock or in inline comments.

Lines 60-66

I think that aligning a portion of the array but not all of it doesn't improve readability much.

Line 109

I think a function name like subfolders_domain_manage_domains_access() would be clearer.

Line 118

First, why are you only running if there is no referrer? Second, it seems that $_SERVER['HTTP_REFERER'] is not standard, so it isn't guaranteed to exist.

Line 165

Generally, it is preferable to use something more specific than $temp.

Line 166

The !empty() check seems redundant, because count will just return 0 and the code will not run.

Lines 158-173

A little more clarification on what this code does would be nice.

Line 186, 187

It's a better practice to use request_uri() instead of $_SERVER['REQUEST_URI'] because latter is Apache-specific. $_SERVER['REDIRECT_URI'] also is an Apache thing, but I don't know of a Drupal alternative. It would be wise to at least point out in a comment that this code is not portable, or better detect the server and do something reasonable if it is not Apache.

Line 188

Do you know about the arg() function? It will get segments of the URL for you.

Line 189

Is there a reason why you aren't caching for non-bootstrap cases?

Lines 193, 210

I would break this code onto multiple lines just to make it a bit easier to read.

Lines 200-206, 220-224

I don't think this code does anything. If there is some side effect I didn't notice, please tell me.

Line 209, 229

I don't think $domain is in scope here.

Line 236

Why are you only returning a value if $bootstrap is true?

This is all that I have time for right now. Hope that helps, and let me know if you have any questions.

manual review:

1. "Code optimization." is not a useful commit message, please use http://drupal.org/node/52287 in the future.
2. Use drupal_unlink() instead of unlink().

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

Trying to remove tag again.

  73 function subfolders_domain_install() {
  74   $t = get_t();
  75   drupal_set_message($t("Subfolders Domain Module has been installed successfully.
  76     Configure the role for each user on the 'Subfolders Domain Roles' tab on the user page on @url.", array('@url' => 'user/USER_ID/subfolders-domain-roles')));
  77 }

Breaking the line within a string, within the t() function is not so cool for translation.

  88 /**
  89  * Implements hook_disable().
  90  */
  91 function subfolders_domain_disable() {

Disabling a module should just "deactivate" it's functionality - not delete any configuration. That's what hook_uninstall() is for.

Try to use more "self describing" variable names, even when it annoys you. $rs is just stupid to read for other developers ;)

'#attributes' => array('style' => array('width:280px'))
Always avoid using CSS directly on elements - rather conditionally include a css file on the settings page.

array(0 => t('No'), 1 => t('Yes')),
If you handle boolean values - use TRUE/FALSE instead of 0/1 - if you need integer - cast them before they are needed - (int) TRUE is still much more readable.
(isset($defaults[$account->uid][$domain_id][$rid])) ? 1 : 0
looks better this way:
(isset($defaults[$account->uid][$domain_id][$rid])) ? TRUE : FALSE
but wait the result of isset() is a boolean anyway:
(isset($defaults[$account->uid][$domain_id][$rid]))

Some parts of your code look very compact. To increase readability make more use of empty lines. Structure your code in blocks and use inline comments to describe what it does (in the more general perspective) and if appropriate why the code is the way it is (eg. on workarounds). (In some places in the .module file your doing this very well)

I couldn't really review your code in detail, it's just too much code and too many parts of it are not very well documented what makes it really hard for me to understand what's going on (without spending hours reviewing).

But in general you code looks fine and API's seem to be used properly, so thumbs up from me.

Thanks for your contribution!

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.