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

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

Status:Needs review» Fixed

Committed to CVS HEAD. Thanks.

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

Direct backport!

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

@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

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

Status:Needs work» Needs review

Changing status... missed that in previous update.

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

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.

Priority:Normal» Critical

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

@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

@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

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

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

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.

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.'));

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

Hunks with translation removed

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

More clean test that fail without patching path.inc

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

+++ 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.

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.

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

Previous patch was wrong

That looks good to me.

Thank you.
Alexis

Status:Needs review» Reviewed & tested by the community

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

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?

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

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.

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

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

@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);
?>

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

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

Suppose patch in #8 is enough for D6

@AlexisWilke could you review D6 patch?

@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

Status:Needs review» Reviewed & tested by the community

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.

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

Status:Reviewed & tested by the community» Fixed

Ok, well, committed.

Status:Fixed» Closed (fixed)

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

Thank you Gábor. 8-)