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));
Comment | File | Size | Author |
---|---|---|---|
#85 | 358315_followup_D7.patch | 1.34 KB | andypost |
#83 | 358315_followup_D7.patch | 1.34 KB | andypost |
#77 | 358315-drupal-lookup-path-alias-order-D6.patch | 2.93 KB | andypost |
#77 | 358315_followup_D7.patch | 1.32 KB | andypost |
#69 | 358315-drupal-lookup-path-alias-order-D6.patch | 2.5 KB | andypost |
Comments
Comment #1
neochief CreditAttribution: neochief commentedBumping to 7.x
Comment #3
catchThis 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.
Comment #4
neochief CreditAttribution: neochief commentedRe-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.
Comment #5
catchneochief, 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.
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedSorting 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.
Comment #7
neochief CreditAttribution: neochief commentedHope it's that you meant by moving pid to the end of index.
Comment #9
catchThat'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.
Comment #10
neochief CreditAttribution: neochief commentedNew patch attached.
Comment #12
neochief CreditAttribution: neochief commentedCould 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.
Comment #13
catchneochief - 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
Comment #14
neochief CreditAttribution: neochief commentedYes, I know. But I have no idea how to improve it.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat'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.Comment #16
catchWhat about ordering by language DESC? edit: shouldn't it do that anyway?
Comment #17
neochief CreditAttribution: neochief commentedI'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.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commentedYes it should! The language should have priority over the empty language. @neochief, could you reroll with that?
Comment #19
neochief CreditAttribution: neochief commentedHere's a patch. Usingfilesort is gone. catch, you're genius :)
Comment #21
neochief CreditAttribution: neochief commentedFood for test bot. Same patch with windows line breaks.
Comment #23
neochief CreditAttribution: neochief commentedSorry guys, I give up. Patch is 100% working, but test bot won't eat it.
Comment #24
neochief CreditAttribution: neochief commentedFresh patch.
Comment #25
neochief CreditAttribution: neochief commentedComment #27
neochief CreditAttribution: neochief commented]:0>
Comment #28
Damien Tournoud CreditAttribution: Damien Tournoud commentedThere were some line ending issues in the patch.
Comment #29
neochief CreditAttribution: neochief commentedYahoo! 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.
Comment #30
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe bot came up green. Let's go!
Comment #31
chx CreditAttribution: chx commentedCare to post an EXPLAIN?
Comment #32
neochief CreditAttribution: neochief commentedComment #33
chx CreditAttribution: chx commentedNo filesort, two rows considered? Then I am happy with this patch.
Comment #35
neochief CreditAttribution: neochief commentedTestBot, requesting retest.
Comment #36
neochief CreditAttribution: neochief commentedComment #37
neochief CreditAttribution: neochief commentedPatch is green. Moving issue to RTBC.
Comment #38
neochief CreditAttribution: neochief commentedHeh, 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.
Comment #39
neochief CreditAttribution: neochief commentedGreat! Moving to RTBC.
Comment #40
sunIt would be neat if you could fix this:
PHPDoc function summaries should be on one line.
Comment #41
catchReworked the comment.
Comment #42
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe update function is wrong:
dst_language_pid
is supposed to be a unique key, so we have to usedb_add_unique_key()
here.Comment #43
neochief CreditAttribution: neochief commentedHere's an update. Fixed unique keys (thanks, Damien) and moved updates to newer function.
Comment #44
mathieu CreditAttribution: mathieu commentedSubscribe.
Comment #45
catchThe updates should be left in the earlier hook_update_N() - we don't provide HEAD - HEAD upgrade paths.
Comment #46
Artem CreditAttribution: Artem commentedSubscribe. Hope it will be ported to 6.x as well
Comment #47
SamRose CreditAttribution: SamRose commentedI am confused. http://drupal.org/files/issues/path.inc_D6.patch is not already tested and part of D6 core code base?
Comment #48
neochief CreditAttribution: neochief commentedHeh, 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?
Comment #49
catchSo 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.
Comment #50
neochief CreditAttribution: neochief commentedThanks for your advice, catch. Here's a patch with new update function.
Comment #52
neochief CreditAttribution: neochief commented"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.
Comment #54
catchIt's probably a system_update_N() numbering conflict.
Comment #55
neochief CreditAttribution: neochief commentedYes, 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).
Comment #56
catchIt 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.
Comment #57
neochief CreditAttribution: neochief commentedComment #58
catchLooks great. Thanks for sticking with it neochief.
Comment #59
neochief CreditAttribution: neochief commentedHooray! :)
Comment #60
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD! Thanks.
Comment #61
catchMoving to D6 for consideration.
Comment #62
andypostStraight backport to d6
D7 system_update_7025() has a problem with whitespace
Suppose it'd be cleared after d6 patch lands.
Comment #63
amariotti CreditAttribution: amariotti commentedSubscribing 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.
Comment #64
ianchan CreditAttribution: ianchan commentedsubscribe
Comment #65
kevinwalsh CreditAttribution: kevinwalsh commentedsubscribin
Comment #66
xurizaemonPatch 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
Comment #67
xurizaemonRe 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.
Comment #68
catch@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.
Comment #69
andypostReroll for current state and Tag
@xurizaemon patch should be in unified format (use cvs diff -up)
Comment #70
dogbertdp CreditAttribution: dogbertdp commentedsubscribe
Comment #71
mathieu CreditAttribution: mathieu commentedMagically, over a month (and a D6 release) later, backport patch still applies and works. RTBC?
Comment #72
NiklasBr CreditAttribution: NiklasBr commentedSubscribing.
Comment #73
DamienMcKennaSubscribing.
Is there anything else that can be done to help persuade the D6 maintainers to commit this?
Comment #74
andypostSuppose we just wait for Gabor to commit this
Comment #75
Gábor HojtsyLooking 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).
Comment #76
neochief CreditAttribution: neochief commentedGábor, pid is already primary key.
Comment #77
andypost@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
Comment #78
klonossubscribing
Comment #79
klonosCombination 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 ;)
Comment #80
Gábor HojtsyThanks, committed.
Comment #81
andypostthere's a follow-up patch in #77 for D7, changes in install should be synchronized.
Comment #82
sunLooks good to me.
Comment #83
andypostI re-roll this against current HEAD
Comment #84
andypostNow this should be 6055 after #729308: update system_update_6054() declared twice in system.install
Comment #85
andypostPatch
Comment #86
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #88
klonosDid this make it to D6 as well? ...I mean the patch from #77
Comment #89
klonos...well??
Comment #90
checker CreditAttribution: checker commentedSubscribing for d6. Maybe reopen this issue?
Comment #91
klonos...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 nowfunction 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.
Comment #92
checker CreditAttribution: checker commentedIs there another issue for d6 patch?
Comment #93
klonosNope!
...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:
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 ;)
Comment #94
roderik(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.)
Comment #95
klonos...good catch Roderik!
Comment #96
checker CreditAttribution: checker commented@roderik
Thank you for this information. I was confused.
Comment #97
andypostFollow-up #818214: Improper table structure for url_alias