Impossible to alter node URLs efficiently

+++ includes/common.inc	10 Jun 2010 01:13:58 -0000
@@ -6608,16 +6684,67 @@ function entity_uri($entity_type, $entit
+  ¶
+      case URI_FORMAT_FLAT_ARRAY:
+        return array('path' => $uri_elements['path']) + $uri_elements['options'];
+  ¶
+      case URI_FORMAT_FLAT_ARRAY_HREF:
+        return array('href' => $uri_elements['path']) + $uri_elements['options'];
+  ¶

extra spaces

+++ modules/comment/comment.module	10 Jun 2010 01:14:02 -0000
@@ -615,22 +615,17 @@ function comment_node_view($node, $view_
+          ) + entity_uri('node', $node, array('fragment' => 'comments'), URI_FORMAT_FLAT_ARRAY_HREF);

huh? links['comm...] = array ... plus uri?

+++ modules/comment/comment.module	10 Jun 2010 01:14:02 -0000
@@ -615,22 +615,17 @@ function comment_node_view($node, $view_
-            );
+            ) + entity_uri('node', $node, array('query' => comment_new_page_count($node->comment_count, $new, $node), 'fragment' => 'new'), URI_FORMAT_FLAT_ARRAY_HREF);

@@ -668,7 +663,7 @@ function comment_node_view($node, $view_
           else {
-            $links['comment-add']['href'] = "node/$node->nid";
+            $links['comment-add'] += entity_uri('node', $node, NULL, URI_FORMAT_FLAT_ARRAY_HREF);

same here and here?

+++ modules/node/node.pages.inc	10 Jun 2010 01:14:05 -0000
@@ -438,7 +438,7 @@ function node_delete_confirm($form, &$fo
+    entity_uri('node', $node, NULL, URI_FORMAT_FLAT_ARRAY),

indent this?

67 critical left. Go review some!

I kind of fell asleep after that. And those notes might be premature at this point...

looks good to me. maybe someone has an idea on how to make this even more "easy on the eyes and hands". its committable as is though.

The rename sounds good, and I like that this patch doesn't have those very long constants. No time to review the whole patch though.

Patch looks good to me. Its RTBC as is. Will wait a day before doing that.

We don't typically do this, but I think we should add a watchdog() to url() so that it screams if you call user/n or node/n or comment/n without going through entity_url. If we don't do that, we're gonna see a lot of underperforming code in contrib. We'd probably pass an $option['entity_url'] = TRUE flag so that url could skip the warning when code does the right thing.

Looks good to me as well, not sure about keeping entity_uri() in (as someone who has custom code calling it), but it's easy to remove if we grep contrib and find it's not there. I still want to sit down with this properly, which I haven't done yet, but previous patches on the old issue looked good and the new functions in this one look straightforward.

I'm happy to defer the niceities that I mentioned in #8. RTBC.

Personally not a big fan of this patch but I need to thinker about it more.

+1 for moving forward on this

-  $form_state['redirect'] = "node/$comment->nid";
+  $form_state['redirect'] = entity_url_arguments('node', node_load($comment->nid))

You can't possibly do this a year after code freeze or something like that. No way. I am with Dries.

Indeed we had but never used taxonomy_term_path I think. Do we want to allow? yes we want to. Did it happen in D7 fully? No. Rome was not built in a day. There's always the next release. I so far found three people who were unhappy with me throwing out the totally useless field_attach_query for EntityFieldQuery -- can you imagine the flak we would get if we would enforce people to change node/$nid? If there is, say, a 20-30 percent speedup on pages then we can talk.

I have no good solutions. An admission of defeat and accepting the reality would be the removal of entity_uri...

Removing entity_uri() is unacceptable at this point unless we put taxonomy_term_path() back in.

The way I understand it, we only had taxonomy_term_path() to allow Forum module to intercept all paths for containers and categories in a forum. Other terms are linked to the regular taxonomy/term/% path. But let's forget about the Taxonomy and Forum module use-case for now.

The question is, how likely this scenario is going to happen elsewhere. I.e., how often are entities going to be abused for a special use-case that handles the entire output of entities on its own menu router paths.

Personally, I always hated Forum module's integration of taxonomy, as it's a pain to work with. This may have changed for D7, but the question remains whether the overall pattern of re-using a subset of entities for a special purpose is something we want to support and encourage.

I'm uncertain. All I know is that taxonomy_term_path() and also this entity_url_arguments() does not work universally. That is, because you only get the path to view the entity; there's no way to retrieve the %/edit task or any other, e.g., the %/revisions/view thingy.

Continuing this consideration, it's obvious that a fully-fledged pattern of re-using and re-routing a subset of entities through different menu router paths is not going to work out cleanly, as you'd have to properly fork all other child paths of the original 'uri callback', too. Invocations of arg() will almost certainly get in your way.

Overall, I think I'd prefer to just introduce entity_url_arguments() for usage in Taxonomy module and Forum module, i.e. limited to replace the (previously?) existing calls only.

In D8, we should think about ways to remove this abuse, as Forum's use-case is not so much about taxonomy, but rather about creating and maintaining proper trees and tree references.

+++ includes/common.inc	11 Jun 2010 22:17:39 -0000
@@ -6572,52 +6578,217 @@ function entity_prepare_view($entity_typ
+function entity_uri($entity_type, $entity, $extra_options = NULL) {
...
+function entity_url($entity_type, $entity, $extra_options = NULL) {
...
+function entity_l($text, $entity_type, $entity, $extra_options = NULL) {
...
+function entity_link_item($text, $entity_type, $entity, $extra_options = NULL) {

When reducing this patch to Taxonomy and Forum, I don't think we need these helper functions.

Note that the entity-object-property caching could cause problems, if you have a forum $node that has not only a forum tid, but also other terms attached. The forum term, i.e. the actual use-case of this entire thing, needs to link to /forum/[tid], whereas all other terms need to link to /taxonomy/term/[tid]. Hence, I guess that this caching needs to be removed.

65 critical left. Go review some!

Removing entity_uri() is unacceptable because image fields can be attached to any entity type and need a way to output a link to the entity without assuming "$entity/$entity_id". That was where it all started.

taxonomy_term_path() was used by taxonomy_redirect module, which is a handy one, it's not just for forums.

Latest follow-ups clarified that this patch is a required and critical API change for D7. Especially the "link to entity" argument manifests this.

Repeating the only remaining issue from my review: The entity url caching probably needs to be removed, since it breaks Forum module's links of taxonomy terms to different paths, if there are also other non-forum terms associated.

marking needs work based on #21 "The entity url caching probably needs to be removed, since it breaks Forum module's links of taxonomy terms to different paths, if there are also other non-forum terms associated."

I think as minimum all hard-coded calls should be replaced with entity_uri()

As far as I can tell, sun is now on board with latest patch and no objections remain. Lets send this back to Dries so he can thinker on it.

Just because image needs to call entity_url does not mean all core and contrib needs to. I am still not convinced we need to convert Drupal 7 on June 28, 2010 to this construct. Yes it dilutes the usability of entity urls -- consider it a denormalization of entity urls into code for speed. Not good. But, there are limits to what changes we allow. To compare, the API changes this last week, as far as I can remember was the undocumented automated settings defaults and the menu active handler hook which should not have existed anyways (and was not documented in the update docs either). Compared to those obscure, undocumented this is a sweeping huuuge change.

What's wrong with only calling entity_url when you absolutely and totally need it? Like image.

I think the last paragraph of #23 makes a good point as to why this isn't really a late sweeping change.

We need to fix this issue to allow a "link to entity" option in field formatters. Although it is indeed late, we need to do a change in this direction some day either way, as these changes merely introduce a property along the lines of $entity->url. In the long run, i.e. beyond D7, we will have to advance on this and add access permission conditions anyway. What counts is the overall move to a dedicated/centralized call to access the view of an entity.

Hence, nothing bad in here. Easily changeable for contrib authors, if any. So let's get this in.

If I understand @effulgentia's comments from #839520: entity_uri should pass the entity data to url() correctly, implementing entity_uri() consistently can vastly improve the efficiency of custom_url_alter() calls, since we can do this at the entity level and avoid having to call an _alter() every single path call.

So I'm for trying to make this work, and can throw some cycles at it this week. I even have a D7 module (Domain Access, 7.x.3 [HEAD] branch) where we can test on node entities.

Just so it's more visible, I repeat the paragraph in question:

I'm not sure. We would certainly be recommending that contrib authors follow the pattern and fix their code to not hard-code entity paths. But, nothing in core breaks if they choose to not follow that recommendation. Breakage would only occur when using such a module with another contrib module that alters the 'uri callback' of a core entity type. So what would happen is someone would post an issue to the module that's hard-coding an entity path that says "please change this line of code that hard-codes node/NID to instead call the appropriate entity_*() function, so that it integrates properly with moduleY". Seems like the typical kind of integration issues that fill a module queue long after a Drupal core version is released.

I can live with this. Yes, let's get this in.

Maybe I'm just doing something stupid, but trying to use hook_entity_info_alter() creates a fatal error after applying the patch.

/**
 * Implements hook_entity_info_alter()
 */
function domain_entity_info_alter(&$entity_info) {
  // Use our own rewrite handler for the node URL.
  $entity_info['node']['uri callback'] = 'domain_entity_uri_node';
}

PHP Fatal error:  Unsupported operand types in /Applications/MAMP/htdocs/drupal-cvs/includes/common.inc on line 6708

And those lines are:

    // Invoke the entity type's or bundle's callback. If there is no callback,
    // set the 'url_arguments' property to FALSE to indicate that it is known to
    // not have its own URL.
    if (isset($callback) && function_exists($callback)) {
      $entity->url_arguments[$entity_type] = $callback($entity) + array('options' => array());
    }

I have no clue why, after an hour of debugging. PHP 5.3.

Thanks.

Patch is now working as expected. In my case, it cuts database queries from 133 to 123 on the home page.

If anyone cares to test, I put a quick Domain Access patch over at #841524: Use entities to rewrite links. This checks the status of all node entities and rewrites the URL as needed, eliminating the need for hook_url_outbound_alter() in most cases.

@agentrickard: I'm surprised this cuts database queries - which ones?

Custom queries in hook_custom_url_alter(), which I don't have to run anymore, since the $entity provides all the context I need to rewrite the url.

I think the one bit that I am confused about here, and can't answer after scanning the documentation, is this:

How does one properly write a link to node/NID/edit using the entity system?

I re-read the patch and its ready IMO. We will re-evaluate for D8.

Yes, let's RTBC this and then add clarity in the documentation. The problem, though, is that the performance gains I was getting bypassing custom_url_alter() resurface for non-canonical paths (e.g. node/NID/edit).

subscribe

subscribing

I have not yet reviewed this patch (not that it matters since it is already RTBC) but I have been advocating ditching 'node/'.$node->nid for long time. See http://drupal.org/node/82751#comment-1242435 for one example. So in principle I'm pretty sure I agree with this patch (though again I have not reviewed it).

Just a reroll

I haven't read the entire issue bug my gut reaction is not liking this. In the first five hunks of the patch, I see calls to entity_url_arguments, entity_url and entity_l. I know that it might be necesarry but still, it's quite confusing.

The other thing is for users we always had theme('username'), is that changing?

It's important to understand that almost everyone on this issue did not feel comfortable with this patch -- until he/she read the reasoning(s).

While it's tagged as API change, it's rather an API addition, as eff's summary explains.

Have we looked at all other possible alternatives to this patch? For example, the OP suggested this:

Why not have entity uri manipulation done via hook_url_outbound_alter()? In Drupal 7, we can't do it, because of performance constraints: every hook_url_outbound_alter() runs for every call to url(). Perhaps a hook_url_outbound_TAG_alter() system similar to hook_query_TAG_alter() might offer a better solution in Drupal 8 than entity_uri().

That was mentioned as being for Drupal 8, but honestly sounds like a much less invasive change than this one. It's not even clear we'd need to play around with hook_url_outbound_alter() anyway; why can't url() itself be smart enough to look at the path and say: "This seems to match one of node/%, user/%, forum/%, etc, so I need to do the entity uri stuff on it internally"? There might be an issue of performance, but seems like it's worth a shot.

I'm not too excited about trying to make module authors understand what is and isn't an "entity" when all they want to do is link to something. It would be really nice if we could try every possible avenue to avoid that, and just be able to tell people to use l() and url() like they always do.

@David: I'm not sure how that could solve the problem space and use-case of "link to entity" options in field formatters?

Also, module authors will have to understand what is what kind of entity (or none at all) anyway, because they have to properly check access permissions. Standardizing on dedicated functions for entities may allow us to advance them in D8+.

url() looking at the path and figuring out what it might be is precisely what this patch is trying to avoid, because it's expensive, and Drupal 7 already has horrible performance in PHP which we don't want to make any worse than it already is.

@David: I'm not sure how that could solve the problem space and use-case of "link to entity" options in field formatters?

Ah, I wasn't suggesting that we get rid of the entity 'uri callback' itself; we can certainly leave that as is. Just that we make url() able to deal with internally.

Also, module authors will have to understand what is what kind of entity (or none at all) anyway, because they have to properly check access permissions.

Maybe, but I can still just call node_access() to do that, without bothering with entities directly.

I think what makes everyone uneasy here is that currently it's simple to create a link; you look through the code and see a path like "node/nid" and link to it via l(). Now here we are saying that you need a deeper understanding of internal Drupal concepts in order to link correctly. For many developers, nodes are abstract enough as a concept, let alone entities :)

url() looking at the path and figuring out what it might be is precisely what this patch is trying to avoid, because it's expensive, and Drupal 7 already has horrible performance in PHP which we don't want to make any worse than it already is.

I am actually not sure it is obvious this would hurt performance; it could be the other way around. For a non-entity URL, the worst it would add is a substr() call or something like that. Plus, for a site that does not alter any uri callbacks (read: "a site with forum module disabled"), we ought to be able to bypass this entirely, via a simple empty() check.

Whereas with the current approach of modifying the calling code we will still always take a performance hit of (a) getting the entity object to link to, or possibly constructing a mock object if it isn't loaded yet (although granted it's usually already available), then (b) passing it to entity_l() or another wrapper function, which does some processing of its own before url() is ever even called. And we do that for every node link, comment link, etc on the site, even though there is a very very good chance that those will just wind up pointing back to the original node/nid URL anyway.

***

I'll see if I can give a stab at writing an alternative patch a bit later. It will probably lead to some ugly code, but I think it is worth having an alternative to compare to.

It would also be cool to get some benchmarks of HEAD, the approach in #49, and the approach David outlines. We can reject it if it does indeed turn out to be a huge CPU suck, but the DX issues with the current approach that he outlines here are non-trivial (and a good deal of the reason so many newcomers to this issue and both core maintainers have been wringing their hands). It's at least worth a shot. And thanks David, for once again magically articulating what I've taken 5-10 stabs in a text editor at doing over the past few weeks. ;) You have a knack for that. ;)

On the performance note, I got really excited about Ken Rickard's #38 where he mentioned he shaved 10 queries off each page with this patch in Domain Access. But then discouraged again in #45 where he mentions that those gains go away when doing any other link, such as node/$node->nid/edit. That seems to be pulling the corner back on either a whole 'nother class of DX issues related to this, or at best another huge round of follow-ups. Neither of which we need at this stage.

Thanks, webchick :)

Here is my attempt at the alternative approach. It needs some cleanup and docs still, but basically seems to work and should be good enough for testing.

In terms of performance, I think this may turn out to be a performance improvement rather than a penalty, based on the discussion above. I guess we'll see. With this patch, inside url() essentially nothing happens if the Forum module is disabled, and even if Forum module is enabled only taxonomy links have anything serious done to them; node/comment/user/etc links bail out after a quick check.

As part of this patch I also went and removed direct entity_uri() calls from everywhere in core where it is (now) possible to do so. This will allow us compare the performance of the two approaches better. Obviously if we decide we want to keep some of the them for e.g. aesthetic or code cleanliness reasons, we could still keep them; the approach here should allow e.g. entity_uri('node', $node) and url('node/' . $node->nid) to be used interchangeably for linking to an entity.

Awesome. Thanks, David. Looks like we have some notices, but hopefully can still test this patch.

Tagging.

+++ includes/common.inc	3 Aug 2010 13:39:01 -0000
@@ -2017,13 +2017,37 @@ function format_username($account) {
+  if (!isset($altered_entity_root_paths)) {

likely needs a MAINTENANCE_MODE != 'install' condition.

+++ includes/common.inc	3 Aug 2010 13:39:01 -0000
@@ -2043,6 +2067,36 @@ function url($path = NULL, array $option
+         $entities = entity_load($entity_type, array($entity_id));

+++ modules/comment/comment.module	3 Aug 2010 13:39:02 -0000
@@ -2214,9 +2214,8 @@ function template_preprocess_comment(&$v
+  $variables['title']     = l($comment->subject, 'comment/' . $comment->cid, array('fragment' => 'comment-' . $comment->cid));
+  $variables['permalink'] = l('#', 'comment/' . $comment->cid, array('fragment' => 'comment-' . $comment->cid));

This likely means that we need to statically cache loaded comments?

+++ includes/common.inc	3 Aug 2010 13:39:01 -0000
@@ -6470,8 +6524,38 @@ function entity_get_info($entity_type = 
+            $entity_info[$name]['altered root paths'] = _entity_uri_collect_root_paths($name, $original_entity_info[$name]);

I don't understand why we aren't assigning a 'uri basepath' per bundle in hook_entity_info() ?

Powered by Dreditor.

Awesome. Thanks, David. Looks like we have some notices, but hopefully can still test this patch.

The notices are only in the D6->D7 upgrade path, so yes, it should be OK. We can worry about fixing that if and only if we wind up choosing this approach :)

likely needs a MAINTENANCE_MODE != 'install' condition.

I think installs are OK, actually. But this getting called during the D6->D7 upgrade might be what leads to those test failures...

This likely means that we need to statically cache loaded comments?

Ah, yes, it seems that entities which are not statically cached could be a problem here. I had a code comment to the effect of trying to construct a dummy object rather than actually calling entity_load(); maybe that is worth pursuing.

Any of this would only matter if a module actually altered the URI callback for comments, so it won't affect any benchmarking of the patch with core. But we'd need to look into it.

I don't understand why we aren't assigning a 'uri basepath' per bundle in hook_entity_info() ?

We could consider it, but mainly because I didn't want to change the API. Also because the basepath alone isn't quite enough; comments use URL fragments as part of their URI, so it seems like we'd still need some kind of URI callback function, which would then mostly duplicate the information contained in any new 'uri basepath' key?

***

BTW, it might be worth highlighting this part of the patch, which was down at the bottom, so easy to miss if skimming it:

  *   - uri callback: A function taking an entity as argument and returning the
  *     uri elements of the entity, e.g. 'path' and 'options'. The actual entity
  *     uri can be constructed by passing these elements to url().
+ *     @todo Document the restriction that entity URIs returned from this
+ *       callback must only use the entity ID (and no other entity property) to
+ *       construct the URI, and that it must be the last part of the path;
+ *       e.g., for nodes, it can be some/random/path/{$node->nid}, plus an
+ *       optional fragment and query string (although I'm not sure the code
+ *       written so far properly supports fragments and query strings). This
+ *       restriction is pretty reasonable and is already met by all core entity
+ *       URI callbacks.

I don't think this limitation is really a problem since I doubt the URI callbacks were ever intended to be some kind of Pathauto replacement or anything, but it's worth noting. This limitation is basically required in order to be able to have performant code in url() in the patch.

We could enforce it perhaps by changing the API so as to only pass the ID in to the URI callback function (rather than the entire entity) but I did not try that for now.

Regarding benchmarks, I don't have time to do that right now (I haven't done it before so would take a little time to get set up), but some thoughts on what might be worth testing if anyone else wants to try in the meantime.

Comparing current HEAD to #49 to #57, as webchick suggested, we might want to test:

1. Standard Drupal installation, visiting a page with a bunch of entity links on it (maybe a node with a bunch of comments?)
2. Standard Drupal installation, visiting a page with a bunch of non-entity links on it (maybe admin/config?)
3. Repeat #1 with Forum module enabled.
4. Repeat #2 with Forum module enabled.
5. Some kind of page with lots of forum links on it, maybe admin/structure/taxonomy/forums after creating a lot of forums?

***

BTW, I noticed that patch #49 has a bug on that page (admin/structure/taxonomy/forums) - it does not rewrite the forum links to forum/[tid] there. The reason seems to be that the menu callback for that page does not work with full $term objects but rather with fake objects constructed from $form_state['values'] which don't have the bundle information attached to them, so entity_uri() cannot construct the correct links. I don't think this would affect the benchmarking, but I thought it was worth mentioning.

I'm already using entity_uri() for paths which require having the full entity available to build and don't use $id at the end of the path - this is precisely to avoid pathauto - by using a combination of hook_url_inbound_alter() and entity_uri() I save 250 * 250k = 62.5m aliases with zero performance penalty for other paths.

entity_uri() works fine for this purpose now (possibly not for node paths which are linked everywhere, but for taxonomy terms it's easy with a formatter), and I'd be extremely unhappy to see a regression there.

Dave Reid was also very keen on having this available for pathauto to allow for similar usage, so it's not just me who wanted to use it for this purpose.

Removing the option would also be a regression from Drupal 6 hook_term_path() which is what entity_uri() is replacing more or less (as a targeted alternative to hook_url_outbound_alter()). So not at all keen.

It also means that if you have any code which calls url() within hook_$entity_load() or hook_entity_load() then you'll get infinite recursion. If necessary I'll go back and find the original issue where this was discussed in depth, I also had long conversations on irc with webchick about it.

On the code I only did a very quick read through, but strtok() would be more readable and likely faster than than strpos() + substr().

@catch: I believe your use case would still be supported. Looking into this more closely, I think I misspoke above: The restriction on paths would only be for the original module implementing hook_entity_info() - i.e. taxonomy module or node module - not the module altering it. This is because the restriction only exists to let url() recognize and transform things like 'node/[nid]' which are passed in to it. If you add or change a URI callback via hook_entity_info_alter(), then I think your URI callback can do whatever it wants. A gotcha, I guess, but that's how it would work.

Which also means using this in Pathauto would be possible - and actually this approach is the only way it could ever be used there, at least reliably, since it guarantees that the alias will always be respected (whereas no one here expects contrib module authors to reliably change all their links to use entity_uri).

Infinite recursion: Yes, that sounds like a potential problem.

BTW, I noticed that patch #49 has a bug on that page (admin/structure/taxonomy/forums) - it does not rewrite the forum links to forum/[tid] there. The reason seems to be that the menu callback for that page does not work with full $term objects but rather with fake objects constructed from $form_state['values'] which don't have the bundle information attached to them, so entity_uri() cannot construct the correct links. I don't think this would affect the benchmarking, but I thought it was worth mentioning.

I also seem to have misspoken here. The bug is real but, has nothing to do with $form_state['values'] - I have no idea where I got that from :) I think because the code in taxonomy_overview_terms() moves things around and at one point does put the term objects as an array within $form.

The ultimate cause of the bug is that that function gets its terms from taxonomy_get_tree(), which says it returns "term objects" but doesn't actually return full entities, so they don't work correctly when passed in to the entity_uri()-related functions.

This can't be used with Pathauto because we need the root path without running it through url() to use as an alias's 'source' field. This is easily possible with entity_uri and it works really well as such not just in Pathauto, but in several other modules I've been porting.

Dave Reid: Do you have a specific example of code that would not work with the patch in #57?

I took a look through the latest Pathauto 7.x-dev and didn't see anything that seems like it would need to change. As described above, #57 keeps entity_uri() working just like it did before; any code that wants or needs to use it would be able to continue to do so.

No, it looks like it would still work, but it is way uglier and hard to follow. I think overall that a person writing code to link to an entity should not even have to care or know what 'path' they should be using and we should be taking care of that for them. This new direction is more restrictive in that paths *must* have the entity ID as the last argument. I'd much rather prefer the direction of the previous version as it's quite obvious to our developers that "hey I'm linking to an entity" rather than "oh, I'm linking to something and Drupal is going to go a crazy on it if it links to certain things"

We just had an epic discussion about this patch in the Boston/Acquia code sprint. We went back and forth between effulgentsia's patch (#49) and David's patch (#57). We mostly agreed on eff's patch since it "moves us in the right direction" unifying entity link building.

Then hefox brought up the example of someone who uses menu admin to add a link to node/n. We have no good way to know that this an entity link, and should go through entity_l(). Anyone have an idea of how to resolve this? ... So, we chose David's patch. David agreed to finish it and resubmit.

Also, to clarify again, it is not totally an "either/or" between the two patches, in the sense that we already have entity_uri() in core and we aren't proposing to get rid of it, and we could add the other entity-linking functions as well. The question is mainly what happens when they aren't (or can't) be used.

This new direction is more restrictive in that paths *must* have the entity ID as the last argument.

As described in #64 you call alter it to be whatever you want so it isn't a real restriction. Also, it's no more of a restriction than telling developers to use entity_l() - if they don't follow it, nothing terrible will break.

For now, I'm going to try to reroll the patch and fix some of the @todos and then hopefully we'll have time to look at benchmarks between the two approaches so we can decide where to go.

Oh, someone writing a menu link is a very good use for David's method. Looking forward to it fleshing out and will like to help review.

Updated the patch:

• Avoided one infinite recursion possibility (not the one mentioned by @catch) by using the variable system to store the info needed by url(), rather than putting it in the $entity_info array as my previous patch did; this is better for a number of reasons, and might also fix the failing tests.
• Fixed the infinite recursion possibility that @catch did mention by adding an entity_is_loading() function; didn't really want to do that, but seemed worth it, especially since this is not the only place where entity loading can lead to infinite recursion (see http://api.drupal.org/api/function/hook_field_load/7) so maybe it makes general sense to have a function that can be checked.
• Handled fragments and query strings correctly, or at least I think so.
• Looked into using strtok() as suggested, but it doesn't seem like it will be useful since we need to pull off the last part of the path not the first, so I stuck with the original code for that.
• Added a test module that is essentially a poor man's version of pathauto; this will be useful for benchmarking, since we can use it to generate a test page where all nodes/comments/etc have their URLs rewritten. It may also wind up being useful for writing eventual simpletests, which is why I put it in the patch file itself.
• Fleshed out some code comments in places where it helped me understand what I was doing, and other minor cleanup :)

Besides documentation, there is only one @todo left in the patch and it's not in an area that has performance implications, so I think this might be ready to benchmark the two extreme solutions (#49 vs this patch) and then decide if/how we want to combine them.

I've reviewing this issue as part of the Copenhagen core summit.

First let's re-roll David's patch from #73 in case there are any core changes.

I really don't like the architecture of the proposed patch in #73 (nor, of course, my re-roll in #75). The patch requires the path system to be aware of the entity system; a circular dependency like this should not exist. The code to explicitly prevent an infinite loop is a clear red flag for a bad architecture of this sort.

I also don't like how this uses the variables system as a cache of modified paths.

We have a couple options here:

(1) Provide a generic, hook-style API that the entity system can use to perform appropriate path modifications.
(2) Go with the original entity wrappers for common path functions. I agree with previous criticism that this doesn't offer a clear integration strategy for hook_menu() implementations.

I'm going to modify David Rothstein's patch to try option (1). I should be able to maintain performance by turning the "entity_altered_root_path_list" into a generic facility for modules to register altered paths.

Oh, apparently the path system already has deep knowledge of the entity system. *sigh*

I don't this as an important issue, certainly not one warranting such massive hacks to common.inc. We seem to already solve the image field and forum requirements with entity_uri(); they can generate links to arbitrary entities. Anything else can continue to be hard-coded as before.

Is anything *actually* broken here? I'm not asking about abstract consistency concerns for complex paths and theoretical usage scenarios.

Patch from #75 was missing the test files, so I'm reuploading #73 (it still applies).

(1) Provide a generic, hook-style API that the entity system can use to perform appropriate path modifications.

We already have that: http://api.drupal.org/api/function/hook_url_outbound_alter/7

The reason I did not use it here was (a) performance, and (b) consistency between calls to url() and entity_uri(); i.e., people who call entity_uri() guarantee that the entity-related URL modifications always take place first (before any others), so we need to preserve that same behavior for direct calls to url().

I agree there is some ugliness here; however, the possibility of infinite recursion is something that anyone with a complex implementation of hook_url_outbound_alter() would need to deal with, so moving code there doesn't solve anything. The existence of the 'uri callback' parameter in hook_entity_info() is what couples the two systems already; this patch is just trying to make that existing coupling work.

I agree this may not be critical, but think that it's at least 'major' -- without solving this issue, the 'uri callback' option in hook_entity_info(), which people are already apparently starting to use in a variety of ways, is essentially useless.

Looks like @sun was the last person to set this back to critical. @sun, do you have an opinion on whether or not it still is?

I spoke with David about this at Dev Summit and he reluctantly agreed that this was the least awful resolution for D7. Really, I think this is RTBC. Anyone want to chime in before that?

To clarify, I spoke with David *Strauss* at Dev Summit. His objections in #76 can now be summed up just like the rest us: "I'm not happy about it, but this patch should go into D7"

@davidR - any chance you can clear up those todo items in the code?

Uhm, backing that much complicated + entity specific logic into url() is really weird.

#79 implements a generic possibility to specify entity-URL alterations. But do we really need to support this additionally to hook_url_outbound_alter()? Why is it critical to add that?

Also, module authors will have to understand what is what kind of entity (or none at all) anyway, because they have to properly check access permissions. Standardizing on dedicated functions for entities may allow us to advance them in D8+.

I must say that sounds better to me. However - when altering entity URLs isn't supported - we really don't need to use that everywhere! If node module defines its path to be on node/$nid it can just use that. If module/entity X wants to support module-altered paths it still could - thus whether using entity_l() or an entity specific alternative is required is up to be decided by the entity type in question. So if an entity wants to support url alterations - go on and solve the problem yourself. -> For taxonomy terms we could just stay with taxonomy_term_path() as in D6, what's wrong with that?

For the use-case of generating links to entities something like entity_l() + #629484: Add entity 'label' key info (e.g. title on node) should be fine.

@fago - i'm not surprised you aren't happy with DavidR's patch. Noone is.

Please read @effulgentsia's patch in #34 and comment which is more like what you are saying. Note that it does not handle the menu item pointing to node/n (for example)

If we let core and contrib modules continue to hard code user/x and node/x paths, and we don't do david's patch, then we leave url_callback impotent in entity API. Please explain how we should resolve this.

#34 is pretty fine, however it still supports altering entity-urls generically - thus requires the use of entity_url(). What I wonder is whether we really need to support this generically? Do we really need to support this for nodes and drop "node/$nid" ? If so, why? If not, introducing entity_l() is rather simple.

If there is no entity lever url alteration maybe some modules might have to use hook_url_outbound_alter() - but what's wrong with that? Getting the associated entity for node/$nid shouldn't be hard, e.g. use menu_get_object().

>If we let core and contrib modules continue to hard code user/x and node/x paths, and we don't do david's patch, then we leave url_callback impotent in entity API.
Why is it impotent? It specifies the uri of the entity, so a module having $entity_type + $entity can determine the URL. The existence of an URI callback doesn't mean we have to support altering it - altering lots of other stuff like the 'base table' would break things too.

We could absolutely support custom menu links and others in a separate contrib module (something like global redirect) by using hook_url_outbound_alter() to rewrite those specific links to their proper entity paths.

I'm with fago here, I don't even see a real problem here. entity_uri() is pretty useful already for people dealing with entities generically, and far away from "impotent". It doesn't say anywhere that it has to support fancy URI alterations that someone might want to do without properly thinking about it.

OK, adding entity_l() is practical, calling the function entity_url_arguments() instead of entity_url() can prevent confusions (even if we have to leave the "deprecated" version in there) and using entity_url($node) instead of 'node/' . $node->nid helps both clarity and consistency – but I neither see anything critical here, nor a bug.

Everyone seems to agree that David_Rothstein's solution (in #79) is the way to go. I've talked to David Strauss, and he is ok with me "stealing" this. I will reroll and do my best to fix the code documentation that is still @todo.

Should we do some benchmarks of this before going any further with writing documentation?

BTW, regarding some of the more recent comments, again, there is definitely a bug here (even if you think nothing else is, the Forum module aspect is certainly a core bug...)

Using hook_url_outbound_alter() was discussed as early as the first post in this issue, so maybe we are going in circles a bit:

Why not have entity uri manipulation done via hook_url_outbound_alter()? In Drupal 7, we can't do it, because of performance constraints: every hook_url_outbound_alter() runs for every call to url().

I'm not sure where the benchmarks are for that, though, but in general, because url() is called so often, even calling a no-op function from within it will add some overhead. We could, of course, just say that's too bad - if your module wants to alter entity URIs and have it work reliably, then you also need to implement hook_url_outbound_alter() and deal with the performance and infinite recursion possibilities yourself. That might not be totally unreasonable to say.... but above @catch described a potential application of being able to use the entity URI system as a substitute for path aliases while avoiding any hook_url_outbound_alter() implementations, and that would not be possible with this approach.

If we want to enable this to occur in a highly-performing way, we probably do need to do it within the core system. Whether that is a priority or not, I leave for others to decide :)

BTW, it looks like there is already some code for the Forum module (with the hook_url_outbound_alter implementation) here: #712076: Infinite loop possibility and performance problems in forum_url_outbound_alter(). I hadn't seen that before, and didn't realize that what my above patch is basically doing is repeating/generalizing that code :)

There were requests here at the CPH code sprint to do some performance profiling on the patch in #79. This was run on my 2.53 Ghz Mac Book Pro with 4 GB of RAM (not in a VM). I used devel to generate 1,000 nodes. I then created a node with a PHP filter that looped and created a link to all 1000 nodes. All test were run using the latest dev release of Drupal 7.

Results before patch:
http://pastie.org/1120444

Requests per second:    11.19 [#/sec] (mean)
Time per request:       446.665 [ms] (mean)
Time per request:       89.333 [ms] (mean, across all concurrent requests)
Transfer rate:          550.43 [Kbytes/sec] received

Results after the patch in #79:
http://pastie.org/1120449

Requests per second:    13.20 [#/sec] (mean)
Time per request:       378.929 [ms] (mean)
Time per request:       75.786 [ms] (mean, across all concurrent requests)
Transfer rate:          648.83 [Kbytes/sec] received

@pifantastic I think we have a third case in #34 (rerolled in #49). Would you mind test that to, with your setup?

Here's a quick summary of my local benchmarks:

• Patch in #79 appears slightly slower (by around 1.5% per call to the l() function)
• Patch in #79 is efficient - uses 15% - 30% fewer resources than HEAD
• Patch in #79 makes fewer DB queries (32 instead of 36, for a php-page with 1000 iterations of l()).
• pifantastic's benchmarks in #90 show improved performance using the patch.

Conclusion: Patch in #79 looks great!

Benchmarking Data

• Using ab against a page with the php filter linking to 1000 nodes
  • HEAD: 7.69 req/sec
  • With patch (#79): 7.32 req/sec
• CLI: 1000 iteration which calls l() directly
• HEAD: 0.0717039108276 seconds
• With patch (#79): 0.0728569030762

CLI script

#!/usr/bin/env php
<?php
$_SERVER['REMOTE_ADDR'] = '127.0.0.1';  // supress notice and PDO exception

define('DRUPAL_ROOT', getcwd());

require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);

$start=microtime(true);

for($i = 2; $i<=1001; $i++) {
  $a = l("Test URL $i", 'node/' . $i);
}

$stop = microtime(true);

echo "Elapsed time: " . ($stop - $start) . PHP_EOL;

AB results

Without patch

 ab -n 100 -c 5 -t 60 http://benchmarks.d7core.local/node/1002
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking benchmarks.d7core.local (be patient)
Finished 462 requests


Server Software:        Apache/2.2.12
Server Hostname:        benchmarks.d7core.local
Server Port:            80

Document Path:          /node/1002
Document Length:        53032 bytes

Concurrency Level:      5
Time taken for tests:   60.057 seconds
Complete requests:      462
Failed requests:        0
Write errors:           0
Total transferred:      24729012 bytes
HTML transferred:       24500784 bytes
Requests per second:    7.69 [#/sec] (mean)
Time per request:       649.971 [ms] (mean)
Time per request:       129.994 [ms] (mean, across all concurrent requests)
Transfer rate:          402.11 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    1   1.6      0      10
Processing:   577  647  34.4    643     900
Waiting:      516  598  32.2    596     829
Total:        577  648  34.4    644     900

Percentage of the requests served within a certain time (ms)
  50%    644
  66%    655
  75%    662
  80%    666
  90%    680
  95%    693
  98%    746
  99%    797
 100%    900 (longest request)

With patch (#79)

ab -n 100 -c 5 -t 60 http://benchmarks.d7corepatched.local/node/1002
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking benchmarks.d7corepatched.local (be patient)
Finished 440 requests


Server Software:        Apache/2.2.12
Server Hostname:        benchmarks.d7corepatched.local
Server Port:            80

Document Path:          /node/1002
Document Length:        53151 bytes

Concurrency Level:      5
Time taken for tests:   60.134 seconds
Complete requests:      440
Failed requests:        0
Write errors:           0
Total transferred:      23657445 bytes
HTML transferred:       23439591 bytes
Requests per second:    7.32 [#/sec] (mean)
Time per request:       683.338 [ms] (mean)
Time per request:       136.668 [ms] (mean, across all concurrent requests)
Transfer rate:          384.19 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    1   1.4      0      10
Processing:   600  678  62.6    671    1187
Waiting:      533  626  60.2    619    1129
Total:        600  679  62.6    671    1187

Percentage of the requests served within a certain time (ms)
  50%    671
  66%    681
  75%    689
  80%    692
  90%    706
  95%    732
  98%    859
  99%   1130
 100%   1187 (longest request)

Xhprof results

So, I'm a bit of a benchmark n00b. Can someone post a summary of what that means in terms of which patch is faster?

IMHO it looks like the patch from #79 uses slightly less resources than HEAD is currently doing (manarth told me how to read the results). Isn't it?

But either me, manarth or pifantastic was able to apply #34. So if we still think about going down that way, we need to reroll that patch and performance test that.

Meanwhile, here is #79 rerolled with fixed @todo comments (no code changes). But with this, I'm not saying this is the way to go, though. Someone how truly can understand the test results will have to decide which way to go...

Edit:

There is one @todo left that someone smarter than me must figure out how to code:

+        // @todo This code does not support the edge case of multiple levels
+        // of alteration; e.g., if a module implements hook_entity_info_alter()
+        // to further rewrite the Forum module's URLs, then calls such as
+        // url('taxonomy/term/2') will properly be translated to the final
+        // value, but direct calls to url('forum/2') won't. To support that, we
+        // would need to replace the drupal_alter() call above with an explicit
+        // loop, and run the code here each time through the loop.
+        _entity_info_extract_uri_callbacks($original_entity_info[$name]);
+        if (!empty($original_entity_info[$name])) {
+          _entity_info_extract_uri_callbacks($info);
+          if ($info != $original_entity_info[$name]) {
+            foreach (_entity_uri_collect_root_paths($name, $original_entity_info[$name]) as $root_path) {
+              $entity_altered_root_path_list[$root_path] = $name;
+            }
+          }
+        }

I've done something similar to manarth. I've created 1000 nodes with devel_generate and added a module which called url for each of those 1000 nodes in hook_init().

I do not see much of a performance difference with on the url calls.

My results from apache bench were very close together, well within one standard deviation. The relative difference was ~1%, with the patched version being faster. The AB tests were run before enabling xhprof.

The time result from xhprof had some variability from one refresh to the other, although, without doing any statistics on them, the results were typically ~10ms slower without the patch. The memory usage was actually slightly lower without the patch, although << 1%. There were fewer function calls with the patch.

This was run on an intel i7 with 8gb of ram.
Server version: Apache/2.2.14 (Ubuntu)
Server built: Apr 13 2010 20:21:26
APC enabled, php 5.2.
Drupal 7-dev latest from cvs.

bench module:
/**
* Implementation of hook_init().
*/
function bench_init() {
global $user;
if (isset($user->uid) && ($user->uid !=0)) {
return;
}
$query = "SELECT nid FROM node WHERE status = 1";
$result = db_query($query);
foreach($result as $row) {
url('node/'. $row->nid);
}
}

Subscribing, because I don't even understand what this is supposed to do at this point. :-)

My thoughts: For a low-level utility like url() and path aliasing to have intimate knowledge of a high-level system like entities indicates a major and serious design flaw in the system. Period. This is yet another circular dependency that is going to bite us sooner or later, and makes enhancing Drupal in the future even harder because the logic for a given system is scattered throughout the guts of a dozen other systems. We've already seen how the tight binding between system.module and theme.inc breaks horribly. That is bad design by every possible metric.

It may well be the case that, given Drupal 7 and that we're a year past code freeze we can't do anything else at this point. I don't know. However, if we do go with the route of "create a circular dependency between path aliases and entities because the system is too brittle to allow us to do anything else" we *must* document that fact in the code; admit that we're doing something horribly bad and suggest that we fix it in Drupal 8 when we can revisit the underlying architecture, which I believe this thread has demonstrated is insufficient.

I also don't understand why #94 is moving from entity_uri() to url() in the rdf.module block. I thought the idea was to always use entity_uri()?

Pretty much agreed with Crell, especially that if it goes in it needs a comment to remove later, couple of extra points though:

1. David Strauss in #77 said "the path system already has deep knowledge of the entity system", I don't agree with this. path.inc has a load of places which mention node module, but these all boil down to examples in comments and variable_get('site_frontpage', 'node'), this is Drupal 4/5/6 code, not the entity system as such. It's also a lot easier to clean up that dependency than the one we'd be adding here, in fact there's a patch for that in #375397: Make Node module optional. menu.inc has things which default to 'node' as well, but that's also in the 'make node optional' patch. path.module has code for nodes, taxonomy and user ui, but as the optional module compared to node or user that's the right place for it given the other options at the moment.

2. I don't get the benchmark results in #92 - could we get an xhprof or webgrind screenshot to show what those 4,000 function calls that were saved were?

subscribing and FWIW at this point perhaps it is sensible to consider whether this is all worth the hassle.

What are the clear arguments for fixing this AND calling it a critical issue:

1. Is it the lack of consistency? As David Strauss is it about accommodating theoretical usage scenarios? Well, that is not really fixed and it is too late to get things consistent.
2. Expected behaviour (i.e. having uri_callback work always as expect): well that may be fixed but at the cost of a worse system overall as Crell says in #99.

It seems to me that the way forward with this is what fago suggests:

1. Make best practice clear
2. Indicate the gotchas with nodes and comments in relation to the uri_callback.

Module developers that need to mess with these things can adapt, they are probably doing it in other situations as well.

Beside the complicated code in common.inc it is not really nice to see modules that are clean like the RDF module go back to using hard-coded info.

Hm, there seems to be a lot of questions about the RDF module changes :) Please see #57 for an explanation. We need them for benchmarking. But we also might wind up keeping them, because all else being equal (i.e., all performance and functionality questions aside), it is cleaner and more straightforward to use url('node/' . $node->nid) rather than entity_uri(); we can reserve a powerful tool like entity_uri() for places where the generality is actually needed (e.g., image_field_formatter_view(), which does not know what kind of entity it is working with). Anyway, that can be decided later.

Where are we with this issue? It looks to me like the above benchmarks were a good start, but AFAICT they did not test with either forum.module or the "entity_uri_test" module enabled (can someone confirm)? If so, they are a useful start (and if they indicate a performance improvement over current D7 HEAD code then it probably shows that entity_uri itself has some overhead), but I think we need to benchmark more, because most of the actual code isn't being tested without enabling one of the above two modules and testing some pages with relevant URLs on them.

Seems like the most useful thing would be to reroll the two patches, and possibly even a third (which would be identical to #94 but with the code moved to hook_url_outbound_alter in forum.module and entity_uri_test.module). The latter would test the idea of leaving some of this to contrib; we're told that has a performance hit but it might be worth checking how slow it actually is. Of course, having the identical code living in contrib rather than core doesn't do anything to solve the actual coupling issue, just sweeps the problem under the rug... but still :)

I probably don't have time to work on this for a bit. @effulgentsia, you said in #98 that you had an alternative idea you wanted to try. Any more thoughts on that?

sub

To respond to the performance hit (comments #106 #85) in contrib if we rewrite links conditionally: In the Domain Access world, we suffer that penalty in D6, because we have to run custom_url_rewrite_outbound() and determine if $path is a node path. menu_get_object() is one option for solving that problem. Much better is being passed entity information.

I believe we will see substantial improvement due to #839520: entity_uri should pass the entity data to url(), which is already in, since it tells us that a url path refers to a node and we don't have to try to discover whether or not a path points to a node, which is where most of the performance penalty is. See http://drupal.org/node/839520#comment-3145282 for the logic on that.

Can contrib live without this patch for performance reasons? Sure, then we're in the same boat as D6.

What we really need is consistency.

I really, really prefer the older patches which had entity_l() etc. and updated the rest of core, adding this to url() makes me sad. However it makes me less sad than the code that was in forum_url_inbound_alter() before it was ripped out in favour of this issue or one of its predecessors, and it's a smaller change to make this close to a beta.

Removing this from url() should be one of the earlier patches to get into D8, (and also we should move all of the entity_*() functions in common.inc to entity.inc unless there's a really good reason not to).

As written in #85, I still think this magic inside of url() is unnecessary + we don't need to force people to use entity_l() neither.

I'd suggest to leave the decision of allowing url alterations to the entity type. Following that we should be able to solve this issue by

• introducing entity_l() / entity_url() as helper function, but keeping it optional
• introducing taxonomy_url() or re-introduce taxonomy_term_path() + make it use the entity uri callback specified in hook_entity_info() + document in its function doc-block that modules may alter the path via altering hook_entity_info()
• fix the forum module to use this alteration possibility
• leave the rest of the system as it is, long-true assumptions like "node/$nid" can stay.

-> Problem solved efficiently + no drastic API changes late in the cycle + no ugly, difficult to understand code in url().

Here's an approach that, with some tweaks, may work by auto-loading entity data before calling the alter hook.

@agentrickard I think you got some of your node access magic into your latest patch. I'm not sure this is relevant:

+++ modules/node/node.module	1 Oct 2010 14:15:47 -0000
@@ -3156,8 +3156,8 @@ function node_access_acquire_grants($nod
-  // If no grants are set, then use the default grant.
-  if (empty($grants)) {
+  // If no grants are set and the node is published, then use the default grant.
+  if (empty($grants) && !empty($node->status)) {
     $grants[] = array('realm' => 'all', 'gid' => 0, 'grant_view' => 1, 'grant_update' => 0, 'grant_delete' => 0);
   }

So here is a re-roll from Ken's latest patch.

Powered by Dreditor.

I don't understand what benefit that would have, and it seems like it would have a cost to hit menu_get_item() for every call to url() and then entity_get_info() for every loader function regardless of whether it's an entity or not? Even functions that are statically cached have a performance hit if they are called that many times as would be the case from inside url().

+  static $rewrite;
+  // Check for url rewrites, which can affect performance.
+  if (!isset($rewrite)) {
+    $rewrite = (bool) count(module_implements('url_outbound_alter'));
+  }

This kind of thing was tried before in #619566: Don't invoke hook_url_outbound_alter() if there's nothing to invoke and then rolled back there, so I don't think we want to add it again.

Really seems like the number one thing we need in this issue right now is benchmarks? We already have tons and tons of permutations of patches to test :)

#110: 823428-entity_uri-110.patch queued for re-testing.

Moving this to D8 for now, would be nice to be able to backport it but not feeling very optimistic at this point.

Moving to a task.

Tagging issues not yet using summary template.

subscribe

Just rerolling.

(removed, apologies for the noise)

This doesn't seem like a major at this point. Please forgive and correct me if I'm wrong.

All what we really would need is to be able to replace the generated route and choose another pattern. I cannot think of a more advanced usecase.

In D8, this is fixed by Drupal\Core\PathProcessor\OutboundPathProcessorInterface::processOutbound(), which passes both the Request object and the entity (as part of $options).

Closing.

This is not fixed, see #2010132-49: Canonical taxonomy term link for forum vocabulary is broken