drupal_lookup_path() documentation and return mismatch

Ooops... Should be "needs review".

Also, as a side note, that code is the same in D6. So it will be nice to back port.

Thank you.
Alexis

Hi guys,

There is actually a problem here... The $source ($src in D6) is set inside the if () statement. (I definitively do not like people doing that!)

In other words, the simplification is wrong. The following would work slightly better! I think that leaving the code this way is quite cryptic though.

      $source = '';
      // $source is set in this if () and must be returned if not empty
      if (!isset($cache['map'][$path_language]) || !($source = array_search($path, $cache['map'][$path_lan
        // Get the most fitting result falling back with alias without language
        if ($source = db_query("SELECT source FROM {url_alias} WHERE alias = :alias AND language IN (:lang..."
                     ':alias' => $path,
                     ':language' => $path_language,
                     ':language_none' => LANGUAGE_NONE))
            ->fetchField()) {
          $cache['map'][$path_language][$source] = $path;
        }
        else {
          // We can't record anything into $map because we do not have a valid
          // index and there is no need because we have not learned anything
          // about any Drupal path. Thus cache to $no_source.
          $cache['no_source'][$path_language][$path] = TRUE;
        }
      }
      return $source ? $source : FALSE;

Sorry for the trouble.

Btw, that means whoever wrote the test is not currently testing whether the function properly returns 'source' when called multiple times.

Thank you.
Alexis

Yes. #4 is like the original with a truthful return (i.e. FALSE instead of '').

There are two patches attached (D6 & D7).

In regard to D7, I have a question in the 'wipe' if (). It resets the whitelist to drupal_path_alias_whitelist_rebuild(); instead of NULL. Is that really intended?

  // Retrieve the path alias whitelist.
  if (!isset($cache['whitelist'])) {
    $cache['whitelist'] = variable_get('path_alias_whitelist', NULL);
    if (!isset($cache['whitelist'])) {
      $cache['whitelist'] = drupal_path_alias_whitelist_rebuild();
    }
  }

  $path_language = $path_language ? $path_language : $language_content->language;

  if ($action == 'wipe') {
    $cache = array();
    $cache['whitelist'] = drupal_path_alias_whitelist_rebuild();
  }

Thank you
Alexis Wilke

Changing status... missed that in previous update.

@andypost,

"why you so dislike current approach?"

Setting a variable in an if() statement is not a good idea. That's all.

But it is a good thing that the result of this is a better test for the function.

One question, do we really need translations in tests?!?

Thank you.
Alexis

andypost,

Do you have a similar (cache) test for the other side (i.e. alias)?

It would be good to have a test for both. At this point you added the test for the 'source' only. (I did not look at the entire test, just wondering...)

Thank you.
Alexis

That looks good to me.

Thank you.
Alexis

andypost,

One question... What's the difference between calling drupal_lookup_path() with the 'wipe' action and drupal_clear_path_cache()?

Is the former a left over from older versions? If so, wouldn't that make more sense:

function drupal_lookup_path($action, $path = '', $path_language = NULL) {
  // 'wipe' is deprecated, do not use! call drupal_clear_path_cache(); instead
  if ($action == 'wipe') {
    drupal_clear_path_cache();
    return;
  }
  ...
  // also remove the if ($action == 'wipe') ... below

Of course, you could just remove it altogether too.

Thank you.
Alexis

@andypost,

I suppose you mean the one in #8.

That looks good to me. It will return the right value (i.e. FALSE) when no alias is available. This works because the array_search() function also returns FALSE when it cannot find an item.

Thank you.
Alexis

Thank you Gábor. 8-)