This is a performance improvement (and corrects an API oversight) for modules that use hook_url_outbound_alter().

Since altering the path is generally done based on the entity, these functions have to re-load the entity. If we attach it to the $options array sent to url(), then we should see performance improvements for that alter hook.

Dave Reid and I discussed this and don't consider this an API change so much as a more correct implementation of the intent of entity_uri().

CommentFileSizeAuthor
#1 839520-entity-uri.patch1.42 KBagentrickard

Comments

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Active » Needs review
StatusFileSize
new 839520-entity-uri.patch1.42 KB

And a patch.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Issue tags: +API change

Tagging.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -API change

This is clearly an oversight in entity_uri that will also make modules that want to rewrite specific entity uris. This is more specific that changing the uri callback of the entity/bundle itself.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Issue tags: +API change
moshe weitzman’s picture

moshe weitzman commented

+1 from me as well. a lesser version of this was requested in #823428-8: Impossible to alter node URLs efficiently

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

@moshe - I deliberately spun this off from that issue.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

effulgentsia’s picture

effulgentsia commented

Awesome! I'm glad this went in, and there may be some great use-cases for it. Note, however, that it only works when entity_uri() is called, so if you want core doing so reliably, please share that opinion on #823428: Impossible to alter node URLs efficiently. Also note, however, that every implementation of hook_url_outbound_alter() slows down every call to url(), because in PHP, stack calls aren't free, even if the function called exits early. entity_uri() deliberately supports per-entity overriding during entity load via the $entity->uri property: see code comments within entity_uri(). Doing so during entity load also means it's cacheable if you use http://drupal.org/project/entitycache.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

I see that now and wonder if it could be documented better that hook_entity_info_alter() can (and should) be used in place of custom_url_alter() in many cases (like mine).

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

Please summarize the impact of this API change and I'll send out a note.

Thanks,
-Randy

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Well, from just this change, the impact is that entity information is now passed to url(), which means that implementations of hook_url_outbound_alter() will be passed the entity type and data, so that they should not need to re-load the entity before processing the url() request.

In practice, it means that instead of checking the URL syntax to see it you need to alter it, you just check the type. E.g.

function example_url_outbound_alter(&$path, &$options, $original_path) {
  // Huge performance and code clarity win here:
  if (!isset($options['entity_type'] || $options['entity_type'] != 'node') {
    return;
  }

  // Make node links absolute.
  $options['absolute'] = TRUE;
}

However, to get the full benefit of this, #823428: Impossible to alter node URLs efficiently has to go in. And doing that means we can use hook_entity_info_alter() instead of hook_url_outbound_alter() in most cases.

moshe weitzman’s picture

moshe weitzman commented

@rfay - there is no api change here. no announcement needed.

effulgentsia’s picture

effulgentsia commented

I'm not sure why this issue has the "API change" tag. I don't think a note needs to go out to the dev list for this one. We will need to update http://drupal.org/node/224333#hook_url_outbound_alter, but as per #11, I think we should only do so after we have resolution on #823428: Impossible to alter node URLs efficiently.

moshe weitzman’s picture

moshe weitzman commented

The word "API change" is pretty ambigous as used in Drupal. If an API enhancement and an interface change (i.e. function arguments, return value) have the same name, then we are hopeless. This issue is an API enhancement.

effulgentsia’s picture

effulgentsia commented
Issue tags: -API change

I added docs requested in #9 to the re-roll in #823428-34: Impossible to alter node URLs efficiently.

Removing the "API change" tag from this issue, as I agree with Moshe that that tag should be reserved for BC breaks. We have an "API addition" tag, though I'm not sure what the utility of it is.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.