Rename hook_xmlsitemap_links

I'm not in favor of renaming the hook. As I posted in #490518: Incompatibility with XML sitemap 2.x:

The hook implementation in 6.x-2.x are very close to doing things the 'Drupal way' and in my opinion seems the most likely to not require any changes in the future. I have put a *lot* of thought into their extendability and code lifetime. Since the 6.x-2.x branch will soon become the recommended branch, we should probably encourage any Drupal 6 modules that want to integrate with XML sitemap to do so with the 6.x-2.x hook implementations.

I just have to respectfully disagree. I'll try to make my point a little clearer. hook_xmlsitemap_links() implies that the function just gets and returns all the link data. Whatever code that calls the hook can decide what to do with that data. For instance, when used in the batch rebuild process, we write those records to the database. At some point that same data could be used to write directly to a file in the calling code. But it's not the hook's job to do that and it would be very un-Drupal-like to do so. It's not hook_xmlsitemap_write_links_to_file(). If anything I believe the 6.x-1.x hook should be renamed to hook_xmlsitemap_write_links(), because frankly that's what it does. What if down the road we need to do something else with the same data? We have to add a different hook that fetches the *exact* same data, but does something different with it when we could just have one hook that returns the data. When the hook just provides the data, we retain a huge amount of flexibility down the road.

For example:
hook_blocks() - returns a list of blocks. It doesn't save them to the database. That's the responsibility of the block.module code that calls this hook.
hook_action_info() - same thing with a list of actions
hook_filter() - same thing with a list of filters
hook_menu() - same thing with a list of menu items.

There is only currently *one* module for Drupal 6 that implements hook_xmlsitemap_links() which is Gallery, which is broken for both 6.x-1.x and 6.x-2.x. We can choose the right path now for both our end users and developers.

Could someone post a before/after snippet of 1.x vs. 2.x hook_xmlsitemap_links() for comparison? That'd make it easier for people not as familiar with the code to chime in as requested at #448000-115: Big, Over-Arching Sitemap Architecture Discussion (tm).

Without having that in front of me though, I do agree with Dave at #5. Hooks that merely return data are more useful, because they allow for multiple uses of that data. For example, perhaps someone wants to write their own extension module which invokes hook_xmlsitemap_links() but doesn't store it in the database in any way. Someone could create a "status" page in their module which shows all of the possible links and which ones are being written to the sitemap. And so on.

I somewhat understand the concern for backwards compatibility, but not really. Drupal breaks APIs all the time between major versions, and developers are used to that. Here's hook_perm() in 6.x and below:

function hook_perm() {
  return array('administer permissions');
}

And in 7.x:

function hook_perm() {
  return array(
    'administer permissions' =>  array(
      'title' => t('Administer permissions'),
      'description' => t('Manage the permissions assigned to user roles. %warning', array('%warning' => t('Warning: Give to trusted roles only; this permission has security implications.'))),
    );
}

In contrib, a 1.x => 2.x version change implies that the same types of API changes may be present, and therefore it would not be unexpected for hook definitions to change. Just document it in CHANGELOG.txt or similar.

Dave also mentioned that only one module out there right now is actually implementing this hook and it's currently broken, so it seems like making this improvement has a negligible effect on any existing code.

Here's the current implementation from xmlsitemap_node.module:

Current 6.x-1.x (6.x-1.0-beta0) which saves the data to the database:

/**
 * Implementation of hook_xmlsitemap_links().
 */
function xmlsitemap_node_xmlsitemap_links() {
  $query = "SELECT n.nid, n.vid, n.type, n.language, n.uid, n.created, n.promote, xn.changed, xn.previously_changed, xn.priority_override, xn.comment_ratio
    FROM {node} n
    INNER JOIN {xmlsitemap_node} xn ON n.nid = xn.nid
    WHERE n.status > 0";
  $result = db_query(db_rewrite_sql($query));
  $row = new stdClass();
  $row->module = 'xmlsitemap_node';
  $row->type = 'node';
  while ($node = db_fetch_object($result)) {
    $row->loc = 'node/'. $node->nid;
    $row->id = $node->nid;
    $row->sid = $node->vid;
    $row->language = $node->language;
    $row->changed = $node->changed;
    $row->changefreq = max(REQUEST_TIME - $node->changed, empty($node->previously_changed) ? $node->changed - $node->created : $node->changed - $node->previously_changed);
    $priority = xmlsitemap_node_get_priority($node);
    $row->priority = ($priority == -1.0) ? $priority : min(max(round($priority, 1), 0.0), 1.0);
    $old_row = db_fetch_object(db_query("SELECT lid, type, priority FROM {xmlsitemap} WHERE loc = '%s'", $row->loc));
    if ($old_row === FALSE) {
      drupal_write_record('xmlsitemap', $row);
    }
    elseif ($old_row->type == 'node' && $old_row->priority != $row->priority) {
      $row->lid = $old_row->lid;
      drupal_write_record('xmlsitemap', $row, 'lid');
    }
  }
}

6.x-1.0-beta5 which wrote the data to a sitemap XML file:

/**
 * Implementation of hook_xmlsitemap_links().
 */
function xmlsitemap_node_xmlsitemap_links($fp, $from, $count = 0) {
  $link = new stdClass();
  $result = _xmlsitemap_node_build_query('*', $from, $count);
  while ($row = db_fetch_object($result)) {
    $changefreq = max($row->changed - $row->previously_changed, REQUEST_TIME - $row->changed);
    $link->loc = url('node/'. $row->pdata, array('absolute' => TRUE));
    $link->changed = $row->changed;
    $link->changefreq = $changefreq;
    $link->priority = $row->priority_override == -2.0 ? $row->priority : $row->priority_override;
    xmlsitemap_output_link($fp, $link);
  }
}

6.x-2.x which just returns the data:

/**
 * Implementation of hook_xmlsitemap_links().
 */
function xmlsitemap_node_xmlsitemap_links($offset = 0, $limit = 0) {
  $links = array();

  $sql = "SELECT n.nid FROM {node} n WHERE n.nid > %d ORDER BY n.nid";
  $query = ($limit ? db_query_range($sql, $offset, 0, $limit) : db_query($sql, $offset));

  while ($nid = db_result($query)) {
    $node = node_load($nid, NULL, TRUE);
    $links[] = xmlsitemap_node_create_link($node);
  }

  return $links;
}

Yeah, I think as long as there's an API function like xmlsitemap_output_link() which writes a single link row to file/database/whatever, changing the return value of hook_xmlsitemap_links() as Dave describes makes sense. Someone who wanted to could still emulate the old behaviour by module_invoke_all()ing 'xmlsitemap_links' and then looping through the results to call said API function.

I don't see any reason to re-name the hook in either version; in both cases, you're grabbing the list of links defined by a given add-on module. The only difference is that the 2.x version de-couples the listing behaviour from the writing behaviour for a bit more added flexibility.