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));
CommentFileSizeAuthor
#85 358315_followup_D7.patch1.34 KBandypost
#83 358315_followup_D7.patch1.34 KBandypost
#77 358315-drupal-lookup-path-alias-order-D6.patch2.93 KBandypost
#77 358315_followup_D7.patch1.32 KBandypost
#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
#57 358315-drupal-lookup-path-alias-order_D7_new_update_1.patch3.51 KBneochief
#55 358315-drupal-lookup-path-alias-order_D7_new_update.patch3.51 KBneochief
#52 358315-drupal-lookup-path-alias-order_D7_new_update.patch2.35 KBneochief
#50 358315-drupal-lookup-path-alias-order_D7_new_update.patch2.82 KBneochief
#48 358315-drupal-lookup-path-alias-order_D7_yet_another.patch2.71 KBneochief
#43 358315-drupal-lookup-path-alias-order_D7_n.patch2.43 KBneochief
#41 358315-drupal-lookup-path-alias-order_D7.patch2.9 KBcatch
#38 358315-drupal-lookup-path-alias-order_D6.patch2.89 KBneochief
#38 358315-drupal-lookup-path-alias-order_D7.patch2.91 KBneochief
#36 358315-drupal-lookup-path-alias-order.patch2.9 KBneochief
#28 358315-drupal-lookup-path-alias-order.patch2.9 KBDamien Tournoud
#24 drupal_path_inc_and_system_install4_0.patch2.8 KBneochief
#21 drupal_path_inc_and_system_install3.patch2.8 KBneochief
#19 drupal_path_inc_and_system_install3.patch2.8 KBneochief
#10 drupal_path_inc_and_system_install.patch2.79 KBneochief
#7 drupal_path_inc_and_system_install.patch2.69 KBneochief
#4 drupal_path_inc_D7.patch1.4 KBneochief
#1 drupal_path_inc_D7.patch1.4 KBneochief
path.inc_D5.patch1.06 KBneochief
path.inc_D6.patch1.28 KBneochief
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neochief’s picture

Version: 6.x-dev » 7.x-dev
FileSize
1.4 KB

Bumping to 7.x

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

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.

neochief’s picture

Status: Needs work » Needs review
FileSize
1.4 KB

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.

catch’s picture

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.

Damien Tournoud’s picture

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.

neochief’s picture

Status: Needs work » Needs review
FileSize
2.69 KB

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.

catch’s picture

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.

neochief’s picture

Status: Needs work » Needs review
FileSize
2.79 KB
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.

neochief’s picture

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.

catch’s picture

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

neochief’s picture

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

Damien Tournoud’s picture

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.

catch’s picture

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

neochief’s picture

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.

Damien Tournoud’s picture

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?

neochief’s picture

Status: Needs work » Needs review
FileSize
2.8 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

neochief’s picture

Status: Needs work » Needs review
FileSize
2.8 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

neochief’s picture

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

neochief’s picture

Fresh patch.

neochief’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

neochief’s picture

]:0>

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
2.9 KB

There were some line ending issues in the patch.

neochief’s picture

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.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

The bot came up green. Let's go!

chx’s picture

Care to post an EXPLAIN?

neochief’s picture

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
chx’s picture

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.

neochief’s picture

Status: Needs work » Needs review

TestBot, requesting retest.

neochief’s picture

neochief’s picture

Status: Needs review » Reviewed & tested by the community

Patch is green. Moving issue to RTBC.

neochief’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.91 KB
2.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.

neochief’s picture

Status: Needs review » Reviewed & tested by the community

Great! Moving to RTBC.

sun’s picture

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.

catch’s picture

Reworked the comment.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

The update function is wrong:

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

neochief’s picture

Status: Needs work » Needs review
FileSize
2.43 KB

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

mathieu’s picture

Subscribe.

catch’s picture

Status: Needs review » Needs work

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

Artem’s picture

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

SamRose’s picture

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

neochief’s picture

Status: Needs work » Needs review
FileSize
2.71 KB

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?

catch’s picture

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.

neochief’s picture

Status: Needs work » Needs review
FileSize
2.82 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

neochief’s picture

Status: Needs work » Needs review
FileSize
2.35 KB

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

catch’s picture

It's probably a system_update_N() numbering conflict.

neochief’s picture

Status: Needs work » Needs review
FileSize
3.51 KB

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

catch’s picture

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.

neochief’s picture

catch’s picture

Status: Needs work » Reviewed & tested by the community

Looks great. Thanks for sticking with it neochief.

neochief’s picture

Hooray! :)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD! Thanks.

catch’s picture

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

Moving to D6 for consideration.

andypost’s picture

Status: Patch (to be ported) » Needs review
Issue tags: +Needs backport to D6
FileSize
667 bytes
2.99 KB

Straight backport to d6

D7 system_update_7025() has a problem with whitespace

Suppose it'd be cleared after d6 patch lands.

amariotti’s picture

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.

ianchan’s picture

subscribe

kevinwalsh’s picture

subscribin

xurizaemon’s picture

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

xurizaemon’s picture

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.

catch’s picture

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

andypost’s picture

Reroll for current state and Tag

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

dogbertdp’s picture

subscribe

mathieu’s picture

Status: Needs review » Reviewed & tested by the community

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

NiklasBr’s picture

Subscribing.

DamienMcKenna’s picture

Subscribing.

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

andypost’s picture

Suppose we just wait for Gabor to commit this

Gábor Hojtsy’s picture

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

neochief’s picture

Gábor, pid is already primary key.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.32 KB
2.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

klonos’s picture

subscribing

klonos’s picture

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed.

andypost’s picture

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.

sun’s picture

Looks good to me.

andypost’s picture

FileSize
1.34 KB

I re-roll this against current HEAD

andypost’s picture

andypost’s picture

FileSize
1.34 KB

Patch

Dries’s picture

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.

klonos’s picture

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

klonos’s picture

...well??

checker’s picture

Subscribing for d6. Maybe reopen this issue?

klonos’s picture

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

checker’s picture

Is there another issue for d6 patch?

klonos’s picture

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

roderik’s picture

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

klonos’s picture

...good catch Roderik!

checker’s picture

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

andypost’s picture