Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
[...]
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
Comment | File | Size | Author |
---|---|---|---|
#28 | 829968-path-fix-d7.patch | 3.05 KB | andypost |
#26 | 829968-path-fix-d7.patch | 3.08 KB | andypost |
#21 | 829968-path-fix-d7.patch | 3.57 KB | andypost |
#20 | 829968-path-fix-d7.patch | 3.57 KB | andypost |
#17 | 829968-path-fix-d7.patch | 2.98 KB | andypost |
Comments
Comment #1
AlexisWilke CreditAttribution: AlexisWilke commentedOoops... 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
Comment #2
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #3
andypostDirect backport!
Comment #4
AlexisWilke CreditAttribution: AlexisWilke commentedHi 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.
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
Comment #5
andypost@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
Comment #6
AlexisWilke CreditAttribution: AlexisWilke commentedYes. #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?
Thank you
Alexis Wilke
Comment #7
AlexisWilke CreditAttribution: AlexisWilke commentedChanging status... missed that in previous update.
Comment #8
andypost@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
$src = array_search($path, $map[$path_language])
would return FALSE or actual value so we need to fix only $src = '' to $src= FALSE
Comment #9
andypostAdded 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.
Comment #10
andypostThink this critical because second and follow-up calls to drupal_get_normal_path() would return FALSE for alias
Comment #11
AlexisWilke CreditAttribution: AlexisWilke commented@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
Comment #12
andypost@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
Comment #13
andypostAbout 'wipe' - suppose we should setup 'first_call' to TRUE
Comment #14
dhthwy CreditAttribution: dhthwy commentedSetting $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.
Comment #15
Dave ReidChanges in test file are re-adding t() when it should be plain strings.
Comment #16
andypostHunks with translation removed
Comment #17
andypostMore clean test that fail without patching path.inc
Comment #18
AlexisWilke CreditAttribution: AlexisWilke commentedandypost,
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
Comment #19
Dries CreditAttribution: Dries commentedSomething reads wrong with this sentence -- at least to me.
Comment #20
andypostAdded similar test for alias and changed code comments.
Comment #21
andypostPrevious patch was wrong
Comment #22
AlexisWilke CreditAttribution: AlexisWilke commentedThat looks good to me.
Thank you.
Alexis
Comment #23
moshe weitzman CreditAttribution: moshe weitzman commentedok, lets call this rtbc then. code looks good to me.
Comment #24
Dries CreditAttribution: Dries commentedSetting 'first_call' to
TRUE
is odd, or at least inconsistent, because we do:Or am I reading that wrong?
Comment #25
dhthwy CreditAttribution: dhthwy commentedempty() 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
Comment #26
andypostRe-roll without
$cache['first_call']
hunk and fixed offset for test after #855380: $language_url should be used to lookup the current path aliasComment #28
andypostUsing $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
Comment #29
AlexisWilke CreditAttribution: AlexisWilke commentedandypost,
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:
Of course, you could just remove it altogether too.
Thank you.
Alexis
Comment #30
andypost@AlexisWilke, there's a small difference - if you already in drupal_lookup_path() so
One function call is less expensive against 3 function calls!
Comment #31
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #32
andypostSuppose patch in #8 is enough for D6
Comment #33
andypost@AlexisWilke could you review D6 patch?
Comment #34
AlexisWilke CreditAttribution: AlexisWilke commented@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
Comment #35
AlexisWilke CreditAttribution: AlexisWilke commentedComment #36
Gábor HojtsyI'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.
Comment #37
andypostGabor,
$src = db_result(...
already returns FALSE if nothing found so this is a just a fix initial valueComment #38
Gábor HojtsyOk, well, committed.
Comment #40
AlexisWilke CreditAttribution: AlexisWilke commentedThank you Gábor. 8-)