domain_site grant applied when unchecking all previous checked domains and checking new ones on block domain vis settings

As I'm not sure I prefer that you consider if you should move this to a bug report. I think the bug is there.

Here is the simple logic I mean:

function domain_blocks_save($module, $delta, $domain_arr) {

  db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta);

  if (empty($domain_arr)) {

    $realm = 'domain_site';
    db_query("INSERT INTO {domain_blocks} (module, delta, realm, domain_id) VALUES ('%s', '%s', '%s', %d)", $module, $delta, $realm, 0);

  }
  else {

    $realm = 'domain_id';
    foreach ($domain_arr as $single_domain_id => $enabled) {
      db_query("INSERT INTO {domain_blocks} (module, delta, realm, domain_id) VALUES ('%s', '%s', '%s', %d)", $module, $delta, $realm, $single_domain_id);
    }

  }

}

And the helper function _domain_blocks_with_realm_load would not be needed at all.

Those would be the two changes.

And if you think, but that way when the domain visibility settings are not changed I'm deleting the db values and writing them again. Just do the check on form submit:

You carry previous values on form:

function domain_blocks_form_alter(&$form, $form_state, $form_id) {

...
...
...

    $form['domain_vis_settings']['domain_blocks'] = array(
      '#type' => 'checkboxes',
      '#title' => t('Display on'),
      '#options' => $options,
      '#required' => FALSE,
      '#description' => t('Limit block display to these affiliates. This block is visible on all domains by default using this theme.'),
      '#default_value' => $block_domains,
    );

    $form['domain_vis_settings']['previous_domain_blocks'] = array(
      '#type' => 'value',
      '#value' => $block_domains,
    );

    $form['#submit'][] = 'domain_blocks_form_submit';

...
...
...

}

And check if there were changes on submit:

function domain_blocks_form_submit($form, &$form_state) {

  $values = $form_state['values'];

  if (array_values(array_filter($values['domain_blocks'])) != $values['previous_domain_blocks']) {

    // Delta may be zero for other modules than block.
    if (!$values['delta'] && $values['module'] == 'block') { 

    ...
    ...
    ...

    domain_blocks_save($values['module'], $values['delta'], $enabled_domains);

  }

}

And just to avoid that case and save some queries. As these queries only take place on administration changes that are not so often, I think are not something to worry about. Is not like they were queries on site display to visitors.

Just configure a domain visibility for a block, assigning it to only one domain. The block should only be shown on that domain. OK that works.

Then reconfigure that block domain visibility settings, just uncheck the previously assigned domain, check another different one and submit. The bug should appear, the new assigned domain is going to be added to domain_blocks table but as $domain_count_saved is 0 (the previously assigned domains, in this case only one, have been unchecked) then domain_site is going to be added too. So the domain_blocks table for that block on that moment has two entries, one for the selected domain, and one with domain_site realm. The block, though assigned only to one domain, will be shown on all domains.

If you prefer all that logic, you should change that codition $domain_count_saved == 0 to ($domain_count_saved == 0 && empty($domain_arr)), that means all previously assigned domains have been unchecked and, besides, and important too to decide to asign a domain_site, no new ones have been checked (just means no domains are checked, but in the logic you are coding you have to check it that way).

But even if you think is better doing all that logic I would review it anyway. In my opinion you should proceed checking the new values, not doing conditions based on previously values:

1. First check if there is nothing checked. Condition: empty(domains_arr). In that case remove any possible row in domain_blocks table for the block ($module, $delta). And assing domain_site realm. Worst thing that could happen, it had just domain_site before and you are deleting it and assigning it again. But would it be worst than loading first all previously values?
2. Else (something is checked).
   1. Get previously values (this is being done with another query but those values really have been queried before on form creation, more or less, changing code all can be retrieved from there).
   2. If previously values contain domain_site delete domain_site and add all values from domain_arr.
   3. Else do all that logic to regenerate values (avoiding add values that are just in database, deleting the values unchecked, and adding new checked values):
      1. Delete from domain_arr those values which are in previously values.(test with values are just present on database)
      2. Delete those previously values that are not in domain_arr. (delete queries)
      3. If domain_arr still has values insert them. (insert queries)

And there is no more that those two cases, or there is nothing selected and domain_site is needed or there is some domains selected and you have to update the database entries.

But the logic you are following based on previous values insteed of new values is really strange, though it would work with that change on the condition $domain_count_saved == 0.

This is how I would do it:

// $Id: domain_blocks.module,v 1.1.2.2 2009/01/06 06:23:37 nonsie Exp $

/**
 * @file
 * Provides domain specific visibility settings for blocks based on user domain access settings
 * Domain Blocks does not define any permissions of its own yet it requires user to have
 * set domain access permission in order to define any domain based visibility settings.
 */

/**
 * Implementation of hook_help().
 * Display help and module information
 * @param path which path of the site we're displaying help
 * @param arg array that holds the current path as would be returned from arg() function
 * @return help text for the path
 */
function domain_blocks_help($path, $arg) {
  $output = '';
  switch ($path) {
    case "admin/help#domain_blocks":
      $output = '<p>'.  t("Provides domain specific visibility settings for blocks") .'</p>';
      break;
  }
  return $output;
}

/**
 * Save domain block data
 * @param string $module
 * @param string $delta
 * @param array $domain_arr
 * @param array $previous_domain_arr
 */
function domain_blocks_save($module, $delta, $domain_arr, $previous_domain_arr) {

  // none of the domains are checked
  if (empty($domain_arr)) {

    // previously some domains were checked
    if (!empty($previous_domain_arr)) {
      db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta);
      // Block is visible on all domains (domain_site grant)
      $realm = 'domain_site';
      db_query("INSERT INTO {domain_blocks} (module, delta, realm, domain_id) VALUES ('%s', '%s', '%s', %d)", $module, $delta, $realm, 0);
    }

  }
  else { // some domains are checked

    // previously none was checked
    if (empty($previous_domain_arr)) {
      // remove domain_site grant - block visibility is defined by individual domains
      $realm = 'domain_site';
      db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s' AND domain_id IN ('%s')", $module, $delta, $realm, 0);
      // insert all domains selected
      $realm = 'domain_id';
      foreach ($domain_arr as $single_domain_id) {
        db_query("INSERT INTO {domain_blocks} (module, delta, realm, domain_id) VALUES ('%s', '%s', '%s', %d)", $module, $delta, $realm, $single_domain_id);
      }

    }
    else {
      
      // previously some domains were checked and now some domains are checked
      
      // unset previously checked values now unchecked
      $domains_to_be_removed = array_diff($previous_domain_arr, $domain_arr);
      $domains_to_be_removed = implode(',', array_values($domains_to_be_removed));
      $realm = 'domain_id';
      if ($domains_to_be_removed) {
        db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s' AND domain_id IN ('%s')", $module, $delta, $realm, $domains_to_be_removed);
      }

      // set new values, now checked and previously not checked
      $domains_to_add = array_diff($domain_arr, $previous_domain_arr);
      $realm = 'domain_id';
      foreach ($domains_to_add as $single_domain_id) {
        db_query("INSERT INTO {domain_blocks} (module, delta, realm, domain_id) VALUES ('%s', '%s', '%s', %d)", $module, $delta, $realm, $single_domain_id);
      }
     
    }

  }

}

/**
 * Implementation of block form_alter().
 */
function domain_blocks_form_alter(&$form, $form_state, $form_id) {
  if (($form_id == 'block_admin_configure' || $form_id == 'block_box_form' || $form_id == 'block_add_block_form')) {
    // If the user is a site admin, show the form, otherwise pass it silently.
    if (user_access('set domain access')) {
      $module = $form['module']['#value'];
      $delta = $form['delta']['#value'];
      $form['domain_vis_settings'] = array(
        '#type' => 'fieldset',
        '#title' => t('Domain specific visibility settings'),
        '#collapsible' => TRUE,
        '#collapsed' => FALSE,
        '#weight' => 0,
      );

      //$block_domains = _domain_blocks_load($module, $delta, 'domain_id');
      $block_domains_with_realm = _domain_blocks_with_realm_load($module, $delta);

      $previous_block_domains = isset($block_domains_with_realm['domain_id']) ? $block_domains_with_realm['domain_id'] : array();

      $block_domains = array();
      foreach ($previous_block_domains as $value) {
        $value == 0 ? $value = -1 : $value = $value;
        $block_domains[] = $value;
      }
      
      $options = array();
      foreach (domain_domains() as $data) {
        // Cannot pass zero in checkboxes.
        ($data['domain_id'] == 0) ? $key = -1 : $key = $data['domain_id'];
        // The domain must be valid.
        if ($data['valid'] || user_access('administer domains')) {
          $options[$key] = $data['sitename'];
        }
      }
      $form['domain_vis_settings']['domain_blocks'] = array(
        '#type' => 'checkboxes',
        '#title' => t('Display on'),
        '#options' => $options,
        '#required' => FALSE,
        '#description' => t('Limit block display to these affiliates. This block is visible on all domains by default using this theme.'),
        '#default_value' => $block_domains,
      );
      $form['domain_vis_settings']['previous_domain_blocks'] = array(
        '#type' => 'value',
        '#value' => $previous_block_domains,
      );
      $form['#submit'][] = 'domain_blocks_form_submit';
    }
  }
}

/**
 * Forms api callback. Submit function
 */
function domain_blocks_form_submit($form, &$form_state) {
  $values = $form_state['values'];
  // Delta may be zero for other modules than block.
  if (!$values['delta'] && $values['module'] == 'block') {
    $values['delta'] = db_result(db_query("SELECT MAX(bid) FROM {boxes}"));
  }
  $enabled_domains = array();
  if (!empty($values['domain_blocks'])) {
    foreach ($values['domain_blocks'] as $single_domain_id => $domain_enabled) {
      if ($domain_enabled) {
        if ($single_domain_id == -1) {
          $single_domain_id = 0;
        }
        $enabled_domains[] = $single_domain_id;
      }
    }
  }
  
  domain_blocks_save($values['module'], $values['delta'], $enabled_domains, $values['previous_domain_blocks']);
}

/**
 * Implementation of hook_db_rewrite_sql().
 */
function domain_blocks_db_rewrite_sql($query, $primary_table, $primary_key, $args) {
  global $_domain;
  if ($primary_table == 'b' && $primary_key == 'bid') {
    $result['join'] .= " INNER JOIN {domain_blocks} db ON db.module = b.module ";
    $result['where'] .= " db.delta = b.delta ";
    $result['where'] .= " AND ((db.realm = 'domain_site' AND db.domain_id = 0) OR (db.realm = 'domain_id' AND db.domain_id = ". $_domain['domain_id'] ."))";
    return $result;
  }
}

/**
 * Look up domain blocks based on block module and delta
 *
 * @param string $module
 * @param string $delta
 * @return array
 */
// this is no used anywhere but maybe you wanted it for some reason
function _domain_blocks_domain_lookup($module = NULL, $delta = NULL) {
  $result = array();
  if (!is_null($module) && !is_null($delta)) {
    $result = db_fetch_array(db_query("SELECT domain_id from {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta));
  }
  return $result;
}

// no more used
function _domain_blocks_load($module, $delta, $realm = NULL) {
  $block = array();
  if (!empty($realm)) {
    $result = db_query("SELECT domain_id FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s'", $module, $delta, $realm);
  }
  else {
    $result = db_query("SELECT domain_id FROM {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta);
  }
  while ($row = db_fetch_array($result)) {
    if ($row['domain_id'] == 0) {
      $row['domain_id'] = -1;
    }
    $block[] = $row['domain_id'];
  }
  return $block;
}

function _domain_blocks_with_realm_load($module, $delta) {
  $block = array();
  $result = db_query("SELECT domain_id, realm FROM {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta);
  while ($row = db_fetch_array($result)) {
    $block[$row['realm']][] = $row['domain_id'];
  }
  return $block;
}

/**
 * Implementation of hook_domainupdate().
 */
function domain_blocks_domainupdate($op, $domain, $edit = array()) {
 switch ($op) {
    case 'delete':
      // remove records
      _domain_blocks_delete($domain);
    case 'update':
    case 'insert':
    default:
    break;
 }
}

function _domain_blocks_delete($domain) {
  db_query("DELETE FROM {domain_blocks} WHERE domain_id = %d AND realm = 'domain_id'", $domain['domain_id']);
}

How it is coded even this:

Worst thing that could happen, it had just domain_site before and you are deleting it and assigning it again.

is being avoided and if now none of the domains are selected and previously none were selected, nothing is done, no queries.

If you prefer all that logic, you should change that codition $domain_count_saved == 0 to ($domain_count_saved == 0 && empty($domain_arr)), that means all previously assigned domains have been unchecked and, besides, and important too to decide to asign a domain_site, no new ones have been checked (just means no domains are checked, but in the logic you are coding you have to check it that way).

And that is because you are changing the value of the parameter $domain_arr in your algorithm inside your save function. If you declare another variable to that purpose, $domains_to_be_added, really the only needed to assign a domain_site is that the parameter $domain_arr is empty (none of the domains are checked). So it would be enough to change the condition from:

$domain_count_saved == 0

to:

empty($domain_arr)

Anyway, in my modest opinion, I think the algorithm I describe and the code in previous message is clearer than the one you are using testing first previous selected domains and then new selected ones.

A minor change on the code I posted. On:

      $domains_to_be_removed = implode(',', array_values($domains_to_be_removed));

the array_values() is not needed as implode just use the values of the array itself, the keys are not important at all, there is no need to regenerate them as 0, 1, 2... So that line could be just simplified to:

      $domains_to_be_removed = implode(',', $domains_to_be_removed);

I move this to a bug report because the bug is present there. You can reproduce it as mentioned before:

• Configure one block domain specific visibility settings to only one domain, 'ExampleDomain1'. The block should only be shown on 'ExampleDomain1' domain.
• Reconfigure the block domain specific visibility settings to another domain, unchecking 'ExampleDomain1' and checking 'ExampleDomain2'. The block should only be shown on 'ExampleDomain2' but it is shown on all domains instead.

The reason why is on #5.

And this is the code I suggest with a modification because I was not taking into account the first time a block configuration form is submited for new blocks created by modules installed after domain blocks, when still there are no rows on {domain_blocks} table for that block and the user has not checked any domain on domain specific visibility settings for that block, domain_site grant must be added.

// $Id: domain_blocks.module,v 1.1.2.2 2009/01/06 06:23:37 nonsie Exp $

/**
* @file
* Provides domain specific visibility settings for blocks based on user domain access settings
* Domain Blocks does not define any permissions of its own yet it requires user to have
* set domain access permission in order to define any domain based visibility settings.
*/

/**
* Implementation of hook_help().
* Display help and module information
* @param path which path of the site we're displaying help
* @param arg array that holds the current path as would be returned from arg() function
* @return help text for the path
*/
function domain_blocks_help($path, $arg) {
  $output = '';
  switch ($path) {
    case "admin/help#domain_blocks":
      $output = '<p>'.  t("Provides domain specific visibility settings for blocks") .'</p>';
      break;
  }
  return $output;
}

/**
* Save domain block data
* @param string $module
* @param string $delta
* @param array $domain_arr
* @param array $previous_domain_arr
*/
function domain_blocks_save($module, $delta, $domain_arr, $previous_domain_arr) {

  // none of the domains are checked
  if (empty($domain_arr)) {

    // previously some domains were checked
    if (!empty($previous_domain_arr)) {
      db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta);
      // Block is visible on all domains (domain_site grant)
      $realm = 'domain_site';
      db_query("INSERT INTO {domain_blocks} (module, delta, realm, domain_id) VALUES ('%s', '%s', '%s', %d)", $module, $delta, $realm, 0);
    }
    elseif ($previous_domain_arr == NULL) {
      // Block is visible on all domains (domain_site grant)
      $realm = 'domain_site';
      db_query("INSERT INTO {domain_blocks} (module, delta, realm, domain_id) VALUES ('%s', '%s', '%s', %d)", $module, $delta, $realm, 0);
    }

  }
  else { // some domains are checked

    // previously none was checked
    if (empty($previous_domain_arr)) {
      // remove domain_site grant - block visibility is defined by individual domains
      $realm = 'domain_site';
      db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s' AND domain_id IN ('%s')", $module, $delta, $realm, 0);
      // insert all domains selected
      $realm = 'domain_id';
      foreach ($domain_arr as $single_domain_id) {
        db_query("INSERT INTO {domain_blocks} (module, delta, realm, domain_id) VALUES ('%s', '%s', '%s', %d)", $module, $delta, $realm, $single_domain_id);
      }

    }
    else {
     
      // previously some domains were checked and now some domains are checked
     
      // unset previously checked values now unchecked
      $domains_to_be_removed = array_diff($previous_domain_arr, $domain_arr);
      $domains_to_be_removed = implode(',', $domains_to_be_removed);
      $realm = 'domain_id';
      if ($domains_to_be_removed) {
        db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s' AND domain_id IN ('%s')", $module, $delta, $realm, $domains_to_be_removed);
      }

      // set new values, now checked and previously not checked
      $domains_to_add = array_diff($domain_arr, $previous_domain_arr);
      $realm = 'domain_id';
      foreach ($domains_to_add as $single_domain_id) {
        db_query("INSERT INTO {domain_blocks} (module, delta, realm, domain_id) VALUES ('%s', '%s', '%s', %d)", $module, $delta, $realm, $single_domain_id);
      }
    
    }

  }

}

/**
* Implementation of block form_alter().
*/
function domain_blocks_form_alter(&$form, $form_state, $form_id) {
  if (($form_id == 'block_admin_configure' || $form_id == 'block_box_form' || $form_id == 'block_add_block_form')) {
    // If the user is a site admin, show the form, otherwise pass it silently.
    if (user_access('set domain access')) {
      $module = $form['module']['#value'];
      $delta = $form['delta']['#value'];
      $form['domain_vis_settings'] = array(
        '#type' => 'fieldset',
        '#title' => t('Domain specific visibility settings'),
        '#collapsible' => TRUE,
        '#collapsed' => FALSE,
        '#weight' => 0,
      );

      $block_domains_with_realm = _domain_blocks_with_realm_load($module, $delta);

      // NULL when there is not record for this block on {domain_blocks} table.
      // array() when domain_site is previously configured for this block.
      // an array of domain_id values when it is previously configured to only some domains.
      $previous_block_domains = empty($block_domains_with_realm) ? NULL : isset($block_domains_with_realm['domain_id']) ? $block_domains_with_realm['domain_id'] : array();

      $block_domains = array();
      foreach ($previous_block_domains as $value) {
        $value == 0 ? $value = -1 : $value = $value;
        $block_domains[] = $value;
      }
     
      $options = array();
      foreach (domain_domains() as $data) {
        // Cannot pass zero in checkboxes.
        ($data['domain_id'] == 0) ? $key = -1 : $key = $data['domain_id'];
        // The domain must be valid.
        if ($data['valid'] || user_access('administer domains')) {
          $options[$key] = $data['sitename'];
        }
      }
      $form['domain_vis_settings']['domain_blocks'] = array(
        '#type' => 'checkboxes',
        '#title' => t('Display on'),
        '#options' => $options,
        '#required' => FALSE,
        '#description' => t('Limit block display to these affiliates. This block is visible on all domains by default using this theme.'),
        '#default_value' => $block_domains,
      );
      $form['domain_vis_settings']['previous_domain_blocks'] = array(
        '#type' => 'value',
        '#value' => $previous_block_domains,
      );
      $form['#submit'][] = 'domain_blocks_form_submit';
    }
  }
}

/**
* Forms api callback. Submit function
*/
function domain_blocks_form_submit($form, &$form_state) {
  $values = $form_state['values'];
  // Delta may be zero for other modules than block.
  if (!$values['delta'] && $values['module'] == 'block') {
    $values['delta'] = db_result(db_query("SELECT MAX(bid) FROM {boxes}"));
  }
  $enabled_domains = array();
  if (!empty($values['domain_blocks'])) {
    foreach ($values['domain_blocks'] as $single_domain_id => $domain_enabled) {
      if ($domain_enabled) {
        if ($single_domain_id == -1) {
          $single_domain_id = 0;
        }
        $enabled_domains[] = $single_domain_id;
      }
    }
  }
 
  domain_blocks_save($values['module'], $values['delta'], $enabled_domains, $values['previous_domain_blocks']);
}

/**
* Implementation of hook_db_rewrite_sql().
*/
function domain_blocks_db_rewrite_sql($query, $primary_table, $primary_key, $args) {
  global $_domain;
  if ($primary_table == 'b' && $primary_key == 'bid') {
    $result['join'] .= " INNER JOIN {domain_blocks} db ON db.module = b.module ";
    $result['where'] .= " db.delta = b.delta ";
    $result['where'] .= " AND ((db.realm = 'domain_site' AND db.domain_id = 0) OR (db.realm = 'domain_id' AND db.domain_id = ". $_domain['domain_id'] ."))";
    return $result;
  }
}

/**
* Look up domain blocks based on block module and delta
*
* @param string $module
* @param string $delta
* @return array
*/
// this is no used anywhere but maybe you wanted it for some reason
function _domain_blocks_domain_lookup($module = NULL, $delta = NULL) {
  $result = array();
  if (!is_null($module) && !is_null($delta)) {
    $result = db_fetch_array(db_query("SELECT domain_id from {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta));
  }
  return $result;
}

// no more used
function _domain_blocks_load($module, $delta, $realm = NULL) {
  $block = array();
  if (!empty($realm)) {
    $result = db_query("SELECT domain_id FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s'", $module, $delta, $realm);
  }
  else {
    $result = db_query("SELECT domain_id FROM {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta);
  }
  while ($row = db_fetch_array($result)) {
    if ($row['domain_id'] == 0) {
      $row['domain_id'] = -1;
    }
    $block[] = $row['domain_id'];
  }
  return $block;
}

function _domain_blocks_with_realm_load($module, $delta) {
  $block = array();
  $result = db_query("SELECT domain_id, realm FROM {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta);
  while ($row = db_fetch_array($result)) {
    $block[$row['realm']][] = $row['domain_id'];
  }
  return $block;
}

/**
* Implementation of hook_domainupdate().
*/
function domain_blocks_domainupdate($op, $domain, $edit = array()) {
switch ($op) {
    case 'delete':
      // remove records
      _domain_blocks_delete($domain);
    case 'update':
    case 'insert':
    default:
    break;
}
}

function _domain_blocks_delete($domain) {
  db_query("DELETE FROM {domain_blocks} WHERE domain_id = %d AND realm = 'domain_id'", $domain['domain_id']);
}

Sorry but still was an error on the code I posted. So here it is finally doing what I explain in the algorith and taking into account when still there is no record in {domain_blocks} database table about that block:

// $Id: domain_blocks.module,v 1.1.2.2 2009/01/06 06:23:37 nonsie Exp $

/**
* @file
* Provides domain specific visibility settings for blocks based on user domain access settings
* Domain Blocks does not define any permissions of its own yet it requires user to have
* set domain access permission in order to define any domain based visibility settings.
*/

/**
* Implementation of hook_help().
* Display help and module information
* @param path which path of the site we're displaying help
* @param arg array that holds the current path as would be returned from arg() function
* @return help text for the path
*/
function domain_blocks_help($path, $arg) {
  $output = '';
  switch ($path) {
    case "admin/help#domain_blocks":
      $output = '<p>'.  t("Provides domain specific visibility settings for blocks") .'</p>';
      break;
  }
  return $output;
}

/**
* Save domain block data
* @param string $module
* @param string $delta
* @param array $domain_arr
* @param array $previous_domain_arr
*/
function domain_blocks_save($module, $delta, $domain_arr, $previous_domain_arr) {

  // none of the domains are checked
  if (empty($domain_arr)) {

    // previously some domains were checked
    if (!empty($previous_domain_arr)) {
      db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta);
      // Block is visible on all domains (domain_site grant)
      $realm = 'domain_site';
      db_query("INSERT INTO {domain_blocks} (module, delta, realm, domain_id) VALUES ('%s', '%s', '%s', %d)", $module, $delta, $realm, 0);
    }
    elseif (!isset($previous_domain_arr)) {
      // Block is visible on all domains (domain_site grant)
      $realm = 'domain_site';
      db_query("INSERT INTO {domain_blocks} (module, delta, realm, domain_id) VALUES ('%s', '%s', '%s', %d)", $module, $delta, $realm, 0);
    }

  }
  else { // some domains are checked

    // previously none was checked
    if (empty($previous_domain_arr)) {
      // remove domain_site grant - block visibility is defined by individual domains
      $realm = 'domain_site';
      db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s' AND domain_id IN ('%s')", $module, $delta, $realm, 0);
      // insert all domains selected
      $realm = 'domain_id';
      foreach ($domain_arr as $single_domain_id) {
        db_query("INSERT INTO {domain_blocks} (module, delta, realm, domain_id) VALUES ('%s', '%s', '%s', %d)", $module, $delta, $realm, $single_domain_id);
      }

    }
    else {
    
      // previously some domains were checked and now some domains are checked
    
      // unset previously checked values now unchecked
      $domains_to_be_removed = array_diff($previous_domain_arr, $domain_arr);
      $domains_to_be_removed = implode(',', $domains_to_be_removed);
      $realm = 'domain_id';
      if ($domains_to_be_removed) {
        db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s' AND domain_id IN ('%s')", $module, $delta, $realm, $domains_to_be_removed);
      }

      // set new values, now checked and previously not checked
      $domains_to_add = array_diff($domain_arr, $previous_domain_arr);
      $realm = 'domain_id';
      foreach ($domains_to_add as $single_domain_id) {
        db_query("INSERT INTO {domain_blocks} (module, delta, realm, domain_id) VALUES ('%s', '%s', '%s', %d)", $module, $delta, $realm, $single_domain_id);
      }
   
    }

  }

}

/**
* Implementation of block form_alter().
*/
function domain_blocks_form_alter(&$form, $form_state, $form_id) {
  if (($form_id == 'block_admin_configure' || $form_id == 'block_box_form' || $form_id == 'block_add_block_form')) {
    // If the user is a site admin, show the form, otherwise pass it silently.
    if (user_access('set domain access')) {
      $module = $form['module']['#value'];
      $delta = $form['delta']['#value'];
      $form['domain_vis_settings'] = array(
        '#type' => 'fieldset',
        '#title' => t('Domain specific visibility settings'),
        '#collapsible' => TRUE,
        '#collapsed' => FALSE,
        '#weight' => 0,
      );

      $block_domains_with_realm = _domain_blocks_with_realm_load($module, $delta);

      // NULL when there is not record for this block on {domain_blocks} table.
      // array() when domain_site is previously configured for this block.
      // an array of domain_id values when it is previously configured to only some domains.
      $previous_block_domains = empty($block_domains_with_realm) ? NULL : (isset($block_domains_with_realm['domain_id']) ? $block_domains_with_realm['domain_id'] : array());

      $block_domains = array();
      if (isset($previous_block_domains)) {
        foreach ($previous_block_domains as $value) {
          $value == 0 ? $value = -1 : $value = $value;
          $block_domains[] = $value;
        }
      }
    
      $options = array();
      foreach (domain_domains() as $data) {
        // Cannot pass zero in checkboxes.
        ($data['domain_id'] == 0) ? $key = -1 : $key = $data['domain_id'];
        // The domain must be valid.
        if ($data['valid'] || user_access('administer domains')) {
          $options[$key] = $data['sitename'];
        }
      }
      $form['domain_vis_settings']['domain_blocks'] = array(
        '#type' => 'checkboxes',
        '#title' => t('Display on'),
        '#options' => $options,
        '#required' => FALSE,
        '#description' => t('Limit block display to these affiliates. This block is visible on all domains by default using this theme.'),
        '#default_value' => $block_domains,
      );
      $form['domain_vis_settings']['previous_domain_blocks'] = array(
        '#type' => 'value',
        '#value' => $previous_block_domains,
      );
      $form['#submit'][] = 'domain_blocks_form_submit';
    }
  }
}

/**
* Forms api callback. Submit function
*/
function domain_blocks_form_submit($form, &$form_state) {
  $values = $form_state['values'];
  // Delta may be zero for other modules than block.
  if (!$values['delta'] && $values['module'] == 'block') {
    $values['delta'] = db_result(db_query("SELECT MAX(bid) FROM {boxes}"));
  }
  $enabled_domains = array();
  if (!empty($values['domain_blocks'])) {
    foreach ($values['domain_blocks'] as $single_domain_id => $domain_enabled) {
      if ($domain_enabled) {
        if ($single_domain_id == -1) {
          $single_domain_id = 0;
        }
        $enabled_domains[] = $single_domain_id;
      }
    }
  }

  domain_blocks_save($values['module'], $values['delta'], $enabled_domains, $values['previous_domain_blocks']);
}

/**
* Implementation of hook_db_rewrite_sql().
*/
function domain_blocks_db_rewrite_sql($query, $primary_table, $primary_key, $args) {
  global $_domain;
  if ($primary_table == 'b' && $primary_key == 'bid') {
    $result['join'] .= " INNER JOIN {domain_blocks} db ON db.module = b.module ";
    $result['where'] .= " db.delta = b.delta ";
    $result['where'] .= " AND ((db.realm = 'domain_site' AND db.domain_id = 0) OR (db.realm = 'domain_id' AND db.domain_id = ". $_domain['domain_id'] ."))";
    return $result;
  }
}

/**
* Look up domain blocks based on block module and delta
*
* @param string $module
* @param string $delta
* @return array
*/
// this is no used anywhere but maybe you wanted it for some reason
function _domain_blocks_domain_lookup($module = NULL, $delta = NULL) {
  $result = array();
  if (!is_null($module) && !is_null($delta)) {
    $result = db_fetch_array(db_query("SELECT domain_id from {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta));
  }
  return $result;
}

// no more used
function _domain_blocks_load($module, $delta, $realm = NULL) {
  $block = array();
  if (!empty($realm)) {
    $result = db_query("SELECT domain_id FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s'", $module, $delta, $realm);
  }
  else {
    $result = db_query("SELECT domain_id FROM {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta);
  }
  while ($row = db_fetch_array($result)) {
    if ($row['domain_id'] == 0) {
      $row['domain_id'] = -1;
    }
    $block[] = $row['domain_id'];
  }
  return $block;
}

function _domain_blocks_with_realm_load($module, $delta) {
  $block = array();
  $result = db_query("SELECT domain_id, realm FROM {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta);
  while ($row = db_fetch_array($result)) {
    $block[$row['realm']][] = $row['domain_id'];
  }
  return $block;
}

/**
* Implementation of hook_domainupdate().
*/
function domain_blocks_domainupdate($op, $domain, $edit = array()) {
switch ($op) {
    case 'delete':
      // remove records
      _domain_blocks_delete($domain);
    case 'update':
    case 'insert':
    default:
    break;
}
}

function _domain_blocks_delete($domain) {
  db_query("DELETE FROM {domain_blocks} WHERE domain_id = %d AND realm = 'domain_id'", $domain['domain_id']);
}

A bracket was missing in the if operator so NULL value was not being assigned to $previous_block_domains as intended when there where no records for that block in database. A check is needed to prevent the foreach (NULL as..) in the stament that generate $block_domains variable when $previous_block_domains is NULL. And I changed the check $previous_block_domains == NULL (that is true too if $previous_block_domains = array()) to !isset($previous_block_domains) that will be true only if $previous_block_domains = NULL.

With those changes now it is correct code for the algorithm I described before and I think it is doing just the same you pretended with all your save logic to avoid as many queries as possible.

Sorry for so many post. This is the last one before you check it again and give some opinion.

Fresh drupal installation, fresh domain access installation, fresh domain blocks installation:

module   delta  realm            domain_id
system   0       domain_site    0
user       0       domain_site    0
user       1       domain_site    0

------------------------------------------------------------------

After configuring block "Powered by Drupal" to only first domain:

module   delta   realm             domain_id
system   0         domain_id      0
user 	     0         domain_site    0
user       1         domain_site    0

Now block Powered by Drupal is only shown on that first domain.

------------------------------------------------------------------

After reconfiguring block "Powered by Drupal", unchecking first domain and checking a second one:

module   delta   realm             domain_id
system   0         domain_id      2
system   0         domain_site    0
user 	     0         domain_site    0
user       1         domain_site    0

Now the block is shown on all domains though it should only be shown on second domain because of the present of that domain_site row on {domain_blocks} table. In this moment you must check going to the different domains to see that the block is there on all domains.

I'm sorry if my english is poor to explain this but I think it is easy to debug manually your save algorithm to see what is happening. I repeat again step by step:

1. Domain_site is deleted
2. The first domian is in database. $domain_count_saved = 1
3. That one is now not checked is has to be removed $num_to_be_removed = 1
4. Now $domain_count_saved = $domain_count_saved - $num_to_be_removed = 1 - 1 = 0
5. It adds the second domain to the table
6. It adds domain_site cause $domains_count_saved is 0

------------------------------------------------------------------

If you enter the configuration form of block "Power by Drupal" it shows correctly only second domain checked but that is because when it reads the records it reads the record with domain_id. If you at that point save again the block configuration, the domain_site record disappears, because in your save logic in this second save with same only second domain checked, what is going to happened is:

1. Domain_site is deleted
2. Now the second domian is in database. $domain_count_saved = 1
3. Now no need to remove any domain $num_to_be_removed = 0
4. Now $domain_count_saved = $domain_count_saved - $num_to_be_removed = 1 - 0 = 1
5. Adds no more domain_id rows cause it is not needed
6. domain_site is not added cause $domains_count_saved isn't 0

module   delta   realm             domain_id
system   0         domain_id      2
user 	     0         domain_site    0
user       1         domain_site    0

At this point with a second save, the block will be shown only on second domain.

------------------------------------------------------------------

The problem is there, so if you don't see it, or you are not looking at the database (in block configuration form the problem is not going to appear if you think it will) or you are not visiting the domains after the reconfiguration as I explained to reproduce the problem and you are visiting the configuration form again and doing the second save that removes domain_site from the database table.

But though the second save eliminates the wrong row that's not mean the problem is not there and I don't think users are going to do a double save of a form. They save once and if they don't visit domains where the block shouldn't appear they even won't notice the bug.

What's more, the bug only appears, let see if I have not explained it correctly, when you uncheck all, all, all, all, previously configured domains, and check some new one(s) (domains not previously checked).

If still you can't reproduce the problem, I can't understand it, I must be very very bad explaining myself. But you only need to follow your logic. A manual debug reveals the problem.

Or if you want I include debug code on 6.x-1.1 version to reveal it.

------------------------------------------------------------------

First of all thanks for looking into it.

I tested and I'm sorry to have to tell I bug was introduced on the changes to patch in #15.

First I'm going to show the warning that appears with 6.x-1.x-dev with patch in #16:

user warning: Duplicate entry 'poll-0-domain_site-0' for key 'PRIMARY' query: INSERT INTO domain_blocks (module, delta, realm, domain_id) VALUES('poll', '0', 'domain_site', 0) in .../drupal6/sites/all/modules/domain_blocks/domain_blocks.module on line 53.

and when it appears:
When you have a module with its domain visibility settings as domain_site (no domains checked) and you enter the configuration form for that block and you save without making changes (that means, the visibility settings intact, still no domains checked).

and why:
The code in patch #15 related to $previous_block_domains is as follow:

      $block_domains_with_realm = _domain_blocks_with_realm_load($module, $delta);

      // NULL when there is not record for this block on {domain_blocks} table.
      // array() when domain_site is previously configured for this block.
      // an array of domain_id values when it is previously configured to only some domains.
      $previous_block_domains = empty($block_domains_with_realm) ? NULL : (isset($block_domains_with_realm['domain_id']) ? $block_domains_with_realm['domain_id'] : array());

You can see on that code that it is using domains with realm. And that function in #15 is as follows:

function _domain_blocks_with_realm_load($module, $delta) {
  $block = array();
  $result = db_query("SELECT domain_id, realm FROM {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta);
  while ($row = db_fetch_array($result)) {
    $block[$row['realm']][] = $row['domain_id'];
  }
  return $block;
}

This function is going to return an empty array if the block is not present at all in the database, otherwise it is going to return $block['domain_site'][0]=0 if domain_site grant is present for that block or $block['domain_id'] = array(0=>0, 1=>1) as an example, or in words, an array of domains ids. And this is what is used to compute $previous_block_domains which would be NULL if this function returns an empty array, it will be array() if this function returns the domain_site array or it would contain the array of domains if this function returns that array.

But you changed it on #16 to:

      $block_lookup = _domain_blocks_load($module, $delta);
      $previous_block_domains = empty($block_lookup) ? NULL : (isset($block_lookup['domain_id']) ? $block_lookup['domain_id'] : array());

....

function _domain_blocks_load($module, $delta) {
  $result = db_query("SELECT domain_id, realm FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = 'domain_id'", $module, $delta);
  while ($row = db_fetch_array($result)) {
    $block[$row['realm']][] = $row['domain_id'];
  }
  return $block;
}

And now that function is not returning the same, it returns NULL if the block is not present in the database, it returns the array of domains if some domains are configured for that block but it does not return the domain_site if that is present, if domain site is present it returns NULL too, so $previous_block_domains is going to be NULL too when domain_site is present on the database and the save function is going to try to insert that domain_site again in the database which is a violation of the primary key for domain_blocks table.

That was probably the most confusing part of the code in #15. Maybe to prevent that confusion is better to do all inside the _domain_blocks_load function, making that function responsible of directly returning the $previous_block_domains (as you almost did with that function documentation but not with its code). I mean something like this:

    // $block_lookup no needed any more
    $previous_block_domains = _domain_blocks_load($module, $delta);

/**
 * Returns block visibility.
 * NULL if not available, array() if using domain_site grant, array of domain ids
 * if using domain_id grant(s).
 * @param string $module
 * @param int $delta
 */
function _domain_blocks_load($module, $delta) {
  $result = db_query("SELECT domain_id, realm FROM {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta);
  while ($row = db_fetch_array($result)) {
    if ($row['realm'] == 'domain_site') {
      return array();
    }
    $block[] = $row['domain_id'];
  }
  return $block;
}

Besides, and I'm very sorry about that, there is a bug in my save code, exactly in this piece of the save code:

      $domains_to_be_removed = array_diff($previous_domain_arr, $domain_arr);
      $domains_to_be_removed = implode(',', $domains_to_be_removed);
      $realm = 'domain_id';
      if ($domains_to_be_removed) {
        db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s' AND domain_id IN ('%s')", $module, $delta, $realm, $domains_to_be_removed);
      }

The if is going to be FALSE when the only domain to be removed is the default domain that has an id of 0, because the variable $domains_to_be_removed is going to be the string "0" that is an equivalent of FALSE. So to solve that, that piece of code should be:

      $domains_to_be_removed = array_diff($previous_domain_arr, $domain_arr);
      if ($domains_to_be_removed) {
        $domains_to_be_removed = implode(',', $domains_to_be_removed);
        $realm = 'domain_id';
        db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s' AND domain_id IN ('%s')", $module, $delta, $realm, $domains_to_be_removed);
      }

That way even in the case the only domain to be removed were the default domain with id 0, the check is being done before imploding the array so $domains_to_be_removed is going to be array(0=>"0") which is not an equivalent of FALSE.

I would do a patch tomorrow starting with version 6.x-1.x-dev with patch #16 applied as an start to make as less changes as possible to your code.

I realize the bug I mention is present in the save code in #15 was just solved in #16.

So the only problem in #16 is the warning when saving a block with domain_site, without making changes.

The function to solve that in #17 was not very good one, because is checking for each row in the result if it has realm 'domain_site'. So it would be better:

    // $block_lookup no needed any more
    $previous_block_domains = _domain_blocks_load($module, $delta);

...
...
...

/**
 * Returns block visibility.
 * NULL if not available, array() if using domain_site grant, keyed array if
 * using domain_id grant(s).
 * @param string $module
 * @param int $delta
 */
function _domain_blocks_load($module, $delta) {
  $result = db_query("SELECT domain_id, realm FROM {domain_blocks} WHERE module = '%s' AND delta = '%s'", $module, $delta);
  while ($row = db_fetch_array($result)) {
    $block[$row['realm']][] = $row['domain_id'];
  }
  if (isset($block['domain_site'])) {
    return array();
  }
  return $block['domain_id'];
}

I attached the patch.

There is an error on the delete query that uses IN, sorry. The query that deletes unchecked domains. As it is now if the admin unchecks more than one domain only the first one is going to be deleted.

          db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s' AND domain_id IN ('%s')", $module, $delta, $realm, $domains_to_be_removed);

needs to be changed to:

          db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s' AND domain_id IN (%s)", $module, $delta, $realm, $domains_to_be_removed);

To use IN at least as possible the query that deletes domain_site can be changed too, cause it is not needed there to check the domain_id column. So it can be changed from:

      db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s' AND domain_id IN ('%s')", $module, $delta, $realm, 0);

to:

      db_query("DELETE FROM {domain_blocks} WHERE module = '%s' AND delta = '%s' AND realm = '%s'", $module, $delta, $realm);

I attach the patch.