i have multilaguage setup and allow urls for user to be different on different language
All is great but after user saves his profile sometimes is redirected to wrong page.
The issue is within drupal_lookup_path as in the url alias table i have this records :
user/213 profile/all/myname und
user/213 profile/all/моетоиме bg
so if you
1) open the site in bg language
2) edit profile
3) call url('user/213') in the user_profile_form_submit function you will get profile/all/myname but if you go on any other page then you will get profile/all/моетоиме as a result and that is the correct one
4) call url('user/213') in any other language and you get user/213 profile/all/myname which is the wanted behavior
the issue is with the language order in drupal_get_path_alias in the
if ($cached = cache_get($cid, 'cache_path')) {
...
the language is bg so it is < und
SELECT source, alias FROM {url_alias} WHERE source IN (:system) AND language IN (:language, :language_none) ORDER BY language ASC, pid ASC
which results in :
user/213 profile/all/моетоиме
user/213 profile/all/myname
and because fetchAllKeyed() will take the last one the wrong url is returned.
what should be the correct one is to actually swap the order in those 2 specific cases as it is later on in the same function
current state :
if ($cached = cache_get($cid, 'cache_path')) {
$cache['system_paths'] = $cached->data;
// Now fetch the aliases corresponding to these system paths.
$args = array(
':system' => $cache['system_paths'],
':language' => $path_language,
':language_none' => LANGUAGE_NONE,
);
// Always get the language-specific alias before the language-neutral
// one. For example 'de' is less than 'und' so the order needs to be
// ASC, while 'xx-lolspeak' is more than 'und' so the order needs to
// be DESC. We also order by pid ASC so that fetchAllKeyed() returns
// the most recently created alias for each source. Subsequent queries
// using fetchField() must use pid DESC to have the same effect.
// For performance reasons, the query builder is not used here.
if ($path_language == LANGUAGE_NONE) {
// Prevent PDO from complaining about a token the query doesn't use.
unset($args[':language']);
$result = db_query('SELECT source, alias FROM {url_alias} WHERE source IN (:system) AND language = :language_none ORDER BY pid ASC', $args);
}
elseif ($path_language < LANGUAGE_NONE) {
$result = db_query('SELECT source, alias FROM {url_alias} WHERE source IN (:system) AND language IN (:language, :language_none) ORDER BY language ASC, pid ASC', $args);
}
else {
$result = db_query('SELECT source, alias FROM {url_alias} WHERE source IN (:system) AND language IN (:language, :language_none) ORDER BY language DESC, pid ASC', $args);
}
$cache['map'][$path_language] = $result->fetchAllKeyed();
// Keep a record of paths with no alias to avoid querying twice.
$cache['no_aliases'][$path_language] = array_flip(array_diff_key($cache['system_paths'], array_keys($cache['map'][$path_language])));
}what i think to be correct :
if ($cached = cache_get($cid, 'cache_path')) {
$cache['system_paths'] = $cached->data;
// Now fetch the aliases corresponding to these system paths.
$args = array(
':system' => $cache['system_paths'],
':language' => $path_language,
':language_none' => LANGUAGE_NONE,
);
// Always get the language-specific alias before the language-neutral
// one. For example 'de' is less than 'und' so the order needs to be
// ASC, while 'xx-lolspeak' is more than 'und' so the order needs to
// be DESC. We also order by pid ASC so that fetchAllKeyed() returns
// the most recently created alias for each source. Subsequent queries
// using fetchField() must use pid DESC to have the same effect.
// For performance reasons, the query builder is not used here.
if ($path_language == LANGUAGE_NONE) {
// Prevent PDO from complaining about a token the query doesn't use.
unset($args[':language']);
$result = db_query('SELECT source, alias FROM {url_alias} WHERE source IN (:system) AND language = :language_none ORDER BY pid ASC', $args);
}
elseif ($path_language < LANGUAGE_NONE) {
$result = db_query('SELECT source, alias FROM {url_alias} WHERE source IN (:system) AND language IN (:language, :language_none) ORDER BY language DESC, pid ASC', $args);
}
else {
$result = db_query('SELECT source, alias FROM {url_alias} WHERE source IN (:system) AND language IN (:language, :language_none) ORDER BY language ASC, pid ASC', $args);
}
$cache['map'][$path_language] = $result->fetchAllKeyed();
// Keep a record of paths with no alias to avoid querying twice.
$cache['no_aliases'][$path_language] = array_flip(array_diff_key($cache['system_paths'], array_keys($cache['map'][$path_language])));
}
this way the specific language row with latest pid will be the last row to get as :
1) if $path_language < LANGUAGE_NONE then order should be language DESC, pid ASC
this way you get undefined language on top of results and specific language on bottom and this is what is wanted
2) $path_language > LANGUAGE_NONE then order should be language ASC, pid ASC
this is only for the case where $cache['map'][$path_language] = $result->fetchAllKeyed(); as this will return the last row for required path.
If I am wrong would be nice to tell me what i am doing wrong.
Kind regards,
Dobromir
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | D8-core-fix_for_language_specific_path_alias-2065977-19.patch | 2.24 KB | pushpinderchauhan |
| #1 | core-fix_for_language_specific_path_alias-2065977-1.patch | 2.79 KB | jmuzz |
Comments
Comment #1
jmuzz commentedI found this exact issue and came to the same conclusion.
Shortly after this code you can see that paths which aren't in the systems path list are looked up individually, and fetchField is used to get the first result (the opposite behavior of fetchAllKeyed). To compensate for the different fetch you can see that they switch to pid DESC there... However they use the same logic to determine the order of the results by language, even though they are now picking out the first result instead of the last result. It's not consistant behavior and it's leading to language neutral links getting displayed on a site when there are language specific paths available.
I agree %100 with the changes you made to the code. I would also change the comment to reflect them.
Comment #2
vflirt commentedHI,
glad I am not the only one as I think this is core bug and is major for any multilanguage site. Thanks for the patch.
Kind Regards
Comment #3
rudiedirkx commentedThe ORDER change should be for
pid, not forlanguage. The language change is to make sure UND is last, and specific languages are first. That's A Good Thing (TM). ThepidORDER is dubious: I'd say the newestpidis more important, but since they're all loaded, that's not relevant, I think.Comment #4
vflirt commentedHi,
I am afraid you have totally missed the point. The pid is important only for aliases for the language but not in global aspect. So if for language 'und' the pid is 200 and for 'bg' is 10 when I open my page in 'bg' language i will always want the alias in 'bg' (with id 10) then the one in 'und' (with id 200).
Also making the UND last is very wrong , if you read documentation about fetchAllKeyed method you will see that it will return the last pair from the result .
You want the UND to be first and the language requested to be last so if there is no alias for that language then the one with UND is used.
Comment #5
jmuzz commentedI had to reapply this patch after the security update when my site started showing URL's for LANGUAGE_NONE again when URL's for the current language were available. It's a pretty fundamental bug, I hope it gets applied soon.
It's important for compatibility with entity translation when you want to have different URL's for the same entity depending on what language is being used. Entity translation needs the LANGUAGE_NONE paths for the paths to work at all, but for Drupal to display the correct option the language specific ones must be there as well. Without this patch there's no way to get the whole thing to work.
Comment #6
rudiedirkx commentedSo the ORDER only matters because fetchAllKeyed overwrites previous results... Wow, that's bad. Whether UND or BG/NL/FR is first, doesn't matter, because they're keyed.
What a bad, unreadable, stupid function.
I had a bug because of Entity Translation + this too (translation of menu:url:path token), but this issue didn't solve it, so I made #2153571: drupal_lookup_path() doesn't respect language, persistent cache is... weird. I fear there's more wrong with this function, but most people don't see it, because they don't have translateable path aliases.
Comment #7
jmuzz commentedOh I didn't see that you tried this already, sorry if I closed your issue incorrectly.
Comment #8
rudiedirkx commentedYou're right. This is it. The
fetchAllKeyedis weird, so the ORDER has to be flipped. It should be exactly opposite though, not just language.Comment #9
vflirt commentedHi,
rudiedirkx : only the language order needs to be change , the pid order should always be ASC because you always want the latest entry to be the alias , this is needed for when you are keeping old aliases and not replacing them in order to not have page not found on old urls.
The fetchAllKeyed is not wierd, it is doing exactly what it should be. And the order DOES MATTER because the key is by source not by language and because all of the aliases we need have the same source then we get only 1 result.
There is nothing in the order to be changed other then what is already in the patch. If you think there is work to be done please describe in what cases this patch is breaking functionality or is not fixing what it is said to be fixing.
Kind Regards,
Dobromir
Comment #10
rudiedirkx commentedYes the order matters. Apparently Drupal wanted to load the lowest pid. I think that's wrong too, but I think that's how it was intended. The other order was bugged too though, so maybe not. Doesn't really matter. This will never be fixed in D7.
Comment #11
vflirt commentedHi,
you are wrong, Drupal always want to load the latest pid , the order has always been pid ASC and therefor the latest pid will be on bottom for the corresponding language as the order is by language first.
The order the is wrong is the one for the language and what is changed is just to revert the language order.
Wether this will be fixed or not is not something i can change or take decision about.
Kind Regards,
Dobromir
Comment #12
jmuzz commentedYes pid ASC will cause it to pick the highest pid as it should. fetchAllKeyed will always pick the value that is last in the order for each key, not the first.
Take a look at the code that comes shortly after the patched part. You will see it uses pid DESC and fetchField, which picks the first value found. All of these queries choose the latest pid as a tie breaker when there is more than one alias for a path+language. I don't see any evidence that the intent is to pick the lowest pid.
It might get fixed in Drupal 7 if the issue ever gets to reviewed and tested by the community.
Comment #13
vflirt commentedAs i see no work more to be done I suggest that is ready for a review. Hope some people will do review it.
Kind Regards,
Dobromir
Comment #14
ricovandevin commentedWe have had the same issue and the patch in #1 solved it. I suggest to get the fix in core.
Comment #15
vflirt commentedupdating the issue to 7.26
would love to see this fixed in core
Comment #16
rbayliss commentedThis is still an issue, and the patch in #1 still applies cleanly. I can confirm it fixes the issue.
Comment #17
vflirt commentedComment #18
David_Rothstein commentedThis would need to go in Drupal 8 first (code is in core/lib/Drupal/Core/Path/AliasStorage.php) and seems like the kind of bug that could seriously use some tests to help verify that what is going on here is correct...
Comment #19
pushpinderchauhan commentedD8 patch.
Comment #20
vflirt commentedHi,
@er.pushpinderrana : this is half changed. As you can see in the patch in comment 1 there are 2 queries that need order change not only 1 :) I will mark as needs work cause the of this.
I am not aware of what tests are needed to prove that what is going on there is correct considering that "what was going on here" has been discussed and tested by different people for over and year.
Should there be tests for that function with different records in the url alias table to make sure it is returning the correct alias with different request : Yes of course but I believe that this is not what is intended with this patch as such tests are on more global level then what this patch is actually changing. If however there are no such tests then writing them would be nice :)
Comment #21
vflirt commentedComment #22
tim.plunkettThere is no IQ component for "path subsystem", but its certainly not routing.
Comment #25
pwolanin commentedIs this the same as #2484411: Manual path aliases are not the same as aliases on the node form?
Comment #26
pwolanin commentedComment #30
andrea.pompili commentedExcellent patch. Works well. Thank you.
Comment #32
joseph.olstadComment #35
quietone commenteddrupal_lookup_path was removed in #1269742: Make path lookup code into a pluggable class in Nov 2012.
Anyone know if the bug reported still exists?
Comment #39
lendudeAs pointed out in #35 the code behind this got completely reworked over the years, so we would need fresh steps to reproduce this, but it seems unlikely that the same logic is still buried somewhere in the refactored code.
In D7 on the other hand, this is still as outlined in the IS, so let's move it there. If there is still a way to reproduce this on D9 please add some steps and feel free to move it back to the correct version where you can reproduce this.