Infinite loop possibility and performance problems in forum_url_outbound_alter()

Agreed, that's a performance disaster: turn on the forum module and every single call to url() will take about 50% longer.

EDIT: just a reminder to remove the temporary infinite loop prevention code in attachLoad (see @todo) added in #699440: Add bundle support to entity_uri callback to remove performance overhead of forum_url_outbound_alter() (regression from D6).

subscribe.

I agree that we should use entity_uri() to avoid having a forum_url_outbound_alter() function at all, and this patch includes an @todo to that effect. But seems like there's a chicken and egg problem. We need this function fixed to make progress on #699440: Add bundle support to entity_uri callback to remove performance overhead of forum_url_outbound_alter() (regression from D6)? So, here's a better implementation, but still temporary, and once it's in, should we just make the replacement of forum_url_outbound_alter() with a forum entity uri callback part of #699440: Add bundle support to entity_uri callback to remove performance overhead of forum_url_outbound_alter() (regression from D6)?

x-post.

I removed the @todo about converting from a hook_url_outbound_alter() implementation to a uri callback, because looking deeper into the entity_uri() function and #699440: Add bundle support to entity_uri callback to remove performance overhead of forum_url_outbound_alter() (regression from D6), I don't see how to do that. The entity type is a 'taxonomy_term', and the system doesn't seem to yet support different uri callbacks based on refinements of that (e.g., based on the vocabulary or some other property of the entity). I'm certainly open to ideas for making the entity system capable of handling those kinds of refinements or to building more routing control into taxonomy_term_uri(), and if that were done, I would definitely support such a system over such a crude use of hook_url_outbound_alter(). But while I think such a thing would make sense, I don't think it solves the core performance problem, because entity_uri() still requires a fully loaded entity, so it just moves the costly taxonomy_term_load() call to somewhere else in the pipeline, but doesn't remove the need for it.

The issue as I see it is how to output paths/uris/urls for an entity when you know the id, but need to know some other simple property, but don't need a full entity load. Seems like a statically cached prepared database statement that could be quickly re-run for a new id could do the trick, but how to integrate that properly into the entity system? Would love some consulting from catch, Crell and/or yched on this. Seems like such a system would work well either when called from a hook_url_outbound_alter() implementation or from whatever code calls entity_uri().

I added a @todo about this, but this patch otherwise fixes the recursion problem, and is faster at detecting when called for a non-taxonomy-term path.

Yes I think we need to have entity_uri() support different callbacks for different bundles of entites. Forum terms would have their own callback.

Hm. Thinking a little more about this...

I was assuming a use-case of something like:

for ($i=1; $i<500; $i++) {
  $output .= l('term ' . $i, 'taxonomy/term/' . $i);
}

Does it make sense in the above code to have to do a full load of each term, which would be needed to call entity_uri()?

But, maybe that's not a good use-case to worry about. Because usually, in practice, we only make links to things that we do have loaded, since we usually want the text of the link to be something like the entity's title. If we think that's the case, then calling entity_uri() isn't so bad, since we already have the loaded entity, and would certainly be faster than having to do a reload from within hook_url_outbound_alter().

Yes I think we need to have entity_uri() support different callbacks for different bundles of entites. Forum terms would have their own callback.

Oh, forum is already set up as a entity bundle. Yes, this would be a simple change and definitely makes sense.

+++ modules/forum/forum.module	20 Feb 2010 17:22:46 -0000
@@ -688,10 +688,27 @@ function forum_form($node, $form_state) 
+  if (substr($path, 0, 14) == 'taxonomy/term/') {

Isn't strpos() way faster than substr()?

143 critical left. Go review some!

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

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

Eeek. This is taking 6 or so lines of readable code and turning them into a 25+ line fire breathing monster. I think I would prefer to fix this in #699440: Add bundle support to entity_uri callback to remove performance overhead of forum_url_outbound_alter() (regression from D6) one way, for all entities, and mark this one won't fix.

What say you?

Agreed that #699440: Add bundle support to entity_uri callback to remove performance overhead of forum_url_outbound_alter() (regression from D6) is where the fix belongs. If that issue is fixed correctly, there won't be a forum_url_outbound_alter() any more. This was an intermediate "improvement", but since the other issue is now "critical", we don't need an intermediate improvement.

There appears to be agreement that at a minimum, the scope of #699440: Add bundle support to entity_uri callback to remove performance overhead of forum_url_outbound_alter() (regression from D6) includes removing forum_url_outbound_alter() entirely.