Merge lp:~nnewton-drupal/pressflow/pathalias into lp:pressflow

[Narayan Newton]

[David Strauss (davidstrauss)]

The whitelist should be handled as a cache item, not a variable.

review: Needs Fixing

[Narayan Newton (nnewton-drupal)]

> The whitelist should be handled as a cache item, not a variable.

Fixed, this is now stored in cache_path. I've also made it use cache_lifetime for path cache items instead of the magic number.

-N

[lp:~nnewton-drupal/pressflow/pathalias]

75. By Narayan Newton on 2010-01-13

Style and whitespace changes

[Narayan Newton (nnewton-drupal)]

Fixed up some issues with comments and changed 0 to FALSE in a few places for style.

Outstanding requests:

1) Check the front page alias cache on cache miss. I'm not going to implement that for this iteration. I believe that significant testing needs to go into that idea to ensure it actually has benefit.

2) Porting the drupal_static function to core. I'm interesting in this, but I think we should do it separately from this issue and then "port" this functionality.

[David Strauss (davidstrauss)]

I think the remaining work is acceptable to shelve right now. I'll try to merge this today.

------Original Message------
From: Narayan Newton
Sender: <email address hidden>
To: Narayan Newton
ReplyTo: <email address hidden>
Subject: Re: [Merge] lp:~nnewton-drupal/pressflow/pathalias into lp:pressflow
Sent: Jan 13, 2010 23:31

Fixed up some issues with comments and changed 0 to FALSE in a few places for style.

Outstanding requests:

1) Check the front page alias cache on cache miss. I'm not going to implement that for this iteration. I believe that significant testing needs to go into that idea to ensure it actually has benefit.

2) Porting the drupal_static function to core. I'm interesting in this, but I think we should do it separately from this issue and then "port" this functionality.
--
https://code.launchpad.net/~nnewton-drupal/pressflow/pathalias/+merge/16705
You are requested to review the proposed merge of lp:~nnewton-drupal/pressflow/pathalias into lp:pressflow.

[David Strauss (davidstrauss)]

This looks great, especially because the replacement is optional. Merging now.

review: Approve

[David Strauss (davidstrauss)]

This looks great, especially because the replacement is optional. Merging now.

review: Approve

=== modified file 'includes/path.inc'
--- includes/path.inc	2009-12-17 00:30:08 +0000
+++ includes/path.inc	2010-01-13 23:26:10 +0000
@@ -25,6 +25,51 @@
 /**
  * Given an alias, return its Drupal system URL if one exists. Given a Drupal
  * system URL return one of its aliases if such a one exists. Otherwise,
+ * return FALSE. This function is a hook and modules can impliment lookup_path 
+ * to override the core behavior.
+ *
+ * @param $action
+ *   One of the following values:
+ *   - wipe: delete the alias cache.
+ *   - alias: return an alias for a given Drupal system path (if one exists).
+ *   - source: return the Drupal system URL for a path alias (if one exists).
+ * @param $path
+ *   The path to investigate for corresponding aliases or system URLs.
+ * @param $path_language
+ *   Optional language code to search the path with. Defaults to the page language.
+ *   If there's no path defined for that language it will search paths without
+ *   language.
+ *
+ * @return
+ *   Either a Drupal system path, an aliased path, or FALSE if no path was
+ *   found.
+ *  
+ */
+function drupal_lookup_path($action, $path = '', $path_language = '') {
+  static $hook_module;
+  
+  if (!isset($hook_module)) {
+    $modules = module_implements('lookup_path');
+    if (count($modules) > 0) {
+      $hook_module = $modules[0];
+    } else {
+      $hook_module = FALSE;
+    }
+  }
+
+  if ($hook_module !== FALSE) {
+    $retval = module_invoke($hook_module, 'lookup_path', $action, $path, $path_language);
+  } else {
+    $retval = _drupal_lookup_path_direct($action, $path, $path_language);
+  }
+  return $retval;
+}
+
+/**
+ * Internal function: The base path alias functionality as provided by Core.
+ *
+ * Given an alias, return its Drupal system URL if one exists. Given a Drupal
+ * system URL return one of its aliases if such a one exists. Otherwise,
  * return FALSE.
  *
  * @param $action
@@ -43,7 +88,7 @@
  *   Either a Drupal system path, an aliased path, or FALSE if no path was
  *   found.
  */
-function drupal_lookup_path($action, $path = '', $path_language = '') {
+function _drupal_lookup_path_direct($action, $path = '', $path_language = '') {
   global $language;
   // $map is an array with language keys, holding arrays of Drupal paths to alias relations
   static $map = array(), $no_src = array(), $has_paths;
 
=== added directory 'modules/path_alias_cache'
=== added file 'modules/path_alias_cache/path_alias_cache.info'
--- modules/path_alias_cache/path_alias_cache.info	1970-01-01 00:00:00 +0000
+++ modules/path_alias_cache/path_alias_cache.info	2010-01-13 23:26:10 +0000
@@ -0,0 +1,3 @@
+name = Path Alias Cache
+description = A path alias implementation which adds a cache to the core version.
+core = 6.x
 
=== added file 'modules/path_alias_cache/path_alias_cache.install'
--- modules/path_alias_cache/path_alias_cache.install	1970-01-01 00:00:00 +0000
+++ modules/path_alias_cache/path_alias_cache.install	2010-01-13 23:26:10 +0000
@@ -0,0 +1,26 @@
+<?php
+
+/**
+ * Implementation of hook_install().
+ */
+function path_alias_cache_install() {
+  // Create tables.
+  drupal_install_schema('path_alias_cache');
+}
+
+/**
+ * Implementation of hook_uninstall().
+ */
+function path_alias_cache_uninstall() {
+  // Remove tables.
+  drupal_uninstall_schema('path_alias_cache');
+}
+
+/**
+ * Implementation of hook_schema().
+ */
+function path_alias_cache_schema() {
+  $schema['cache_path'] = drupal_get_schema_unprocessed('system', 'cache');
+  return $schema;
+}
+
 
=== added file 'modules/path_alias_cache/path_alias_cache.module'
--- modules/path_alias_cache/path_alias_cache.module	1970-01-01 00:00:00 +0000
+++ modules/path_alias_cache/path_alias_cache.module	2010-01-13 23:26:10 +0000
@@ -0,0 +1,199 @@
+<?php
+
+/*
+ * Implementation of hook_lookup_path
+ *
+ * This version of lookup_path is almost exactly what is in core, but adds a cache table.
+ *
+ */
+function path_alias_cache_lookup_path($action, $path = '', $path_language = '') {
+  global $language;
+  $cache = &path_alias_cache_static(__FUNCTION__, array(
+    'map' => array(),
+    'no_src' => array(),
+    'whitelist' => NULL,
+    'system_paths' => array(),
+    'no_aliases' => array(),
+    'first_call' => TRUE,
+  ));
+
+  // Retrieve the path alias whitelist.
+  if (!isset($cache['whitelist'])) {
+    if ($cached = cache_get('path_alias_whitelist', 'cache_path')) {
+      $cache['whitelist'] = $cached->data;
+    } else {
+      $cache['whitelist'] = path_alias_cache_path_alias_whitelist_rebuild();
+    }
+  }
+
+  $path_language = $path_language ? $path_language : $language->language;
+
+  if ($action == 'wipe') {
+    $cache = array();
+    $cache['whitelist'] = path_alias_cache_path_alias_whitelist_rebuild();
+  }
+  elseif ($cache['whitelist'] && $path != '') {
+    if ($action == 'alias') {
+      // During the first call to path_alias_cache_lookup_path() per language, load the
+      // expected system paths for the page from cache.
+      if (!empty($cache['first_call'])) {
+        $cache['first_call'] = FALSE;
+
+        if (!isset($cache['map'][$path_language]) || !is_array($cache['map'][$path_language])) {
+          $cache['map'][$path_language] = array();
+        }
+        // Load system paths from cache.
+        $cid = $_GET['q'];
+        if ($cached = cache_get($cid, 'cache_path')) {
+          $cache['system_paths'] = $cached->data;
+          // Now fetch the aliases corresponding to these system paths.
+          // We order by ASC and overwrite array keys to ensure the correct
+          // alias is used when there are multiple aliases per path.
+          $placeholders = db_placeholders($cache['system_paths'], 'varchar');
+          $result = db_query("SELECT src, dst FROM {url_alias} WHERE src IN($placeholders) AND language IN('%s', '') ORDER BY language ASC", $cache['system_paths'], $path_language);
+          while ($record = db_fetch_object($result)) {
+            if (!isset($cache['map'][$path_language][$record->src])) {
+              $cache['map'][$path_language][$record->src] = $record->dst;
+            }
+          }
+          // Keep a record of paths with no alias to avoid querying twice.
+          $cache['no_aliases'][$path_language] = array_flip(array_diff_key($cache['system_paths'], array_keys($cache['map'][$path_language])));
+        }
+      }
+      // If the alias has already been loaded, return it.
+      if (isset($cache['map'][$path_language][$path])) {
+        return $cache['map'][$path_language][$path];
+      }
+      // Check the path whitelist, if the top_level part before the first "/"
+      // is not in the list, then there is no need to do anything further,
+      // it is not in the database.
+      elseif (!isset($cache['whitelist'][strtok($path, '/')])) {
+        return FALSE;
+      }
+      // For system paths which were not cached, query aliases individually.
+      else if (!isset($cache['no_aliases'][$path_language][$path])) {
+        // Get the most fitting result falling back with alias without language
+        $alias = db_result(db_query("SELECT dst FROM {url_alias} WHERE src = '%s' AND language IN('%s', '') ORDER BY language DESC", $path, $path_language));
+        $cache['map'][$path_language][$path] = $alias;
+        return $alias;
+      }
+    }
+    // Check no_src for this $path in case we've already determined that there
+    // isn't a path that has this alias
+    elseif ($action == 'source' && !isset($cache['no_src'][$path_language][$path])) {
+      // Look for the value $path within the cached map
+      $src = '';
+      if (!isset($cache['map'][$path_language]) || !($src = array_search($path, $cache['map'][$path_language]))) {
+        // Get the most fitting result falling back with alias without language
+        if ($src = db_result(db_query("SELECT src FROM {url_alias} WHERE dst = '%s' AND language IN('%s', '') ORDER BY language DESC", $path, $path_language))) {
+          $cache['map'][$path_language][$src] = $path;
+        }
+        else {
+          // We can't record anything into map because we do not have a valid
+          // index and there is no need because we have not learned anything
+          // about any Drupal path. Thus cache to no_src.
+          $cache['no_src'][$path_language][$path] = TRUE;
+        }
+      }
+      return $src;
+    }
+  }
+
+  return FALSE;
+}
+
+/**
+ * Implementation of hook_exit
+ * We use this to cache the paths on a page, for later requests.
+ */
+function path_alias_cache_exit() {
+  path_alias_cache_cache_system_paths();
+}
+
+/**
+ * Cache system paths for a page.
+ *
+ * Cache an array of the system paths available on each page. We assume
+ * that aliases will be needed for the majority of these paths during
+ * subsequent requests, and load them in a single query during
+ * drupal_lookup_path().
+ */
+function path_alias_cache_cache_system_paths() {
+  // Check if the system paths for this page were loaded from cache in this
+  // request to avoid writing to cache on every request.
+  $cache = &path_alias_cache_static('path_alias_cache_lookup_path', array());
+  if (!$cache['system_paths']) {
+    // Generate a cache ID (cid) specifically for this page.
+    $cid = $_GET['q'];
+    // The static $map array used by drupal_lookup_path() includes all
+    // system paths for the page request.
+    if ($paths = current($cache['map'])) {
+      $data = array_keys($paths);
+      $expire = $_SERVER['REQUEST_TIME'] + variable_get('cache_lifetime', 0);
+      cache_set($cid, $data, 'cache_path', $expire);
+    }
+  }
+}
+
+
+/**
+ * Central static variable storage.
+ *
+ * @param $name
+ *   Globally unique name for the variable. For a function with only one static,
+ *   variable, the function name (e.g. via the PHP magic __FUNCTION__ constant)
+ *   is recommended. For a function with multiple static variables add a
+ *   distinguishing suffix to the function name for each one.
+ * @param $default_value
+ *   Optional default value.
+ * @param $reset
+ *   TRUE to reset a specific named variable, or all variables if $name is NULL.
+ *   Resetting every variable should only be used, for example, for running
+ *   unit tests with a clean environment. Should be used only though via
+ *   function drupal_static_reset().
+ *
+ * @return
+ *   Returns a variable by reference if $reset is FALSE.
+ */
+function &path_alias_cache_static($name, $default_value = NULL, $reset = FALSE) {
+  static $data = array();
+
+  // Reset a single value, or all values.
+  if ($reset) {
+    if (isset($name)) {
+      unset($data[$name]);
+    }
+    else {
+      $data = array();
+    }
+    // We must return a reference to a variable.
+    $dummy = NULL;
+    return $dummy;
+  }
+
+  if (!isset($data[$name])) {
+    $data[$name] = $default_value;
+  }
+
+  return $data[$name];
+}
+
+/**
+ * Rebuild the path alias white list.
+ *
+ * @return
+ *   An array containing a white list of path aliases.
+ */
+function path_alias_cache_path_alias_whitelist_rebuild() {
+  // For each alias in the database, get the top level component of the system
+  // path it corresponds to. This is the portion of the path before the first "/"
+  // if present, otherwise the whole path itself.
+  $whitelist = array();
+  $result = db_query("SELECT SUBSTRING_INDEX(src, '/', 1) AS path FROM {url_alias} GROUP BY path");
+  while ($row = db_fetch_object($result)) {
+    $whitelist[$row->path] = TRUE;
+  }
+  cache_set('path_alias_whitelist', $whitelist, 'cache_path');
+  return $whitelist;
+}
+