domain_path currently loads aliases for all nodes and domains once and caches the results. I believe we should be loading only the paths that are necessary for the individual page load and store accordingly . To do this I have created a patch that replaces domain_path_paths with domain_path_lookup_path which mimics drupal_lookup_path.

This patch also will handle loading the language specific paths when necessary.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

grndlvl’s picture

If accepted we should probably call this 7.x-2.x as it changes around the core of domain_path.

agentrickard’s picture

I dunno about that, we only have a beta release, so changing the inner workings shouldn't be an issue.

agentrickard’s picture

Status: Needs review » Needs work

It doesn't seem to be working for me against 7.x.3-dev. To test:

* Enable the Domain Switcher block.
* Edit a node
* Give the node a different path for some domains.
* Make the node visible on 'all affiliates'
* Save the node.
* From the node view page, check the links in the Domain Switcher block

Those links should be aliased properly, but are not.

agentrickard’s picture

Reverting the patch and the links work as expected.

grndlvl’s picture

Changed out domain_path_domain_path() to use the original_path when looking up an alias && return Drupal alias if we don't have one as it did before.

/**
 * Implements hook_domainpath().
 */
function domain_path_domain_path($domain_id, &$path, &$options, $original_path) {
  $path_language = isset($options['language']->language) ? $options['language']->language : NULL;
  if ($alias = domain_path_lookup_path('alias', $original_path, $domain_id, $path_language)) {
    $path = $alias;
    $options['alias'] = TRUE;
  }
  elseif($alias = drupal_get_path_alias($original_path)) {
    $path = $alias;
    $options['alias'] = TRUE;
  }
}
grndlvl’s picture

Status: Needs work » Needs review
grndlvl’s picture

Forgot about the pathlanguage for the call to drupal_get_path_alias.

grndlvl’s picture

Had a notice on node/add for new nodes.

agentrickard’s picture

Seems to be working. This is likely the best approach, though I suspect a load test would be nice.

I did add one bit, to skip the lookup if $options['absolute'] = TRUE, because if that's set, then we are dealing with an external link that doesn't need a path lookup.

Also standardized the comments.

agentrickard’s picture

I also wonder if we can cluster these queries by the source, so that we don't have to run an individual query for each domain.

grndlvl’s picture

Maybe, but in most cases I would not think that an alias would be loaded for multiple domains on a page so it has the potential to load unnecessary data. With the domain switcher module && admin pages it will query the same path multiple times to get for each domain's alias and on node/edit but in other cases it should only be looking @ one domain -- right?

- edit
Thinking about this more it really would not be much more data assuming that only a subset of the domain paths are set.

agentrickard’s picture

Let's move forward without that premature optimization, then.

agentrickard’s picture

You now have commit access.

grndlvl’s picture

Looking @ this further we should include the whitelist mechanism from drupal_lookup_path to account for system paths we should not care about.

agentrickard’s picture

Agreed. Should we commit this first and add that as a follow-up? I'm a big fan of incremental improvements.

grndlvl’s picture

Status: Needs review » Fixed

Committed in fdf786a

agentrickard’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Fixed » Patch (to be ported)

Should probably be backported to d6, if anyone cares.

i-trokhanenko’s picture

Issue summary: View changes
Status: Patch (to be ported) » Closed (outdated)

Closing a Drupal 6 issue. It's time. Thanks!