Hi guys,

I noticed that the function would return two different values (if checked with ===) when called twice with an unknown alias.

The documentation says that the function will return FALSE if no path is found. Not an empty string. And some people will do $result === FALSE and their code will fail.

* @return
*   Either a Drupal system path, an aliased path, or FALSE if no path was
*   found.

The reason is here, notice that $source is set to an empty string instead of FALSE.

<?php
   
[...]

    elseif (
$action == 'source' && !isset($cache['no_source'][$path_language][$path])) {
     
// Look for the value $path within the cached $map
     
$source = '';
      if (!isset(
$cache['map'][$path_language]) || !($source = array_search($path, $cache['map'][$path_lan
guage
]))) {
       
// 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
uage, :language_none) ORDER BY language DESC, pid DESC"
, array(
                    
':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;
    }

    [...]
?>

Although the test can be optimized as shown in the attached patch.

Thank you.
Alexis Wilke

Files: 
CommentFileSizeAuthor
#28 829968-path-fix-d7.patch3.05 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,477 pass(es).
[ View ]
#26 829968-path-fix-d7.patch3.08 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 22,458 pass(es), 4 fail(s), and 4 exception(es).
[ View ]
#21 829968-path-fix-d7.patch3.57 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,147 pass(es).
[ View ]
#20 829968-path-fix-d7.patch3.57 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 22,145 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#17 829968-path-fix-d7.patch2.98 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,143 pass(es).
[ View ]
#16 829968-path-fix-d7.patch2.75 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,150 pass(es).
[ View ]
#13 829968-path-fix-d7.patch3.71 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,144 pass(es).
[ View ]
#9 829968-path-fix-d7.patch3.26 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,133 pass(es).
[ View ]
#8 829968-path-fix-d6.patch936 bytesandypost
#8 829968-path-fix-d7.patch1.43 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,138 pass(es).
[ View ]
#6 path-FALSE-return2.6.x.patch1.77 KBAlexisWilke
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path-FALSE-return2.6.x.patch.
[ View ]
#6 path-FALSE-return2-7.x.patch2.39 KBAlexisWilke
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path-FALSE-return2-7.x.patch.
[ View ]
#3 829968-path-FALSE-d6.patch1.23 KBandypost
path-FALSE-return-7.x.patch1.31 KBAlexisWilke
PASSED: [[SimpleTest]]: [MySQL] 20,935 pass(es).
[ View ]

Comments

AlexisWilke’s picture

Assigned:Unassigned» AlexisWilke
Status:Active» Needs review

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

Dries’s picture

Status:Needs review» Fixed

Committed to CVS HEAD. Thanks.

andypost’s picture

Version:7.x-dev» 6.x-dev
Status:Fixed» Needs review
StatusFileSize
new1.23 KB

Direct backport!

AlexisWilke’s picture

Status:Needs review» Needs work

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.

<?php
      $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

andypost’s picture

@AlexisWilke not agreed here, $source is a local variable for internal if() so I see no reason to make it's visibility out of if() scope.

Code in #4 adds additional:
- useless assignment
- useless compare

PS: I think we should fix this in D6 and go fix D7 issues

AlexisWilke’s picture

StatusFileSize
new2.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path-FALSE-return2-7.x.patch.
[ View ]
new1.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path-FALSE-return2.6.x.patch.
[ View ]

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?

<?php
 
// 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

AlexisWilke’s picture

Status:Needs work» Needs review

Changing status... missed that in previous update.

andypost’s picture

Version:6.x-dev» 7.x-dev
Priority:Minor» Critical
StatusFileSize
new1.43 KB
PASSED: [[SimpleTest]]: [MySQL] 22,138 pass(es).
[ View ]
new936 bytes

@AlexisWilke, why you so dislike current approach?

Currently this broken in HEAD because if value already in $map array so result would FALSE!!!

http://php.net/manual/en/function.array-search.php

<?php
 $src
= array_search($path, $map[$path_language])
?>

would return FALSE or actual value so we need to fix only $src = '' to $src= FALSE

andypost’s picture

Priority:Critical» Normal
StatusFileSize
new3.26 KB
PASSED: [[SimpleTest]]: [MySQL] 22,133 pass(es).
[ View ]

Added test which broken before patch.

drupal_lookup_path() returns FALSE for 'source' if there was a call for source already.

I found no ability to clean $cache['map'] to make a test clean. So Test relies on previous url() which fills a cache['map] for node.

andypost’s picture

Priority:Normal» Critical

Think this critical because second and follow-up calls to drupal_get_normal_path() would return FALSE for alias

AlexisWilke’s picture

@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’s picture

@AlexisWilke Messages in tests are always should have translation, so this just a fix while I on this test. There's an issue to remove this #500866: [META] remove t() from assert message

About setting variable in if() - this is a common practice

andypost’s picture

StatusFileSize
new3.71 KB
PASSED: [[SimpleTest]]: [MySQL] 22,144 pass(es).
[ View ]

About 'wipe' - suppose we should setup 'first_call' to TRUE

dhthwy’s picture

Setting $source in that if block is fine because $source is only ever touched when that if block is triggered. That's common practice and how it should be as andypost pointed out.

Dave Reid’s picture

Status:Needs review» Needs work

Changes in test file are re-adding t() when it should be plain strings.

-    $this->assertTrue(($french_node), 'Node found in database.');
+    $this->assertTrue(($french_node), t('Node found in database.'));

+    $this->assertEqual($french_node_path, 'node/' . $french_node->nid, t('Normal path is the same.'));

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new2.75 KB
PASSED: [[SimpleTest]]: [MySQL] 22,150 pass(es).
[ View ]

Hunks with translation removed

andypost’s picture

StatusFileSize
new2.98 KB
PASSED: [[SimpleTest]]: [MySQL] 22,143 pass(es).
[ View ]

More clean test that fail without patching path.inc

AlexisWilke’s picture

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

Dries’s picture

+++ modules/path/path.test 17 Jul 2010 23:49:08 -0000
@@ -295,17 +295,21 @@ class PathLanguageTestCase extends Drupa
+    // Check that source for alias returns from cache.

Something reads wrong with this sentence -- at least to me.

andypost’s picture

StatusFileSize
new3.57 KB
FAILED: [[SimpleTest]]: [MySQL] 22,145 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Added similar test for alias and changed code comments.

andypost’s picture

StatusFileSize
new3.57 KB
PASSED: [[SimpleTest]]: [MySQL] 22,147 pass(es).
[ View ]

Previous patch was wrong

AlexisWilke’s picture

That looks good to me.

Thank you.
Alexis

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

ok, lets call this rtbc then. code looks good to me.

Dries’s picture

Setting 'first_call' to TRUE is odd, or at least inconsistent, because we do:

<?php
     
if (!empty($cache['first_call'])) {
       
$cache['first_call'] = FALSE;
?>

Or am I reading that wrong?

dhthwy’s picture

empty() is ok for checking FALSE, but it does look a little odd using it on a boolean.

doh, nevermind, haven't had my coffee yet =P

andypost’s picture

StatusFileSize
new3.08 KB
FAILED: [[SimpleTest]]: [MySQL] 22,458 pass(es), 4 fail(s), and 4 exception(es).
[ View ]

Re-roll without $cache['first_call'] hunk and fixed offset for test after #855380: $language_url should be used to lookup the current path alias

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 829968-path-fix-d7.patch, failed testing.

andypost’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new3.05 KB
PASSED: [[SimpleTest]]: [MySQL] 22,477 pass(es).
[ View ]

Using $french_alias against $edit['path[alias]'] in testing more sane and self-explained, so back to RTBC

$edit['path[alias]'] been changed in #855380: $language_url should be used to lookup the current path alias

AlexisWilke’s picture

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:

<?php
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’s picture

@AlexisWilke, there's a small difference - if you already in drupal_lookup_path() so

<?php
$cache
= array();
$cache['whitelist'] = drupal_path_alias_whitelist_rebuild();
?>

One function call is less expensive against 3 function calls!

<?php
drupal_clear_path_cache
();
=>
drupal_static_reset('drupal_lookup_path');
drupal_path_alias_whitelist_rebuild($source);
?>
Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

andypost’s picture

Version:7.x-dev» 6.x-dev
Priority:Critical» Normal
Status:Fixed» Needs review

Suppose patch in #8 is enough for D6

andypost’s picture

@AlexisWilke could you review D6 patch?

AlexisWilke’s picture

@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

AlexisWilke’s picture

Status:Needs review» Reviewed & tested by the community
Gábor Hojtsy’s picture

I'm thinking whether its best to fix the docs to the return value instead of the return value according to the docs. Living code might or might not get hurt if we change return values here just to conform to docs.

andypost’s picture

Gabor, $src = db_result(... already returns FALSE if nothing found so this is a just a fix initial value

Gábor Hojtsy’s picture

Status:Reviewed & tested by the community» Fixed

Ok, well, committed.

Status:Fixed» Closed (fixed)

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

AlexisWilke’s picture

Thank you Gábor. 8-)