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

Proposed by Narayan Newton
Status: Merged
Merge reported by: David Strauss
Merged at revision: not available
Proposed branch: lp:~nnewton-drupal/pressflow/pathalias
Merge into: lp:pressflow
Diff against target: 308 lines (+274/-1)
4 files modified
includes/path.inc (+46/-1)
modules/path_alias_cache/path_alias_cache.info (+3/-0)
modules/path_alias_cache/path_alias_cache.install (+26/-0)
modules/path_alias_cache/path_alias_cache.module (+199/-0)
To merge this branch: bzr merge lp:~nnewton-drupal/pressflow/pathalias
Reviewer Review Type Date Requested Status
David Strauss Approve
Review via email: mp+16705@code.launchpad.net

This proposal supersedes a proposal from 2009-12-21.

To post a comment you must log in.
Revision history for this message
David Strauss (davidstrauss) wrote : Posted in a previous version of this proposal

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

review: Needs Fixing
Revision history for this message
Narayan Newton (nnewton-drupal) wrote : Posted in a previous version of this proposal

> 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

75. By Narayan Newton

Style and whitespace changes

Revision history for this message
Narayan Newton (nnewton-drupal) wrote :

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.

Revision history for this message
David Strauss (davidstrauss) wrote :

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.

Revision history for this message
David Strauss (davidstrauss) wrote :

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

review: Approve
Revision history for this message
David Strauss (davidstrauss) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'includes/path.inc'
2--- includes/path.inc 2009-12-17 00:30:08 +0000
3+++ includes/path.inc 2010-01-13 23:26:10 +0000
4@@ -25,6 +25,51 @@
5 /**
6 * Given an alias, return its Drupal system URL if one exists. Given a Drupal
7 * system URL return one of its aliases if such a one exists. Otherwise,
8+ * return FALSE. This function is a hook and modules can impliment lookup_path
9+ * to override the core behavior.
10+ *
11+ * @param $action
12+ * One of the following values:
13+ * - wipe: delete the alias cache.
14+ * - alias: return an alias for a given Drupal system path (if one exists).
15+ * - source: return the Drupal system URL for a path alias (if one exists).
16+ * @param $path
17+ * The path to investigate for corresponding aliases or system URLs.
18+ * @param $path_language
19+ * Optional language code to search the path with. Defaults to the page language.
20+ * If there's no path defined for that language it will search paths without
21+ * language.
22+ *
23+ * @return
24+ * Either a Drupal system path, an aliased path, or FALSE if no path was
25+ * found.
26+ *
27+ */
28+function drupal_lookup_path($action, $path = '', $path_language = '') {
29+ static $hook_module;
30+
31+ if (!isset($hook_module)) {
32+ $modules = module_implements('lookup_path');
33+ if (count($modules) > 0) {
34+ $hook_module = $modules[0];
35+ } else {
36+ $hook_module = FALSE;
37+ }
38+ }
39+
40+ if ($hook_module !== FALSE) {
41+ $retval = module_invoke($hook_module, 'lookup_path', $action, $path, $path_language);
42+ } else {
43+ $retval = _drupal_lookup_path_direct($action, $path, $path_language);
44+ }
45+ return $retval;
46+}
47+
48+/**
49+ * Internal function: The base path alias functionality as provided by Core.
50+ *
51+ * Given an alias, return its Drupal system URL if one exists. Given a Drupal
52+ * system URL return one of its aliases if such a one exists. Otherwise,
53 * return FALSE.
54 *
55 * @param $action
56@@ -43,7 +88,7 @@
57 * Either a Drupal system path, an aliased path, or FALSE if no path was
58 * found.
59 */
60-function drupal_lookup_path($action, $path = '', $path_language = '') {
61+function _drupal_lookup_path_direct($action, $path = '', $path_language = '') {
62 global $language;
63 // $map is an array with language keys, holding arrays of Drupal paths to alias relations
64 static $map = array(), $no_src = array(), $has_paths;
65
66=== added directory 'modules/path_alias_cache'
67=== added file 'modules/path_alias_cache/path_alias_cache.info'
68--- modules/path_alias_cache/path_alias_cache.info 1970-01-01 00:00:00 +0000
69+++ modules/path_alias_cache/path_alias_cache.info 2010-01-13 23:26:10 +0000
70@@ -0,0 +1,3 @@
71+name = Path Alias Cache
72+description = A path alias implementation which adds a cache to the core version.
73+core = 6.x
74
75=== added file 'modules/path_alias_cache/path_alias_cache.install'
76--- modules/path_alias_cache/path_alias_cache.install 1970-01-01 00:00:00 +0000
77+++ modules/path_alias_cache/path_alias_cache.install 2010-01-13 23:26:10 +0000
78@@ -0,0 +1,26 @@
79+<?php
80+
81+/**
82+ * Implementation of hook_install().
83+ */
84+function path_alias_cache_install() {
85+ // Create tables.
86+ drupal_install_schema('path_alias_cache');
87+}
88+
89+/**
90+ * Implementation of hook_uninstall().
91+ */
92+function path_alias_cache_uninstall() {
93+ // Remove tables.
94+ drupal_uninstall_schema('path_alias_cache');
95+}
96+
97+/**
98+ * Implementation of hook_schema().
99+ */
100+function path_alias_cache_schema() {
101+ $schema['cache_path'] = drupal_get_schema_unprocessed('system', 'cache');
102+ return $schema;
103+}
104+
105
106=== added file 'modules/path_alias_cache/path_alias_cache.module'
107--- modules/path_alias_cache/path_alias_cache.module 1970-01-01 00:00:00 +0000
108+++ modules/path_alias_cache/path_alias_cache.module 2010-01-13 23:26:10 +0000
109@@ -0,0 +1,199 @@
110+<?php
111+
112+/*
113+ * Implementation of hook_lookup_path
114+ *
115+ * This version of lookup_path is almost exactly what is in core, but adds a cache table.
116+ *
117+ */
118+function path_alias_cache_lookup_path($action, $path = '', $path_language = '') {
119+ global $language;
120+ $cache = &path_alias_cache_static(__FUNCTION__, array(
121+ 'map' => array(),
122+ 'no_src' => array(),
123+ 'whitelist' => NULL,
124+ 'system_paths' => array(),
125+ 'no_aliases' => array(),
126+ 'first_call' => TRUE,
127+ ));
128+
129+ // Retrieve the path alias whitelist.
130+ if (!isset($cache['whitelist'])) {
131+ if ($cached = cache_get('path_alias_whitelist', 'cache_path')) {
132+ $cache['whitelist'] = $cached->data;
133+ } else {
134+ $cache['whitelist'] = path_alias_cache_path_alias_whitelist_rebuild();
135+ }
136+ }
137+
138+ $path_language = $path_language ? $path_language : $language->language;
139+
140+ if ($action == 'wipe') {
141+ $cache = array();
142+ $cache['whitelist'] = path_alias_cache_path_alias_whitelist_rebuild();
143+ }
144+ elseif ($cache['whitelist'] && $path != '') {
145+ if ($action == 'alias') {
146+ // During the first call to path_alias_cache_lookup_path() per language, load the
147+ // expected system paths for the page from cache.
148+ if (!empty($cache['first_call'])) {
149+ $cache['first_call'] = FALSE;
150+
151+ if (!isset($cache['map'][$path_language]) || !is_array($cache['map'][$path_language])) {
152+ $cache['map'][$path_language] = array();
153+ }
154+ // Load system paths from cache.
155+ $cid = $_GET['q'];
156+ if ($cached = cache_get($cid, 'cache_path')) {
157+ $cache['system_paths'] = $cached->data;
158+ // Now fetch the aliases corresponding to these system paths.
159+ // We order by ASC and overwrite array keys to ensure the correct
160+ // alias is used when there are multiple aliases per path.
161+ $placeholders = db_placeholders($cache['system_paths'], 'varchar');
162+ $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);
163+ while ($record = db_fetch_object($result)) {
164+ if (!isset($cache['map'][$path_language][$record->src])) {
165+ $cache['map'][$path_language][$record->src] = $record->dst;
166+ }
167+ }
168+ // Keep a record of paths with no alias to avoid querying twice.
169+ $cache['no_aliases'][$path_language] = array_flip(array_diff_key($cache['system_paths'], array_keys($cache['map'][$path_language])));
170+ }
171+ }
172+ // If the alias has already been loaded, return it.
173+ if (isset($cache['map'][$path_language][$path])) {
174+ return $cache['map'][$path_language][$path];
175+ }
176+ // Check the path whitelist, if the top_level part before the first "/"
177+ // is not in the list, then there is no need to do anything further,
178+ // it is not in the database.
179+ elseif (!isset($cache['whitelist'][strtok($path, '/')])) {
180+ return FALSE;
181+ }
182+ // For system paths which were not cached, query aliases individually.
183+ else if (!isset($cache['no_aliases'][$path_language][$path])) {
184+ // Get the most fitting result falling back with alias without language
185+ $alias = db_result(db_query("SELECT dst FROM {url_alias} WHERE src = '%s' AND language IN('%s', '') ORDER BY language DESC", $path, $path_language));
186+ $cache['map'][$path_language][$path] = $alias;
187+ return $alias;
188+ }
189+ }
190+ // Check no_src for this $path in case we've already determined that there
191+ // isn't a path that has this alias
192+ elseif ($action == 'source' && !isset($cache['no_src'][$path_language][$path])) {
193+ // Look for the value $path within the cached map
194+ $src = '';
195+ if (!isset($cache['map'][$path_language]) || !($src = array_search($path, $cache['map'][$path_language]))) {
196+ // Get the most fitting result falling back with alias without language
197+ 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))) {
198+ $cache['map'][$path_language][$src] = $path;
199+ }
200+ else {
201+ // We can't record anything into map because we do not have a valid
202+ // index and there is no need because we have not learned anything
203+ // about any Drupal path. Thus cache to no_src.
204+ $cache['no_src'][$path_language][$path] = TRUE;
205+ }
206+ }
207+ return $src;
208+ }
209+ }
210+
211+ return FALSE;
212+}
213+
214+/**
215+ * Implementation of hook_exit
216+ * We use this to cache the paths on a page, for later requests.
217+ */
218+function path_alias_cache_exit() {
219+ path_alias_cache_cache_system_paths();
220+}
221+
222+/**
223+ * Cache system paths for a page.
224+ *
225+ * Cache an array of the system paths available on each page. We assume
226+ * that aliases will be needed for the majority of these paths during
227+ * subsequent requests, and load them in a single query during
228+ * drupal_lookup_path().
229+ */
230+function path_alias_cache_cache_system_paths() {
231+ // Check if the system paths for this page were loaded from cache in this
232+ // request to avoid writing to cache on every request.
233+ $cache = &path_alias_cache_static('path_alias_cache_lookup_path', array());
234+ if (!$cache['system_paths']) {
235+ // Generate a cache ID (cid) specifically for this page.
236+ $cid = $_GET['q'];
237+ // The static $map array used by drupal_lookup_path() includes all
238+ // system paths for the page request.
239+ if ($paths = current($cache['map'])) {
240+ $data = array_keys($paths);
241+ $expire = $_SERVER['REQUEST_TIME'] + variable_get('cache_lifetime', 0);
242+ cache_set($cid, $data, 'cache_path', $expire);
243+ }
244+ }
245+}
246+
247+
248+/**
249+ * Central static variable storage.
250+ *
251+ * @param $name
252+ * Globally unique name for the variable. For a function with only one static,
253+ * variable, the function name (e.g. via the PHP magic __FUNCTION__ constant)
254+ * is recommended. For a function with multiple static variables add a
255+ * distinguishing suffix to the function name for each one.
256+ * @param $default_value
257+ * Optional default value.
258+ * @param $reset
259+ * TRUE to reset a specific named variable, or all variables if $name is NULL.
260+ * Resetting every variable should only be used, for example, for running
261+ * unit tests with a clean environment. Should be used only though via
262+ * function drupal_static_reset().
263+ *
264+ * @return
265+ * Returns a variable by reference if $reset is FALSE.
266+ */
267+function &path_alias_cache_static($name, $default_value = NULL, $reset = FALSE) {
268+ static $data = array();
269+
270+ // Reset a single value, or all values.
271+ if ($reset) {
272+ if (isset($name)) {
273+ unset($data[$name]);
274+ }
275+ else {
276+ $data = array();
277+ }
278+ // We must return a reference to a variable.
279+ $dummy = NULL;
280+ return $dummy;
281+ }
282+
283+ if (!isset($data[$name])) {
284+ $data[$name] = $default_value;
285+ }
286+
287+ return $data[$name];
288+}
289+
290+/**
291+ * Rebuild the path alias white list.
292+ *
293+ * @return
294+ * An array containing a white list of path aliases.
295+ */
296+function path_alias_cache_path_alias_whitelist_rebuild() {
297+ // For each alias in the database, get the top level component of the system
298+ // path it corresponds to. This is the portion of the path before the first "/"
299+ // if present, otherwise the whole path itself.
300+ $whitelist = array();
301+ $result = db_query("SELECT SUBSTRING_INDEX(src, '/', 1) AS path FROM {url_alias} GROUP BY path");
302+ while ($row = db_fetch_object($result)) {
303+ $whitelist[$row->path] = TRUE;
304+ }
305+ cache_set('path_alias_whitelist', $whitelist, 'cache_path');
306+ return $whitelist;
307+}
308+

Subscribers

People subscribed via source and target branches

to status/vote changes: