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

Comments

jmuzz’s picture

Category: support » bug
Status: Active » Needs review
StatusFileSize
new2.79 KB

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

vflirt’s picture

HI,

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

rudiedirkx’s picture

The ORDER change should be for pid, not for language. The language change is to make sure UND is last, and specific languages are first. That's A Good Thing (TM). The pid ORDER is dubious: I'd say the newest pid is more important, but since they're all loaded, that's not relevant, I think.

vflirt’s picture

Hi,
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.

jmuzz’s picture

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

rudiedirkx’s picture

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

jmuzz’s picture

Oh I didn't see that you tried this already, sorry if I closed your issue incorrectly.

rudiedirkx’s picture

Issue summary: View changes
Status: Needs review » Needs work

You're right. This is it. The fetchAllKeyed is weird, so the ORDER has to be flipped. It should be exactly opposite though, not just language.

vflirt’s picture

Hi,
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

rudiedirkx’s picture

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

vflirt’s picture

Hi,

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

jmuzz’s picture

Version: 7.23 » 7.24

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

vflirt’s picture

Status: Needs work » Needs review

As 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

ricovandevin’s picture

We have had the same issue and the patch in #1 solved it. I suggest to get the fix in core.

vflirt’s picture

Version: 7.24 » 7.26

updating the issue to 7.26
would love to see this fixed in core

rbayliss’s picture

This is still an issue, and the patch in #1 still applies cleanly. I can confirm it fixes the issue.

vflirt’s picture

Version: 7.26 » 7.32
Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Version: 7.32 » 8.0.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7, +Needs tests

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

pushpinderchauhan’s picture

Status: Needs work » Needs review
StatusFileSize
new2.24 KB

D8 patch.

vflirt’s picture

Hi,

@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 :)

vflirt’s picture

Status: Needs review » Needs work
tim.plunkett’s picture

Component: routing system » path.module

There is no IQ component for "path subsystem", but its certainly not routing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pwolanin’s picture

pwolanin’s picture

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andrea.pompili’s picture

Excellent patch. Works well. Thank you.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joseph.olstad’s picture

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

drupal_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?

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

lendude’s picture

Version: 9.4.x-dev » 7.x-dev
Issue tags: -Needs backport to D7 +Bug Smash Initiative

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

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.