At this time, if you have several aliases for the one place, and will request alias from drupal_lookup_path(), it will return first occured alias in database.

For example, you have such url_alias table:

pid    src           dst   lng
123    node/10       abc   ru
124    node/10       bcd   ru

This situation commonly occurs, if you're using pathauto with "Create a new alias. Leave the existing alias functioning" option enabled. In this case, drupal_lookup_path('alias', 'node/10', 'ru') will return 'abc', but not 'bcd', which is needed, because it was created later than 'abc'.

Solution of this problem is very simple. We should add additional sort by PID to the alias lookup query:

-      $alias = db_result(db_query("SELECT dst FROM {url_alias} WHERE src = '%s' AND language IN('%s', '') ORDER BY language DESC", $path, $path_language));
+      $alias = db_result(db_query("SELECT dst FROM {url_alias} WHERE src = '%s' AND language IN('%s', '') ORDER BY language, pid DESC", $path, $path_language));
Files: 
CommentFileSizeAuthor
#85 358315_followup_D7.patch1.34 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 18,183 pass(es).
[ View ]
#83 358315_followup_D7.patch1.34 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 18,185 pass(es).
[ View ]
#77 358315-drupal-lookup-path-alias-order-D6.patch2.93 KBandypost
#77 358315_followup_D7.patch1.32 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 358315_followup_D7.patch.
[ View ]
#69 358315-drupal-lookup-path-alias-order-D6.patch2.5 KBandypost
#67 358315-drupal-lookup-path-alias-order-D6.patch1.84 KBxurizaemon
#62 drupal-lookup-path-alias-order-D6.patch2.99 KBandypost
#62 path-whitespace-D7.patch667 bytesandypost
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in path-whitespace-D7.patch.
[ View ]
#57 358315-drupal-lookup-path-alias-order_D7_new_update_1.patch3.51 KBneochief
Passed: 11784 passes, 0 fails, 0 exceptions
[ View ]
#55 358315-drupal-lookup-path-alias-order_D7_new_update.patch3.51 KBneochief
Invalid patch format in 358315-drupal-lookup-path-alias-order_D7_new_update_1.patch.
[ View ]
#52 358315-drupal-lookup-path-alias-order_D7_new_update.patch2.35 KBneochief
Failed: Failed to install HEAD.
[ View ]
#50 358315-drupal-lookup-path-alias-order_D7_new_update.patch2.82 KBneochief
Failed: Failed to install HEAD.
[ View ]
#48 358315-drupal-lookup-path-alias-order_D7_yet_another.patch2.71 KBneochief
Invalid patch format in 358315-drupal-lookup-path-alias-order_D7_yet_another.patch.
[ View ]
#43 358315-drupal-lookup-path-alias-order_D7_n.patch2.43 KBneochief
Failed: Failed to apply patch.
[ View ]
#41 358315-drupal-lookup-path-alias-order_D7.patch2.9 KBcatch
Failed: Failed to apply patch.
[ View ]
#38 358315-drupal-lookup-path-alias-order_D6.patch2.89 KBneochief
#38 358315-drupal-lookup-path-alias-order_D7.patch2.91 KBneochief
Failed: Failed to apply patch.
[ View ]
#36 358315-drupal-lookup-path-alias-order.patch2.9 KBneochief
Failed: Failed to apply patch.
[ View ]
#28 358315-drupal-lookup-path-alias-order.patch2.9 KBDamien Tournoud
Failed: Failed to install HEAD.
[ View ]
#24 drupal_path_inc_and_system_install4_0.patch2.8 KBneochief
Failed: Invalid PHP syntax in includes/path.inc .
[ View ]
#21 drupal_path_inc_and_system_install3.patch2.8 KBneochief
Failed: Invalid PHP syntax in includes/path.inc .
[ View ]
#19 drupal_path_inc_and_system_install3.patch2.8 KBneochief
Failed: Invalid PHP syntax in includes/path.inc .
[ View ]
#10 drupal_path_inc_and_system_install.patch2.79 KBneochief
Failed: Invalid PHP syntax.
[ View ]
#7 drupal_path_inc_and_system_install.patch2.69 KBneochief
Failed: Invalid PHP syntax.
[ View ]
#4 drupal_path_inc_D7.patch1.4 KBneochief
Failed: Failed to apply patch.
[ View ]
#1 drupal_path_inc_D7.patch1.4 KBneochief
Failed: Failed to apply patch.
[ View ]
path.inc_D5.patch1.06 KBneochief
path.inc_D6.patch1.28 KBneochief

Comments

Version:6.x-dev» 7.x-dev
StatusFileSize
new1.4 KB
Failed: Failed to apply patch.
[ View ]

Bumping to 7.x

Status:Needs review» Needs work

The last submitted patch failed testing.

This needs index tweaks as well - we just got rid of filesort in HEAD for this query #363262: Missing index on url_alias table

Also I'd much rather see us have an 'active' column in this table per #106094: Prevent duplicate pages (active URL alias) than relying on the serial field to tell us which is the right one.

Status:Needs work» Needs review
StatusFileSize
new1.4 KB
Failed: Failed to apply patch.
[ View ]

Re-submitting to test-bed.

@catch Did I missed something, or neither of patches in both of those issues aren't changing url_alias table at all? Nothing should actually work without my changes or your proposal with active column.

neochief, no I missed something - I thought those patches added an active column, but they use pid the same as your patch, so my bad there.

Status:Needs review» Needs work

Sorting on an autoincremented primary key is ugly as hell, but I guess we have no other choice. And catch is right, you need to tweak indexes too, to add pid at the end of the existing index.

Status:Needs work» Needs review
StatusFileSize
new2.69 KB
Failed: Invalid PHP syntax.
[ View ]

Hope it's that you meant by moving pid to the end of index.

Status:Needs review» Needs work

The last submitted patch failed testing.

That's right (would be nice to run the new query through explain with that index just as a sanity check). However we don't provide HEAD - HEAD upgrade paths, so better to change the update which added src_language as an index in the first place.

Status:Needs work» Needs review
StatusFileSize
new2.79 KB
Failed: Invalid PHP syntax.
[ View ]

EXPLAIN SELECT dst FROM url_alias WHERE src = 'node' AND language IN('en', '') ORDER BY language, pid DESC
id  select_type  table      type   possible_keys     key               key_len ref   rows  Extra
1   SIMPLE       url_alias  range  src_language_pid  src_language_pid  424     NULL  2     Usingwhere;Usingfilesort

EXPLAIN SELECT src FROM url_alias WHERE dst = 'node' AND language IN('en', '') ORDER BY language, pid DESC
id  select_type  table      type   possible_keys     key               key_len ref   rows  Extra
1   SIMPLE       url_alias  range  dst_language_pid  dst_language_pid  424     NULL  2     Usingwhere;Usingfilesort

New patch attached.

Status:Needs review» Needs work

The last submitted patch failed testing.

Could you please also point me why last patch failed testing? It's very straight forward, I can't see any "Invalid PHP syntax" in it.

neochief - that sometimes happens because of windows line breaks, there's an outstanding test framework bug which fixes the underlying issue I think.

That EXPLAIN doesn't look encouraging - we just removed that filesort in HEAD - #363262: Missing index on url_alias table

Yes, I know. But I have no idea how to improve it.

But I have no idea how to improve it.

That's because of the pid DESC: MySQL can't use indexes when you mix directions.

We could add a column that is the equivalent of -pid, but I'm not sure it really worth it.

What about ordering by language DESC? edit: shouldn't it do that anyway?

I'm afraid it worth that. For example, if you're moving a large site into drupal, there's no better way to handle redirects from old URLs (if their count too big to place everything into .htaccess).

I thought about adding another column as timestamp. It will perfectly solve the main problem, but I not sure about performance impact it may cause.

What about ordering by language DESC? edit: shouldn't it do that anyway?

Yes it should! The language should have priority over the empty language. @neochief, could you reroll with that?

Status:Needs work» Needs review
StatusFileSize
new2.8 KB
Failed: Invalid PHP syntax in includes/path.inc .
[ View ]

Here's a patch. Usingfilesort is gone. catch, you're genius :)

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.8 KB
Failed: Invalid PHP syntax in includes/path.inc .
[ View ]

Food for test bot. Same patch with windows line breaks.

Status:Needs review» Needs work

The last submitted patch failed testing.

Sorry guys, I give up. Patch is 100% working, but test bot won't eat it.

StatusFileSize
new2.8 KB
Failed: Invalid PHP syntax in includes/path.inc .
[ View ]

Fresh patch.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch failed testing.

]:0>

Status:Needs work» Needs review
StatusFileSize
new2.9 KB
Failed: Failed to install HEAD.
[ View ]

There were some line ending issues in the patch.

Yahoo! After this patch gets to the core, we can make progress on this #269877: path_set_alias() doesn't account for same alias in different languages either.

Status:Needs review» Reviewed & tested by the community

The bot came up green. Let's go!

Care to post an EXPLAIN?

EXPLAIN SELECT src
FROM url_alias
WHERE dst = "node/123"
AND language
IN (
"en", "ru"
)
ORDER BY language DESC , pid DESC

id select_type table type possible_keys key key_len ref rows Extra
1 SIMPLE url_alias range dst_language_pid dst_language_pid 424 NULL 2 Using where

No filesort, two rows considered? Then I am happy with this patch.

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

TestBot, requesting retest.

StatusFileSize
new2.9 KB
Failed: Failed to apply patch.
[ View ]

Status:Needs review» Reviewed & tested by the community

Patch is green. Moving issue to RTBC.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.91 KB
Failed: Failed to apply patch.
[ View ]
new2.89 KB

Heh, actually, there is an error in the patch. I found it during preparation of backport to 6x. New indexes weren't named as 'dst_language_pid' and 'src_language_pid' in schema. Here's an update and version for 6.x.

Status:Needs review» Reviewed & tested by the community

Great! Moving to RTBC.

It would be neat if you could fix this:

/**
- * Replace src index on the {url_alias} table with src, language.
+ * Replace src index on the {url_alias} table with src, language and add pid
+ * column to {url_alias} indexes.

PHPDoc function summaries should be on one line.

StatusFileSize
new2.9 KB
Failed: Failed to apply patch.
[ View ]

Reworked the comment.

Status:Reviewed & tested by the community» Needs work

The update function is wrong:

<?php
db_add_index($ret, 'url_alias', 'src_language', array('src', 'language'));
db_drop_index($ret, 'url_alias', 'dst_language');
db_add_index($ret, 'url_alias', 'dst_language_pid', array('dst', 'language', 'pid'));
db_add_index($ret, 'url_alias', 'src_language_pid', array('src', 'language', 'pid'));
  
db_drop_index($ret, 'url_alias', 'src');
?>

dst_language_pid is supposed to be a unique key, so we have to use db_add_unique_key() here.

Status:Needs work» Needs review
StatusFileSize
new2.43 KB
Failed: Failed to apply patch.
[ View ]

Here's an update. Fixed unique keys (thanks, Damien) and moved updates to newer function.

Subscribe.

Status:Needs review» Needs work

The updates should be left in the earlier hook_update_N() - we don't provide HEAD - HEAD upgrade paths.

Subscribe. Hope it will be ported to 6.x as well

I am confused. http://drupal.org/files/issues/path.inc_D6.patch is not already tested and part of D6 core code base?

Status:Needs work» Needs review
StatusFileSize
new2.71 KB
Invalid patch format in 358315-drupal-lookup-path-alias-order_D7_yet_another.patch.
[ View ]

Heh, from the las time I made a patch, the updates moved from update_7018 to update_6049. Here's a patch which supplies that update with our updates.

I'm totally confused with update functions order. Catch, could you please help me wih this patch if it will be not proper one more time?

Status:Needs review» Needs work

So the moving of that update was because the language index was committed to Drupal 6 - however Drupal 6 sites have already run that update, so we can't simply modify it.

I think the order has to be something like this:

1. Add the new index in system_update_700x()

2. If it's committed, try to backport to Drupal 6 with a system_update_6050()

3. If the patch is accepted into Drupal 6, remove the system_update_700x()

So please repost with a (new) 700x() update, and let's worry about Drupal 6 later.

Status:Needs work» Needs review
StatusFileSize
new2.82 KB
Failed: Failed to install HEAD.
[ View ]

Thanks for your advice, catch. Here's a patch with new update function.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.35 KB
Failed: Failed to install HEAD.
[ View ]

"Failed: Failed to install HEAD" — strange, I've done this manually and everything works fine. Maybe old Index-patches bug with test bed. Resubmitting the patch.

Status:Needs review» Needs work

The last submitted patch failed testing.

It's probably a system_update_N() numbering conflict.

Status:Needs work» Needs review
StatusFileSize
new3.51 KB
Invalid patch format in 358315-drupal-lookup-path-alias-order_D7_new_update_1.patch.
[ View ]

Yes, you're right, my copy was outdated. Here's a new patch, that introduces a new hook_update_N and a soring for system pathes query (that appeared recently, I guess).

Status:Needs review» Needs work

It should be

ORDER BY language ASC, pid ASC
for the system paths query so that the array keys get overwritten properly in case of multiple paths.

StatusFileSize
new3.51 KB
Passed: 11784 passes, 0 fails, 0 exceptions
[ View ]

Status:Needs work» Reviewed & tested by the community

Looks great. Thanks for sticking with it neochief.

Hooray! :)

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD! Thanks.

Version:7.x-dev» 6.x-dev
Status:Fixed» Patch (to be ported)

Moving to D6 for consideration.

Status:Patch (to be ported)» Needs review
Issue tags:+needs backport to D6
StatusFileSize
new667 bytes
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in path-whitespace-D7.patch.
[ View ]
new2.99 KB

Straight backport to d6

D7 system_update_7025() has a problem with whitespace

Suppose it'd be cleared after d6 patch lands.

Subscribing for the D6 version. Is the patch in #62 something that should be applied to a production server? I'm assuming not, but would like to know when this will be applied. This error really "owning" my production site at the moment.

subscribe

subscribin

Patch in #62 needed to be modified to account for the incremented system update before it would behave nicely, but seems to have resolved the issue on our site, monolingual with locale module disabled.

Was seeing errors of the form,
user warning: Duplicate entry 'content/get-reviews-' for key 2 query: INSERT INTO url_alias (src, dst, language) VALUES ('node/86694', 'content/get-reviews', '') in /path/to/drupal/drupal-6.13/modules/path/path.module on line 112.

Now wondering if a workaround would be to enable locale and run a single language. Too late for me now though :P

Re the issue alluded to in #66, the release of drupal 6.13 included system_update_6051() so that patch needs to be updated or else there's a fatal error due to the conflicting function names.

This patch bumps the update ID used in #62 above by one, which makes it work against current Drupal (both 6.x-CVS and 6.13).

I'm unsure of the correct method of obtaining or "owning" a system_update_n() function so forgive me if I've not followed correct procedure there.

@xurizaemon: unfortunately there's no way to reserve an update, so just re-rolling the patch if another one gets in first with the same number is exactly right.

Issue tags:+Performance
StatusFileSize
new2.5 KB

Reroll for current state and Tag

@xurizaemon patch should be in unified format (use cvs diff -up)

subscribe

Status:Needs review» Reviewed & tested by the community

Magically, over a month (and a D6 release) later, backport patch still applies and works. RTBC?

Subscribing.

Subscribing.

Is there anything else that can be done to help persuade the D6 maintainers to commit this?

Suppose we just wait for Gabor to commit this

Status:Reviewed & tested by the community» Needs work

Looking at the key changes, seems like we had an interim patch which used a unique key, but that was reverted to a non-unique key in the meantime? I'm not seeing the reason. Why not add a unique key for these? pid should be unique combined with anything as far as I see.

(Maybe this even needs work for D7 to fix the update code).

Gábor, pid is already primary key.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 358315_followup_D7.patch.
[ View ]
new2.93 KB

@Gábor Hojtsy now patch with unique key with D7 follow-up

by the way D7 http://api.drupal.org/api/function/system_update_7025/7 is broken with this follow-up it should be fixed

anyway, this change is superseded by http://api.drupal.org/api/function/system_update_7042/7

subscribing

Combination of the patches in #77 and the one in #269877: path_set_alias() doesn't account for same alias in different languages #146 solved my issues and saved my life!

Hope to see them two committed to the next 6.16 ;)

Status:Reviewed & tested by the community» Fixed

Thanks, committed.

Version:6.x-dev» 7.x-dev
Status:Fixed» Reviewed & tested by the community

there's a follow-up patch in #77 for D7, changes in install should be synchronized.

Looks good to me.

StatusFileSize
new1.34 KB
PASSED: [[SimpleTest]]: [MySQL] 18,185 pass(es).
[ View ]

I re-roll this against current HEAD

StatusFileSize
new1.34 KB
PASSED: [[SimpleTest]]: [MySQL] 18,183 pass(es).
[ View ]

Patch

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

Status:Fixed» Closed (fixed)

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

Did this make it to D6 as well? ...I mean the patch from #77

...well??

Subscribing for d6. Maybe reopen this issue?

...to answer my own question in #88... Yes, the changes from the patch in #77 did make it in D6 (I've checked 6.17). The only diff is that function system_update_6054 is now function system_update_6055 ;)

D6 patch also required some changes in ../includes/path.inc while D7 didn't. So, I guess we are good with D6 too and this issue doesn't need to be opened again.

Is there another issue for d6 patch?

Nope!

...What usually happens with drupal issues is they get fixed for D7 and then are backported to D6 (hence the 'patch (to be ported)' status). This is what normally happens to bugs that effect both versions and to feature requests too. That's why you'll often see people post:

New features go to the latest version, then backported

The same thing happens to major versions of modules too. For example if there is a 6.x-1.x line of versions and a 6.x-2.x, bug fixes go to the next version and then ported to the older one (if it is still actively maintained and if possible). There are cases where maintainers choose not to port patches to older versions. Especially if these patches are introducing new features.

Some developers like to work on D6 first and then port their patches to D7. I guess they do this because there is not enough people using D7 yet and same goes for D7 versions of modules. This is what I vote for too. If there are not enough users to test, I guess it takes longer for the patch to get committed and thus for issues to get fixed. I believe this situation is going to change once we have a stable D7 version (or at least a Release Candidate).

...anyways, I think we are getting off-topic here and this is a really bad thing. We should be posting questions and answers related to the issues and not using issue queues for generic discussions.

I believe you should research on your own to find out how patches evolve before they get committed. These links might help and I think they are a nice start (especially the 1st link and the links in that page's bottom):

http://drupal.org/forum-posting
http://drupal.org/node/10261
http://drupal.org/patch/submit

Remember: Google is your friend and there's also a search box available in the top right corner of every Drupal.org page. Don't be afraid to use it ;)

Issue tags:-needs backport to D6

(Just un-confusing myself, the posters from #88 onwards, and anyone else who stumbles onto this issue page from another still-open path related issue.

I assume it's customary to remove tags when they're not needed anymore.)

...good catch Roderik!

@roderik
Thank you for this information. I was confused.