Add bundle support to entity_uri callback to remove performance overhead of forum_url_outbound_alter() (regression from D6)

I like this a lot. Lets fix up those test failures and move ahead.

I'd think that forum.module could hook_entity_info_alter() taxonomy.module to add it's own uri_callback (or whatever we end up calling it) - which then checks for forum vocabulary.

Additionally the check on machine name is wrong, it should use variable_get('forum_nav_vocabulary');

This looks great to me except for the hunk which has to account for the brokenness in http://api.drupal.org/api/function/forum_url_outbound_alter/7

1. Doing a regexp on every url.

2. Doing an taxonomy_term_load() on every url that matches.

Instead we should have forum_entity_info_alter() change the entity_path() callback to it's own, handle the forums vocabulary properly, then ensure entity_path() is used in field formatters and other places consistently as the replacement to taxonomy_term_path().

That doesn't have to be dealt with in this issue, but the infinite recursion possibilities this currently has can happen anywhere. Note that's not just a problem with this patch - any module anywhere running url('node/1'); inside a hook_$something_load() could end up triggering infinite recursion.

Opened #712076: Infinite loop possibility and performance problems in forum_url_outbound_alter().

subscribe

I'm bumping this to critical. We have a major performance / infinite recursion regression in trying to use hook_url_outbound_alter() instead of taxonomy_term_path() between D6 and D7. Fixing that mainly means a change from taxonomy_term_path($term) to taxonomy_term_uri($term), and adding bundle support to the entity uri callbacks key, which is only an internal change in entity api which will affect very few, if any, entity modules in the wild. See #712076: Infinite loop possibility and performance problems in forum_url_outbound_alter() which has a workaround for both issues but needs to be fixed fully here, marking that other issue RTBC so we can make progress in this one.

Also retitling.

Here's a summary of some general discussion tonight in #drupal. It kinda spans multiple issues, but figured I'd stick it here as kind of a central 'bridging' place.

catch first pointed me to #712076: Infinite loop possibility and performance problems in forum_url_outbound_alter() as an "easy" patch to commit. :P (Note to self: never trust catch ;))

What that patch does, however, is take 6 or so lines of easily understandable code and turn them into a 25 line kitten killing monster which includes its own recursion detection. When I see code like that in a one-off module it creates the distinct impression that other modules are going to need similar code to handle that. Ick. So I would actually recommend not removing the recursion detection from this one and instead keeping it there. (and marking the other issue won't fix)

However, this approach is only really going to solve the issue for entities. There are tons of other modules out there (Dave Reid mentioned feedburner) that also will implement hook_url_outbound_alter() and don't get to benefit from this approach. It makes me seriously question whether we ought to roll that patch back since it was fairly late to begin with, and this makes at least 2-3 serious performance regressions as a result...

On this patch in particular, my concern is that lines like this:

- drupal_add_html_head_link(array('rel' => 'canonical', 'href' => url('node/' . $node->nid)), TRUE);
+ drupal_add_html_head_link(array('rel' => 'canonical', 'href' => $node->uri), TRUE);

...are really confusing, because nowhere else do we do this sort of "magic" pre-url()ing of urls, so I could see module developers scratching their head a good bit, and ending up doing double-duty on calling url(). I'm also not real keen on that 'url' parameter to $options. It feels like we ought to be consistent everywhere (but catch informs me this may not be possible).

In the forum issue, Dave Reid mentioned an idea about different modules passing various "contexts" they need in that $options array, to make hook_url_alter() more performant. So for example:

url('taxonomy/term/' . $term->tid, array('term' => $term));

and forum_url_outbound_alter could check for $options['term']

This, however, strikes me as a complete nightmare since there'd be no way to document the possible values in that $options array in the url() documentation, and creates a precedent for any random module sticking whatever random crap they want in a honking array that gets passed around a lot.

So.. not a lot of answers here, I'm afraid, but those are my first impressions.

Rollback++ apparently http://drupal.org/node/320331#comment-2188534 is not so cached after all.

Whether or not to rollback hook_url_(in|out)bound_alter() should be discussed in a separate issue. But I don't think it's fair to treat forum.module's inappropriate use of the hook as an indicator that the hook is bad. hook_url_outbound_alter() is architecturally sound when used correctly. Correct uses include the locale module. Also, http://drupal.org/project/domain. And to handle proxy issues as described in #535612-14: Fix Drupal 6 url() function to call custom_url_rewrite_outbound for external links too. That there are at least 3 independent legitimate uses of this hook makes me like it as a hook rather than a hard-coded function name like custom_url_rewrite_outbound(), but I can also appreciate chx's perspective that it's not worth the performance cost. Anyway, that cost-benefit analysis belongs in a separate issue. If we decide to keep the hook, we should document it better, so that mistakes like forum_url_outbound_alter() don't happen again in core, and are minimized in contrib.

Regardless whether we rollback that hook, I think we all agree here that forum_url_outbound_alter() needs to go. And for that to happen, we need entity_uri() to support per-bundle 'uri callback'. This is just an abstraction of what D6's taxonomy_term_path() does, so it will give us everything that function gave us, but for all entities.

Some of the code in #19 still needs work and can be made much cleaner once we have per-bundle uri callbacks supported.

However, this approach is only really going to solve the issue for entities. There are tons of other modules out there (Dave Reid mentioned feedburner) that also will implement hook_url_outbound_alter() and don't get to benefit from this approach.

Flexible path creation for entities is a problem that Views and Panels have been struggling with for years. Solving it is a huge leap forward that we should be massively proud of. Sure, flexible path creation for non-entities would be great, but we're not gonna get that in D7 at this point. If that leads to some contrib modules using hook_url_outbound_alter() to hack around that limitation, that's the price we pay for not having achieved perfect path architecture yet. But these are contrib modules that are already hacking in this way via the http://drupal.org/project/url_alter module.

Yeah I think the main thing here is to create a division of responsibility between altering arbitrary paths, and allowing paths which have a relationship to something we already know about, and have their own API attached. So if we can only do entity_path() here then that fixes the regressions, we'll find out during the D7 cycle what crazy implementations are done to hook_url_outbound_alter() which could be handled better via another method. The main purpose of this issue should be to not ship with any of those.

+++ includes/common.inc
@@ -2165,6 +2165,8 @@ function drupal_attributes(array $attributes = array()) {
  *       escaped HTML.
  *     - 'alias' (default FALSE)
  *       Whether the given path is an alias already.
+ *     - 'url' (default FALSE)
+ *       Whether the given path has been passed to url() already.
  * @return
  *   an HTML string containing a link to the given path.
  */
@@ -2176,6 +2178,7 @@ function l($text, $path, array $options = array()) {
   $options += array(
       'attributes' => array(),
       'html' => FALSE,
+      'url' => FALSE,
     );
 
   // Append active class.
@@ -2221,7 +2224,7 @@ function l($text, $path, array $options = array()) {
   }
   // The result of url() is a plain-text URL. Because we are using it here
   // in an HTML argument context, we need to encode it properly.
-  return '<a href="' . check_plain(url($path, $options)) . '"' . drupal_attributes($options['attributes']) . '>' . ($options['html'] ? $text : check_plain($text)) . '</a>';
+  return '<a href="' . check_plain(($options['url'] ? $path : url($path, $options))) . '"' . drupal_attributes($options['attributes']) . '>' . ($options['html'] ? $text : check_plain($text)) . '</a>';
 }

How do you guys (and gals) feel about pulling back a bit on the scope of this issue and taking all this l() optimization out? I'd like to see this issue be about bundle support for entity_uri(), making sure all places that make links to entities use entity_uri() (either directly or via a $entity->uri) instead of hard-coding a path (see for example, template_preprocess_username()), and removing forum_url_outbound_alter(). Maybe once we accomplish all that, we can open a new issue for optimizing url() calls.

+++ includes/entity.inc
@@ -229,6 +229,30 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+    // Add the URI of each entity if available.
+    foreach ($queried_entities as $queried_entity) {
+      if ($url_structure = entity_uri($this->entityType, $queried_entity)) {
+        // Prepare variables to be passed to url().
+        $path = $url_structure['path'];
+        $options = isset($url_structure['options']) ? $url_structure['options'] : array();
+        // Implementations of hook_url_outbound_alter() might try to load the
+        // same entity, so there is a risk of infinite recursion when calling
+        // url(), see forum.module for example. To avoid this recursion, we only
+        // call url() the first time an entity is being loaded. An empty uri is
+        // assigned on eventual subsequent recursive loads.
+        // @todo remove when http://drupal.org/node/712076 is fixed.
+        static $loaded_entities;
+        if (empty($loaded_entities[$path])) {
+          $loaded_entities[$path] = TRUE;
+          $queried_entity->uri = url($path, $options);
+          unset($loaded_entities[$path]);
+        }
+        else {
+          $queried_entity->uri = NULL;
+        }
+      }
+    }
+

If we're getting rid of forum_url_outbound_alter(), do we need all this? Also, as per above, can we make $queried_entity->uri be the result of calling entity_uri() rather than flattening it with a url() call?

+++ modules/forum/forum.module
@@ -219,6 +219,24 @@ function forum_init() {
+ * Implements hook_entity_info_alter().
+ */
+function forum_entity_info_alter(&$entity_info) {
+  // Overrides the generic uri callback of the taxonomy_term entity type to
+  // provide a specific uri for each term of the forums vocabulary.
+  $entity_info['taxonomy_term']['bundles']['forums']['uri callback'] = 'forum_uri';
+}

module_invoke_all() does array_merge_recursive(). So, can this be forum_entity_info() instead? Alter hooks are for making changes, not for adding the primary info data. Or is there something special about entities that makes doing this in an alter hook preferable?

143 critical left. Go review some!

#32 was x-post with #31. CNR for bot.

The $info['bundles'][$bundle]['uri callback'] property needs to be documented in the phpdoc for hook_entity_info() ?

+++ includes/common.inc
@@ -6437,10 +6440,17 @@ function entity_prepare_view($entity_type, $entities) {
+  // Uses the bundle specific uri callback if it exists.
+  if (isset($info['bundles'][$bundle]['uri callback']) && function_exists($info['bundles'][$bundle]['uri callback'])) {
+    return $info['bundles'][$bundle]['uri callback']($entity) + array('options' => array());
+  }
+  // Otherwise uses the generic entity uri callback for this entity type.
   if (isset($info['uri callback']) && function_exists($info['uri callback'])) {
     return $info['uri callback']($entity) + array('options' => array());
   }

Nitpick : can we refactor this into :
- some logic ending up with $function = something;
- return $fuction($entity) + array('options' => array());

Powered by Dreditor.

Well, if we have to optimize url(), how about this way? This changes entity_uri() to return an object instead of an array. That's why I'm adding the "API change" tag to this issue. But, there's only one place (image.field.inc) in HEAD that currently calls entity_uri() (this patch adds more), and I doubt that that many contrib modules have started calling it either, so I think it's an acceptable trade-off considering it allows the desired url() optimization in what I think is a nice way.

This patch doesn't include the forum_url module from #34, and instead changes path.test to be caught up to the removal of forum_url_outbound_alter().

This patch converts all the places that #34 converted from hard-coded urls to entity_uri() driven urls, but this is not complete. There's a larger sweep that's needed. But, I'd like to get a check from people here on whether this is looking like a good route before doing that sweep.

microbenchmarks look good. I also profiled with xhprof on a node with 48 comments and standard display settings, and while it looks like only about .5% improvement in time, general profile of URL looks much more encouraging. Attached screenshots of the url() detail for comparison.

Overall code looks good too. Only thing which concerns me if the caching in url depends on always passing in the same object and acting on it by resource - if we end up cloning by default in #154859: Document that cached entity objects are not necessarily cloned any more then couldn't we end up with different objects being passed in each time, and loose the benefit of caching there?

Thought about this some more, and it's extremely likely that the caching would be on the object passed around the various node view / build functions, so even if it's cloned from node_load() eventually, it's still going to be the same object passed by resource in between all those functions when we run URL on it - certainly it would be for comment listings and similar where this optimization is most pressing.

This is a very important patch in the sense we absolutely need to remove forum_url_outbound_alter() (as opposed to simply rolling back hook_url_outbound_alter() altogether), and because RDF can add up to 20% overhead to a node + 50 comment listing, a lot of which comes from url(). While it has API changes, they're either additions or contained to entity_info() which has few implementations outside core (and which will run into the same problems we have here so it's a critical bug for the lack of bundle support just by itself).

I think we should go ahead and commit this asap to get the API change out of the way, then do the much larger sweep which effulgentsia mentions in #37 afterwards. The microbenchmarks look great, the profile looks much better url(entity_uri('taxonomy_term', $term)) is a lot better than other ideas we had like having $options['entity'] or similar. As mentioned above, this pattern could also be used for non-entities if we found suitable places for it, which it looks like there are, so it's a generic solution as well. RTBC.

No. Just no. This can not be. Move that hunk out from url() into a wrapper, say entity_url(). You can't possibly suggest a core function that accepts an entity or an arbitrary path. do_everything($foo, $bar, $universe); is the next?

You can't possibly suggest a core function that accepts an entity or an arbitrary path.

That's not what #38 suggests. It suggests that url() and l(), which both take $path and $options as two separate parameters also take a single parameter that is an object containing ->path and ->options properties. That object is not an entity, but a uri (I guess, a "uri" being something that contains enough structured information to become a "url"). The problem trying to be solved is this: how to make it so that when url() is called multiple times for the same 'path' and 'options', that its result can be cached? If overloading the first parameter of url() is unacceptable, do you have any other ideas? Static cache within url() keyed on a serialized combination of $path and $options?

i spoke to chx in irc, he suggested an entity_url() wrapper around url(). So keep the caching / is_object() (we wouldn't need a check because could guarantee object) outside url(). Didn't play with that yet though.

This removes the is_object() from url() that chx disapproves of. This also removes the need for the BC break of changing entity_uri() to return an object instead of an array, so removing the "API change" tag: yay!

Getting better! Thanks. But array('fragment' => "comment-$comment->cid") this is gone and i can't see where it's added back.

comment_uri()

OK, one last question , why forum_entity_info and not entity_info_alter?

why forum_entity_info and not entity_info_alter?

From #32: Alter hooks are for making changes, not for adding the primary info data.

forum.module is the primary module responsible for the forum bundle. If it were making a change to anything about a taxonomy_term entity other than the forum bundle, then alter() would make more sense. Do you agree or am I missing something in our hook_*_info()/hook_*_info_alter() pattern?

Edited to add. Never mind, i didn't know about comment_uri()

Ignore this patch.

@noahb: Thanks for the good intentions :). Re-uploading prior patch to clarify that it's the one that's on the table.

re effulgentsia #48 : well, taxonomy_entity_info() does take care of exposing info for all taxo bundles (i.e taxo vocabs), including the forum vocab, right ?
Then it does look like forum_entity_info() in the patch only alters what taxonomy_entity_info() defined - for that matter it doesn't declare any of the other 'bundle' properties (label, admin), but relies on those set by taxonomy_entity_info().
Or am I missing something ?

About the comment in forum_entity_info() - would it make sense to have the 'forum_nav_vocabulary' variable store the vocab machine name instead of the vid ?

Sorry for the spam - I re-read #32, and I'd tend to disagree. Relying on array_merge_recursive(), and having two separate hook implementations define (partial) primary data for an entry, happens to work but is kind of confusing IMO. forum.module is not defining the primary data, it is tweaking the primary data exposed by taxonomy.module. Sounds very much like an alter hook to me.

Patch looks good. I'm not sure why we need to call url() recursively though, why not check at the end of the function if we had a cache miss and add to cache there?

I think we should use forum_entity_info_alter() as well, there's no particular benefit to using either, but if it'd been _alter() in the patch no-one would have mentioned it, so that automatically leans me towards that.

forum_nav_vocabulary() having a machine name instead of vid sounds good, not necessary for this patch though.

Changed to forum_entity_info_alter().

I'm not sure why we need to call url() recursively though, why not check at the end of the function if we had a cache miss and add to cache there?

url() returns in 5 different places, so that would need to be refactored or adding to the cache would need to be duplicated 5 times. An extra stack call adds 1us to the cache miss (and only when the cache is being employed), but we're already paying a couple us for the drupal_static(). Is it worth attempting the optimization you suggest as part of this issue?

Argh I'd forgotten url() returns so many times, NIH then, let's not get into a completely url() refactoring here, keeping this to less than 20k is nice ;)

Will try to run some benchmarks later today.

Added an @todo explaining the situation so that someone who's inspired to can attempt a url() refactor in a separate issue.

Node with some comments on it. Enabled forum module so we can see the impact of removing forum_url_outbound_alter()

Before patch:

Concurrency Level:      1
Time taken for tests:   65.782 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      10514000 bytes
HTML transferred:       9984000 bytes
Requests per second:    15.20 [#/sec] (mean)
Time per request:       65.782 [ms] (mean)
Time per request:       65.782 [ms] (mean, across all concurrent requests)
Transfer rate:          156.08 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    55   66  18.2     60     523
Waiting:       49   59  17.2     54     497
Total:         55   66  18.2     60     523

After patch:

Concurrency Level:      1
Time taken for tests:   64.785 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      10514000 bytes
HTML transferred:       9984000 bytes
Requests per second:    15.44 [#/sec] (mean)
Time per request:       64.785 [ms] (mean)
Time per request:       64.785 [ms] (mean, across all concurrent requests)
Transfer rate:          158.49 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    55   65  10.3     60      96
Waiting:       49   58   9.4     54      88
Total:         55   65  10.3     60      96

In xhprof, total cpu used for url() and the functions it calls went down from 40,000us to 28,000us

RTBC.

+++ includes/common.inc	2 Apr 2010 03:07:42 -0000
@@ -6430,11 +6467,49 @@ function entity_prepare_view($entity_typ
+      // By default, url() does not statically cache its results, because the
+      // risk of cache miss overhead outweighs the benefit of cache hit savings.
+      // Entity URIs, however, are more likely to pass through url() multiple
+      // times, especially with RDF enabled.
+      if (!isset($entity->uri['options']['cache'])) {
+        $entity->uri['options']['cache'] = TRUE;
+      }

Personally, I'm not a big fan of the cache handling in this patch, and the fact that people can turn on/off caching. It adds nuances and complexity to url() that are not easily understood without a lot of benchmarking. I recommend that we remove that for now.

url() is one of the worst CPU hogs in core at the moment, top 5 or so, and the main use for this cache is to be invoked transparently when people use entity_uri().

However, the critical bug and API change here (bundle support, removing forum_url_outbound_alter()) doesn't require the caching to be added, so I'm uploading a patch without that hunk, and I've opened #763574: Add caching for url() so we can work on it separately. Patch is untested.

Diff confirms that the only change from #57 is the removal of any modification to url() and the removal of adding a 'cache' key to what entity_uri() returns. Bot's happy, so RTBC.

Title change to reflect #60.

Fails to apply locally. Asking for a re-test.

#60: entity_uri.patch queued for re-testing.

Diff confirms #66 as equivalent to #60.

Great. Committed to CVS HEAD.

Here's the sweep I promised in #37. It might not be complete, but can this be reviewed? I'd like confirmation that this is in fact what we intended when we introduced a entity_uri() function.

Another try.

To actually allow rewriting of links, this is exactly what we need to do. Patch looks good, on the @todos, it looks to be only comment module which is causing problems? Generally we only render comments from a single node, although D6 used to call node load 3-4 times for each comment, which with drupal_static() was still needlessly expensive when viewing a lot (now it'd be getting them from the entity loader which is probably not much better), the answer to this was to pass $node around with $comments to the functions that need it - http://api-drupal.pajunas.com/api/function/comment_view_multiple for example. For the admin case, we can probably do something similar, for RDF less so.

Once again. This also solves the @todos by using entity_create_stub_entity(), though catch's idea (#73) of getting a real $node object into the necessary comment functions may be a better way to go.

oh, I really like this. As of now we have entity_uri() and the possibility to override it for bundles, however most places in core hard-code the url. It also catches some hard-coded taxonomy urls, which cleans up the forum-alter-url use case. ++

@array_merge($uri['options'], $options)
Perhaps we should use array_merge_recursive() here? What if entity uri callback makes use of 'attributes' or 'query' and the caller wants to do so too?

Perhaps we should use array_merge_recursive() here?

I like the idea, but: array_merge_recursive(array('language' => 'en'), array('language' => 'es')) results in array('language' => array('en', 'es')). This seems broken. How do we deal with this in other places that we use array_merge_recursive()? Scalars should replace. Only arrays should merge.

Please see #791860: array_merge_recursive() is never what we want in Drupal: add a drupal_array_merge_recursive() function instead. for a solution to the array_merge_recursive() problem.

>How do we deal with this in other places that we use array_merge_recursive()? Scalars should replace. Only arrays should merge.
We just use array_merge_recursive() and hope nothing breaks :D Anyway, we can incorporate drupal(_array)?_merge() here once it's fixed.

This is a re-roll only. I'll follow-up with implementing #76 and #79 later today.

This implements #76 (see #78/#81).

Re #79.1: It's related in that the example code includes a hard-coded path to 'user/UID'. This could be fixed by using entity_url(), but the example is wrong: it should be linking to 'user/UID/edit', so that's what the patch does.

Re #79.2: I really struggled with this, because I'm not really sure if we're intending to support entity types whose URIs are not URLs. Since Drupal is about making things available on the web, are we anticipating entity types whose URI is a URN rather than a URL? And if so, does our entity_uri() function handle that? But then we have things like the PHPDoc of entity_uri() saying @return: An array containing the 'path' and 'options' keys used to build the uri of the entity, and matching the signature of url() implying that we're gonna use the url() function to build a uri. So please see if the PHPDoc I added to entity_url() makes sense.

+++ includes/common.inc	15 May 2010 23:04:29 -0000
@@ -6593,16 +6596,82 @@ function entity_uri($entity_type, $entit
+ *   // Returns 'http://example.com/node/1?fragment=comment-form.
+ *   entity_url('node', $node, array('absolute' => TRUE, 'fragment' => 'comment-form'));

No, that's not how fragments work :)

83 critical left. Go review some!

Changing issue title to focus attention on the part that hasn't already landed.

+++ includes/common.inc	15 May 2010 23:04:29 -0000
@@ -6593,16 +6596,82 @@ function entity_uri($entity_type, $entit
+ * @param $options
+ *   Options to pass to url() in addition to the ones returned by entity_uri().
+ * @return
+ *   A URL string as returned by url().
+ *
+ * @see entity_uri()

put a newline between the different doc blocks (in this case between @param and @return)

+++ includes/common.inc	15 May 2010 23:04:29 -0000
@@ -6593,16 +6596,82 @@ function entity_uri($entity_type, $entit
+ * @param $options
+ *   Options to pass to l() in addition to the ones returned by entity_uri().
+ * @return
+ *   An HTML anchor tag as returned by l().

put newline between @param and @return

Powered by Dreditor.

thanks.

with the file.

No patch?

Damnit crossposting

This looks great. We need this to get the functionality of hook_term_path() - which was removed and replaced by hook_url_outbound_alter() in a way which caused a whole range of issues, back into Drupal 7.

Only question on the patch so far is do we really need those entity_create_stub_entity() calls in there. It seems like in some cases (comment deletion) we could just do a node_load() instead - those pages aren't in the critical path at all.

Agreed with #93. I left entity_create_stub_entity() in rdf_comment_load(), because that does seem to be critical path. But I commented it. I'm a little confused why we need to generate those variables there at all (do we ever need them other than when rendering a comment, in which case should we move that code to rdf_preprocess_comment()?), but perhaps that should be a separate issue?

@scor: thanks! Given #95, we should leave rdf_comment_load() as it is in #94. Potentially, someone who feels inspired can open a separate issue about removing those variables from rdf_comment_load(), but the performance analysis of that issue would need to include the considerations you mention. On another topic, can you please comment on #94's PHPDoc for entity_url() and whether it addresses #79.2 without introducing anything inaccurate. Please see #84 about my general concerns about how we use URI and URL. It's beyond the scope of this issue to fix incorrect usage of the terms in HEAD, but I don't want this patch adding any new misinformation. Thanks!

needs work to address the points in #96

I didn't intend #96 to imply "needs work". I would like a review from scor on the PHPDoc of entity_url(). Other than that, catch may want to review if #94 adequately addresses #93. Unless one of them (or another reviewer) has further changes to recommend, I think this can be RTBC.

The changes + comment in #94 looks good to me. Didn't give the patch another full once-over, but didn't see any other issues last time either.

Thanks! Is this better? If so, then given catch's and fago's reviews, I think we're RTBC.

Anyone up for RTBCing this? Sure would be nice to get this into alpha5 to help contrib authors be aware of this change.

I think we're good now.

Just a reminder of why this is needed:

* The hook_url_outbound_alter() patch removed hook_term_path(). This was used by forum module. forum_url_inbound_alter() led to a high potential for infinite recursion alongside performance issues.
* Rather than just put back hook_term_path(), we already had a need of entity URI for RDF, so we added the entity_uri() function instead.
* This patch just applies the use of that function consistently across core, introducing entity_l() and entity_url() as additional helpers, after the initial API change went in.

#101: entity_uri-use-699440-101.patch queued for re-testing.

Thanks for the summary. But changes made throughout this patch leave me scratching my head as to when I'm supposed to use which function.

1. Do we strictly need the entity_uri() function? Most people wouldn't be able to tell you what URI vs URL is without consulting Wikipedia (which, to prove the point, is linked in the description of entity_url()), and we don't have a corresponding uri() function for symmetry.

2. I think we need far better, and cross-referenced, documentation as to when to use l() or url() vs. entity_l() or entity_url(). The documentation that exists now is technically accurate, but doesn't help me as a module author to not screw up my links to things.

Reverting issue status to as it was in #69. Opened a new issue to continue this discussion (so people can join without wading through all the back story): #823428: Impossible to alter node URLs efficiently.

Re #106:

Do we strictly need the entity_uri() function?

I wrote in the new issue that I'm leaning towards removing it in D8, but we already have it in D7, and don't have a hook_url_outbound_TAG_alter() system in place that could make entity_uri() obsolete.

Most people wouldn't be able to tell you what URI vs URL is

Indeed. And I agree #101 didn't help address that. URIs are just a superset of URLs, but for Drupal entities, they're really the same thing, as all Drupal entity URIs are also URLs. So, I think we're just slowly changing terminology within Drupal from URL to URI, because URLs are so 1990s, and all the hip web technologists prefer the word URI. The patch in the new issue tries to make this more clear.